[v2,6/7] dt-bindings: net: realtek: Add property to configure LED mode
diff mbox series

Message ID 20190703193724.246854-6-mka@chromium.org
State Superseded
Headers show
Series
  • [v2,1/7] dt-bindings: net: Add bindings for Realtek PHYs
Related show

Commit Message

Matthias Kaehlcke July 3, 2019, 7:37 p.m. UTC
The LED behavior of some Realtek PHYs is configurable. Add the
property 'realtek,led-modes' to specify the configuration of the
LEDs.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v2:
- patch added to the series
---
 .../devicetree/bindings/net/realtek.txt         |  9 +++++++++
 include/dt-bindings/net/realtek.h               | 17 +++++++++++++++++
 2 files changed, 26 insertions(+)
 create mode 100644 include/dt-bindings/net/realtek.h

Comments

Andrew Lunn July 3, 2019, 8:07 p.m. UTC | #1
On Wed, Jul 03, 2019 at 12:37:23PM -0700, Matthias Kaehlcke wrote:

Hi Matthias

Maybe add a #define for 0, so we know what it does.

> +#define RTL8211E_LINK_10		1
> +#define RTL8211E_LINK_100		2
> +#define RTL8211E_LINK_1000		4
Heiner Kallweit July 3, 2019, 8:13 p.m. UTC | #2
On 03.07.2019 21:37, Matthias Kaehlcke wrote:
> The LED behavior of some Realtek PHYs is configurable. Add the
> property 'realtek,led-modes' to specify the configuration of the
> LEDs.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> Changes in v2:
> - patch added to the series
> ---
>  .../devicetree/bindings/net/realtek.txt         |  9 +++++++++
>  include/dt-bindings/net/realtek.h               | 17 +++++++++++++++++
>  2 files changed, 26 insertions(+)
>  create mode 100644 include/dt-bindings/net/realtek.h
> 
> diff --git a/Documentation/devicetree/bindings/net/realtek.txt b/Documentation/devicetree/bindings/net/realtek.txt
> index 71d386c78269..40b0d6f9ee21 100644
> --- a/Documentation/devicetree/bindings/net/realtek.txt
> +++ b/Documentation/devicetree/bindings/net/realtek.txt
> @@ -9,6 +9,12 @@ Optional properties:
>  
>  	SSC is only available on some Realtek PHYs (e.g. RTL8211E).
>  
> +- realtek,led-modes: LED mode configuration.
> +
> +	A 0..3 element vector, with each element configuring the operating
> +	mode of an LED. Omitted LEDs are turned off. Allowed values are
> +	defined in "include/dt-bindings/net/realtek.h".
> +
>  Example:
>  
>  mdio0 {
> @@ -20,5 +26,8 @@ mdio0 {
>  		reg = <1>;
>  		realtek,eee-led-mode-disable;
>  		realtek,enable-ssc;
> +		realtek,led-modes = <RTL8211E_LINK_ACTIVITY
> +				     RTL8211E_LINK_100
> +				     RTL8211E_LINK_1000>;
>  	};
>  };
> diff --git a/include/dt-bindings/net/realtek.h b/include/dt-bindings/net/realtek.h
> new file mode 100644
> index 000000000000..8d64f58d58f8
> --- /dev/null
> +++ b/include/dt-bindings/net/realtek.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _DT_BINDINGS_REALTEK_H
> +#define _DT_BINDINGS_REALTEK_H
> +
> +/* LED modes for RTL8211E PHY */
> +
> +#define RTL8211E_LINK_10		1
> +#define RTL8211E_LINK_100		2
> +#define RTL8211E_LINK_1000		4
> +#define RTL8211E_LINK_10_100		3
> +#define RTL8211E_LINK_10_1000		5
> +#define RTL8211E_LINK_100_1000		6
> +#define RTL8211E_LINK_10_100_1000	7
> +
> +#define RTL8211E_LINK_ACTIVITY		(1 << 16)

I don't see where this is used.

