Hi Laurent, On 02-03-2017 15:38, Laurent Pinchart wrote: > 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. > This series is definitely a good starting point and a good improvement. We can think later about abstracting even more the phy from the controller. I think this will be a major step and will reflect better how the HW is modeled. You can add my reviewed-by in all the patches in this series in your next submission (or in the merge). Also, if you do a next submission what do you think about moving all the dw-hdmi files to a new folder called for example 'synopsys' or 'dw-hdmi'? Because we already have 5 files. I think 'synopsys' instead of 'dw-hdmi' would be better because in the future we may add support for other protocols (for example display port). Side note: I think you missed in the cc Archit Taneja Best regards, Jose Miguel Abreu _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
