Re: [PATCH v20 2/6] phy: core: Add phy_get_by_of_node()

[Neil Armstrong <neil.armstrong@linaro.org>]

On 9/18/26 17:35, johannes.goede@oss.qualcomm.com wrote:
> Hi Neil,
> 
> On 18-Sep-26 16:10, Neil Armstrong wrote:
>> On 9/18/26 15:56, Bryan O'Donoghue wrote:
>>> Add new function phy_get_by_of_node() allowing lookup of a phy by
>>> device_node. Separates existing logic in _of_phy_get() into an internal
>>> helper method _of_phy_get_with_args() to allow for reuse in new method.
>>>
>>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>> ---
>>>    drivers/phy/phy-core.c  | 83 ++++++++++++++++++++++++++++++++++++++-----------
>>>    include/linux/phy/phy.h |  6 ++++
>>>    2 files changed, 71 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
>>> index 89addd732bff3..42c27a2b45159 100644
>>> --- a/drivers/phy/phy-core.c
>>> +++ b/drivers/phy/phy-core.c
>>> @@ -606,6 +606,38 @@ int phy_validate(struct phy *phy, enum phy_mode mode, int submode,
>>>    }
>>>    EXPORT_SYMBOL_GPL(phy_validate);
>>>    +/**
>>> + * _of_phy_get_with_args() - lookup and obtain a reference to a phy by of_phandle_args
>>> + * @args: of_phandle_args to the phy
>>> + *
>>> + * Returns: the phy from the provider's of_xlate, -ENODEV if disabled,
>>> + * -EPROBE_DEFER if the provider is not yet registered.
>>> + */
>>> +static struct phy *_of_phy_get_with_args(struct of_phandle_args *args)
>>> +{
>>> +    struct phy *phy;
>>> +    struct phy_provider *phy_provider;
>>> +
>>> +    lockdep_assert_held(&phy_provider_mutex);
>>> +
>>> +    phy_provider = of_phy_provider_lookup(args->np);
>>> +    if (IS_ERR(phy_provider) || !try_module_get(phy_provider->owner))
>>> +        return ERR_PTR(-EPROBE_DEFER);
>>> +
>>> +    if (!of_device_is_available(args->np)) {
>>> +        dev_warn(phy_provider->dev, "Requested PHY is disabled\n");
>>> +        phy = ERR_PTR(-ENODEV);
>>> +        goto out_put_module;
>>> +    }
>>> +
>>> +    phy = phy_provider->of_xlate(phy_provider->dev, args);
>>> +
>>> +out_put_module:
>>> +    module_put(phy_provider->owner);
>>> +
>>> +    return phy;
>>> +}
>>> +
>>>    /**
>>>     * _of_phy_get() - lookup and obtain a reference to a phy by phandle
>>>     * @np: device_node for which to get the phy
>>> @@ -620,8 +652,7 @@ EXPORT_SYMBOL_GPL(phy_validate);
>>>    static struct phy *_of_phy_get(struct device_node *np, int index)
>>>    {
>>>        int ret;
>>> -    struct phy_provider *phy_provider;
>>> -    struct phy *phy = NULL;
>>> +    struct phy *phy;
>>>        struct of_phandle_args args;
>>>          lockdep_assert_held(&phy_provider_mutex);
>>> @@ -637,22 +668,7 @@ static struct phy *_of_phy_get(struct device_node *np, int index)
>>>            goto out_put_node;
>>>        }
>>>    -    phy_provider = of_phy_provider_lookup(args.np);
>>> -    if (IS_ERR(phy_provider) || !try_module_get(phy_provider->owner)) {
>>> -        phy = ERR_PTR(-EPROBE_DEFER);
>>> -        goto out_put_node;
>>> -    }
>>> -
>>> -    if (!of_device_is_available(args.np)) {
>>> -        dev_warn(phy_provider->dev, "Requested PHY is disabled\n");
>>> -        phy = ERR_PTR(-ENODEV);
>>> -        goto out_put_module;
>>> -    }
>>> -
>>> -    phy = phy_provider->of_xlate(phy_provider->dev, &args);
>>> -
>>> -out_put_module:
>>> -    module_put(phy_provider->owner);
>>> +    phy = _of_phy_get_with_args(&args);
>>>      out_put_node:
>>>        of_node_put(args.np);
>>> @@ -1001,6 +1017,37 @@ struct phy *devm_of_phy_get_by_index(struct device *dev, struct device_node *np,
>>>    }
>>>    EXPORT_SYMBOL_GPL(devm_of_phy_get_by_index);
>>>    +/**
>>> + * phy_get_by_of_node() - lookup and obtain a reference to a phy by device_node
>>> + * @np: node containing the phy
>>> + *
>>> + * Returns: the phy associated with the device node or ERR_PTR.
>>> + */
>>> +struct phy *phy_get_by_of_node(struct device_node *np)
>>> +{
>>> +    struct of_phandle_args args = { .np = np, .args_count = 0 };
>>> +    struct phy *phy;
>>> +
>>> +    mutex_lock(&phy_provider_mutex);
>>
>> I don't really understand the usage of lockdep_assert_held(), but why
>> does _of_phy_get() uses lockdep_assert_held() and here you use mutex_lock()
>> like before patch 1 ?
> 
> Hi, not Bryan but I can answer this question.
> 
> lockdep_assert_held() check that the mutex is lock and triggers
> a WARN() when not held.
> 
> So lockdep_assert_held() is used in functions which expect to
> be called with the mutex already locked, like the new
> _of_phy_get_with_args() helper which is added here.
> 
> This new helper factors out bits of _of_phy_get(), which itself
> already has lockdep_assert_held(), but since this new helper
> also is directly called from the new phy_get_by_of_node() it
> is good for it to also check the mutex is locked itself.
> 
> And since the new phy_get_by_of_node() calls this new helper
> it must lock the mutex.
> 
> Note that _of_phy_get() is already always called with the mutex
> locked from of_phy_get(), phy_get(), or devm_of_phy_get_by_index().

Ok thanks for the detailed explanation, I missed that !

Neil

> 
> Regards,
> 
> Hans
> 
> 
> 
> 
>>
>> Neil
>>
>>> +
>>> +    phy = _of_phy_get_with_args(&args);
>>> +    if (IS_ERR(phy))
>>> +        goto out_unlock;
>>> +
>>> +    if (!try_module_get(phy->ops->owner)) {
>>> +        phy = ERR_PTR(-EPROBE_DEFER);
>>> +        goto out_unlock;
>>> +    }
>>> +
>>> +    get_device(&phy->dev);
>>> +
>>> +out_unlock:
>>> +    mutex_unlock(&phy_provider_mutex);
>>> +
>>> +    return phy;
>>> +}
>>> +EXPORT_SYMBOL_GPL(phy_get_by_of_node);
>>> +
>>>    /**
>>>     * phy_create() - create a new phy
>>>     * @dev: device that is creating the new phy
>>> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
>>> index ea47975e288ae..71c2e16397130 100644
>>> --- a/include/linux/phy/phy.h
>>> +++ b/include/linux/phy/phy.h
>>> @@ -284,6 +284,7 @@ struct phy *devm_of_phy_optional_get(struct device *dev, struct device_node *np,
>>>                         const char *con_id);
>>>    struct phy *devm_of_phy_get_by_index(struct device *dev, struct device_node *np,
>>>                         int index);
>>> +struct phy *phy_get_by_of_node(struct device_node *np);
>>>    void of_phy_put(struct phy *phy);
>>>    void phy_put(struct device *dev, struct phy *phy);
>>>    void devm_phy_put(struct device *dev, struct phy *phy);
>>> @@ -493,6 +494,11 @@ static inline struct phy *devm_of_phy_get_by_index(struct device *dev,
>>>        return ERR_PTR(-ENOSYS);
>>>    }
>>>    +static inline struct phy *phy_get_by_of_node(struct device_node *np)
>>> +{
>>> +    return ERR_PTR(-ENOSYS);
>>> +}
>>> +
>>>    static inline void of_phy_put(struct phy *phy)
>>>    {
>>>    }
>>>
>>
>>
>