This diff was a part of D4168 review. Per Andrew's suggestion it was split in two parts: HDMI and IPU
Details
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
sys/arm/freescale/imx/imx6_hdmi.c | ||
---|---|---|
96 | Why not uint8_t? | |
118–119 | Why the cast? | |
157–159 | Is the todo to get it working on something other than DVI mode? | |
370–371 | Extra newline | |
436–438 | What are these magic numbers? | |
650 | You're setting this here and on line 676. | |
653 | Why track this if you pass 0 to bus_release_resource? | |
723 | return (hdmi_edid_read(...)); | |
sys/arm/freescale/imx/imx6_hdmireg.h | ||
29 | #ifndef should have a space (I have no idea why the style is different between this and #define). |
Rest of the comments were addressed in new revision
sys/arm/freescale/imx/imx6_hdmi.c | ||
---|---|---|
159–161 | Yes, for now it's only DVI mode. Next stage is going to be adding more real HDMI stuff | |
438–440 | 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 | ||
---|---|---|
102 | 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. | |
559 | missing empty line | |
604 | 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. | |
725 | return (hdmi_edid_read(device_get_softc(dev), ...) :) | |
sys/arm/freescale/imx/imx6_hdmireg.h | ||
69 | 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 | 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 | Right. And registration of ofw_iicbus was added in r292157. |