diff mbox series

[v3,2/9] i2c: mlxbf: Use dev_err_probe in probe function

Message ID 20230808012954.1643834-3-liaochang1@huawei.com (mailing list archive)
State New, archived
Headers show
Series Use dev_err_probe in i2c probe function | expand

Commit Message

Liao, Chang Aug. 8, 2023, 1:29 a.m. UTC 
Use the dev_err_probe function instead of dev_err in the probe function
so that the printed messge includes the return value and also handles
-EPROBE_DEFER nicely.

Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
Signed-off-by: Liao Chang <liaochang1@huawei.com>
---
 drivers/i2c/busses/i2c-mlxbf.c | 50 ++++++++++++----------------------
 1 file changed, 17 insertions(+), 33 deletions(-)

Comments

Krzysztof Kozlowski Aug. 8, 2023, 8:36 a.m. UTC | #1 
On 08/08/2023 03:29, Liao Chang wrote:
> Use the dev_err_probe function instead of dev_err in the probe function
> so that the printed messge includes the return value and also handles
> -EPROBE_DEFER nicely.
> 
> Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
> Signed-off-by: Liao Chang <liaochang1@huawei.com>

...

> @@ -2413,10 +2399,8 @@ static int mlxbf_i2c_probe(struct platform_device *pdev)
>  	ret = devm_request_irq(dev, irq, mlxbf_i2c_irq,
>  			       IRQF_SHARED | IRQF_PROBE_SHARED,
>  			       dev_name(dev), priv);
> -	if (ret < 0) {
> -		dev_err(dev, "Cannot get irq %d\n", irq);
> -		return ret;
> -	}
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Cannot get irq %d\n", irq);

I don't think this is needed:
https://lore.kernel.org/all/20230721094641.77189-1-frank.li@vivo.com/

Best regards,
Krzysztof
Markus Elfring Aug. 8, 2023, 10:32 a.m. UTC | #2 
…
> so that the printed messge includes …

How do you think about to avoid another typo in similar wordings?

Regards,
Markus
Andi Shyti Aug. 8, 2023, 11:29 a.m. UTC | #3 
Hi Krzysztof,

On Tue, Aug 08, 2023 at 10:36:40AM +0200, Krzysztof Kozlowski wrote:
> On 08/08/2023 03:29, Liao Chang wrote:
> > Use the dev_err_probe function instead of dev_err in the probe function
> > so that the printed messge includes the return value and also handles
> > -EPROBE_DEFER nicely.
> > 
> > Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
> > Signed-off-by: Liao Chang <liaochang1@huawei.com>
> 
> ...
> 
> > @@ -2413,10 +2399,8 @@ static int mlxbf_i2c_probe(struct platform_device *pdev)
> >  	ret = devm_request_irq(dev, irq, mlxbf_i2c_irq,
> >  			       IRQF_SHARED | IRQF_PROBE_SHARED,
> >  			       dev_name(dev), priv);
> > -	if (ret < 0) {
> > -		dev_err(dev, "Cannot get irq %d\n", irq);
> > -		return ret;
> > -	}
> > +	if (ret < 0)
> > +		return dev_err_probe(dev, ret, "Cannot get irq %d\n", irq);
> 
> I don't think this is needed:
> https://lore.kernel.org/all/20230721094641.77189-1-frank.li@vivo.com/

Hmm, that's a bit borderline, I'd say. The change to
devm_request_irq/devm_request_threaded_irq_probe seems like
something for another series. But for now, I think I'll accept
this as it is since it fits within the scope of this current
series.

Andi
Krzysztof Kozlowski Aug. 8, 2023, 11:31 a.m. UTC | #4 
On 08/08/2023 13:29, Andi Shyti wrote:
> Hi Krzysztof,
> 
> On Tue, Aug 08, 2023 at 10:36:40AM +0200, Krzysztof Kozlowski wrote:
>> On 08/08/2023 03:29, Liao Chang wrote:
>>> Use the dev_err_probe function instead of dev_err in the probe function
>>> so that the printed messge includes the return value and also handles
>>> -EPROBE_DEFER nicely.
>>>
>>> Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
>>> Signed-off-by: Liao Chang <liaochang1@huawei.com>
>>
>> ...
>>
>>> @@ -2413,10 +2399,8 @@ static int mlxbf_i2c_probe(struct platform_device *pdev)
>>>  	ret = devm_request_irq(dev, irq, mlxbf_i2c_irq,
>>>  			       IRQF_SHARED | IRQF_PROBE_SHARED,
>>>  			       dev_name(dev), priv);
>>> -	if (ret < 0) {
>>> -		dev_err(dev, "Cannot get irq %d\n", irq);
>>> -		return ret;
>>> -	}
>>> +	if (ret < 0)
>>> +		return dev_err_probe(dev, ret, "Cannot get irq %d\n", irq);
>>
>> I don't think this is needed:
>> https://lore.kernel.org/all/20230721094641.77189-1-frank.li@vivo.com/
> 
> Hmm, that's a bit borderline, I'd say. The change to

