Hi Jose,
(CC'ing Archit)
On Friday 03 Mar 2017 15:57:57 Jose Abreu wrote:
On 02-03-2017 15:38, Laurent Pinchart wrote:
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_opsThat 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).
Thank you !
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).
I've posted a patch for that.
Side note: I think you missed in the cc Archit Taneja
Oops, indeed :-/ Fixed on this patch, and I'll make sure to CC him on v5.