Re: [PATCH 2/5] dt-bindings: net: Add MTIP L2 switch description (fec,mtip-switch.yaml)

[Krzysztof Kozlowski <krzk@kernel.org>]

On 25/03/2025 13:15, Lukasz Majewski wrote:
> Hi Krzysztof,
> 
>> On 25/03/2025 12:57, Lukasz Majewski wrote:
>>> This patch provides description of the MTIP L2 switch available in
>>> some NXP's SOCs - imx287, vf610.
>>>
>>> Signed-off-by: Lukasz Majewski <lukma@denx.de>
>>> ---
>>>  .../bindings/net/fec,mtip-switch.yaml         | 160
>>> ++++++++++++++++++  
>>
>> Use compatible as filename.
> 
> I've followed the fsl,fec.yaml as an example. This file has description
> for all the device tree sources from fec_main.c


That's a 14 year old binding, so clear antipattern.

> 
> I've considered adding the full name - e.g. fec,imx287-mtip-switch.yaml
> but this driver could (and probably will) be extended to vf610.

Unless you add vf610 now, this should follow the compatible name.

> 
> So what is the advised way to go?
> 
>>
>>>  1 file changed, 160 insertions(+)
>>>  create mode 100644
>>> Documentation/devicetree/bindings/net/fec,mtip-switch.yaml
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/net/fec,mtip-switch.yaml
>>> b/Documentation/devicetree/bindings/net/fec,mtip-switch.yaml new
>>> file mode 100644 index 000000000000..cd85385e0f79 --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/fec,mtip-switch.yaml
>>> @@ -0,0 +1,160 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/net/fsl,mtip-switch.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Freescale MTIP Level 2 (L2) switch
>>> +
>>> +maintainers:
>>> +  - Lukasz Majewski <lukma@denx.de>
>>> +  
>>
>> description?
> 
> Ok.
> 
>>
>>> +allOf:
>>> +  - $ref: ethernet-controller.yaml#
>>> +
>>> +properties:
>>> +  compatible:
>>> +    oneOf:  
>>
>> Drop, you have only one variant.
> 
> Ok, for imx287 this can be dropped, and then extended with vf610.
> 
>>
>>> +      - enum:
>>> +	  - imx287-mtip-switch  
>>
>> This wasn't tested. Except whitespace errors, above compatible does
>> not have format of compatible. Please look at other NXP bindings.
>>
>> Missing blank line.
>>
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  interrupts:
>>> +    maxItems: 3  
>>
>> Need to list items instead.
>>
>>> +
>>> +  clocks:
>>> +    maxItems: 4
>>> +    description:
>>> +      The "ipg", for MAC ipg_clk_s, ipg_clk_mac_s that are for
>>> register accessing.
>>> +      The "ahb", for MAC ipg_clk, ipg_clk_mac that are bus clock.
>>> +      The "ptp"(option), for IEEE1588 timer clock that requires
>>> the clock.
>>> +      The "enet_out"(option), output clock for external device,
>>> like supply clock
>>> +      for PHY. The clock is required if PHY clock source from SOC.
>>>  
>>
>> Same problems. This binding does not look at all as any other
>> binding. I finish review here, but the code has similar trivial
>> issues all the way, including incorrect indentation. Start from well
>> reviewed existing binding or example-schema.
> 
> As I've stated above - this code is reduced copy of fsl,fec.yaml...

Don't take the worst, old code with all the anti-patterns we point out
on each review, as an example.

Take the most recent, well reviewed binding as an example. Or
example-schema.

Best regards,
Krzysztof