[net-next,1/2] dt-bindings: Document QCA808x PHYs

  Add Documentation for QCA808x PHYs for the additional property for the
active high LED setting and also document the LED configuration for this
PHY.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 .../devicetree/bindings/net/qca,qca808x.yaml  | 66 +++++++++++++++++++
 1 file changed, 66 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/qca,qca808x.yaml
  

On 09/12/2023 02:48, Christian Marangi wrote:
> Add Documentation for QCA808x PHYs for the additional property for the
> active high LED setting and also document the LED configuration for this
> PHY.
> 

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.

> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  .../devicetree/bindings/net/qca,qca808x.yaml  | 66 +++++++++++++++++++
>  1 file changed, 66 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/qca,qca808x.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/qca,qca808x.yaml b/Documentation/devicetree/bindings/net/qca,qca808x.yaml
> new file mode 100644
> index 000000000000..73cfff357311
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/qca,qca808x.yaml
> @@ -0,0 +1,66 @@
> +# SPDX-License-Identifier: GPL-2.0+

Dual license as checkpath and writing-bindings ask.

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/qca,qca808x.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Atheros QCA808X PHY
> +
> +maintainers:
> +  - Christian Marangi <ansuelsmth@gmail.com>
> +
> +description:
> +  Bindings for Qualcomm Atheros QCA808X PHYs

Drop "Bindings for" and then entire sentence seems not useful.

> +
> +  QCA808X PHYs can have up to 3 LEDs attached.
> +  All 3 LEDs are disabled by default.
> +  2 LEDs have dedicated pins with the 3rd LED having the
> +  double function of Interrupt LEDs/GPIO or additional LED.
> +
> +  By default this special PIN is set to LED function.
> +
> +allOf:
> +  - $ref: ethernet-phy.yaml#
> +
> +select:
> +  properties:
> +    compatible:
> +      contains:
> +        enum:
> +          - ethernet-phy-id004d.d101

I have impression that this is continuation of some other patchset...
Anyway, id004d.d101 is specific to QCA808x?

> +  required:
> +    - compatible
> +
> +properties:
> +  qca,led-active-high:
> +    description: Set all the LEDs to active high to be turned on.
> +    type: boolean


Best regards,
Krzysztof
  

On Mon, Dec 11, 2023 at 11:19:50AM +0100, Krzysztof Kozlowski wrote:
> On 09/12/2023 02:48, Christian Marangi wrote:
> > Add Documentation for QCA808x PHYs for the additional property for the
> > active high LED setting and also document the LED configuration for this
> > PHY.
> > 
> 
> 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.
>

Yes sorry.

> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >  .../devicetree/bindings/net/qca,qca808x.yaml  | 66 +++++++++++++++++++
> >  1 file changed, 66 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/net/qca,qca808x.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/net/qca,qca808x.yaml b/Documentation/devicetree/bindings/net/qca,qca808x.yaml
> > new file mode 100644
> > index 000000000000..73cfff357311
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/qca,qca808x.yaml
> > @@ -0,0 +1,66 @@
> > +# SPDX-License-Identifier: GPL-2.0+
> 
> Dual license as checkpath and writing-bindings ask.
> 

Oh didn't notice the warning.

> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/qca,qca808x.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Qualcomm Atheros QCA808X PHY
> > +
> > +maintainers:
> > +  - Christian Marangi <ansuelsmth@gmail.com>
> > +
> > +description:
> > +  Bindings for Qualcomm Atheros QCA808X PHYs
> 
> Drop "Bindings for" and then entire sentence seems not useful.
> 

Was following the pattern used for other qcom PHY. Ok will drop!

> > +
> > +  QCA808X PHYs can have up to 3 LEDs attached.
> > +  All 3 LEDs are disabled by default.
> > +  2 LEDs have dedicated pins with the 3rd LED having the
> > +  double function of Interrupt LEDs/GPIO or additional LED.
> > +
> > +  By default this special PIN is set to LED function.
> > +
> > +allOf:
> > +  - $ref: ethernet-phy.yaml#
> > +
> > +select:
> > +  properties:
> > +    compatible:
> > +      contains:
> > +        enum:
> > +          - ethernet-phy-id004d.d101
> 
> I have impression that this is continuation of some other patchset...
> Anyway, id004d.d101 is specific to QCA808x?
> 

