Re: [PATCH net-next v2 1/2] dt-bindings: net: dp83822: Add support for GPIO2 clock output

[Dimitri Fedrau]

Am Wed, Dec 11, 2024 at 10:43:40AM +0100 schrieb Krzysztof Kozlowski:
> On Wed, Dec 11, 2024 at 09:04:39AM +0100, Dimitri Fedrau wrote:
> > The GPIO2 pin on the DP83822 can be configured as clock output. Add
> > binding to support this feature.
> > 
> > Signed-off-by: Dimitri Fedrau <dimitri.fedrau@xxxxxxxxxxxx>
> > ---
> >  .../devicetree/bindings/net/ti,dp83822.yaml         |  7 +++++++
> >  include/dt-bindings/net/ti-dp83822.h                | 21 +++++++++++++++++++++
> >  2 files changed, 28 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/ti,dp83822.yaml b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
> > index 784866ea392b2083e93d8dc9aaea93b70dc80934..4a4dc794f21162c6a61c3daeeffa08e666034679 100644
> > --- a/Documentation/devicetree/bindings/net/ti,dp83822.yaml
> > +++ b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
> > @@ -96,6 +96,13 @@ properties:
> >        - master
> >        - slave
> >  
> > +  ti,gpio2-clk-out:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: |
> > +       DP83822 PHY only.
> > +       Muxing option for GPIO2 pin. See dt-bindings/net/ti-dp83822.h for
> > +       applicable values. When omitted, the PHY's default will be left as is.
> 
> 1. Missing constraints, this looks like enum.
> 2. Missing explanation of values.
> 3. This should be most likely a string.
> 4. Extend your example with this. 
>
Ok, will fix it.

> > +
> >  required:
> >    - reg
> >  
> > diff --git a/include/dt-bindings/net/ti-dp83822.h b/include/dt-bindings/net/ti-dp83822.h
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..d569c90618b7bcae9ffe44eb041f7dae2e74e5d1
> > --- /dev/null
> > +++ b/include/dt-bindings/net/ti-dp83822.h
> > @@ -0,0 +1,21 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only OR MIT */
> > +/*
> > + * Device Tree constants for the Texas Instruments DP83822 PHY
> > + *
> > + * Copyright (C) 2024 Liebherr-Electronics and Drives GmbH
> > + *
> > + * Author: Dimitri Fedrau <dimitri.fedrau@xxxxxxxxxxxx>
> > + */
> > +
> > +#ifndef _DT_BINDINGS_TI_DP83822_H
> > +#define _DT_BINDINGS_TI_DP83822_H
> > +
> > +/* IO_MUX_GPIO_CTRL - Clock source selection */
> > +#define DP83822_CLK_SRC_MAC_IF			0x0
> > +#define DP83822_CLK_SRC_XI			0x1
> > +#define DP83822_CLK_SRC_INT_REF			0x2
> > +#define DP83822_CLK_SRC_RMII_MASTER_MODE_REF	0x4
> > +#define DP83822_CLK_SRC_FREE_RUNNING		0x6
> > +#define DP83822_CLK_SRC_RECOVERED		0x7
> 
> These are not really bindings but some register values. Hex numbers
> indicate that. Don't store register values as bindings, because this
> is neither necessary nor helping.
>
Ok, got it. Have seen similar in <dt-bindings/net/ti-dp83867.h> or
<dt-bindings/net/ti-dp83869.h>, is it wrong there ?

Best regards,
Dimitri Fedrau