linux-kernel - Re: [PATCH net-next] net: phy: dp83822: Add support for PHY LEDs on DP83822

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: <4f40c476-565f-4f74-8cab-7250045fdd90@lunn.ch>
Date: Wed, 18 Dec 2024 21:19:47 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Dimitri Fedrau <dima.fedrau@...il.com>
Cc: Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] net: phy: dp83822: Add support for PHY LEDs on
 DP83822

On Wed, Dec 18, 2024 at 07:17:52PM +0100, Dimitri Fedrau wrote:
> Am Wed, Dec 18, 2024 at 06:16:20PM +0100 schrieb Andrew Lunn:
> > > By the way. Wouldn't it be helpful adding a u32 max_leds to
> > > struct phy_driver ? Every driver supporting PHY LEDs validates index at the
> > > moment. With max_leds it should be easy to check it in of_phy_leds and
> > > return with an error if index is not valid.
> > 
> > I have been considering it. However, so far developers have been good
> > at adding the checks, because the first driver had the checks, cargo
> > cult at its best.
> > 
> > If we are going to add it, we should do it early, before there are too
> > many PHY drivers which need updating.
> >
> Another solution without breaking others driver would be to add a
> callback in struct phy_driver:

Adding the maximum number of LEDs to struct phy_driver will not break
anything. But we would want to remove all the tests for the index
value from the drivers, since they become pointless. That will be
easier to do when there are less drivers which need editing.

> int (*led_validate_index)(struct phy_device *dev, int index)
> It should be called in of_phy_led right after reading in reg property:
> if (phydev->drv->led_validate_index)
> 	ret = phydev->drv->led_validate_index(phydev, index);
> 
> This would solve another isssue I have. The LED pins of the DP83822 can
> be multiplexed. Not all of them have per default a LED function. So I
> need to set them up. In dp83822_of_init_leds I iterate over all DT nodes
> in leds to get the information which of the pins should output LED
> function. Using the callback would eleminate the need for copying code of
> functions of_phy_leds and of_phy_led.

Your hardware is pretty unique. It might be best to keep it in the
driver, until there is a second driver which needs the same. I also
think you need the complete configuration in order to validate it, not
each LED one by one, which your led_validate_index() would provide.

	Andrew