> +
> +#endif
>
Heiner Kallweit July 3, 2019, 8:22 p.m. UTC | #3
On 03.07.2019 22:13, Heiner Kallweit wrote:
> On 03.07.2019 21:37, Matthias Kaehlcke wrote:
>> The LED behavior of some Realtek PHYs is configurable. Add the
>> property 'realtek,led-modes' to specify the configuration of the
>> LEDs.
>>
>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>> ---
>> Changes in v2:
>> - patch added to the series
>> ---
>>  .../devicetree/bindings/net/realtek.txt         |  9 +++++++++
>>  include/dt-bindings/net/realtek.h               | 17 +++++++++++++++++
>>  2 files changed, 26 insertions(+)
>>  create mode 100644 include/dt-bindings/net/realtek.h
>>
>> diff --git a/Documentation/devicetree/bindings/net/realtek.txt b/Documentation/devicetree/bindings/net/realtek.txt
>> index 71d386c78269..40b0d6f9ee21 100644
>> --- a/Documentation/devicetree/bindings/net/realtek.txt
>> +++ b/Documentation/devicetree/bindings/net/realtek.txt
>> @@ -9,6 +9,12 @@ Optional properties:
>>  
>>  	SSC is only available on some Realtek PHYs (e.g. RTL8211E).
>>  
>> +- realtek,led-modes: LED mode configuration.
>> +
>> +	A 0..3 element vector, with each element configuring the operating
>> +	mode of an LED. Omitted LEDs are turned off. Allowed values are
>> +	defined in "include/dt-bindings/net/realtek.h".
>> +
>>  Example:
>>  
>>  mdio0 {
>> @@ -20,5 +26,8 @@ mdio0 {
>>  		reg = <1>;
>>  		realtek,eee-led-mode-disable;
>>  		realtek,enable-ssc;
>> +		realtek,led-modes = <RTL8211E_LINK_ACTIVITY
>> +				     RTL8211E_LINK_100
>> +				     RTL8211E_LINK_1000>;
>>  	};
>>  };
>> diff --git a/include/dt-bindings/net/realtek.h b/include/dt-bindings/net/realtek.h
>> new file mode 100644
>> index 000000000000..8d64f58d58f8
>> --- /dev/null
>> +++ b/include/dt-bindings/net/realtek.h
>> @@ -0,0 +1,17 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _DT_BINDINGS_REALTEK_H
>> +#define _DT_BINDINGS_REALTEK_H
>> +
>> +/* LED modes for RTL8211E PHY */
>> +
>> +#define RTL8211E_LINK_10		1
>> +#define RTL8211E_LINK_100		2
>> +#define RTL8211E_LINK_1000		4
>> +#define RTL8211E_LINK_10_100		3
>> +#define RTL8211E_LINK_10_1000		5
>> +#define RTL8211E_LINK_100_1000		6
>> +#define RTL8211E_LINK_10_100_1000	7
>> +
>> +#define RTL8211E_LINK_ACTIVITY		(1 << 16)
> 
> I don't see where this is used.
> 
Clear now, disregard my comment.

>> +
>> +#endif
>>
>
Florian Fainelli July 3, 2019, 9:37 p.m. UTC | #4
On 7/3/19 12:37 PM, Matthias Kaehlcke wrote:
> The LED behavior of some Realtek PHYs is configurable. Add the
> property 'realtek,led-modes' to specify the configuration of the
> LEDs.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> Changes in v2:
> - patch added to the series
> ---
>  .../devicetree/bindings/net/realtek.txt         |  9 +++++++++
>  include/dt-bindings/net/realtek.h               | 17 +++++++++++++++++
>  2 files changed, 26 insertions(+)
>  create mode 100644 include/dt-bindings/net/realtek.h
> 
> diff --git a/Documentation/devicetree/bindings/net/realtek.txt b/Documentation/devicetree/bindings/net/realtek.txt
> index 71d386c78269..40b0d6f9ee21 100644
> --- a/Documentation/devicetree/bindings/net/realtek.txt
> +++ b/Documentation/devicetree/bindings/net/realtek.txt
> @@ -9,6 +9,12 @@ Optional properties:
>  
>  	SSC is only available on some Realtek PHYs (e.g. RTL8211E).
>  
> +- realtek,led-modes: LED mode configuration.
> +
> +	A 0..3 element vector, with each element configuring the operating
> +	mode of an LED. Omitted LEDs are turned off. Allowed values are
> +	defined in "include/dt-bindings/net/realtek.h".

This should probably be made more general and we should define LED modes
that makes sense regardless of the PHY device, introduce a set of
generic functions for validating and then add new function pointer for
setting the LED configuration to the PHY driver. This would allow to be
more future proof where each PHY driver could expose standard LEDs class
devices to user-space, and it would also allow facilities like: ethtool
-p to plug into that.

Right now, each driver invents its own way of configuring LEDs, that
does not scale, and there is not really a good reason for that other
than reviewing drivers in isolation and therefore making it harder to
extract the commonality. Yes, I realize that since you are the latest
person submitting something in that area, you are being selected :)

