diff mbox series

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

Message ID 20220420124053.853891-3-kai.heng.feng@canonical.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [1/5] net: mdio: Mask PHY only when its ACPI node is present | expand

Checks

Context Check Description
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cover_letter warning Series does not have a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 416 this patch: 416
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 297 this patch: 297
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 401 this patch: 401
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 17 lines checked
netdev/kdoc fail Errors and warnings before: 0 this patch: 1
netdev/source_inline success Was 0 now: 0
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Kai-Heng Feng April 20, 2022, 12:40 p.m. UTC 
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.

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(+)

Comments

Andrew Lunn April 20, 2022, 1:01 p.m. UTC | #1 
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
Kai-Heng Feng April 21, 2022, 3:15 a.m. UTC | #2 
On Wed, Apr 20, 2022 at 9:01 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> 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.

OK, will do once other concerns are ironed out.

Kai-Heng

>
>        Andrew
diff mbox series

Patch

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");
+
 	return 0;
 }
 EXPORT_SYMBOL(fwnode_mdiobus_register_phy);
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;
 #ifdef CONFIG_LED_TRIGGER_PHY
 	struct phy_led_trigger *phy_led_triggers;
 	unsigned int phy_num_led_triggers;