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

[Dimitri Fedrau <dima.fedrau@gmail.com>]

Hi Stefan,

Am Sat, Feb 08, 2025 at 05:44:26PM +0100 schrieb Stefan Eichenberger:
> On Fri, Feb 07, 2025 at 05:24:20PM +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@gmail.com>
> 
> Reviewed-by: Stefan Eichenberger <eichest@gmail.com>
> 

thanks for reviewing. I just noticed that led0 is enabled in
mv88q222x_config_init, but I think it should be enabled in
mv88q2xxx_config_init because LED configuration is same for all
mv88q2xxx devices. What do you think ?

> > ---
> > Changes in v2:
> > - Renamed MDIO_MMD_PCS_MV_LED_FUNC_CTRL_GPIO_MASK to
> >   MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_1_MASK (Stefan)
> > - Renamed MDIO_MMD_PCS_MV_LED_FUNC_CTRL_TX_EN_MASK to
> >   MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_0_MASK (Stefan)
> > - Added comment for disabling tx disable feature (Stefan)
> > - Added defines for all led functions (Andrew)
> > - Move enabling of led0 function from mv88q2xxx_probe to
> >   mv88q222x_config_init. When the hardware reset pin is connected to a GPIO
> >   for example and we bring the interface down and up again, the content
> >   of the register MDIO_MMD_PCS_MV_RESET_CTRL is resetted to default.
> >   This means LED function is disabled and can't be enabled again.
> > - Link to v1: https://lore.kernel.org/r/20250110-marvell-88q2xxx-leds-v1-1-22e7734941c2@gmail.com
> > ---
> >  drivers/net/phy/marvell-88q2xxx.c | 175 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 175 insertions(+)
> > 
> > diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
> > index a3996471a1c9a5d4060d5d19ce44aa70e902a83f..2d5ea3e1b26219bb1e050222347c2688903e2430 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,22 @@
> >  #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_LED_1_MASK	GENMASK(7, 4)
> > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_0_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_LINK_1000BT1	0x2 /* Blink 3x for 1000BT1 link established */
> > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_RX_TX_ON		0x3 /* Receive or transmit activity */
> > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_RX_TX		0x4 /* Blink on 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_COPPER	0x6 /* Copper Link established */
> > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_1000BT1_ON	0x7 /* 1000BT1 link established */
> > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_FORCE_OFF		0x8 /* Force off */
> > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_FORCE_ON		0x9 /* Force on */
> > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_FORCE_HIGHZ	0xa /* Force Hi-Z */
> > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_FORCE_BLINK	0xb /* Force blink */
> > +
> >  #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,8 +115,12 @@
> >  
> >  #define MDIO_MMD_PCS_MV_TDR_OFF_CUTOFF			65246
> >  
> > +#define MV88Q2XXX_LED_INDEX_TX_ENABLE	0
> > +#define MV88Q2XXX_LED_INDEX_GPIO	1
> > +
> >  struct mv88q2xxx_priv {
> >  	bool enable_temp;
> > +	bool enable_led0;
> >  };
> >  
> >  struct mmd_val {
> > @@ -740,15 +764,62 @@ static int mv88q2xxx_hwmon_probe(struct phy_device *phydev)
> >  }
> >  #endif
> >  
> > +#if IS_ENABLED(CONFIG_OF_MDIO)
> > +static int mv88q2xxx_leds_probe(struct phy_device *phydev)
> > +{
> > +	struct device_node *node = phydev->mdio.dev.of_node;
> > +	struct mv88q2xxx_priv *priv = phydev->priv;
> > +	struct device_node *leds;
> > +	int ret = 0;
> > +	u32 index;
> > +
> > +	if (!node)
> > +		return 0;
> > +
> > +	leds = of_get_child_by_name(node, "leds");
> > +	if (!leds)
> > +		return 0;
> > +
> > +	for_each_available_child_of_node_scoped(leds, led) {
> > +		ret = of_property_read_u32(led, "reg", &index);
> > +		if (ret)
> > +			goto exit;
> > +
> > +		if (index > MV88Q2XXX_LED_INDEX_GPIO) {
> > +			ret = -EINVAL;
> > +			goto exit;
> > +		}
> > +
> > +		if (index == MV88Q2XXX_LED_INDEX_TX_ENABLE)
> > +			priv->enable_led0 = true;
> > +	}
> > +
> > +exit:
> > +	of_node_put(leds);
> > +
> > +	return ret;
> > +}
> > +
> > +#else
> > +static int mv88q2xxx_leds_probe(struct phy_device *phydev)
> > +{
> > +	return 0;
> > +}
> > +#endif
> > +
> >  static int mv88q2xxx_probe(struct phy_device *phydev)
> >  {
> >  	struct mv88q2xxx_priv *priv;
> > +	int ret;
> >  
> >  	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
> >  	if (!priv)
> >  		return -ENOMEM;
> >  
> >  	phydev->priv = priv;
> > +	ret = mv88q2xxx_leds_probe(phydev);
> > +	if (ret)
> > +		return ret;
> >  
> >  	return mv88q2xxx_hwmon_probe(phydev);
> >  }
> > @@ -829,6 +900,15 @@ static int mv88q222x_config_init(struct phy_device *phydev)
> >  			return ret;
> >  	}
> >  
> > +	/* Enable LED function and disable TX disable feature on LED/TX_ENABLE */
> > +	if (priv->enable_led0) {
> > +		ret = phy_clear_bits_mmd(phydev, MDIO_MMD_PCS,
> > +					 MDIO_MMD_PCS_MV_RESET_CTRL,
> > +					 MDIO_MMD_PCS_MV_RESET_CTRL_TX_DISABLE);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +
> >  	if (phydev->c45_ids.device_ids[MDIO_MMD_PMAPMD] == PHY_ID_88Q2220_REVB0)
> >  		return mv88q222x_revb0_config_init(phydev);
> >  	else
> > @@ -918,6 +998,98 @@ static int mv88q222x_cable_test_get_status(struct phy_device *phydev,
> >  	return 0;
> >  }
> >  
> > +static int mv88q2xxx_led_mode(u8 index, unsigned long rules)
> > +{
> > +	switch (rules) {
> > +	case BIT(TRIGGER_NETDEV_LINK):
> > +		return MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK;
> > +	case BIT(TRIGGER_NETDEV_LINK_1000):
> > +		return MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_1000BT1_ON;
> > +	case BIT(TRIGGER_NETDEV_TX):
> > +		return MDIO_MMD_PCS_MV_LED_FUNC_CTRL_TX;
> > +	case BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX):
> > +		return MDIO_MMD_PCS_MV_LED_FUNC_CTRL_RX_TX;
> > +	case BIT(TRIGGER_NETDEV_LINK) | BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX):
> > +		return MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_RX_TX;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +}
> > +
> > +static int mv88q2xxx_led_hw_is_supported(struct phy_device *phydev, u8 index,
> > +					 unsigned long rules)
> > +{
> > +	int mode;
> > +
> > +	mode = mv88q2xxx_led_mode(index, rules);
> > +	if (mode < 0)
> > +		return mode;
> > +
> > +	return 0;
> > +}
> > +
> > +static int mv88q2xxx_led_hw_control_set(struct phy_device *phydev, u8 index,
> > +					unsigned long rules)
> > +{
> > +	int mode;
> > +
> > +	mode = mv88q2xxx_led_mode(index, rules);
> > +	if (mode < 0)
> > +		return mode;
> > +
> > +	if (index == MV88Q2XXX_LED_INDEX_TX_ENABLE)
> > +		return phy_modify_mmd(phydev, MDIO_MMD_PCS,
> > +				      MDIO_MMD_PCS_MV_LED_FUNC_CTRL,
> > +				      MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_0_MASK,
> > +				      FIELD_PREP(MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_0_MASK,
> > +						 mode));
> > +	else
> > +		return phy_modify_mmd(phydev, MDIO_MMD_PCS,
> > +				      MDIO_MMD_PCS_MV_LED_FUNC_CTRL,
> > +				      MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_1_MASK,
> > +				      FIELD_PREP(MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_1_MASK,
> > +						 mode));
> > +}
> > +
> > +static int mv88q2xxx_led_hw_control_get(struct phy_device *phydev, u8 index,
> > +					unsigned long *rules)
> > +{
> > +	int val;
> > +
> > +	val = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_MMD_PCS_MV_LED_FUNC_CTRL);
> > +	if (val < 0)
> > +		return val;
> > +
> > +	if (index == MV88Q2XXX_LED_INDEX_TX_ENABLE)
> > +		val = FIELD_GET(MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_0_MASK, val);
> > +	else
> > +		val = FIELD_GET(MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_1_MASK, val);
> > +
> > +	switch (val) {
> > +	case MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK:
> > +		*rules = BIT(TRIGGER_NETDEV_LINK);
> > +		break;
> > +	case MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_1000BT1_ON:
> > +		*rules = BIT(TRIGGER_NETDEV_LINK_1000);
> > +		break;
> > +	case MDIO_MMD_PCS_MV_LED_FUNC_CTRL_TX:
> > +		*rules = BIT(TRIGGER_NETDEV_TX);
> > +		break;
> > +	case MDIO_MMD_PCS_MV_LED_FUNC_CTRL_RX_TX:
> > +		*rules = BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX);
> > +		break;
> > +	case MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_RX_TX:
> > +		*rules = BIT(TRIGGER_NETDEV_LINK) | BIT(TRIGGER_NETDEV_TX) |
> > +			 BIT(TRIGGER_NETDEV_RX);
> > +		break;
> > +	default:
> > +		*rules = 0;
> > +		break;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static struct phy_driver mv88q2xxx_driver[] = {
> >  	{
> >  		.phy_id			= MARVELL_PHY_ID_88Q2110,
> > @@ -953,6 +1125,9 @@ static struct phy_driver mv88q2xxx_driver[] = {
> >  		.get_sqi_max		= mv88q2xxx_get_sqi_max,
> >  		.suspend		= mv88q2xxx_suspend,
> >  		.resume			= mv88q2xxx_resume,
> > +		.led_hw_is_supported	= mv88q2xxx_led_hw_is_supported,
> > +		.led_hw_control_set	= mv88q2xxx_led_hw_control_set,
> > +		.led_hw_control_get	= mv88q2xxx_led_hw_control_get,
> >  	},
> >  };
> >  
> > 
> > ---
> > base-commit: f84db3bc8abc2141839cdb9454061633a4ce1db7
> > change-id: 20241221-marvell-88q2xxx-leds-69a4037b5157
> > 
> > Best regards,
> > -- 
> > Dimitri Fedrau <dima.fedrau@gmail.com>
> >