> +
>  Example:
>  
>  mdio0 {
> @@ -20,5 +26,8 @@ mdio0 {
>  		reg = <1>;
>  		realtek,eee-led-mode-disable;
>  		realtek,enable-ssc;
> +		realtek,led-modes = <RTL8211E_LINK_ACTIVITY
> +				     RTL8211E_LINK_100
> +				     RTL8211E_LINK_1000>;
>  	};
>  };
> diff --git a/include/dt-bindings/net/realtek.h b/include/dt-bindings/net/realtek.h
> new file mode 100644
> index 000000000000..8d64f58d58f8
> --- /dev/null
> +++ b/include/dt-bindings/net/realtek.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _DT_BINDINGS_REALTEK_H
> +#define _DT_BINDINGS_REALTEK_H
> +
> +/* LED modes for RTL8211E PHY */
> +
> +#define RTL8211E_LINK_10		1
> +#define RTL8211E_LINK_100		2
> +#define RTL8211E_LINK_1000		4
> +#define RTL8211E_LINK_10_100		3
> +#define RTL8211E_LINK_10_1000		5
> +#define RTL8211E_LINK_100_1000		6
> +#define RTL8211E_LINK_10_100_1000	7
> +
> +#define RTL8211E_LINK_ACTIVITY		(1 << 16)
> +
> +#endif
>
Matthias Kaehlcke July 3, 2019, 11:23 p.m. UTC | #5
Hi Florian,

On Wed, Jul 03, 2019 at 02:37:47PM -0700, Florian Fainelli wrote:
> On 7/3/19 12:37 PM, Matthias Kaehlcke wrote:
> > The LED behavior of some Realtek PHYs is configurable. Add the
> > property 'realtek,led-modes' to specify the configuration of the
> > LEDs.
> > 
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > Changes in v2:
> > - patch added to the series
> > ---
> >  .../devicetree/bindings/net/realtek.txt         |  9 +++++++++
> >  include/dt-bindings/net/realtek.h               | 17 +++++++++++++++++
> >  2 files changed, 26 insertions(+)
> >  create mode 100644 include/dt-bindings/net/realtek.h
> > 
> > diff --git a/Documentation/devicetree/bindings/net/realtek.txt b/Documentation/devicetree/bindings/net/realtek.txt
> > index 71d386c78269..40b0d6f9ee21 100644
> > --- a/Documentation/devicetree/bindings/net/realtek.txt
> > +++ b/Documentation/devicetree/bindings/net/realtek.txt
> > @@ -9,6 +9,12 @@ Optional properties:
> >  
> >  	SSC is only available on some Realtek PHYs (e.g. RTL8211E).
> >  
> > +- realtek,led-modes: LED mode configuration.
> > +
> > +	A 0..3 element vector, with each element configuring the operating
> > +	mode of an LED. Omitted LEDs are turned off. Allowed values are
> > +	defined in "include/dt-bindings/net/realtek.h".
> 
> This should probably be made more general and we should define LED modes
> that makes sense regardless of the PHY device, introduce a set of
> generic functions for validating and then add new function pointer for
> setting the LED configuration to the PHY driver. This would allow to be
> more future proof where each PHY driver could expose standard LEDs class
> devices to user-space, and it would also allow facilities like: ethtool
> -p to plug into that.
> 
> Right now, each driver invents its own way of configuring LEDs, that
> does not scale, and there is not really a good reason for that other
> than reviewing drivers in isolation and therefore making it harder to
> extract the commonality. Yes, I realize that since you are the latest
> person submitting something in that area, you are being selected :)

