Re: [PATCH net-next v2] dt-bindings: net: ti: k3-am654-cpsw-nuss: Document Serdes PHY

[Siddharth Vadapalli]

Hello Krzysztof,

On 08/03/23 18:04, Krzysztof Kozlowski wrote:
> On 08/03/2023 09:38, Siddharth Vadapalli wrote:
>> Hello Krzysztof,
>>
>> On 08/03/23 14:04, Krzysztof Kozlowski wrote:
>>> On 08/03/2023 06:18, Siddharth Vadapalli wrote:
>>>> Update bindings to include Serdes PHY as an optional PHY, in addition to
>>>> the existing CPSW MAC's PHY. The CPSW MAC's PHY is required while the
>>>> Serdes PHY is optional. The Serdes PHY handle has to be provided only
>>>> when the Serdes is being configured in a Single-Link protocol. Using the
>>>> name "serdes-phy" to represent the Serdes PHY handle, the am65-cpsw-nuss
>>>> driver can obtain the Serdes PHY and request the Serdes to be
>>>> configured.
>>>>
>>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@xxxxxx>
>>>> ---
>>>>
>>>> Hello,
>>>>
>>>> This patch corresponds to the Serdes PHY bindings that were missed out in
>>>> the series at:
>>>> https://lore.kernel.org/r/20230104103432.1126403-1-s-vadapalli@xxxxxx/
>>>> This was pointed out at:
>>>> https://lore.kernel.org/r/CAMuHMdW5atq-FuLEL3htuE3t2uO86anLL3zeY7n1RqqMP_rH1g@xxxxxxxxxxxxxx/
>>>>
>>>> Changes from v1:
>>>> 1. Describe phys property with minItems, items and description.
>>>> 2. Use minItems and items in phy-names.
>>>> 3. Remove the description in phy-names.
>>>>
>>>> v1:
>>>> https://lore.kernel.org/r/20230306094750.159657-1-s-vadapalli@xxxxxx/
>>>>
>>>>  .../bindings/net/ti,k3-am654-cpsw-nuss.yaml        | 14 ++++++++++++--
>>>>  1 file changed, 12 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml b/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml
>>>> index 900063411a20..0fb48bb6a041 100644
>>>> --- a/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml
>>>> +++ b/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml
>>>> @@ -126,8 +126,18 @@ properties:
>>>>              description: CPSW port number
>>>>  
>>>>            phys:
>>>> -            maxItems: 1
>>>> -            description: phandle on phy-gmii-sel PHY
>>>> +            minItems: 1
>>>> +            items:
>>>> +              - description: CPSW MAC's PHY.
>>>> +              - description: Serdes PHY. Serdes PHY is required only if
>>>> +                             the Serdes has to be configured in the
>>>> +                             Single-Link configuration.
>>>> +
>>>> +          phy-names:
>>>> +            minItems: 1
>>>> +            items:
>>>> +              - const: mac-phy
>>>> +              - const: serdes-phy
>>>
>>> Drop "phy" suffixes.
>>
>> The am65-cpsw driver fetches the Serdes PHY by looking for the string
>> "serdes-phy". Therefore, modifying the string will require changing the driver's
>> code as well. Please let me know if it is absolutely necessary to drop the phy
>> suffix. If so, I will post a new series with the changes involving dt-bindings
>> changes and the driver changes.
> 
> Why the driver uses some properties before adding them to the binding?

I missed adding the bindings for the Serdes PHY as a part of the series
mentioned in the section below the tearline of the patch. With this patch, I am
attempting to fix it.

> 
> And is it correct method of adding ABI? You add incorrect properties
> without documentation and then use this as an argument "driver already
> does it"?

I apologize if my earlier comment appeared to justify the usage of "serdes-phy"
based on the driver already using it. I did not mean it in that sense. I simply
meant to ask if dropping "phy" suffixes was a suggestion or a rule. In that
context, I felt that if it was a suggestion, I would prefer retaining the names
with the "phy" suffixes, since the driver is already using it. Additionally, I
also mentioned in my earlier comment that if it is necessary to drop the "phy"
suffix, then I will do so and add another patch to change the string the driver
looks for as well.

I shall take it that dropping "phy" suffixes is a rule/necessity. With this, I
will post the v3 series making this change, along with the patch to update the
string the driver looks for.

Regards,
Siddharth.