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

[Alex Elder <elder@ieee.org>]

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 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) {
>