Hi Jose, On Thursday 02 Mar 2017 14:50:02 Jose Abreu wrote: > On 02-03-2017 13:41, Laurent Pinchart wrote: > >> Hmm, this is kind of confusing. Why do you need a phy_ops and > >> then a separate configure_phy function? Can't we just use phy_ops > >> always? If its external phy then it would need to implement all > >> phy_ops functions. > > > > The phy_ops are indeed meant to support vendor PHYs. The .configure_phy() > > operation is meant to support Synopsys PHYs that don't comply with the > > HDMI TX MHL and 3D PHYs I2C register layout and thus need a different > > configure function, but can share the other operations with the HDMI TX > > MHL and 3D PHYs. Ideally I'd like to move that code to the dw-hdmi core, > > but at the moment I don't have enough information to decide whether the > > register layout corresponds to the standard Synopsys HDMI TX 2.0 PHY or > > if it has been modified by the vendor. Once we'll have a second SoC using > > the same PHY we should have more information to consolidate the code. Of > > course, if you can share the HDMI TX 2.0 PHY datasheet, I won't mind > > reworking the code now ;-) > > Well, if I want to keep my job I can't share the datasheet, and I > do want to keep my job :) That's so selfish :-D > As far as I know this shouldn't change much. I already used (it > was like a year ago) the dw-hdmi driver in a HDMI TX 2.0 PHY. I really wonder what exactly has been integrated in the Renesas R-Car Gen3 SoC. The PHY is certainly reported as an HDMI TX 2.0 PHY, but the registers seem different compared to the 2.0 PHY you've tested. > But I am not following your logic here, sorry: You are mentioning a > custom phy, right? Custom is probably a bad name. From what I've been told it's a standard Synopsys PHY, but I can't rule out vendor-specific customizations. > If its custom then it should implement its own phy_ops. And I don't think > that phy logic should go into core, this should all be extracted because I > really believe it will make the code easier to read. Imagine this > organization: > > Folders/Files: > drm/bridge/dw-hdmi/dw-hdmi-core.c > drm/bridge/dw-hdmi/dw-hdmi-phy-synopsys.c > drm/bridge/dw-hdmi/dw-hdmi-phy-*renesas*.c > drm/bridge/dw-hdmi/dw-hdmi-phy-something.c > Structures: > dw_hdmi_pdata > dw_hdmi_phy_pdata > dw_hdmi_phy_ops That looks good to me. > As the phy is interacted using controller we would need to pass > some callbacks to the phy, but ultimately the phy would be a > platform driver which could have its own compatible string that > would be associated with controller (not sure exactly about this > because I only used this in non-dt). We already have glue code, having to provide separate glue and PHY drivers seems a bit overkill to me. If we could get rid of glue code by adding a node for the PHY in DT that would be nice, but if we have to keep the glue then we can as well use platform data there. > This is just an idea though. I followed this logic in the RX side > and its very easy to add a new phy now, its a matter of creating > a new file, implement ops and associate it with controller. Of > course some phys will be very similar, for that we can use minor > tweaks via id detection. > > What do you think? It sounds neat overall, but I wonder whether it should really block this series, or be added on top of it :-) I think we're already moving in the right direction here. -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
