Message-ID: <dbbe3cd2-3329-d267-338b-8e513209ddcd@linaro.org>
Date:   Wed, 8 Mar 2023 13:34:25 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH net-next v2] dt-bindings: net: ti: k3-am654-cpsw-nuss:
 Document Serdes PHY
Content-Language: en-US
To:     Siddharth Vadapalli <s-vadapalli@ti.com>,
        krzysztof.kozlowski+dt@linaro.org
Cc:     davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        linux@armlinux.org.uk, pabeni@redhat.com, robh+dt@kernel.org,
        nsekhar@ti.com, rogerq@kernel.org, netdev@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, srk@ti.com
References: <20230308051835.276552-1-s-vadapalli@ti.com>
 <1ffed720-322c-fa73-1160-5fd73ce3c7c2@linaro.org>
 <7b6e8131-8e5b-88bc-69f7-b737c0c35bb6@ti.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <7b6e8131-8e5b-88bc-69f7-b737c0c35bb6@ti.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@ti.com>
>>> ---
>>>
>>> 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@ti.com/
>>> This was pointed out at:
>>> https://lore.kernel.org/r/CAMuHMdW5atq-FuLEL3htuE3t2uO86anLL3zeY7n1RqqMP_rH1g@mail.gmail.com/
>>>
>>> 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@ti.com/
>>>
>>>  .../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?

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"?

Best regards,
Krzysztof