I see the merit of your proposal to come up with a generic mechanism
to configure Ethernet LEDs, however I can't justify spending much of
my work time on this. If it is deemed useful I'm happy to send another
version of the current patchset that addresses the reviewer's comments,
but if the implementation of a generic LED configuration interface is
a requirement I will have to abandon at least the LED configuration
part of this series.
Rob Herring July 10, 2019, 3:55 p.m. UTC | #6
On Wed, Jul 3, 2019 at 5:23 PM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> Hi Florian,
>
> On Wed, Jul 03, 2019 at 02:37:47PM -0700, Florian Fainelli wrote:
> > On 7/3/19 12:37 PM, Matthias Kaehlcke wrote:
> > > The LED behavior of some Realtek PHYs is configurable. Add the
> > > property 'realtek,led-modes' to specify the configuration of the
> > > LEDs.
> > >
> > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > ---
> > > Changes in v2:
> > > - patch added to the series
> > > ---
> > >  .../devicetree/bindings/net/realtek.txt         |  9 +++++++++
> > >  include/dt-bindings/net/realtek.h               | 17 +++++++++++++++++
> > >  2 files changed, 26 insertions(+)
> > >  create mode 100644 include/dt-bindings/net/realtek.h
> > >
> > > diff --git a/Documentation/devicetree/bindings/net/realtek.txt b/Documentation/devicetree/bindings/net/realtek.txt
> > > index 71d386c78269..40b0d6f9ee21 100644
> > > --- a/Documentation/devicetree/bindings/net/realtek.txt
> > > +++ b/Documentation/devicetree/bindings/net/realtek.txt
> > > @@ -9,6 +9,12 @@ Optional properties:
> > >
> > >     SSC is only available on some Realtek PHYs (e.g. RTL8211E).
> > >
> > > +- realtek,led-modes: LED mode configuration.
> > > +
> > > +   A 0..3 element vector, with each element configuring the operating
> > > +   mode of an LED. Omitted LEDs are turned off. Allowed values are
> > > +   defined in "include/dt-bindings/net/realtek.h".
> >
> > This should probably be made more general and we should define LED modes
> > that makes sense regardless of the PHY device, introduce a set of
> > generic functions for validating and then add new function pointer for
> > setting the LED configuration to the PHY driver. This would allow to be
> > more future proof where each PHY driver could expose standard LEDs class
> > devices to user-space, and it would also allow facilities like: ethtool
> > -p to plug into that.
> >
> > Right now, each driver invents its own way of configuring LEDs, that
> > does not scale, and there is not really a good reason for that other
> > than reviewing drivers in isolation and therefore making it harder to
> > extract the commonality. Yes, I realize that since you are the latest
> > person submitting something in that area, you are being selected :)

I agree.

> I see the merit of your proposal to come up with a generic mechanism
> to configure Ethernet LEDs, however I can't justify spending much of
> my work time on this. If it is deemed useful I'm happy to send another
> version of the current patchset that addresses the reviewer's comments,
> but if the implementation of a generic LED configuration interface is
> a requirement I will have to abandon at least the LED configuration
> part of this series.

Can you at least define a common binding for this. Maybe that's just
removing 'realtek'. While the kernel side can evolve to a common
infrastructure, the DT bindings can't.

Rob
Florian Fainelli July 10, 2019, 4:28 p.m. UTC | #7
On 7/10/19 8:55 AM, Rob Herring wrote:
> On Wed, Jul 3, 2019 at 5:23 PM Matthias Kaehlcke <mka@chromium.org> wrote:
>>
>> Hi Florian,
>>
>> On Wed, Jul 03, 2019 at 02:37:47PM -0700, Florian Fainelli wrote:
>>> On 7/3/19 12:37 PM, Matthias Kaehlcke wrote:
>>>> The LED behavior of some Realtek PHYs is configurable. Add the
>>>> property 'realtek,led-modes' to specify the configuration of the
>>>> LEDs.
>>>>
>>>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>>>> ---
>>>> Changes in v2:
>>>> - patch added to the series
>>>> ---
>>>>  .../devicetree/bindings/net/realtek.txt         |  9 +++++++++
>>>>  include/dt-bindings/net/realtek.h               | 17 +++++++++++++++++
>>>>  2 files changed, 26 insertions(+)
>>>>  create mode 100644 include/dt-bindings/net/realtek.h
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/realtek.txt b/Documentation/devicetree/bindings/net/realtek.txt
>>>> index 71d386c78269..40b0d6f9ee21 100644
>>>> --- a/Documentation/devicetree/bindings/net/realtek.txt
>>>> +++ b/Documentation/devicetree/bindings/net/realtek.txt
>>>> @@ -9,6 +9,12 @@ Optional properties:
>>>>
>>>>     SSC is only available on some Realtek PHYs (e.g. RTL8211E).
>>>>
>>>> +- realtek,led-modes: LED mode configuration.
>>>> +
>>>> +   A 0..3 element vector, with each element configuring the operating
>>>> +   mode of an LED. Omitted LEDs are turned off. Allowed values are
>>>> +   defined in "include/dt-bindings/net/realtek.h".
>>>
>>> This should probably be made more general and we should define LED modes
>>> that makes sense regardless of the PHY device, introduce a set of
>>> generic functions for validating and then add new function pointer for
>>> setting the LED configuration to the PHY driver. This would allow to be
>>> more future proof where each PHY driver could expose standard LEDs class
>>> devices to user-space, and it would also allow facilities like: ethtool
>>> -p to plug into that.
>>>
>>> Right now, each driver invents its own way of configuring LEDs, that
>>> does not scale, and there is not really a good reason for that other
>>> than reviewing drivers in isolation and therefore making it harder to
>>> extract the commonality. Yes, I realize that since you are the latest
>>> person submitting something in that area, you are being selected :)
> 
> I agree.
> 
>> I see the merit of your proposal to come up with a generic mechanism
>> to configure Ethernet LEDs, however I can't justify spending much of
>> my work time on this. If it is deemed useful I'm happy to send another
>> version of the current patchset that addresses the reviewer's comments,
>> but if the implementation of a generic LED configuration interface is
>> a requirement I will have to abandon at least the LED configuration
>> part of this series.
> 
> Can you at least define a common binding for this. Maybe that's just
> removing 'realtek'. While the kernel side can evolve to a common
> infrastructure, the DT bindings can't.

