Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware

From: Andrew Lunn <andrew@lunn.ch>
To: Kai-Heng Feng <kai.heng.feng@canonical.com>
Cc: hkallweit1@gmail.com, linux@armlinux.org.uk,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware
Date: Fri, 21 Jan 2022 14:22:15 +0100	[thread overview]
Message-ID: <Yeqzhx3GbMzaIbj6@lunn.ch> (raw)
In-Reply-To: <CAAd53p7NjvzsBs2aWTP-3GMjoyefMmLB3ou+7fDcrNVfKwALHw@mail.gmail.com>

On Fri, Jan 21, 2022 at 12:01:35PM +0800, Kai-Heng Feng wrote:
> On Thu, Jan 20, 2022 at 10:26 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Thu, Jan 20, 2022 at 01:19:29PM +0800, Kai-Heng Feng wrote:
> > > BIOS on Dell Edge Gateway 3200 already makes its own phy LED setting, so
> > > instead of setting another value, keep it untouched and restore the saved
> > > value on system resume.
> > >
> > > Introduce config_led() callback in phy_driver() to make the implemtation
> > > generic.
> >
> > I'm also wondering if we need to take a step back here and get the
> > ACPI guys involved. I don't know much about ACPI, but shouldn't it
> > provide a control method to configure the PHYs LEDs?
> >
> > We already have the basics for defining a PHY in ACPI. See:
> >
> > https://www.kernel.org/doc/html/latest/firmware-guide/acpi/dsd/phy.html
> 
> These properties seem to come from device-tree.

They are similar to what DT has, but expressed in an ACPI way. DT has
been used with PHY drivers for a long time, but ACPI is new. The ACPI
standard also says nothing about PHYs. So Linux has defined its own
properties, which we expect all ACPI machine to use. According to the
ACPI maintainers, this is within the ACPI standard. Maybe at some
point somebody will submit the current definitions to the standards
body for approval, or maybe the standard will do something completely
different, but for the moment, this is what we have, and what you
should use.

> > so you could extend this to include a method to configure the LEDs for
> > a specific PHY.
> 
> How to add new properties? Is it required to add new properties to
> both DT and ACPI?

Since all you are adding is a boolean, 'Don't touch the PHY LED
configuration', it should be easy to do for both.

What is interesting for Marvell PHYs is WoL, which is part of LED
configuration. I've not checked, but i guess there are other PHYs
which reuse LED output for a WoL interrupt. So it needs to be clearly
defined if we expect the BIOS to also correctly configure WoL, or if
Linux is responsible for configuring WoL, even though it means
changing the LED configuration.

	 Andrew