I used enum assuming eventually more qca808x PHY will come... Yes that
ID is specific and it's the id of QCA8081. Better to use const?

> > +  required:
> > +    - compatible
> > +
> > +properties:
> > +  qca,led-active-high:
> > +    description: Set all the LEDs to active high to be turned on.
> > +    type: boolean
> 
> 
> Best regards,
> Krzysztof
>
  

> +properties:
> +  qca,led-active-high:
> +    description: Set all the LEDs to active high to be turned on.
> +    type: boolean

I would of expected active high is the default. An active low property
would make more sense. It should also be a generic property, not a
vendor property. As such, we either want the phylib core to parse it,
or the LED core.

       Andrew
  

On Mon, Dec 11, 2023 at 04:44:06PM +0100, Andrew Lunn wrote:
> > +properties:
> > +  qca,led-active-high:
> > +    description: Set all the LEDs to active high to be turned on.
> > +    type: boolean
> 
> I would of expected active high is the default. An active low property
> would make more sense. It should also be a generic property, not a
> vendor property. As such, we either want the phylib core to parse it,
> or the LED core.
>

Mhhh with a generic property and LED core or phylib handling it... How
it would work applying that setting on PHY side?

Adding the check to the set_brightness set_blink hw_control API?
  

On Mon, Dec 11, 2023 at 04:44:06PM +0100, Andrew Lunn wrote:
> > +properties:
> > +  qca,led-active-high:
> > +    description: Set all the LEDs to active high to be turned on.
> > +    type: boolean
> 
> I would of expected active high is the default. An active low property
> would make more sense. It should also be a generic property, not a
> vendor property. As such, we either want the phylib core to parse it,
> or the LED core.
>

Also sorry for the double email... Any help for the problem of the
missing link_2500 define in net-next? (merged in Lee tree?)
  

On Mon, Dec 11, 2023 at 04:49:00PM +0100, Christian Marangi wrote:
> On Mon, Dec 11, 2023 at 04:44:06PM +0100, Andrew Lunn wrote:
> > > +properties:
> > > +  qca,led-active-high:
> > > +    description: Set all the LEDs to active high to be turned on.
> > > +    type: boolean
> > 
> > I would of expected active high is the default. An active low property
> > would make more sense. It should also be a generic property, not a
> > vendor property. As such, we either want the phylib core to parse it,
> > or the LED core.
> >
> 
> Also sorry for the double email... Any help for the problem of the
> missing link_2500 define in net-next? (merged in Lee tree?)

You need to email Lee and Jakub, ask for a stable branch which can be
pulled into net-next.

       Andrew
  

> Mhhh with a generic property and LED core or phylib handling it... How
> it would work applying that setting on PHY side?

Add a .led_set_polarity callback to the PHY driver structure?

Take a look at other LED drivers. Does anything similar already exist?
It is unlikely that PHYs are the only sort of LED to have a polarity.

   Andrew
  

On Mon, Dec 11, 2023 at 04:51:56PM +0100, Andrew Lunn wrote:
> On Mon, Dec 11, 2023 at 04:49:00PM +0100, Christian Marangi wrote:
> > On Mon, Dec 11, 2023 at 04:44:06PM +0100, Andrew Lunn wrote:
> > > > +properties:
> > > > +  qca,led-active-high:
> > > > +    description: Set all the LEDs to active high to be turned on.
> > > > +    type: boolean
> > > 
> > > I would of expected active high is the default. An active low property
> > > would make more sense. It should also be a generic property, not a
> > > vendor property. As such, we either want the phylib core to parse it,
> > > or the LED core.
> > >
> > 
> > Also sorry for the double email... Any help for the problem of the
> > missing link_2500 define in net-next? (merged in Lee tree?)
> 
> You need to email Lee and Jakub, ask for a stable branch which can be
> pulled into net-next.
>

Thanks I sent a followup email to the merged series.
  

