Messages in this thread | | | From | Kai-Heng Feng <> | Date | Fri, 22 Apr 2022 11:49:08 +0800 | Subject | Re: [PATCH 4/5] net: phy: marvell: Add LED accessors for Marvell 88E1510 |
| |
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
| |