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

[Dimitri Fedrau]

Am Wed, Dec 18, 2024 at 09:19:47PM +0100 schrieb Andrew Lunn:
> 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.
>
Just adding the number will not break anything, but probably the test
that should be implemented in of_phy_led. Except the test gets skipped if
maximum number of LEDs is not set(zero). Otherwise we would have to
change all existing drivers to set the maximum numbers of LEDs.

> > 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.
>
I have implemented LED support for marvell-88q2xxx.c which I wanted to
upstream after LED support for DP83822 is done. There I have the same
issue.
You are right, I need the whole configuration. But at the moment I read
it in the same way as it is done in of_phy_led and save indices to
bool led_pin_enable[DP83822_MAX_LED_PINS] and set them up later in
dp83822_config_init.

How do we proceed ? Implement maximum numbers of LEDs ? Skip the
validation index callback when upstreaming LED support for
marvell-88q2xxx.c ?

Best regards,
Dimitri