That would be a great start, and that is actually what I had in mind
(should have been more specific), I was not going to have you Matthias
do the grand slam and convert all this LED configuration into the LEDs
class etc. that would not be fair.

It seems to be that we can fairly easily agree on a common binding for
LED configuration, I would define something along those lines to be
flexible:

phy-led-configuration = <LED_NUM_MASK LED_CFG_MASK>;

where LED_NUM_MASK is one of:

0 -> link
1 -> activity
2 -> speed

that way you can define single/dual/triple LED configurations by
updating the bitmask.

LED_CFG_MASK is one of:

0 -> LED_CFG_10
1 -> LED_CFG_100
2 -> LED_CFG_1000

(let's assume 1Gbps or less for now)

or this can be combined in a single cell with a left shift.

Andrew, Heiner, do you see that approach working correctly and scaling
appropriately?
Matthias Kaehlcke July 12, 2019, 5:20 p.m. UTC | #8
On Wed, Jul 10, 2019 at 09:55:12AM -0600, Rob Herring wrote:
> On Wed, Jul 3, 2019 at 5:23 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> >
> > Hi Florian,
> >
> > On Wed, Jul 03, 2019 at 02:37:47PM -0700, Florian Fainelli wrote:
> > > On 7/3/19 12:37 PM, Matthias Kaehlcke wrote:
> > > > The LED behavior of some Realtek PHYs is configurable. Add the
> > > > property 'realtek,led-modes' to specify the configuration of the
> > > > LEDs.
> > > >
> > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > > ---
> > > > Changes in v2:
> > > > - patch added to the series
> > > > ---
> > > >  .../devicetree/bindings/net/realtek.txt         |  9 +++++++++
> > > >  include/dt-bindings/net/realtek.h               | 17 +++++++++++++++++
> > > >  2 files changed, 26 insertions(+)
> > > >  create mode 100644 include/dt-bindings/net/realtek.h
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/net/realtek.txt b/Documentation/devicetree/bindings/net/realtek.txt
> > > > index 71d386c78269..40b0d6f9ee21 100644
> > > > --- a/Documentation/devicetree/bindings/net/realtek.txt
> > > > +++ b/Documentation/devicetree/bindings/net/realtek.txt
> > > > @@ -9,6 +9,12 @@ Optional properties:
> > > >
> > > >     SSC is only available on some Realtek PHYs (e.g. RTL8211E).
> > > >
> > > > +- realtek,led-modes: LED mode configuration.
> > > > +
> > > > +   A 0..3 element vector, with each element configuring the operating
> > > > +   mode of an LED. Omitted LEDs are turned off. Allowed values are
> > > > +   defined in "include/dt-bindings/net/realtek.h".
> > >
> > > This should probably be made more general and we should define LED modes
> > > that makes sense regardless of the PHY device, introduce a set of
> > > generic functions for validating and then add new function pointer for
> > > setting the LED configuration to the PHY driver. This would allow to be
> > > more future proof where each PHY driver could expose standard LEDs class
> > > devices to user-space, and it would also allow facilities like: ethtool
> > > -p to plug into that.
> > >
> > > Right now, each driver invents its own way of configuring LEDs, that
> > > does not scale, and there is not really a good reason for that other
> > > than reviewing drivers in isolation and therefore making it harder to
> > > extract the commonality. Yes, I realize that since you are the latest
> > > person submitting something in that area, you are being selected :)
> 
> I agree.
> 
> > I see the merit of your proposal to come up with a generic mechanism
> > to configure Ethernet LEDs, however I can't justify spending much of
> > my work time on this. If it is deemed useful I'm happy to send another
> > version of the current patchset that addresses the reviewer's comments,
> > but if the implementation of a generic LED configuration interface is
> > a requirement I will have to abandon at least the LED configuration
> > part of this series.
> 
> Can you at least define a common binding for this. Maybe that's just
> removing 'realtek'. While the kernel side can evolve to a common
> infrastructure, the DT bindings can't.

