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

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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
630–634 ↗(On Diff #10223)

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

sys/arm/freescale/imx/imx6_hdmireg.h
67 ↗(On Diff #10223)

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
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).

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 ↗(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.

ian added inline comments.Dec 13 2015, 1:05 AM
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.

mmel added a subscriber: mmel.Dec 13 2015, 2:11 PM
mmel added inline comments.
sys/arm/freescale/imx/imx_i2c.c
321 ↗(On Diff #11183)

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
604 ↗(On Diff #11402)

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 ↗(On Diff #11183)

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 ↗(On Diff #11183)

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

618–619 ↗(On Diff #11402)

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.