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

[Parthiban.Veerasooran]

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.
> 
>>
>> 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?
> 
> 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.
> 
>> +
>> +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.

Best Regards,
Parthiban V
> 
>> +            local-mac-address = [04 05 06 01 02 03];
>> +            oa-chunk-size = <64>;
>> +            oa-tx-cut-through;
>> +            oa-rx-cut-through;
>> +            oa-protected;
> 
> 
> 
> Best regards,
> Krzysztof
>