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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4d02f786-e87e-4588-87ed-b5fa414a4b5a@redhat.com>
Date: Thu, 16 Jan 2025 10:58:38 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Marek Vasut <marex@...x.de>, netdev@...r.kernel.org
Cc: "David S. Miller" <davem@...emloft.net>, Andrew Lunn <andrew@...n.ch>,
 Eric Dumazet <edumazet@...gle.com>, Heiner Kallweit <hkallweit1@...il.com>,
 Jakub Kicinski <kuba@...nel.org>, Russell King <linux@...linux.org.uk>,
 Tristram Ha <tristram.ha@...rochip.com>, UNGLinuxDriver@...rochip.com,
 Vladimir Oltean <olteanv@...il.com>, Woojung Huh
 <woojung.huh@...rochip.com>, linux-kernel@...r.kernel.org
Subject: Re: [net-next,PATCH 2/2] net: phy: micrel: Add KSZ87XX Switch LED
 control

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