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

On Tue, Aug 08, 2023 at 05:55:29PM +0200, Krzysztof Kozlowski wrote:

On 08/08/2023 13:47, Andi Shyti wrote:

quoted

Hi Krzysztof,

On Tue, Aug 08, 2023 at 01:31:31PM +0200, Krzysztof Kozlowski wrote:

quoted

On 08/08/2023 13:29, Andi Shyti wrote:

quoted

Hi Krzysztof,

On Tue, Aug 08, 2023 at 10:36:40AM +0200, Krzysztof Kozlowski wrote:

quoted

On 08/08/2023 03:29, Liao Chang wrote:

quoted

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 <redacted>

...

quoted

@@ -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/ (local)

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?

quoted

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/ (local)

That's double standards.

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

Thanks for your inputs,
Andi

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel