Re: [PATCH v1] net: ipa: make use of dev_err_cast_probe()

[Hongbo Li <lihongbo22@huawei.com>]

On 2024/8/29 9:27, Alex Elder wrote:
> On 8/28/24 11:07 AM, Simon Horman wrote:
>> On Wed, Aug 28, 2024 at 04:41:15PM +0800, Yuesong Li wrote:
>>> Using dev_err_cast_probe() to simplify the code.
>>>
>>> Signed-off-by: Yuesong Li <liyuesong@vivo.com>
>>> ---
>>>   drivers/net/ipa/ipa_power.c | 5 ++---
>>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/ipa/ipa_power.c b/drivers/net/ipa/ipa_power.c
>>> index 65fd14da0f86..248bcc0b661e 100644
>>> --- a/drivers/net/ipa/ipa_power.c
>>> +++ b/drivers/net/ipa/ipa_power.c
>>> @@ -243,9 +243,8 @@ ipa_power_init(struct device *dev, const struct 
>>> ipa_power_data *data)
>>>       clk = clk_get(dev, "core");
>>>       if (IS_ERR(clk)) {
>>> -        dev_err_probe(dev, PTR_ERR(clk), "error getting core clock\n");
>>> -
>>> -        return ERR_CAST(clk);
>>> +        return dev_err_cast_probe(dev, clk,
>>> +                "error getting core clock\n");
>>>       }
>>>       ret = clk_set_rate(clk, data->core_clock_rate);
>>
>> Hi,
>>
>> There are lot of clean-up patches floating around at this time.
>> And I'm unsure if you are both on the same team or not, but in
>> any case it would be nice if there was some co-ordination.
>> Because here we have two different versions of the same patch.
>> Which, from a maintainer and reviewer pov is awkward.
I apologize for causing confusion, and I will strive to submit patches 
that are truly valuable in the future.

Thanks,
Hongbo

> 
> I just noticed this (looking at the patch from Hongbo Li).
> 
>> In principle the change(s) look(s) fine to me. But there are some minor
>> problems.
>>
>> 1. For the patch above, it should be explicitly targeted at net-next.
>>     (Or net if it was a bug fix, which it is not.)
>>
>>     Not a huge problem, as this is the default. But please keep this 
>> in mind
>>     for future posts.
>>
>>     Subject: [PATCH vX net-next]: ...
>>
>> 2. For the patch above, the {} should be dropped, as in the patch below.
> 
> Agreed.
> 
>> 3. For both patches. The dev_err_cast_probe should be line wrapped,
>>     and the indentation should align with the opening (.
>>
>>         return dev_err_cast_probe(dev, clk,
>>                       "error getting core clock\n");
>>
>> I'd like to ask you to please negotiate amongst yourselves and
>> post _just one_ v2 which addresses the minor problems highlighted above.
> 
> Thank you Simon, you are correct and I appreciate your proposed
> solution.  I'll await a followup patch (perhaps jointly signed
> off?)
> 
>                      -Alex
> 
>> Thanks!
>>
>> On Wed, Aug 28, 2024 at 08:15:51PM +0800, Hongbo Li wrote:
>>> Using dev_err_cast_probe() to simplify the code.
>>>
>>> Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
>>> ---
>>>   drivers/net/ipa/ipa_power.c | 7 ++-----
>>>   1 file changed, 2 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/net/ipa/ipa_power.c b/drivers/net/ipa/ipa_power.c
>>> index 65fd14da0f86..c572da9e9bc4 100644
>>> --- a/drivers/net/ipa/ipa_power.c
>>> +++ b/drivers/net/ipa/ipa_power.c
>>> @@ -242,11 +242,8 @@ ipa_power_init(struct device *dev, const struct 
>>> ipa_power_data *data)
>>>       int ret;
>>>       clk = clk_get(dev, "core");
>>> -    if (IS_ERR(clk)) {
>>> -        dev_err_probe(dev, PTR_ERR(clk), "error getting core clock\n");
>>> -
>>> -        return ERR_CAST(clk);
>>> -    }
>>> +    if (IS_ERR(clk))
>>> +        return dev_err_cast_probe(dev, clk, "error getting core 
>>> clock\n");
>>>       ret = clk_set_rate(clk, data->core_clock_rate);
>>>       if (ret) {
>>
> 
>