Re: [PATCH 1/5] phy: adin: remove broken support for adi,phy-mode-override

Josua Mayer
2022-05-04T08:52:20Z

Hi Nate,

Am 02.05.22 um 16:25 schrieb Nate Drude:
> Hi Josua,
>
> On Sun, 2022-05-01 at 15:41 +0300, Josua Mayer wrote:
>> The adin_get_phy_mode_override function does not compile, because it
>> is
>> missing both declaration and implementation of
>> phy_get_interface_by_name.
>>
>> Remove the whole function for now, since the missing implementation
>> is
>> not included in mainline Linux - and thus can not be copied.
>>
>> Signed-off-by: Josua Mayer <josua@solid-run.com>
>> ---
>>  drivers/net/phy/adin.c | 34 ----------------------------------
>>  1 file changed, 34 deletions(-)
>>
>> diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
>> index cff841ab3d..2433e76fea 100644
>> --- a/drivers/net/phy/adin.c
>> +++ b/drivers/net/phy/adin.c
>> @@ -94,35 +94,6 @@ static u32 adin_get_reg_value(struct phy_device
>> *phydev,
>>         return rc;
>>  }
>>
>> -/**
>> - * adin_get_phy_mode_override - Get phy-mode override for adin PHY
>> - *
>> - * The function gets phy-mode string from property 'adi,phy-mode-
>> override'
>> - * and return its index in phy_interface_strings table, or -1 in
>> error case.
>> - */
>> -int adin_get_phy_mode_override(struct phy_device *phydev)
>> -{
>> -       ofnode node = phy_get_ofnode(phydev);
>> -       const char *phy_mode_override;
>> -       const char *prop_phy_mode_override = "adi,phy-mode-override";
>> -       int override_interface;
>> -
>> -       phy_mode_override = ofnode_read_string(node,
>> prop_phy_mode_override);
>> -       if (!phy_mode_override)
>> -               return -ENODEV;
>> -
>> -       debug("%s: %s = '%s'\n",
>> -             __func__, prop_phy_mode_override, phy_mode_override);
>> -
>> -       override_interface =
>> phy_get_interface_by_name(phy_mode_override);
>> -
>> -       if (override_interface < 0)
>> -               printf("%s: %s = '%s' is not valid\n",
>> -                      __func__, prop_phy_mode_override,
>> phy_mode_override);
>> -
>> -       return override_interface;
>> -}
>> -
>>  static u16 adin_ext_read(struct phy_device *phydev, const u32
>> regnum)
>>  {
>>         u16 val;
>> @@ -148,11 +119,6 @@ static int adin_config_rgmii_mode(struct
>> phy_device *phydev)
>>  {
>>         u16 reg_val;
>>         u32 val;
>> -       int phy_mode_override = adin_get_phy_mode_override(phydev);
>> -
>> -       if (phy_mode_override >= 0) {
>> -               phydev->interface = (phy_interface_t)
>> phy_mode_override;
>> -       }
>>
>>         reg_val = adin_ext_read(phydev, ADIN1300_GE_RGMII_CFG);
>>
> The patch that introduced adin_get_phy_mode_override was tested against
> the U-Boot master branch. Unfortunately, phy_get_interface_by_name was
> removed just a few days before by:
> https://github.com/u-boot/u-boot/commit/123ca114e07ecf28aa2538748d733e2b22d8b8b5
:(
> Can we fix adin_get_phy_mode_override instead of removing it?
I will take a look in the coming days and see if I understand how to fix
this.
I don't really mind if we fix it, or remove it - removing was the faster
way getting my patch-set out of course ...
> We can
> implement part of ofnode_read_phy_mode. This needs to be tested, but
> for example:
>
> diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
> index cff841ab3d..6a05b9fd05 100644
> --- a/drivers/net/phy/adin.c
> +++ b/drivers/net/phy/adin.c
> @@ -100,27 +100,28 @@ static u32 adin_get_reg_value(struct phy_device
> *phydev,
> * The function gets phy-mode string from property 'adi,phy-mode-
> override'
> * and return its index in phy_interface_strings table, or -1 in error
> case.
> */
> -int adin_get_phy_mode_override(struct phy_device *phydev)
> +phy_interface_t adin_get_phy_mode_override(struct phy_device *phydev)
> {
> ofnode node = phy_get_ofnode(phydev);
> const char *phy_mode_override;
> const char *prop_phy_mode_override = "adi,phy-mode-override";
> - int override_interface;
> + int i;
>
> phy_mode_override = ofnode_read_string(node,
> prop_phy_mode_override);
> +
> if (!phy_mode_override)
> - return -ENODEV;
> + return PHY_INTERFACE_MODE_NA;
>
> debug("%s: %s = '%s'\n",
> __func__, prop_phy_mode_override, phy_mode_override);
>
> - override_interface =
> phy_get_interface_by_name(phy_mode_override);
> + for (i = 0; i < PHY_INTERFACE_MODE_MAX; i++)
> + if (!strcmp(phy_mode_override,
> phy_interface_strings[i]))
> + return i;
>
> - if (override_interface < 0)
> - printf("%s: %s = '%s' is not valid\n",
> - __func__, prop_phy_mode_override,
> phy_mode_override);
> + debug("%s: Invalid PHY interface '%s'\n", __func__,
> phy_mode_override);
>
> - return override_interface;
> + return PHY_INTERFACE_MODE_NA;
> }
>
> static u16 adin_ext_read(struct phy_device *phydev, const u32 regnum)
> @@ -148,10 +149,10 @@ static int adin_config_rgmii_mode(struct
> phy_device *phydev)
> {
> u16 reg_val;
> u32 val;
> - int phy_mode_override = adin_get_phy_mode_override(phydev);
> + phy_interface_t phy_mode_override =
> adin_get_phy_mode_override(phydev);
>
> - if (phy_mode_override >= 0) {
> - phydev->interface = (phy_interface_t)
> phy_mode_override;
> + if (phy_mode_override != PHY_INTERFACE_MODE_NA) {
> + phydev->interface = phy_mode_override;
> }
>
> reg_val = adin_ext_read(phydev, ADIN1300_GE_RGMII_CFG);
>
> Thanks,
> Nate