What's borderline exactly? devm_request_threaded_irq_probe() is coming,
right? If it is accepted this hunk is useless and soon should be
replaced with proper one.

Instead of making many trivial changes doing the same, all these series
should be aligned.

> devm_request_irq/devm_request_threaded_irq_probe seems like
> something for another series. But for now, I think I'll accept
> this as it is since it fits within the scope of this current
> series.


Best regards,
Krzysztof
Andi Shyti Aug. 8, 2023, 11:47 a.m. UTC | #5 
Hi Krzysztof,

On Tue, Aug 08, 2023 at 01:31:31PM +0200, Krzysztof Kozlowski wrote:
> On 08/08/2023 13:29, Andi Shyti wrote:
> > Hi Krzysztof,
> > 
> > On Tue, Aug 08, 2023 at 10:36:40AM +0200, Krzysztof Kozlowski wrote:
> >> On 08/08/2023 03:29, Liao Chang wrote:
> >>> Use the dev_err_probe function instead of dev_err in the probe function
> >>> so that the printed messge includes the return value and also handles
> >>> -EPROBE_DEFER nicely.
> >>>
> >>> Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
> >>> Signed-off-by: Liao Chang <liaochang1@huawei.com>
> >>
> >> ...
> >>
> >>> @@ -2413,10 +2399,8 @@ static int mlxbf_i2c_probe(struct platform_device *pdev)
> >>>  	ret = devm_request_irq(dev, irq, mlxbf_i2c_irq,
> >>>  			       IRQF_SHARED | IRQF_PROBE_SHARED,
> >>>  			       dev_name(dev), priv);
> >>> -	if (ret < 0) {
> >>> -		dev_err(dev, "Cannot get irq %d\n", irq);
> >>> -		return ret;
> >>> -	}
> >>> +	if (ret < 0)
> >>> +		return dev_err_probe(dev, ret, "Cannot get irq %d\n", irq);
> >>
> >> I don't think this is needed:
> >> https://lore.kernel.org/all/20230721094641.77189-1-frank.li@vivo.com/
> > 
> > Hmm, that's a bit borderline, I'd say. The change to
> 
> What's borderline exactly? devm_request_threaded_irq_probe() is coming,
> right? If it is accepted this hunk is useless and soon should be
> replaced with proper one.

Such change is out of the scope of this series, there are two
options that I'd prefer (in the listed order):

 1. accept the patch as it is, this patch is not sent today the
    first time and at the current state it's correct.
 2. not accept a change on this line

Replacing devm_request_irq belongs to another series and,
besides, I don't want to ask Liao to hold on this series for such
trivialities.

Thank you,
Andi

> Instead of making many trivial changes doing the same, all these series
> should be aligned.
> 
> > devm_request_irq/devm_request_threaded_irq_probe seems like
> > something for another series. But for now, I think I'll accept
> > this as it is since it fits within the scope of this current
> > series.
> 
> 
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski Aug. 8, 2023, 3:55 p.m. UTC | #6 
On 08/08/2023 13:47, Andi Shyti wrote:
> Hi Krzysztof,
> 
> On Tue, Aug 08, 2023 at 01:31:31PM +0200, Krzysztof Kozlowski wrote:
>> On 08/08/2023 13:29, Andi Shyti wrote:
>>> Hi Krzysztof,
>>>
>>> On Tue, Aug 08, 2023 at 10:36:40AM +0200, Krzysztof Kozlowski wrote:
>>>> On 08/08/2023 03:29, Liao Chang wrote:
>>>>> Use the dev_err_probe function instead of dev_err in the probe function
>>>>> so that the printed messge includes the return value and also handles
>>>>> -EPROBE_DEFER nicely.
>>>>>
>>>>> Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
>>>>> Signed-off-by: Liao Chang <liaochang1@huawei.com>
>>>>
>>>> ...
>>>>
>>>>> @@ -2413,10 +2399,8 @@ static int mlxbf_i2c_probe(struct platform_device *pdev)
>>>>>  	ret = devm_request_irq(dev, irq, mlxbf_i2c_irq,
>>>>>  			       IRQF_SHARED | IRQF_PROBE_SHARED,
>>>>>  			       dev_name(dev), priv);
>>>>> -	if (ret < 0) {
>>>>> -		dev_err(dev, "Cannot get irq %d\n", irq);
>>>>> -		return ret;
>>>>> -	}
>>>>> +	if (ret < 0)
>>>>> +		return dev_err_probe(dev, ret, "Cannot get irq %d\n", irq);
>>>>
>>>> I don't think this is needed:
>>>> https://lore.kernel.org/all/20230721094641.77189-1-frank.li@vivo.com/
>>>
>>> Hmm, that's a bit borderline, I'd say. The change to
>>
>> What's borderline exactly? devm_request_threaded_irq_probe() is coming,
>> right? If it is accepted this hunk is useless and soon should be
>> replaced with proper one.
> 
> Such change is out of the scope of this series, there are two
> options that I'd prefer (in the listed order):
> 
>  1. accept the patch as it is, this patch is not sent today the
>     first time and at the current state it's correct.
>  2. not accept a change on this line