Defining a common binding sounds good to me, I will follow up on
Florian's reply to this.
Matthias Kaehlcke July 12, 2019, 5:28 p.m. UTC | #9
Hi Florian,

On Wed, Jul 10, 2019 at 09:28:39AM -0700, Florian Fainelli wrote:
> On 7/10/19 8:55 AM, Rob Herring wrote:
> > On Wed, Jul 3, 2019 at 5:23 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> >>
> >> Hi Florian,
> >>
> >> On Wed, Jul 03, 2019 at 02:37:47PM -0700, Florian Fainelli wrote:
> >>> On 7/3/19 12:37 PM, Matthias Kaehlcke wrote:
> >>>> The LED behavior of some Realtek PHYs is configurable. Add the
> >>>> property 'realtek,led-modes' to specify the configuration of the
> >>>> LEDs.
> >>>>
> >>>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> >>>> ---
> >>>> Changes in v2:
> >>>> - patch added to the series
> >>>> ---
> >>>>  .../devicetree/bindings/net/realtek.txt         |  9 +++++++++
> >>>>  include/dt-bindings/net/realtek.h               | 17 +++++++++++++++++
> >>>>  2 files changed, 26 insertions(+)
> >>>>  create mode 100644 include/dt-bindings/net/realtek.h
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/net/realtek.txt b/Documentation/devicetree/bindings/net/realtek.txt
> >>>> index 71d386c78269..40b0d6f9ee21 100644
> >>>> --- a/Documentation/devicetree/bindings/net/realtek.txt
> >>>> +++ b/Documentation/devicetree/bindings/net/realtek.txt
> >>>> @@ -9,6 +9,12 @@ Optional properties:
> >>>>
> >>>>     SSC is only available on some Realtek PHYs (e.g. RTL8211E).
> >>>>
> >>>> +- realtek,led-modes: LED mode configuration.
> >>>> +
> >>>> +   A 0..3 element vector, with each element configuring the operating
> >>>> +   mode of an LED. Omitted LEDs are turned off. Allowed values are
> >>>> +   defined in "include/dt-bindings/net/realtek.h".
> >>>
> >>> This should probably be made more general and we should define LED modes
> >>> that makes sense regardless of the PHY device, introduce a set of
> >>> generic functions for validating and then add new function pointer for
> >>> setting the LED configuration to the PHY driver. This would allow to be
> >>> more future proof where each PHY driver could expose standard LEDs class
> >>> devices to user-space, and it would also allow facilities like: ethtool
> >>> -p to plug into that.
> >>>
> >>> Right now, each driver invents its own way of configuring LEDs, that
> >>> does not scale, and there is not really a good reason for that other
> >>> than reviewing drivers in isolation and therefore making it harder to
> >>> extract the commonality. Yes, I realize that since you are the latest
> >>> person submitting something in that area, you are being selected :)
> > 
> > I agree.
> > 
> >> I see the merit of your proposal to come up with a generic mechanism
> >> to configure Ethernet LEDs, however I can't justify spending much of
> >> my work time on this. If it is deemed useful I'm happy to send another
> >> version of the current patchset that addresses the reviewer's comments,
> >> but if the implementation of a generic LED configuration interface is
> >> a requirement I will have to abandon at least the LED configuration
> >> part of this series.
> > 
> > Can you at least define a common binding for this. Maybe that's just
> > removing 'realtek'. While the kernel side can evolve to a common
> > infrastructure, the DT bindings can't.
> 
> That would be a great start, and that is actually what I had in mind
> (should have been more specific), I was not going to have you Matthias
> do the grand slam and convert all this LED configuration into the LEDs
> class etc. that would not be fair.
> 
> It seems to be that we can fairly easily agree on a common binding for
> LED configuration, I would define something along those lines to be
> flexible:
> 
> phy-led-configuration = <LED_NUM_MASK LED_CFG_MASK>;
> 
> where LED_NUM_MASK is one of:
> 
> 0 -> link
> 1 -> activity
> 2 -> speed

I don't understand this proposal completely. Is LED_NUM_MASK actually
a mask/set (potentially containing multiple LEDs) or is it "one of"
the LEDs?

Are you suggesting to assign each LED a specific role (link, activity,
speed)?

Could you maybe post a specific example involving multiple LEDs?

Thanks

Matthias

