Re: [PATCH v3 08/18] iio: adc: stm32: Simplify with dev_err_probe()

[Peter Rosin]

Hi!

On 2020-09-09 21:57, Krzysztof Kozlowski wrote:
> On Wed, 9 Sep 2020 at 20:36, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>>
>> On Sat, 29 Aug 2020 08:47:16 +0200
>> Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
>>
>>> Common pattern of handling deferred probe can be simplified with
>>> dev_err_probe().  Less code and also it prints the error value.
>>>
>>> Signed-off-by: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
>>>
>> I don't have the thread to hand, but this tripped a warning next
>> and the patch was dropped as a result. See below.
> 
> Thanks for letting me know. If you mean the warning caused by:
> https://lore.kernel.org/lkml/20200909073716.GA560912@xxxxxxxxx/
> then the driver-core patch was dropped, not the iio one:
> https://lore.kernel.org/linux-next/20200909074130.GB561485@xxxxxxxxx/T/#t
> 
> So we are good here :)

No, we are definitely not good. See below. That means "See below", and
not "Please take a guess at what is being talking about".

> Best regards,
> Krzysztof
> 
>> Jonathan
>>> ---
>>>
>>> Changes since v2:
>>> 1. Wrap dev_err_probe() lines at 80 character
>>>
>>> Changes since v1:
>>> 1. Convert to devm_clk_get_optional
>>> 2. Update also stm32-dfsdm-core and stm32-dac-core.
>>> 3. Wrap around 100 characters (accepted by checkpatch).
>>> ---
>>>  drivers/iio/adc/stm32-adc-core.c   | 75 ++++++++++--------------------
>>>  drivers/iio/adc/stm32-adc.c        | 10 ++--
>>>  drivers/iio/adc/stm32-dfsdm-adc.c  | 10 ++--
>>>  drivers/iio/adc/stm32-dfsdm-core.c |  9 ++--
>>>  drivers/iio/dac/stm32-dac-core.c   |  5 +-
>>>  5 files changed, 35 insertions(+), 74 deletions(-)
>>>
>>> diff --git a/drivers/iio/adc/stm32-adc-core.c b/drivers/iio/adc/stm32-adc-core.c
>>> index 0e2068ec068b..3f27b4817a42 100644
>>> --- a/drivers/iio/adc/stm32-adc-core.c
>>> +++ b/drivers/iio/adc/stm32-adc-core.c
>>> @@ -582,11 +582,9 @@ static int stm32_adc_core_switches_probe(struct device *dev,
>>>       priv->syscfg = syscon_regmap_lookup_by_phandle(np, "st,syscfg");
>>>       if (IS_ERR(priv->syscfg)) {
>>>               ret = PTR_ERR(priv->syscfg);
>>> -             if (ret != -ENODEV) {
>>> -                     if (ret != -EPROBE_DEFER)
>>> -                             dev_err(dev, "Can't probe syscfg: %d\n", ret);
>>> -                     return ret;
>>> -             }
>>> +             if (ret != -ENODEV)
>>> +                     return dev_err_probe(dev, ret, "Can't probe syscfg\n");
>>> +
>>>               priv->syscfg = NULL;
>>>       }
>>>
>>> @@ -596,12 +594,9 @@ static int stm32_adc_core_switches_probe(struct device *dev,
>>>               priv->booster = devm_regulator_get_optional(dev, "booster");
>>>               if (IS_ERR(priv->booster)) {
>>>                       ret = PTR_ERR(priv->booster);
>>> -                     if (ret != -ENODEV) {
>>> -                             if (ret != -EPROBE_DEFER)
>>> -                                     dev_err(dev, "can't get booster %d\n",
>>> -                                             ret);
>>> -                             return ret;
>>> -                     }
>>> +                     if (ret != -ENODEV)
>>> +                             dev_err_probe(dev, ret, "can't get booster\n");
>>
>> This tripped a warning and got the patch dropped because we no longer
>> return on error.

As Jonathan already said, we no longer return in this hunk. I.e., you have
clobbered the error path.

Cheers,
Peter