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

[Josua Mayer <josua@solid-run.com>]

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