> that way you can define single/dual/triple LED configurations by
> updating the bitmask.
> 
> LED_CFG_MASK is one of:
> 
> 0 -> LED_CFG_10
> 1 -> LED_CFG_100
> 2 -> LED_CFG_1000
> 
> (let's assume 1Gbps or less for now)
> 
> or this can be combined in a single cell with a left shift.
> 
> Andrew, Heiner, do you see that approach working correctly and scaling
> appropriately?
Matthias Kaehlcke July 22, 2019, 5:14 p.m. UTC | #10
On Wed, Jul 10, 2019 at 09:55:12AM -0600, Rob Herring wrote:
> On Wed, Jul 3, 2019 at 5:23 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> >
> > Hi Florian,
> >
> > On Wed, Jul 03, 2019 at 02:37:47PM -0700, Florian Fainelli wrote:
> > > On 7/3/19 12:37 PM, Matthias Kaehlcke wrote:
> > > > The LED behavior of some Realtek PHYs is configurable. Add the
> > > > property 'realtek,led-modes' to specify the configuration of the
> > > > LEDs.
> > > >
> > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > > ---
> > > > Changes in v2:
> > > > - patch added to the series
> > > > ---
> > > >  .../devicetree/bindings/net/realtek.txt         |  9 +++++++++
> > > >  include/dt-bindings/net/realtek.h               | 17 +++++++++++++++++
> > > >  2 files changed, 26 insertions(+)
> > > >  create mode 100644 include/dt-bindings/net/realtek.h
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/net/realtek.txt b/Documentation/devicetree/bindings/net/realtek.txt
> > > > index 71d386c78269..40b0d6f9ee21 100644
> > > > --- a/Documentation/devicetree/bindings/net/realtek.txt
> > > > +++ b/Documentation/devicetree/bindings/net/realtek.txt
> > > > @@ -9,6 +9,12 @@ Optional properties:
> > > >
> > > >     SSC is only available on some Realtek PHYs (e.g. RTL8211E).
> > > >
> > > > +- realtek,led-modes: LED mode configuration.
> > > > +
> > > > +   A 0..3 element vector, with each element configuring the operating
> > > > +   mode of an LED. Omitted LEDs are turned off. Allowed values are
> > > > +   defined in "include/dt-bindings/net/realtek.h".
> > >
> > > This should probably be made more general and we should define LED modes
> > > that makes sense regardless of the PHY device, introduce a set of
> > > generic functions for validating and then add new function pointer for
> > > setting the LED configuration to the PHY driver. This would allow to be
> > > more future proof where each PHY driver could expose standard LEDs class
> > > devices to user-space, and it would also allow facilities like: ethtool
> > > -p to plug into that.
> > >
> > > Right now, each driver invents its own way of configuring LEDs, that
> > > does not scale, and there is not really a good reason for that other
> > > than reviewing drivers in isolation and therefore making it harder to
> > > extract the commonality. Yes, I realize that since you are the latest
> > > person submitting something in that area, you are being selected :)
> 
> I agree.
> 
> > I see the merit of your proposal to come up with a generic mechanism
> > to configure Ethernet LEDs, however I can't justify spending much of
> > my work time on this. If it is deemed useful I'm happy to send another
> > version of the current patchset that addresses the reviewer's comments,
> > but if the implementation of a generic LED configuration interface is
> > a requirement I will have to abandon at least the LED configuration
> > part of this series.
> 
> Can you at least define a common binding for this. Maybe that's just
> removing 'realtek'. While the kernel side can evolve to a common
> infrastructure, the DT bindings can't.

I'm working on a generic binding.

I wonder what is the best process for reviewing/landing it, I'm
doubting between two options:

a) only post the binding doc and the generic PHY code that reads
   the configuration from the DT. Post Realtek patches once
   the binding/generic code has been acked.

   pros: no churn from Realtek specific patches
   cons: initially no (real) user of the new binding

b) post generic and Realtek changes together

   pros: the binding has a user initially
   cons: churn from Realtek specific patches

I can do either, depending on what maintainers/reviewers prefer. I'm
slightly inclined towards a)
Andrew Lunn July 22, 2019, 7:01 p.m. UTC | #11
On Mon, Jul 22, 2019 at 10:14:18AM -0700, Matthias Kaehlcke wrote:
> I'm working on a generic binding.
> 
> I wonder what is the best process for reviewing/landing it, I'm
> doubting between two options:
> 
> a) only post the binding doc and the generic PHY code that reads
>    the configuration from the DT. Post Realtek patches once
>    the binding/generic code has been acked.
> 
>    pros: no churn from Realtek specific patches
>    cons: initially no (real) user of the new binding
> 
> b) post generic and Realtek changes together
> 
>    pros: the binding has a user initially
>    cons: churn from Realtek specific patches
> 
> I can do either, depending on what maintainers/reviewers prefer. I'm
> slightly inclined towards a)

