Page MenuHomeFreeBSD

i.MX6 HDMI framer driver
ClosedPublic

Authored by gonzo on Nov 16 2015, 4:06 AM.

Details

Summary

This diff was a part of D4168 review. Per Andrew's suggestion it was split in two parts: HDMI and IPU

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

gonzo updated this revision to Diff 10223.Nov 16 2015, 4:06 AM
gonzo retitled this revision from to i.MX6 HDMI framer driver.
gonzo updated this object.
gonzo edited the test plan for this revision. (Show Details)
gonzo added reviewers: ian, andrew, imp.
gonzo set the repository for this revision to rS FreeBSD src repository.
gonzo mentioned this in D4168: i.MX6 IPU drivers.
andrew added inline comments.Nov 17 2015, 12:04 PM
sys/arm/freescale/imx/imx6_hdmi.c
631–635

Why set up the interrupt if you're not using it?

sys/arm/freescale/imx/imx6_hdmireg.h
68

There is some inconsistent indentation in this file.

gonzo updated this revision to Diff 10685.Dec 2 2015, 11:28 PM
  • Indentation cleanup in imx6_hdmireg.h
  • Removed unused interrupt stuff
andrew added inline comments.Dec 12 2015, 6:49 PM
sys/arm/freescale/imx/imx6_hdmi.c
97

Why not uint8_t?

119–120

Why the cast?

158–160

Is the todo to get it working on something other than DVI mode?

371–372

Extra newline

437–439

What are these magic numbers?

651

You're setting this here and on line 676.

654

Why track this if you pass 0 to bus_release_resource?

724

return (hdmi_edid_read(...));

sys/arm/freescale/imx/imx6_hdmireg.h
30

#ifndef should have a space (I have no idea why the style is different between this and #define).

gonzo updated this revision to Diff 11183.Dec 12 2015, 8:38 PM
gonzo removed rS FreeBSD src repository as the repository for this revision.

Address Andrew's comments

gonzo added a comment.Dec 12 2015, 8:41 PM

Rest of the comments were addressed in new revision

sys/arm/freescale/imx/imx6_hdmi.c
158–160

Yes, for now it's only DVI mode. Next stage is going to be adding more real HDMI stuff

437–439

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.

ian added inline comments.Dec 13 2015, 1:05 AM
sys/arm/freescale/imx/imx6_hdmi.c
101

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

missing empty line

603

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

return (hdmi_edid_read(device_get_softc(dev), ...) :)

sys/arm/freescale/imx/imx6_hdmireg.h
68

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.

mmel added a subscriber: mmel.Dec 13 2015, 2:11 PM
mmel added inline comments.
sys/arm/freescale/imx/imx_i2c.c
321

Right. And registration of ofw_iicbus was added in r292157.

gonzo updated this revision to Diff 11402.Dec 18 2015, 1:18 AM

Address latest round of review comments

gonzo added inline comments.Dec 18 2015, 1:20 AM
sys/arm/freescale/imx/imx6_hdmi.c
605

iicbus_transfer_excl works only for devices that are immediate children of iicbus, so I replaced it with iic_require_bus et al.

sys/arm/freescale/imx/imx_i2c.c
321

Thanks for committing it upstream, I removed this workaround

andrew accepted this revision.Dec 18 2015, 8:00 AM
andrew edited edge metadata.
andrew added inline comments.
sys/arm/freescale/imx/imx6_hdmi.c
158–160

Ok, the comment make it sound like DVI still needs to be done.

619–620

Extra newline.

This revision is now accepted and ready to land.Dec 18 2015, 8:00 AM
This revision was automatically updated to reflect the committed changes.