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

Josua Mayer
2022-05-08T09:34:21Z

Hi Nate, Maintainers,

This patch is now superseded by Nate's
[PATCH] phy: adin: fix broken support for adi,phy-mode-override
submitted yesterday.

The remainder of this patchset cleanly applies on top of it though,
so I won't roll v2.

Let me know if there is any further feedback on this series, or if it
can be applied.

Am 05.05.22 um 18:30 schrieb Josua Mayer:
> \o/
>
> Am 04.05.22 um 15:46 schrieb Nate Drude:
>> Hi Josua,
>>
>> On Wed, 2022-05-04 at 11:51 +0300, Josua Mayer wrote:
>>> 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://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fu-boot%2Fu-boot%2Fcommit%2F123ca114e07ecf28aa2538748d733e2b22d8b8b5&amp;data=05%7C01%7CNate.D%40variscite.com%7C467af650626f4902ae2b08da2dab6225%7C399ae6ac38f44ef094a8440b0ad581de%7C1%7C0%7C637872511359995609%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=Ok3YOO8rheEZuRa%2Fn04NZf37TsPQ7hzArUojFZ4QDUg%3D&amp;reserved=0
>>>>
>>> :(
>>>> 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
>> If you prefer, I can submit a separate patch to fix it. I'll wait to
>> hear from you to avoid duplicated work.
> I won't be able to look at this before Sunday, so feel free to work on
> it.
>>> 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
>> Thanks,
>> Nate
>>
- Josua Mayer