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

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

[Yuesong Li]

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);
-- 
2.34.1

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

[Simon Horman]

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.

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.

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.

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

-- 
pw-bot: cr

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

[Alex Elder]

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

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

[Hongbo Li]

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

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

[Alex Elder]

On 8/28/24 3:41 AM, 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");

This looks to me like a simple replacement with equivalent code.

Reviewed-by: Alex Elder <elder@kernel.org>


>   	}
>   
>   	ret = clk_set_rate(clk, data->core_clock_rate);