On 11/12/2023 13:18, Christian Marangi wrote:
>>> +  QCA808X PHYs can have up to 3 LEDs attached.
>>> +  All 3 LEDs are disabled by default.
>>> +  2 LEDs have dedicated pins with the 3rd LED having the
>>> +  double function of Interrupt LEDs/GPIO or additional LED.
>>> +
>>> +  By default this special PIN is set to LED function.
>>> +
>>> +allOf:
>>> +  - $ref: ethernet-phy.yaml#
>>> +
>>> +select:
>>> +  properties:
>>> +    compatible:
>>> +      contains:
>>> +        enum:
>>> +          - ethernet-phy-id004d.d101
>>
>> I have impression that this is continuation of some other patchset...
>> Anyway, id004d.d101 is specific to QCA808x?
>>
> 
> I used enum assuming eventually more qca808x PHY will come... Yes that
> ID is specific and it's the id of QCA8081. Better to use const?

No, it is fine. I just wanted to be sure that this will not be matched
by other bindings.

Best regards,
Krzysztof
  

On Mon, Dec 11, 2023 at 04:54:12PM +0100, Andrew Lunn wrote:
> > Mhhh with a generic property and LED core or phylib handling it... How
> > it would work applying that setting on PHY side?
> 
> Add a .led_set_polarity callback to the PHY driver structure?
> 
> Take a look at other LED drivers. Does anything similar already exist?
> It is unlikely that PHYs are the only sort of LED to have a polarity.
>

Interesting topic... With a quick grep on Documentation for polarity of
high, I can't find any use of it...

Also main problem is that the thing is controlled globally and not per
LED. (can be handled internally to the driver with some priv and check
magic)

Is it worth to impemement the additional API to control this? And I
guess a egenric binding should be added to ethernet-phy? Or should it be
added to LEDs?
  

On Mon, Dec 11, 2023 at 06:19:46PM +0100, Christian Marangi wrote:
> On Mon, Dec 11, 2023 at 04:54:12PM +0100, Andrew Lunn wrote:
> > > Mhhh with a generic property and LED core or phylib handling it... How
> > > it would work applying that setting on PHY side?
> > 
> > Add a .led_set_polarity callback to the PHY driver structure?
> > 
> > Take a look at other LED drivers. Does anything similar already exist?
> > It is unlikely that PHYs are the only sort of LED to have a polarity.
> >
> 
> Interesting topic... With a quick grep on Documentation for polarity of
> high, I can't find any use of it...

As i said, active-high is the default. So there is no need to specify
it. But if you look in Documentation/devicetree/binding/leds for
'active-low' you will find a few examples.

> Also main problem is that the thing is controlled globally and not per
> LED. (can be handled internally to the driver with some priv and check
> magic)

Ah, missed that. Marvell PHYs have polarity per LED.

It would be better to describe this correctly, so one property at a
higher level. We can then in the future add an 'active-low' property
per PHY.

> Is it worth to impemement the additional API to control this? And I
> guess a egenric binding should be added to ethernet-phy? Or should it be
> added to LEDs?

