Re: [PATCH] net: phy: marvell-88q2xxx: Add support for PHY LEDs on 88q2xxx

[Stefan Eichenberger]

Hi Dimitri,

On Mon, Jan 13, 2025 at 10:25:24AM +0100, Dimitri Fedrau wrote:
> Hi Stefan,
> 
> Am Mon, Jan 13, 2025 at 09:05:21AM +0100 schrieb Stefan Eichenberger:
> > On Fri, Jan 10, 2025 at 07:30:58PM +0100, Dimitri Fedrau wrote:
> > > Hi Stefan,
> > > 
> > > Am Fri, Jan 10, 2025 at 06:27:43PM +0100 schrieb Stefan Eichenberger:
> > > > Hi Dimitri ,
> > > > 
> > > > On Fri, Jan 10, 2025 at 04:10:04PM +0100, Dimitri Fedrau wrote:
> > > > > Marvell 88Q2XXX devices support up to two configurable Light Emitting
> > > > > Diode (LED). Add minimal LED controller driver supporting the most common
> > > > > uses with the 'netdev' trigger.
> > > > > 
> > > > > Signed-off-by: Dimitri Fedrau <dima.fedrau@xxxxxxxxx>
> > > > > ---
> > > > >  drivers/net/phy/marvell-88q2xxx.c | 161 ++++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 161 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
> > > > > index 5107f58338aff4ed6cfea4d91e37282d9bb60ba5..bef3357b9d279fca5d1f86ff0eaa0d45a699e3f9 100644
> > > > > --- a/drivers/net/phy/marvell-88q2xxx.c
> > > > > +++ b/drivers/net/phy/marvell-88q2xxx.c
> > > > > @@ -8,6 +8,7 @@
> > > > >   */
> > > > >  #include <linux/ethtool_netlink.h>
> > > > >  #include <linux/marvell_phy.h>
> > > > > +#include <linux/of.h>
> > > > >  #include <linux/phy.h>
> > > > >  #include <linux/hwmon.h>
> > > > >  
> > > > > @@ -27,6 +28,9 @@
> > > > >  #define MDIO_MMD_AN_MV_STAT2_100BT1		0x2000
> > > > >  #define MDIO_MMD_AN_MV_STAT2_1000BT1		0x4000
> > > > >  
> > > > > +#define MDIO_MMD_PCS_MV_RESET_CTRL		32768
> > > > > +#define MDIO_MMD_PCS_MV_RESET_CTRL_TX_DISABLE	0x8
> > > > > +
> > > > >  #define MDIO_MMD_PCS_MV_INT_EN			32784
> > > > >  #define MDIO_MMD_PCS_MV_INT_EN_LINK_UP		0x0040
> > > > >  #define MDIO_MMD_PCS_MV_INT_EN_LINK_DOWN	0x0080
> > > > > @@ -40,6 +44,15 @@
> > > > >  #define MDIO_MMD_PCS_MV_GPIO_INT_CTRL			32787
> > > > >  #define MDIO_MMD_PCS_MV_GPIO_INT_CTRL_TRI_DIS		0x0800
> > > > >  
> > > > > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL			32790
> > > > > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_GPIO_MASK		GENMASK(7, 4)
> > > > > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_TX_EN_MASK	GENMASK(3, 0)
> > > > > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK		0x0 /* Link established */
> > > > > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_RX_TX	0x1 /* Link established, blink for rx or tx activity */
> > > > > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_RX_TX		0x4 /* Receive or Transmit activity */
> > > > > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_TX		0x5 /* Transmit activity */
> > > > > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_1000BT1	0x7 /* 1000BT1 link established */
> > > > > +
> > > > >  #define MDIO_MMD_PCS_MV_TEMP_SENSOR1			32833
> > > > >  #define MDIO_MMD_PCS_MV_TEMP_SENSOR1_RAW_INT		0x0001
> > > > >  #define MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT		0x0040
> > > > > @@ -95,6 +108,9 @@
> > > > >  
> > > > >  #define MDIO_MMD_PCS_MV_TDR_OFF_CUTOFF			65246
> > > > >  
> > > > > +#define MV88Q2XXX_LED_INDEX_TX_ENABLE	0
> > > > > +#define MV88Q2XXX_LED_INDEX_GPIO	1
> > > > 
> > > > Not sure if I understand this. TX_ENABLE would be LED0 and GPIO would be
> > > > LED1? In my datasheet the 88Q222x only has a GPIO pin (which is also
> > > > TX_ENABLE), is this a problem? Would we need a led_count variable per
> > > > chip? 
> > > > 
> > > Yes you understand it correctly.
> > > Looking at the datasheets for 88Q212x, 88Q211x and 88Q222x, they have all
> > > TX_ENABLE and GPIO pin. Registers are also the same. Did I miss anything ?
> > > For which device GPIO pin and TX_ENABLE are the same ?
> > > 
> > > > In the 88Q2110 I can see that there is a TX_ENABLE (0) and a GPIO (1)
> > > > pin. In the register description they just call it LED [0] Control and
> > > > LED [1] Control. Maybe calling it LED_0 and LED_1 would be easier to
> > > > understand? Same for MDIO_MMD_PCS_MV_LED_FUNC_CTRL_GPIO_MASK and
> > > > MDIO_MMD_PCS_MV_LED_FUNC_CTRL_TX_EN_MASK.
> > > > 
> > > I named them just as the pin. Probably it would be easier to understand,
> > > but the mapping between pin and index would be lost. What do you think ?
> > 
> > I missed this one in the previous mail, sorry. I personally would name
> > it LED_0_CONTROL_MASK and LED_1_CONTROL_MASK because the description of
> > the register is "3:0 LED [0] Control". As index I would probably also
> > call it LED_0_INDEX and LED_1_INDEX because it is not directly related
> > to the pin functionality. But that's just my personal preference not
> > sure if it is really better.
> >
> I think you are right, since the datasheet for 88Q222x is a bit messy
> and datasheets for 88Q212x and 88Q211x name them "LED [0] Control" and
> "LED [1] Control" I will do as you suggest. Thanks for pointing out.
> 
> I would stick to the index because you assign functionality when you
> configure DT to do so. How should one know which pins belongs to which led
> index, there is no documentation on this.

Perfect thanks, I agree with you on the index. If you can rename the
mask and add the comment as written in the previous mail, I'm happy with
the change.

Regards,
Stefan