diff mbox series

[2/4] ASoC: codecs: wcd934x: Simplify with dev_err_probe

Message ID 20230417141453.919158-2-krzysztof.kozlowski@linaro.org
State New
Headers
Series [1/4] ASoC: codecs: wcd9335: Simplify with dev_err_probe |

Commit Message

Krzysztof Kozlowski April 17, 2023, 2:14 p.m. UTC 
  Code can be a bit simpler with dev_err_probe().

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 sound/soc/codecs/wcd934x.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

Comments

Mark Brown April 17, 2023, 3:33 p.m. UTC | #1 
On Mon, Apr 17, 2023 at 04:14:51PM +0200, Krzysztof Kozlowski wrote:

> Code can be a bit simpler with dev_err_probe().

> -	if (IS_ERR(wcd->if_regmap)) {
> -		dev_err(dev, "Failed to allocate ifc register map\n");
> -		return PTR_ERR(wcd->if_regmap);
> -	}
> +	if (IS_ERR(wcd->if_regmap))
> +		return dev_err_probe(dev, PTR_ERR(wcd->if_regmap),
> +				     "Failed to allocate ifc register map\n");

This is a functional change.
Krzysztof Kozlowski April 17, 2023, 3:43 p.m. UTC | #2 
On 17/04/2023 17:33, Mark Brown wrote:
> On Mon, Apr 17, 2023 at 04:14:51PM +0200, Krzysztof Kozlowski wrote:
> 
>> Code can be a bit simpler with dev_err_probe().
> 
>> -	if (IS_ERR(wcd->if_regmap)) {
>> -		dev_err(dev, "Failed to allocate ifc register map\n");
>> -		return PTR_ERR(wcd->if_regmap);
>> -	}
>> +	if (IS_ERR(wcd->if_regmap))
>> +		return dev_err_probe(dev, PTR_ERR(wcd->if_regmap),
>> +				     "Failed to allocate ifc register map\n");
> 
> This is a functional change.

Hmm... I don't see it. Return value is the same, same message is
printed, same condition. Did I make some copy-paste error?

Best regards,
Krzysztof
Mark Brown April 17, 2023, 3:58 p.m. UTC | #3 
On Mon, Apr 17, 2023 at 05:43:03PM +0200, Krzysztof Kozlowski wrote:
> On 17/04/2023 17:33, Mark Brown wrote:
> > On Mon, Apr 17, 2023 at 04:14:51PM +0200, Krzysztof Kozlowski wrote:

> >> -	if (IS_ERR(wcd->if_regmap)) {
> >> -		dev_err(dev, "Failed to allocate ifc register map\n");
> >> -		return PTR_ERR(wcd->if_regmap);
> >> -	}
> >> +	if (IS_ERR(wcd->if_regmap))
> >> +		return dev_err_probe(dev, PTR_ERR(wcd->if_regmap),
> >> +				     "Failed to allocate ifc register map\n");

> > This is a functional change.

> Hmm... I don't see it. Return value is the same, same message is
> printed, same condition. Did I make some copy-paste error?

You've replaced an unconditional dev_err() with dev_err_probe().
Krzysztof Kozlowski April 17, 2023, 4 p.m. UTC | #4 
On 17/04/2023 17:58, Mark Brown wrote:
> On Mon, Apr 17, 2023 at 05:43:03PM +0200, Krzysztof Kozlowski wrote:
>> On 17/04/2023 17:33, Mark Brown wrote:
>>> On Mon, Apr 17, 2023 at 04:14:51PM +0200, Krzysztof Kozlowski wrote:
> 
>>>> -	if (IS_ERR(wcd->if_regmap)) {
>>>> -		dev_err(dev, "Failed to allocate ifc register map\n");
>>>> -		return PTR_ERR(wcd->if_regmap);
>>>> -	}
>>>> +	if (IS_ERR(wcd->if_regmap))
>>>> +		return dev_err_probe(dev, PTR_ERR(wcd->if_regmap),
>>>> +				     "Failed to allocate ifc register map\n");
> 
>>> This is a functional change.
> 
>> Hmm... I don't see it. Return value is the same, same message is
>> printed, same condition. Did I make some copy-paste error?
> 
> You've replaced an unconditional dev_err() with dev_err_probe().

Which is the core of this change... so what is here surprising? Yes,
that's functional change and I never wrote that dev_err_probe is equal
dev_err. It is similar but offers benefits and one difference - does not
print DEFER. Which is in general exactly what we want.

Best regards,
Krzysztof
Mark Brown April 17, 2023, 4:27 p.m. UTC | #5 
On Mon, Apr 17, 2023 at 06:00:59PM +0200, Krzysztof Kozlowski wrote:
> On 17/04/2023 17:58, Mark Brown wrote:

> > You've replaced an unconditional dev_err() with dev_err_probe().

> Which is the core of this change... so what is here surprising? Yes,
> that's functional change and I never wrote that dev_err_probe is equal
> dev_err. It is similar but offers benefits and one difference - does not
> print DEFER. Which is in general exactly what we want.

