Page MenuHomeFreeBSD

i.MX6 HDMI framer driver
ClosedPublic

Authored by gonzo on Nov 16 2015, 4:06 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 20, 3:43 AM
Unknown Object (File)
Wed, Mar 20, 12:07 AM
Unknown Object (File)
Sat, Mar 2, 4:11 PM
Unknown Object (File)
Feb 15 2024, 11:09 PM
Unknown Object (File)
Feb 15 2024, 11:09 PM
Unknown Object (File)
Feb 15 2024, 11:08 PM
Unknown Object (File)
Feb 15 2024, 11:08 PM
Unknown Object (File)
Feb 15 2024, 11:08 PM
Subscribers

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
Tests Skipped

Event Timeline

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 - subversion.
gonzo mentioned this in D4168: i.MX6 IPU drivers.
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.

  • Indentation cleanup in imx6_hdmireg.h
  • Removed unused interrupt stuff
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 removed rS FreeBSD src repository - subversion as the repository for this revision.

Address Andrew's comments

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.

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 inline comments.
sys/arm/freescale/imx/imx_i2c.c
321

Right. And registration of ofw_iicbus was added in r292157.

Address latest round of review comments

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