Since it is above individual LEDs, i would not add it to the generic
LED binding. But it could go inside the leds object of
ethernet-phy.yaml.

           leds {
                #address-cells = <1>;
                #size-cells = <0>;

		active-low;

                led@0 {
                    reg = <0>;
                    color = <LED_COLOR_ID_WHITE>;
                    function = LED_FUNCTION_LAN;
                    default-state = "keep";
                };

	Andrew
  

On Mon, Dec 11, 2023 at 06:49:02PM +0100, Andrew Lunn wrote:
> On Mon, Dec 11, 2023 at 06:19:46PM +0100, Christian Marangi wrote:
> > On Mon, Dec 11, 2023 at 04:54:12PM +0100, Andrew Lunn wrote:
> > > > Mhhh with a generic property and LED core or phylib handling it... How
> > > > it would work applying that setting on PHY side?
> > > 
> > > Add a .led_set_polarity callback to the PHY driver structure?
> > > 
> > > Take a look at other LED drivers. Does anything similar already exist?
> > > It is unlikely that PHYs are the only sort of LED to have a polarity.
> > >
> > 
> > Interesting topic... With a quick grep on Documentation for polarity of
> > high, I can't find any use of it...
> 
> As i said, active-high is the default. So there is no need to specify
> it. But if you look in Documentation/devicetree/binding/leds for
> 'active-low' you will find a few examples.
>

Yes I was searching more and I just notice active-low and led-active-low
usage for bcm6358.

> > Also main problem is that the thing is controlled globally and not per
> > LED. (can be handled internally to the driver with some priv and check
> > magic)
> 
> Ah, missed that. Marvell PHYs have polarity per LED.
> 
> It would be better to describe this correctly, so one property at a
> higher level. We can then in the future add an 'active-low' property
> per PHY.
> 
> > Is it worth to impemement the additional API to control this? And I
> > guess a egenric binding should be added to ethernet-phy? Or should it be
> > added to LEDs?
> 
> Since it is above individual LEDs, i would not add it to the generic
> LED binding. But it could go inside the leds object of
> ethernet-phy.yaml.
> 
>            leds {
>                 #address-cells = <1>;
>                 #size-cells = <0>;
> 
> 		active-low;
> 
>                 led@0 {
>                     reg = <0>;
>                     color = <LED_COLOR_ID_WHITE>;
>                     function = LED_FUNCTION_LAN;
>                     default-state = "keep";
>                 };
>

Ok! And I guess the additional API will (initially to be later expanded
for other usage?) take this value and call the set polarity based on
this correct? 

bool active_low = of_property_read_bool(leds_node, "active-low");

.led_set_polarity(struct phy_device *phydev, bool active_low);


Maybe a more flexible approach might be scan for both. (either in leds
node or in the led subnode)

.led_set_polarity(struct phy_device *phydev, int index, bool active_low);

Where index is -1 if it's global and the led index if it's in the led
node?

PHY driver will know what to ignore/use as I can't immagine to have a
PHY that have both global and per LED polarity. What do you think?
  

On Sat, Dec 09, 2023 at 02:48:27AM +0100, Christian Marangi wrote:
> Add Documentation for QCA808x PHYs for the additional property for the
> active high LED setting and also document the LED configuration for this
> PHY.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  .../devicetree/bindings/net/qca,qca808x.yaml  | 66 +++++++++++++++++++
>  1 file changed, 66 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/qca,qca808x.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/qca,qca808x.yaml b/Documentation/devicetree/bindings/net/qca,qca808x.yaml
> new file mode 100644
> index 000000000000..73cfff357311
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/qca,qca808x.yaml
> @@ -0,0 +1,66 @@
> +# SPDX-License-Identifier: GPL-2.0+

Dual license as checkpatch.pl points out.

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/qca,qca808x.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Atheros QCA808X PHY
> +
> +maintainers:
> +  - Christian Marangi <ansuelsmth@gmail.com>
> +
> +description:
> +  Bindings for Qualcomm Atheros QCA808X PHYs
> +
> +  QCA808X PHYs can have up to 3 LEDs attached.
> +  All 3 LEDs are disabled by default.
> +  2 LEDs have dedicated pins with the 3rd LED having the
> +  double function of Interrupt LEDs/GPIO or additional LED.
> +
> +  By default this special PIN is set to LED function.
> +
> +allOf:
> +  - $ref: ethernet-phy.yaml#
> +
> +select:
> +  properties:
> +    compatible:
> +      contains:
> +        enum:
> +          - ethernet-phy-id004d.d101

Move this to properties and drop the select.

> +  required:
> +    - compatible
> +
> +properties:
> +  qca,led-active-high:
> +    description: Set all the LEDs to active high to be turned on.
> +    type: boolean
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/leds/common.h>
> +
> +    mdio {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        ethernet-phy@0 {
> +            compatible = "ethernet-phy-id004d.d101";
> +            reg = <0>;
> +            qca,led-active-high;
> +
> +            leds {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +
> +                led@0 {
> +                    reg = <0>;
> +                    color = <LED_COLOR_ID_GREEN>;
> +                    function = LED_FUNCTION_WAN;
> +                    default-state = "keep";
> +                };
> +            };
> +        };
> +    };
> -- 
> 2.40.1
>