This diff was a part of D4168 review. Per Andrew's suggestion it was split in two parts: HDMI and IPU
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| sys/arm/freescale/imx/imx6_hdmi.c | ||
|---|---|---|
| 96 ↗ | (On Diff #10685) | Why not uint8_t? |
| 118–119 ↗ | (On Diff #10685) | Why the cast? |
| 157–159 ↗ | (On Diff #10685) | Is the todo to get it working on something other than DVI mode? |
| 370–371 ↗ | (On Diff #10685) | Extra newline |
| 436–438 ↗ | (On Diff #10685) | What are these magic numbers? |
| 650 ↗ | (On Diff #10685) | You're setting this here and on line 676. |
| 653 ↗ | (On Diff #10685) | Why track this if you pass 0 to bus_release_resource? |
| 723 ↗ | (On Diff #10685) | return (hdmi_edid_read(...)); |
| sys/arm/freescale/imx/imx6_hdmireg.h | ||
| 29 ↗ | (On Diff #10685) | #ifndef should have a space (I have no idea why the style is different between this and #define). |
Comment Actions
Rest of the comments were addressed in new revision
| sys/arm/freescale/imx/imx6_hdmi.c | ||
|---|---|---|
| 158–160 ↗ | (On Diff #11183) | Yes, for now it's only DVI mode. Next stage is going to be adding more real HDMI stuff |
| 437–439 ↗ | (On Diff #11183) | Some magic numbers from HDMI spec. They're not combination of flags - they're bits to fill data lines that are not used during certain kind of transfers. I don't knwo why these particular numbers are used. |
| sys/arm/freescale/imx/imx6_hdmi.c | ||
|---|---|---|
| 101 ↗ | (On Diff #11183) | I can't really tell what the runtime context is here, but if sleeping is allowed then pause() would be better than delay -- an imx6 can get a lot of other work done in a millisecond. |
| 558 ↗ | (On Diff #11183) | missing empty line |
| 603 ↗ | (On Diff #11183) | Is this a dedicated bus so that you can be sure that 2 different threads will never try to access the bus at the same time? if not, use iicbus_transfer_excl() to take ownership of the bus during the transaction. |
| 724 ↗ | (On Diff #11183) | return (hdmi_edid_read(device_get_softc(dev), ...) :) |
| sys/arm/freescale/imx/imx6_hdmireg.h | ||
| 68 ↗ | (On Diff #11183) | When doing this style where the bit definitions follow the register name/addr I like to indent the bit names with a tab plus 2 spaces instead of 2 tabs, it keeps it from going beyond col 80 so fast. (and style(9) requires a tab but doesn't say you *can't* add a couple spaces after it. :) |
| sys/arm/freescale/imx/imx_i2c.c | ||
| 321 ↗ | (On Diff #11183) | This doesn't seem right. The iic device exists just to provide /dev/iicN for userland access. I think sc->iicbus should be registered as the provider for the xref, because the usage is iicbus_transfer() which wants the device_t of the bus and calls IICBUS_TRANSFER(device_get_parent(bus)) which gets control into this driver. If it's working with the iic device, it seems like it must be by accident. |
| sys/arm/freescale/imx/imx_i2c.c | ||
|---|---|---|
| 321 ↗ | (On Diff #11183) | Right. And registration of ofw_iicbus was added in r292157. |