Re: [PATCH 4/5] net: phy: marvell: Add LED accessors for Marvell 88E1510

[Kai-Heng Feng <kai.heng.feng@canonical.com>]

On Thu, Apr 21, 2022 at 8:57 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Thu, Apr 21, 2022 at 08:24:00PM +0800, Kai-Heng Feng wrote:
> > On Thu, Apr 21, 2022 at 7:51 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > > This is not feasible.
> > > > If BIOS can define a method and restore the LED by itself, it can put
> > > > the method inside its S3 method and I don't have to work on this at
> > > > the first place.
> > >
> > > So maybe just declare the BIOS as FUBAR and move on to the next issue
> > > assigned to you.
> > >
> > > Do we really want the maintenance burden of this code for one machines
> > > BIOS?
> >
> > Wasn't this the "set precedence" we discussed earlier for? Someone has
> > to be the first, and more users will leverage the new property we
> > added.
>
> I both agree and disagree. I'm trying to make this feature generic,
> unlike you who seem to be doing the minimal, only saving one of three
> LED configuration registers. But on the other hand, i'm not sure there
> will be more users. Do you have a list of machines where the BIOS is
> FUBAR? Is it one machine? A range of machines from one vendor, or
> multiple vendors with multiple machines. I would feel better about the
> maintenance burden if i knew that this was going to be used a lot.

Right now it's only one machine. But someone has to be the first :)

>
> > > Maybe the better solution is to push back on the vendor and its
> > > BIOS, tell them how they should of done this, if the BIOS wants to be
> > > in control of the LEDs it needs to offer the methods to control the
> > > LEDs. And then hopefully the next machine the vendor produces will
> > > have working BIOS.
> >
> > The BIOS doesn't want to control the LED. It just provides a default
> > LED setting suitable for this platform, so the driver can use this
> > value over the hardcoded one in marvell phy driver.
>
> Exactly, it wants to control the LED, and tell the OS not to touch it
> ever.

That doesn't mean it wants to control the LED, it's still the phy
driver controls it.

>
> > So this really has nothing to do with with any ACPI method.
> > I believe the new property can be useful for DT world too.
>
> DT generally never trusts the bootloader to do anything. So i doubt
> such a DT property would ever be used. Also, DT is about describing
> the hardware, not how to configure the hardware. So you could list
> there is a PHY LED, what colour it is, etc. But in general, you would
> not describe how it is configured, that something else is configuring
> it and it should be left alone.

What if let the property list to the raw value of the LED should be?
So it can fall under "describing hardware" like 'clock-frequency' property.

>
> > > Your other option is to take part in the effort to add control of the
> > > LEDs via the standard Linux LED subsystem. The Marvel PHY driver is
> > > likely to be one of the first to gain support this for. So you can
> > > then totally take control of the LED from the BIOS and put it in the
> > > users hands. And such a solution will be applicable to many machines,
> > > not just one.
> >
> > This series just wants to use the default value platform firmware provides.
> > Create a sysfs to let user meddle with LED value doesn't really help
> > the case here.
>
> I would disagree. You can add a systemd service to configure it at
> boot however you want. It opens up the possibility to implement
> ethtool --identify in a generic way, etc. It is a much more powerful
> and useful feature than saying 'don't touch', and also it justify the
> maintenance burden.

That just pushed the maintenance burden to another subsystem and I
doubt it will bring more users than current approach.

Kai-Heng

>
>      Andrew