The 2 is what I commented here. This change should not be made and
instead we should just switch all such users to new API, because this is
preferred for all error messages, when applicable and does not result in
lost context. If there was no such API, sure, but we have this API coming.

> 
> Replacing devm_request_irq belongs to another series and,
> besides, I don't want to ask Liao to hold on this series for such
> trivialities.

So the comment about this redundant and unneeded change, thus switching
to new API you call 'triviality' but a comment of yours of changing the
tone of error message to 'please' is appropriate.
https://lore.kernel.org/all/20230807231320.svssge6uymw3jiho@intel.intel/

That's double standards.

Best regards,
Krzysztof
Andi Shyti Aug. 9, 2023, 8:02 a.m. UTC | #7 
On Tue, Aug 08, 2023 at 05:55:29PM +0200, Krzysztof Kozlowski wrote:
> On 08/08/2023 13:47, Andi Shyti wrote:
> > Hi Krzysztof,
> > 
> > On Tue, Aug 08, 2023 at 01:31:31PM +0200, Krzysztof Kozlowski wrote:
> >> On 08/08/2023 13:29, Andi Shyti wrote:
> >>> Hi Krzysztof,
> >>>
> >>> On Tue, Aug 08, 2023 at 10:36:40AM +0200, Krzysztof Kozlowski wrote:
> >>>> On 08/08/2023 03:29, Liao Chang wrote:
> >>>>> Use the dev_err_probe function instead of dev_err in the probe function
> >>>>> so that the printed messge includes the return value and also handles
> >>>>> -EPROBE_DEFER nicely.
> >>>>>
> >>>>> Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
> >>>>> Signed-off-by: Liao Chang <liaochang1@huawei.com>
> >>>>
> >>>> ...
> >>>>
> >>>>> @@ -2413,10 +2399,8 @@ static int mlxbf_i2c_probe(struct platform_device *pdev)
> >>>>>  	ret = devm_request_irq(dev, irq, mlxbf_i2c_irq,
> >>>>>  			       IRQF_SHARED | IRQF_PROBE_SHARED,
> >>>>>  			       dev_name(dev), priv);
> >>>>> -	if (ret < 0) {
> >>>>> -		dev_err(dev, "Cannot get irq %d\n", irq);
> >>>>> -		return ret;
> >>>>> -	}
> >>>>> +	if (ret < 0)
> >>>>> +		return dev_err_probe(dev, ret, "Cannot get irq %d\n", irq);
> >>>>
> >>>> I don't think this is needed:
> >>>> https://lore.kernel.org/all/20230721094641.77189-1-frank.li@vivo.com/
> >>>
> >>> Hmm, that's a bit borderline, I'd say. The change to
> >>
> >> What's borderline exactly? devm_request_threaded_irq_probe() is coming,
> >> right? If it is accepted this hunk is useless and soon should be
> >> replaced with proper one.
> > 
> > Such change is out of the scope of this series, there are two
> > options that I'd prefer (in the listed order):
> > 
> >  1. accept the patch as it is, this patch is not sent today the
> >     first time and at the current state it's correct.
> >  2. not accept a change on this line
> 
> The 2 is what I commented here. This change should not be made and
> instead we should just switch all such users to new API, because this is
> preferred for all error messages, when applicable and does not result in
> lost context. If there was no such API, sure, but we have this API coming.

