Re: [PATCH 2/5] net: mdio: Add "use-firmware-led" firmware property

[Andrew Lunn <andrew@lunn.ch>]

On Wed, Apr 20, 2022 at 08:40:49PM +0800, Kai-Heng Feng wrote:
> Some system may prefer preset PHY LED setting instead of the one
> hardcoded in the PHY driver, so adding a new firmware
> property, "use-firmware-led", to cope with that.
> 
> On ACPI based system the ASL looks like this:
> 
>     Scope (_SB.PC00.OTN0)
>     {
>         Device (PHY0)
>         {
>             Name (_ADR, One)  // _ADR: Address
>             Name (_DSD, Package (0x02)  // _DSD: Device-Specific Data
>             {
>                 ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */,
>                 Package (0x01)
>                 {
>                     Package (0x02)
>                     {
>                         "use-firmware-led",
>                         One
>                     }
>                 }
>             })
>         }
> 
>         Name (_DSD, Package (0x02)  // _DSD: Device-Specific Data
>         {
>             ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */,
>             Package (0x01)
>             {
>                 Package (0x02)
>                 {
>                     "phy-handle",
>                     PHY0
>                 }
>             }
>         })
>     }
> 
> Basically use the "phy-handle" reference to read the "use-firmware-led"
> boolean.

Please update Documentation/firmware-guide/acpi/dsd/phy.rst

Please also Cc: the ACPI list. I have no idea what the naming
convention is for ACPI properties.

> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
>  drivers/net/mdio/fwnode_mdio.c | 4 ++++
>  include/linux/phy.h            | 1 +
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
> index 1c1584fca6327..bfca67b42164b 100644
> --- a/drivers/net/mdio/fwnode_mdio.c
> +++ b/drivers/net/mdio/fwnode_mdio.c
> @@ -144,6 +144,10 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
>  	 */
>  	if (mii_ts)
>  		phy->mii_ts = mii_ts;
> +
> +	phy->use_firmware_led =
> +		fwnode_property_read_bool(child, "use-firmware-led");
> +

This is an ACPI only property. It is not valid in DT. It does not
fulfil the DT naming requirement etc. So please move this up inside
the if (is_acpi_node(child)) clause.

> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 36ca2b5c22533..53e693b3430ec 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -656,6 +656,7 @@ struct phy_device {
>  	/* Energy efficient ethernet modes which should be prohibited */
>  	u32 eee_broken_modes;
>  
> +	bool use_firmware_led;

There is probably a two byte hole after mdix_ctrl. So please consider
adding it there. Also, don't forget to update the documentation.

       Andrew