Re: [net-next,PATCH 2/2] net: phy: micrel: Add KSZ87XX Switch LED control

[Paolo Abeni <pabeni@redhat.com>]

On 1/13/25 1:15 AM, Marek Vasut wrote:
> The KSZ87xx switch contains LED control registers. There is one shared
> global control register bitfield which affects behavior of all LEDs on
> all ports, the Register 11 (0x0B): Global Control 9 bitfield [5:4].
> There is also one per-port Register 29/45/61 (0x1D/0x2D/0x3D): Port 1/2/3
> Control 10 bit 7 which controls enablement of both LEDs on each port
> separately.
> 
> Expose LED brightness control and HW offload support for both of the two
> programmable LEDs on this KSZ87XX Switch. Note that on KSZ87xx there are
> three or more instances of simple KSZ87XX Switch PHY, one for each port,
> however, the registers which control the LED behavior are mostly shared.
> 
> Introduce LED brightness control using Register 29/45/61 (0x1D/0x2D/0x3D):
> Port 1/2/3 Control 10 bit 7. This bit selects between LEDs disabled and
> LEDs set to Function mode. In case LED brightness is set to 0, both LEDs
> are turned off, otherwise both LEDs are configured to Function mode which
> follows the global Register 11 (0x0B): Global Control 9 bitfield [5:4]
> setting.

@Andrew, @Russel: can the above problem be address with the current phy
API? or does phy device need to expose a new brightness_get op?

[...]
> @@ -891,6 +892,112 @@ static int ksz8795_match_phy_device(struct phy_device *phydev)
>  	return ksz8051_ksz8795_match_phy_device(phydev, false);
>  }
>  
> +#define KSZ8795_LED_COUNT	2
> +
> +static const unsigned long ksz8795_led_rules_map[4][2] = {
> +	{
> +		/* Control Bits = 2'b00 => LEDx_0=Link/ACT LEDx_1=Speed */
> +		BIT(TRIGGER_NETDEV_LINK) | BIT(TRIGGER_NETDEV_RX) |
> +		BIT(TRIGGER_NETDEV_TX),
> +		BIT(TRIGGER_NETDEV_LINK_100)
> +	}, {
> +		/* Control Bits = 2'b01 => LEDx_0=Link     LEDx_1=ACT */
> +		BIT(TRIGGER_NETDEV_LINK),
> +		BIT(TRIGGER_NETDEV_RX) | BIT(TRIGGER_NETDEV_TX)
> +	}, {
> +		/* Control Bits = 2'b10 => LEDx_0=Link/ACT LEDx_1=Duplex */
> +		BIT(TRIGGER_NETDEV_LINK) | BIT(TRIGGER_NETDEV_RX) |
> +		BIT(TRIGGER_NETDEV_TX),
> +		BIT(TRIGGER_NETDEV_FULL_DUPLEX)
> +	}, {
> +		/* Control Bits = 2'b11 => LEDx_0=Link     LEDx_1=Duplex */
> +		BIT(TRIGGER_NETDEV_LINK),
> +		BIT(TRIGGER_NETDEV_FULL_DUPLEX)
> +	}
> +};
> +
> +static int ksz8795_led_brightness_set(struct phy_device *phydev, u8 index,
> +				      enum led_brightness value)
> +{
> +	/* Turn all LEDs on this port on or off */
> +	/* Emulated rmw of Register 29/45/61 (0x1D/0x2D/0x3D): Port 1/2/3 Control 10 */
> +	return phy_modify(phydev, 0x0d00, BIT(7), (value == LED_OFF) ? BIT(7) : 0);

Please defines macros for all the above 'magic numbers'

> +}
> +
> +static int ksz8795_led_hw_is_supported(struct phy_device *phydev, u8 index,
> +				       unsigned long rules)
> +{
> +	const unsigned long mask[2] = {
> +		BIT(TRIGGER_NETDEV_LINK) | BIT(TRIGGER_NETDEV_RX) |
> +		BIT(TRIGGER_NETDEV_TX),
> +		BIT(TRIGGER_NETDEV_LINK_100) | BIT(TRIGGER_NETDEV_RX) |
> +		BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_FULL_DUPLEX)
> +	};
> +
> +	if (index >= KSZ8795_LED_COUNT)
> +		return -EINVAL;
> +
> +	/* Filter out any other unsupported triggers. */
> +	if (rules & ~mask[index])
> +		return -EOPNOTSUPP;
> +
> +	/* RX and TX are not differentiated, either both are set or not set. */
> +	if (!(rules & BIT(TRIGGER_NETDEV_RX)) ^ !(rules & BIT(TRIGGER_NETDEV_TX)))
> +		return -EOPNOTSUPP;
> +
> +	return 0;
> +}
> +
> +static int ksz8795_led_hw_control_get(struct phy_device *phydev, u8 index,
> +				      unsigned long *rules)
> +{
> +	int val;
> +
> +	if (index >= KSZ8795_LED_COUNT)
> +		return -EINVAL;
> +
> +	/* Emulated read of Register 11 (0x0B): Global Control 9 */
> +	val = phy_read(phydev, 0x0b00);
> +	if (val < 0)
> +		return val;
> +
> +	/* Extract bits [5:4] and look up matching LED configuration */
> +	*rules = ksz8795_led_rules_map[(val >> 4) & 0x3][index];

This calls for FIELD_GET() usage.

[...]
> @@ -5666,10 +5773,15 @@ static struct phy_driver ksphy_driver[] = {
>  }, {
>  	.name		= "Micrel KSZ87XX Switch",
>  	/* PHY_BASIC_FEATURES */
> +	.probe		= kszphy_probe,
>  	.config_init	= kszphy_config_init,
>  	.match_phy_device = ksz8795_match_phy_device,
>  	.suspend	= genphy_suspend,
>  	.resume		= genphy_resume,
> +	.led_brightness_set = ksz8795_led_brightness_set,
> +	.led_hw_is_supported = ksz8795_led_hw_is_supported,
> +	.led_hw_control_get = ksz8795_led_hw_control_get,
> +	.led_hw_control_set = ksz8795_led_hw_control_set,

The preferred style is to use an additional tab to align all the '=' in
this new block.

/P