Re: [PATCH 2/4] dt-bindings: net: Add onsemi NCN26010 ethernet controller

[Krzysztof Kozlowski <krzk@kernel.org>]

On 18/09/2023 08:58, Jay Monkman wrote:
> 

Drop stray blank line.

> Add devicetree bindings for onsemi's NCN26010 10BASE-T1S
> ethernet controller.

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC (and consider --no-git-fallback argument). It might
happen, that command when run on an older kernel, gives you outdated
entries. Therefore please be sure you base your patches on recent Linux
kernel.

> 
> Signed-off-by: Jay Monkman <jtm@lopingdog.com>
> ---
>  .../devicetree/bindings/net/onnn,macphy.yaml  | 94 +++++++++++++++++++
>  1 file changed, 94 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/onnn,macphy.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/onnn,macphy.yaml b/Documentation/devicetree/bindings/net/onnn,macphy.yaml
> new file mode 100644
> index 000000000000..1813da81b95f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/onnn,macphy.yaml
> @@ -0,0 +1,94 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/onnn,macphy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: onsemi 10BASE-T1S Ethernet Controller
> +
> +maintainers:
> +  - Jay Monkman <jtm@lopingdog.com>
> +
> +description: |

Do not need '|' unless you need to preserve formatting.

> +  Bindings for onsemi 10BASE-T1S ethernet controller.

Drop "bindings for" and instead describe the hardware.

> +
> +  Supported devices:
> +    ncn26010

Supported by what? By bindings? Or driver? Drop, instead, describe the
hardware.

> +
> +allOf:
> +  - $ref: ethernet-controller.yaml#
> +
> +properties:
> +  compatible:
> +    const: onnn,ncn26010
> +
> +  ctrl-protected:
> +    description: |

Do not need '|' unless you need to preserve formatting.

Is it a generic property? Does not look like. All non-standard
properties need vendor prefix.

> +      Enables control data read/write protection mode.

I don't understand this and it is very close to duplicate the property
name. Describe what it does in the hardware, not in the driver, so it
will be justified to have it in bindings in the first place.

> +    type: boolean
> +
> +  poll-interval:

Missing units.

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      Defines how often the MACPHY thread runs in milliseconds.

thread? So Linux kthread? Nope, not a bindings property.

> +      If not specified, a default value of 5 is used.
> +
> +  tx-fcs-calc:
> +    description: |
> +      Enables driver calculation of the FCS on transmitted frames.
> +    type: boolean

Driver? Not hardware? So drop the property.

> +
> +  rx-fcs-check:
> +    description: |
> +      Enables driver checking of the FCS on received frames
> +    type: boolean

Not a bindings property.

> +
> +  interrupts:
> +    maxItems: 1
> +
> +  reg:
> +    maxItems: 1
> +
> +  mdio:
> +    $ref: mdio.yaml#
> +    unevaluatedProperties: false
> +
> +required:
> +  - compatible

Nothing else is required?

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    spi {
> +      #address-cells = <1>;
> +      #size-cells = <0>;

If this is a SPI device, then you miss spi-peripheral-props.

> +
> +      ethernet@0 {
> +        compatible = "onnn,ncn26010";
> +        reg = <0>; /* CE0 */

CE0? Isn't chip select obvious from the reg=0?


Best regards,
Krzysztof