Hi Matthias

It is normal to include one user of any generic API which is added,
just to make is clear how an API should be used.

     Andrew
Matthias Kaehlcke July 22, 2019, 7:14 p.m. UTC | #12
Hi Andrew,

On Mon, Jul 22, 2019 at 09:01:33PM +0200, Andrew Lunn wrote:
> On Mon, Jul 22, 2019 at 10:14:18AM -0700, Matthias Kaehlcke wrote:
> > I'm working on a generic binding.
> > 
> > I wonder what is the best process for reviewing/landing it, I'm
> > doubting between two options:
> > 
> > a) only post the binding doc and the generic PHY code that reads
> >    the configuration from the DT. Post Realtek patches once
> >    the binding/generic code has been acked.
> > 
> >    pros: no churn from Realtek specific patches
> >    cons: initially no (real) user of the new binding
> > 
> > b) post generic and Realtek changes together
> > 
> >    pros: the binding has a user initially
> >    cons: churn from Realtek specific patches
> > 
> > I can do either, depending on what maintainers/reviewers prefer. I'm
> > slightly inclined towards a)
> 
> Hi Matthias
> 
> It is normal to include one user of any generic API which is added,
> just to make is clear how an API should be used.

as of now it isn't even an API, the phy_device populates a new array
in its struct with the values from the DT. PHY drivers access the
array directly. Is it still preferable to post everything together?

(maybe I'm too concerned about 'noise' from the driver patches while
 we are figuring out what exactly the binding should be).

Thanks

Matthias
Andrew Lunn July 22, 2019, 7:38 p.m. UTC | #13
> as of now it isn't even an API, the phy_device populates a new array
> in its struct with the values from the DT. PHY drivers access the
> array directly. Is it still preferable to post everything together?
> 
> (maybe I'm too concerned about 'noise' from the driver patches while
>  we are figuring out what exactly the binding should be).

We should try to have the DT parsing made generic in phylib, and add
new driver API calls to actually configure the LEDs.

Please also take a look at the Linux generic LED binding. It would be
nice to have something compatible with that. With time, the code could
morph into being part of the generic LED subsystem. So we are mostly
talking about triggers. But we offload the trigger to the hardware,
rather than have software trigger the blinking of the LEDs. So
something like:

ethernet-phy0  {
	reg = <0>;

	leds {
		phy-led@0 {
	     	      reg = <0>
		      label = "left:green";
		      linux,default-trigger = "phy_link_1000_active";
		}
		phy-led@1 {
	     	      reg = <1>
		      label = "right:red";
		      linux,default-trigger = "phy_collision";
		}
	}
}

      Andrew

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/net/realtek.txt b/Documentation/devicetree/bindings/net/realtek.txt
index 71d386c78269..40b0d6f9ee21 100644
--- a/Documentation/devicetree/bindings/net/realtek.txt
+++ b/Documentation/devicetree/bindings/net/realtek.txt
@@ -9,6 +9,12 @@  Optional properties:
 
 	SSC is only available on some Realtek PHYs (e.g. RTL8211E).
 
+- realtek,led-modes: LED mode configuration.
+
+	A 0..3 element vector, with each element configuring the operating
+	mode of an LED. Omitted LEDs are turned off. Allowed values are
+	defined in "include/dt-bindings/net/realtek.h".
+
 Example:
 
 mdio0 {
@@ -20,5 +26,8 @@  mdio0 {
 		reg = <1>;
 		realtek,eee-led-mode-disable;
 		realtek,enable-ssc;
+		realtek,led-modes = <RTL8211E_LINK_ACTIVITY
+				     RTL8211E_LINK_100
+				     RTL8211E_LINK_1000>;
 	};
 };
diff --git a/include/dt-bindings/net/realtek.h b/include/dt-bindings/net/realtek.h
new file mode 100644
index 000000000000..8d64f58d58f8
--- /dev/null
+++ b/include/dt-bindings/net/realtek.h
@@ -0,0 +1,17 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _DT_BINDINGS_REALTEK_H
+#define _DT_BINDINGS_REALTEK_H
+
+/* LED modes for RTL8211E PHY */
+
+#define RTL8211E_LINK_10		1
+#define RTL8211E_LINK_100		2
+#define RTL8211E_LINK_1000		4
+#define RTL8211E_LINK_10_100		3
+#define RTL8211E_LINK_10_1000		5
+#define RTL8211E_LINK_100_1000		6
+#define RTL8211E_LINK_10_100_1000	7
+
+#define RTL8211E_LINK_ACTIVITY		(1 << 16)
+
+#endif