[PATCH v4 7/9] drm: bridge: dw-hdmi: Add support for custom PHY configuration

[Jose Abreu]

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