To me the patch is correct... I am OK also with 2, but I find 1
more complete... let's say that it's a matter of taste?

> > Replacing devm_request_irq belongs to another series and,
> > besides, I don't want to ask Liao to hold on this series for such
> > trivialities.
> 
> So the comment about this redundant and unneeded change, thus switching
> to new API you call 'triviality' but a comment of yours of changing the
> tone of error message to 'please' is appropriate.
> https://lore.kernel.org/all/20230807231320.svssge6uymw3jiho@intel.intel/
> 
> That's double standards.

That was a joke, the review was somewhere else in my comment.

Thanks for your inputs,
Andi
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-mlxbf.c b/drivers/i2c/busses/i2c-mlxbf.c
index ae66bdd1b737..5ee82016c805 100644
--- a/drivers/i2c/busses/i2c-mlxbf.c
+++ b/drivers/i2c/busses/i2c-mlxbf.c
@@ -2323,10 +2323,8 @@  static int mlxbf_i2c_probe(struct platform_device *pdev)
 
 		ret = mlxbf_i2c_init_resource(pdev, &priv->smbus,
 					      MLXBF_I2C_SMBUS_RES);
-		if (ret < 0) {
-			dev_err(dev, "Cannot fetch smbus resource info");
-			return ret;
-		}
+		if (ret < 0)
+			return dev_err_probe(dev, ret, "Cannot fetch smbus resource info");
 
 		priv->timer->io = priv->smbus->io;
 		priv->mst->io = priv->smbus->io + MLXBF_I2C_MST_ADDR_OFFSET;
@@ -2334,39 +2332,29 @@  static int mlxbf_i2c_probe(struct platform_device *pdev)
 	} else {
 		ret = mlxbf_i2c_init_resource(pdev, &priv->timer,
 					      MLXBF_I2C_SMBUS_TIMER_RES);
-		if (ret < 0) {
-			dev_err(dev, "Cannot fetch timer resource info");
-			return ret;
-		}
+		if (ret < 0)
+			return dev_err_probe(dev, ret, "Cannot fetch timer resource info");
 
 		ret = mlxbf_i2c_init_resource(pdev, &priv->mst,
 					      MLXBF_I2C_SMBUS_MST_RES);
-		if (ret < 0) {
-			dev_err(dev, "Cannot fetch master resource info");
-			return ret;
-		}
+		if (ret < 0)
+			return dev_err_probe(dev, ret, "Cannot fetch master resource info");
 
 		ret = mlxbf_i2c_init_resource(pdev, &priv->slv,
 					      MLXBF_I2C_SMBUS_SLV_RES);
-		if (ret < 0) {
-			dev_err(dev, "Cannot fetch slave resource info");
-			return ret;
-		}
+		if (ret < 0)
+			return dev_err_probe(dev, ret, "Cannot fetch slave resource info");
 	}
 
 	ret = mlxbf_i2c_init_resource(pdev, &priv->mst_cause,
 				      MLXBF_I2C_MST_CAUSE_RES);
-	if (ret < 0) {
-		dev_err(dev, "Cannot fetch cause master resource info");
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Cannot fetch cause master resource info");
 
 	ret = mlxbf_i2c_init_resource(pdev, &priv->slv_cause,
 				      MLXBF_I2C_SLV_CAUSE_RES);
-	if (ret < 0) {
-		dev_err(dev, "Cannot fetch cause slave resource info");
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Cannot fetch cause slave resource info");
 
 	adap = &priv->adap;
 	adap->owner = THIS_MODULE;
@@ -2397,11 +2385,9 @@  static int mlxbf_i2c_probe(struct platform_device *pdev)
 	 * does not really hurt, then keep the code as is.
 	 */
 	ret = mlxbf_i2c_init_master(pdev, priv);
-	if (ret < 0) {
-		dev_err(dev, "failed to initialize smbus master %d",
-			priv->bus);
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed to initialize smbus master %d",
+				     priv->bus);
 
 	mlxbf_i2c_init_timings(pdev, priv);
 
@@ -2413,10 +2399,8 @@  static int mlxbf_i2c_probe(struct platform_device *pdev)
 	ret = devm_request_irq(dev, irq, mlxbf_i2c_irq,
 			       IRQF_SHARED | IRQF_PROBE_SHARED,
 			       dev_name(dev), priv);
-	if (ret < 0) {
-		dev_err(dev, "Cannot get irq %d\n", irq);
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Cannot get irq %d\n", irq);
 
 	priv->irq = irq;