This may well be what you intended to do but it's not what the commit
message says that the change is doing, unlike patch 1 this isn't an open
coded dev_err_probe() that's being replaced.
Krzysztof Kozlowski April 17, 2023, 4:32 p.m. UTC | #6 
On 17/04/2023 18:27, Mark Brown wrote:
> On Mon, Apr 17, 2023 at 06:00:59PM +0200, Krzysztof Kozlowski wrote:
>> On 17/04/2023 17:58, Mark Brown wrote:
> 
>>> You've replaced an unconditional dev_err() with dev_err_probe().
> 
>> Which is the core of this change... so what is here surprising? Yes,
>> that's functional change and I never wrote that dev_err_probe is equal
>> dev_err. It is similar but offers benefits and one difference - does not
>> print DEFER. Which is in general exactly what we want.
> 
> This may well be what you intended to do but it's not what the commit
> message says that the change is doing, unlike patch 1 this isn't an open
> coded dev_err_probe() that's being replaced.

But my patch 1 (and my other patches some time ago for wsa speakers)
does exactly the same as this one here in few other places - introduces
better message printing of EPROBE_DEFER. Only in one place it is equivalent.

If you prefer, I can mention the message/EPROBE_DEFER difference in
commit msgs.

Best regards,
Krzysztof
Mark Brown April 17, 2023, 5:23 p.m. UTC | #7 
On Mon, Apr 17, 2023 at 06:32:07PM +0200, Krzysztof Kozlowski wrote:

> If you prefer, I can mention the message/EPROBE_DEFER difference in
> commit msgs.

I know you prefer to maintain exacting standards in these areas.
Krzysztof Kozlowski April 17, 2023, 5:30 p.m. UTC | #8 
On 17/04/2023 19:23, Mark Brown wrote:
> On Mon, Apr 17, 2023 at 06:32:07PM +0200, Krzysztof Kozlowski wrote:
> 
>> If you prefer, I can mention the message/EPROBE_DEFER difference in
>> commit msgs.
> 
> I know you prefer to maintain exacting standards in these areas.

I don't understand what do you mean here. Do you wish me to re-phrase
the commit msg or change something in the code?

Best regards,
Krzysztof
Mark Brown April 17, 2023, 5:46 p.m. UTC | #9 
On Mon, Apr 17, 2023 at 07:30:35PM +0200, Krzysztof Kozlowski wrote:
> On 17/04/2023 19:23, Mark Brown wrote:
> > On Mon, Apr 17, 2023 at 06:32:07PM +0200, Krzysztof Kozlowski wrote:

> >> If you prefer, I can mention the message/EPROBE_DEFER difference in
> >> commit msgs.

> > I know you prefer to maintain exacting standards in these areas.

> I don't understand what do you mean here. Do you wish me to re-phrase
> the commit msg or change something in the code?

Your commit message does not accurately describe the change the patch
makes, it should do so.
diff mbox series

Patch

diff --git a/sound/soc/codecs/wcd934x.c b/sound/soc/codecs/wcd934x.c
index 783479a4d535..56487ad2f048 100644
--- a/sound/soc/codecs/wcd934x.c
+++ b/sound/soc/codecs/wcd934x.c
@@ -5868,10 +5868,9 @@  static int wcd934x_codec_parse_data(struct wcd934x_codec *wcd)
 	slim_get_logical_addr(wcd->sidev);
 	wcd->if_regmap = regmap_init_slimbus(wcd->sidev,
 				  &wcd934x_ifc_regmap_config);
-	if (IS_ERR(wcd->if_regmap)) {
-		dev_err(dev, "Failed to allocate ifc register map\n");
-		return PTR_ERR(wcd->if_regmap);
-	}
+	if (IS_ERR(wcd->if_regmap))
+		return dev_err_probe(dev, PTR_ERR(wcd->if_regmap),
+				     "Failed to allocate ifc register map\n");
 
 	of_property_read_u32(dev->parent->of_node, "qcom,dmic-sample-rate",
 			     &wcd->dmic_sample_rate);
@@ -5923,19 +5922,15 @@  static int wcd934x_codec_probe(struct platform_device *pdev)
 	memcpy(wcd->tx_chs, wcd934x_tx_chs, sizeof(wcd934x_tx_chs));
 
 	irq = regmap_irq_get_virq(data->irq_data, WCD934X_IRQ_SLIMBUS);
-	if (irq < 0) {
-		dev_err(wcd->dev, "Failed to get SLIM IRQ\n");
-		return irq;
-	}
+	if (irq < 0)
+		return dev_err_probe(wcd->dev, irq, "Failed to get SLIM IRQ\n");
 
 	ret = devm_request_threaded_irq(dev, irq, NULL,
 					wcd934x_slim_irq_handler,
 					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
 					"slim", wcd);
-	if (ret) {
-		dev_err(dev, "Failed to request slimbus irq\n");
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to request slimbus irq\n");
 
 	wcd934x_register_mclk_output(wcd);
 	platform_set_drvdata(pdev, wcd);