Re: [PATCH v4 3/3] net: phy: leds: add support for led triggers on phy link state change
On Thu, Oct 13, 2016 at 10:46:34AM -0400, David Miller wrote:
> From: Zach Brown
> Date: Tue, 11 Oct 2016 15:26:20 -0500
>
> > From: Josh Cartwright
> >
> > Create an option CONFIG_LED_TRIGGER_PHY (default n), which will
> > create a set of led triggers for each instantiated PHY device. There is
> > one LED trigger per link-speed, per-phy.
> >
> > This allows for a user to configure their system to allow a set of LEDs
> > to represent link state changes on the phy.
> >
> > Signed-off-by: Josh Cartwright
> > Signed-off-by: Nathan Sullivan
> > Signed-off-by: Zach Brown
> ...
> > + static const char * const name_suffix[] = {
> > + "10Mbps",
> > + "100Mbps",
> > + "1Gbps",
> > + "2.5Gbps",
> > + "10Gbps",
>
> This choice of both the array size and the speeds to support seems
> entirely arbitrary and is inappropriate for a generic driver of this
> kind.
>
> This seems to be hard coding this to support the list of speeds
> supported by whatever driver you want to use with this new LED
> facility, and sorry that's not how we build nice generic pieces of
> infrastructure.
>
> Thanks.
The speeds listed are the speeds found in the phy_speed_to_str function in
phy.c.
They are also the speeds found in the struct phy_setting settings array,
which is commented with
"/* A mapping of all SUPPORTED settings to speed/duplex */"
We believed they represented the commonly supported speeds of phys.
Do you have suggestions on how to better handle the choice of the array size
and the speeds?
Thanks.
Re: [PATCH v4 3/3] net: phy: leds: add support for led triggers on phy link state change
> Do you have suggestions on how to better handle the choice of the array size > and the speeds? phydev->supported lists the speeds this phy supports. Andrew
Re: [PATCH v4 3/3] net: phy: leds: add support for led triggers on phy link state change
From: Zach Brown
Date: Thu, 13 Oct 2016 10:42:46 -0500
> Do you have suggestions on how to better handle the choice of the array size
> and the speeds?
All of the speed values are simply the rate in bits per second.
There is therefore no reason you can't just print the raw value and
scale it to whatever is appropriate ("MB/s", "GB/s", etc.)
Re: [PATCH v4 3/3] net: phy: leds: add support for led triggers on phy link state change
From: Zach Brown
Date: Tue, 11 Oct 2016 15:26:20 -0500
> From: Josh Cartwright
>
> Create an option CONFIG_LED_TRIGGER_PHY (default n), which will
> create a set of led triggers for each instantiated PHY device. There is
> one LED trigger per link-speed, per-phy.
>
> This allows for a user to configure their system to allow a set of LEDs
> to represent link state changes on the phy.
>
> Signed-off-by: Josh Cartwright
> Signed-off-by: Nathan Sullivan
> Signed-off-by: Zach Brown
...
> + static const char * const name_suffix[] = {
> + "10Mbps",
> + "100Mbps",
> + "1Gbps",
> + "2.5Gbps",
> + "10Gbps",
This choice of both the array size and the speeds to support seems
entirely arbitrary and is inappropriate for a generic driver of this
kind.
This seems to be hard coding this to support the list of speeds
supported by whatever driver you want to use with this new LED
facility, and sorry that's not how we build nice generic pieces of
infrastructure.
Thanks.
