Re: [RFC PATCH net-next 6/6] microchip: lan865x: add device-tree support for Microchip's LAN865X MACPHY

[Parthiban.Veerasooran]

Hi Krzysztof,

On 12/09/23 6:47 pm, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 12/09/2023 14:15, Parthiban.Veerasooran@xxxxxxxxxxxxx wrote:
>> Hi Krzysztof,
>>
>> Thank you for reviewing the patch.
>>
>> On 10/09/23 4:25 pm, Krzysztof Kozlowski wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 08/09/2023 16:29, Parthiban Veerasooran wrote:
>>>> Add device-tree support for Microchip's LAN865X MACPHY for configuring
>>>> the OPEN Alliance 10BASE-T1x MACPHY Serial Interface parameters.
>>>
>>> Please use subject prefixes matching the subsystem. You can get them for
>>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>>> your patch is touching.
>> Ok sure, so it will become like,
>>
>> dt-bindings: net: add device-tree support for Microchip's LAN865X MACPHY
>>
>> I will correct it in the next revision.
> 
> "device-tree support for " is redundant, drop
Ah ok will do that.
> 
>>>
>>>>
>>>> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@xxxxxxxxxxxxx>
>>>> ---
>>>>    .../bindings/net/microchip,lan865x.yaml       | 54 +++++++++++++++++++
>>>>    MAINTAINERS                                   |  1 +
>>>>    2 files changed, 55 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/net/microchip,lan865x.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/microchip,lan865x.yaml b/Documentation/devicetree/bindings/net/microchip,lan865x.yaml
>>>> new file mode 100644
>>>> index 000000000000..3465b2c97690
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/net/microchip,lan865x.yaml
>>>> @@ -0,0 +1,54 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/net/microchip,lan865x.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Microchip LAN8650/1 10BASE-T1S MACPHY Ethernet Controllers
>>>> +
>>>> +maintainers:
>>>> +  - Parthiban Veerasooran <parthiban.veerasooran@xxxxxxxxxxxxx>
>>>> +
>>>> +description: |
>>>> +  Device tree properties for LAN8650/1 10BASE-T1S MACPHY Ethernet
>>>
>>> Drop "Device tree properties for" and instead describe the hardware.
>> sure, will do it.
>>>
>>>> +  controller.
>>>> +
>>>> +allOf:
>>>> +  - $ref: ethernet-controller.yaml#
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    items:
>>>
>>> No need for items. Just enum.
>> Ok noted.
>>>
>>>
>>>> +      - enum:
>>>> +          - microchip,lan865x
>>>
>>> No wildcards in compatibles.
>> Yes then we don't need enum also isn't it?
> 
> I don't see correlation between these two. Please read the writing
> bindings guidelines.
Ok, will check it out.
> 
> 
>>>
>>> Missing blank line.
>> Ok will add it.
>>>
>>>
>>>
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  local-mac-address: true
>>>> +  oa-chunk-size: true
>>>> +  oa-tx-cut-through: true
>>>> +  oa-rx-cut-through: true
>>>> +  oa-protected: true
>>>
>>> What are all these? Where are they defined that you skip description,
>>> type and vendor prefix?
>> Ok missed it. Will do it in the next revision.
> 
> No, drop them or explain why they are hardware properties.
Will separate hardware specific and OA specific properties.
> 
>>>
>>>> +
>>>> +required:
>>>> +  - compatible
>>>> +  - reg
>>>> +
>>>> +additionalProperties: false
>>>> +
>>>> +examples:
>>>> +  - |
>>>> +    spi {
>>>> +        #address-cells = <1>;
>>>> +        #size-cells = <0>;
>>>> +
>>>> +        ethernet@1{
>>>
>>> Missing space
>> Ok will add it.
>>>
>>>> +            compatible = "microchip,lan865x";
>>>> +            reg = <1>; /* CE0 */
>>>
>>> CE0? chip-select? What does this comment mean in this context?
>> Yes it is chip-select. Will add proper comment.
> 
> Why? isn't reg obvious?
Sorry, yes it is reg. The comment is wrong. Will remove it.

Best Regards,
Parthiban V
> 
> Best regards,
> Krzysztof
> 
>