Re: [PATCH v2 7/7] net: phy: realtek: configure RTL8211E LEDs

[Heiner Kallweit <hkallweit1@gmail.com>]

On 03.07.2019 21:37, Matthias Kaehlcke wrote:
> Configure the RTL8211E LEDs behavior when the device tree property
> 'realtek,led-modes' is specified.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> Changes in v2:
> - patch added to the series
> ---
>  drivers/net/phy/realtek.c | 63 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 61 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index 45fee4612031..559aec547738 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -9,6 +9,7 @@
>   * Copyright (c) 2004 Freescale Semiconductor, Inc.
>   */
>  #include <linux/bitops.h>
> +#include <linux/bits.h>
>  #include <linux/device.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> @@ -35,6 +36,15 @@
>  #define RTL8211E_EEE_LED_MODE1			0x05
>  #define RTL8211E_EEE_LED_MODE2			0x06
>  
> +/* RTL8211E extension page 44 */
> +#define RTL8211E_LACR				0x1a
> +#define RLT8211E_LACR_LEDACTCTRL_SHIFT		4
> +#define RLT8211E_LACR_LEDACTCTRL_MASK		GENMASK(6, 4)
> +#define RTL8211E_LCR				0x1c
> +#define RTL8211E_LCR_LEDCTRL_MASK		(GENMASK(2, 0) | \
> +						 GENMASK(6, 4) | \
> +						 GENMASK(10, 8))
> +
>  /* RTL8211E extension page 160 */
>  #define RTL8211E_SCR				0x1a
>  #define RTL8211E_SCR_DISABLE_RXC_SSC		BIT(2)
> @@ -124,6 +134,56 @@ static int rtl8211e_disable_eee_led_mode(struct phy_device *phydev)
>  	return phy_restore_page(phydev, oldpage, ret);
>  }
>  
> +static int rtl8211e_config_leds(struct phy_device *phydev)
> +{
> +	struct device *dev = &phydev->mdio.dev;
> +	int count, i, oldpage, ret;
> +	u16 lacr_bits = 0, lcr_bits = 0;
> +
> +	if (!dev->of_node)
> +		return 0;
> +
> +	if (of_property_read_bool(dev->of_node, "realtek,eee-led-mode-disable"))
> +		rtl8211e_disable_eee_led_mode(phydev);
> +
> +	count = of_property_count_elems_of_size(dev->of_node,
> +						"realtek,led-modes",
> +						sizeof(u32));
> +	if (count < 0 || count > 3)
> +		return -EINVAL;
> +
> +	for (i = 0; i < count; i++) {
> +		u32 val;
> +
> +		of_property_read_u32_index(dev->of_node,
> +					   "realtek,led-modes", i, &val);
> +		lacr_bits |= (u16)(val >> 16) <<
> +			(RLT8211E_LACR_LEDACTCTRL_SHIFT + i);

This may be done in an easier to read way:

if (val & RTL8211E_LINK_ACTIVITY)
	lacr_bits |= BIT(RLT8211E_LACR_LEDACTCTRL_SHIFT + i);


> +		lcr_bits |= (u16)(val & 0xf) << (i * 4);
> +	}
> +
> +	oldpage = rtl8211e_select_ext_page(phydev, 44);
> +	if (oldpage < 0) {
> +		dev_err(dev, "failed to select extended page: %d\n", oldpage);

In a PHY driver it may be more appropriate to use phydev_err() et al,
even though effectively it's the same.

> +		goto err;
> +	}
> +
> +	ret = __phy_modify(phydev, RTL8211E_LACR,
> +			   RLT8211E_LACR_LEDACTCTRL_MASK, lacr_bits);
> +	if (ret) {
> +		dev_err(dev, "failed to write LACR reg: %d\n", ret);
> +		goto err;
> +	}
> +
> +	ret = __phy_modify(phydev, RTL8211E_LCR,
> +			   RTL8211E_LCR_LEDCTRL_MASK, lcr_bits);
> +	if (ret)
> +		dev_err(dev, "failed to write LCR reg: %d\n", ret);
> +
> +err:
> +	return phy_restore_page(phydev, oldpage, ret);
> +}
> +
>  static int rtl8211e_config_init(struct phy_device *phydev)
>  {
>  	struct device *dev = &phydev->mdio.dev;
> @@ -137,8 +197,7 @@ static int rtl8211e_config_init(struct phy_device *phydev)
>  			dev_err(dev, "failed to enable SSC on RXC: %d\n", ret);
>  	}
>  
> -	if (of_property_read_bool(dev->of_node, "realtek,eee-led-mode-disable"))
> -		rtl8211e_disable_eee_led_mode(phydev);
> +	rtl8211e_config_leds(phydev);
>  
>  	return 0;
>  }
>