Date:   Tue, 16 May 2023 14:31:13 -0700
From:   Kevin Hilman <khilman@...libre.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Julien Stephan <jstephan@...libre.com>
Cc:     robh@...nel.org, chunkuang.hu@...nel.org,
        linux-mediatek@...ts.infradead.org,
        Florian Sylvestre <fsylvestre@...libre.com>,
        Chunfeng Yun <chunfeng.yun@...iatek.com>,
        Andy Hsieh <andy.hsieh@...iatek.com>,
        Vinod Koul <vkoul@...nel.org>,
        Kishon Vijay Abraham I <kishon@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        "moderated list:ARM/Mediatek USB3 PHY DRIVER" 
        <linux-arm-kernel@...ts.infradead.org>,
        "open list:GENERIC PHY FRAMEWORK" <linux-phy@...ts.infradead.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/2] dt-bindings: phy: add mediatek mipi csi driver v
 0.5

Hi Krzysztof, 

Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org> writes:

> On 16/05/2023 19:00, Kevin Hilman wrote:
>>>>>> +  compatible:
>>>>>> +    enum:
>>>>>> +      - mediatek,phy-mipi-csi-0-5
>>>>>
>>>>> SoC based compatibles. 0-5 is odd.
>>>>>
>>>>>> +
>>>>>> +  reg:
>>>>>> +    maxItems: 1
>>>>>> +
>>>>>> +  '#phy-cells':
>>>>>> +    const: 0
>>>>>> +
>>>>>> +  mediatek,is_cdphy:
>>>>>
>>>>> No underscores in node names.
>>>>>
>>>>>> +    description:
>>>>>> +      Specify if the current phy support CDPHY configuration
>>>>>
>>>>> Why this cannot be implied from compatible? Add specific compatibles.
>>>>>
>>>>>
>>>> This cannot be implied by compatible because the number of phys depends
>>>> on the soc and each phy can be either D-PHY only or CD-PHY capable.
>>>> For example mt8365 has 2 phy: CSI0 and CSI1. CSI1 is DPHY only and CSI0 is CD-PHY
>>>
>>> So it is SoC specific so why it cannot be implied by compatible? I don't
>>> understand. You will have SoC specific compatibles, right? or you just
>>> ignored my comments here?
>> 
>> Julien, I think you had SoC specific compatibles in an earlier version
>> but then changed it to be generic based on reviewer feedback.  However,
>> that earlier version of the driver was trying to do a bunch of SoC
>> specific logic internally and support multiple SoCs.  You've now greatly
>> simplified the driver, with only a few SoC specific decisions needed.
>> These can be implied by the driver based SoC specific compatible, as
>> Krzysztof suggests, so you should just go back to having SoC specific
>> compatibles.
>> 
>
> Yes. If there is common part, e.g. several SoCs use the same device with
> same programming model, then the generic recommendation is to have
> SoC-based fallback (used also in the driver) and SoC-specific compatibles.
>
> Second accepted solution is to have generic fallback which does not use
> SoC in the compatible (and of course mandatory SoC-specific comaptibles).
>
> Third is to use versioned IP blocks.
>
> The second case also would work, if it is applicable to you (you really
> have fallback matching all devices). Third solution depends on your
> versioning and Rob expressed dislike about it many times.
>
> We had many discussions on mailing lists, thus simplifying the review -
> I recommend the first choice. For a better recommendation you should say
> a bit more about the block in different SoCs.

I'll try to say a bit more about the PHY block, but in fact, it's not
just about differences between SoCs. On the same SoC, 2 different PHYs
may have different features/capabilities.

For example, on MT8365, There are 2 PHYs: CSI0 and CSI1.  CSI0 can
function as a C-PHY or a D-PHY, but CSI1 can only function as D-PHY
(used as the example in the binding patch[1].)  On another related SoC,
there are 3 PHYs, where CSI0 is C-D but CSI1 & CSI2 are only D.

So that's why it seems (at least to me) that while we need SoC
compatible, it's not enough.  We also need properties to describe
PHY-specific features (e.g. C-D PHY)

Of course, we could rely only on SoC-specific compatibles describe this.
But then driver will need an SoC-specific table with the number of PHYs
and per-PHY features for each SoC encoded in the driver.  Since the
driver otherwise doesn't (and shouldn't, IMHO) need to know how many
PHYs are on each SoC, I suggested to Julien that perhaps the additional
propery was the better solution.  

To me it seems redundant to have the driver encode PHYs-per-SoC info,
when the per-SoC DT is going to have the same info, so my suggestion was
to simplify the driver and have this kind of hardware description in the
DT, and keep the driver simple, but we are definitely open to learning
the "right way" of doing this.

Thanks for your review and guidance,

Kevin

[1] https://lore.kernel.org/linux-mediatek/20230515090551.1251389-2-jstephan@baylibre.com/

Powered by blists - more mailing lists