Re: [PATCH v2 1/5] iio: adc: ad7192: use devm_regulator_get_enable_read_voltage

[David Lechner <dlechner@baylibre.com>]

On 6/17/24 11:01 AM, Alisa-Dariana Roman wrote:
> On 17.06.2024 18:28, David Lechner wrote:
>> On Mon, Jun 17, 2024 at 9:10 AM Alisa-Dariana Roman
>> <alisadariana@gmail.com> wrote:
>>>
>>> On 17.06.2024 16:48, David Lechner wrote:
>>>> On 6/17/24 8:38 AM, Alisa-Dariana Roman wrote:
>>>>> On 17.06.2024 16:22, David Lechner wrote:
>>>>>> On Mon, Jun 17, 2024 at 4:35 AM Alisa-Dariana Roman
>>>>>> <alisadariana@gmail.com> wrote:
>>>>>>>
>>>>>>> On 15.06.2024 15:08, Jonathan Cameron wrote:
>>>>>>>> On Wed, 12 Jun 2024 16:03:05 -0500
>>>>>>>> David Lechner <dlechner@baylibre.com> wrote:
>>>>>>>>
>>>>>>>>> This makes use of the new devm_regulator_get_enable_read_voltage()
>>>>>>>>> function to reduce boilerplate code.
>>>>>>>>>
>>>>>>>>> Error messages have changed slightly since there are now fewer places
>>>>>>>>> where we print an error. The rest of the logic of selecting which
>>>>>>>>> supply to use as the reference voltage remains the same.
>>>>>>>>>
>>>>>>>>> Also 1000 is replaced by MILLI in a few places for consistency.
>>>>>>>>>
>>>>>>>>> Signed-off-by: David Lechner <dlechner@baylibre.com>
>>>>>>>>
>>>>>>>> Complicated bit of code, but seems correct.
>>>>>>>> However, it crossed with Alisa-Dariana switching adding a
>>>>>>>> struct device *dev = &spi->dev to probe() that I picked up earlier
>>>>>>>> today.
>>>>>>>>
>>>>>>>> I could unwind that but given Alisa-Dariana has a number of
>>>>>>>> other patches on this driver in flight, I'd like the two of you
>>>>>>>> to work out the best resolution between you.  Maybe easiest option
>>>>>>>> is that Alisa-Dariana sends this a first patch of the next
>>>>>>>> series I should pick up.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> Jonathan
>>>>>>> I will add this patch to my series and send it shortly.
>>>>>>>
>>>>>>> Kind regards,
>>>>>>> Alisa-Dariana Roman.
>>>>>>
>>>>>> Great, thanks!
>>>>>
>>>>> Just one quick question:
>>>>>
>>>>> I am getting two such warnings when running the checkpatch script:
>>>>>
>>>>> WARNING: else is not generally useful after a break or return
>>>>> #1335: FILE: ./drivers/iio/adc/ad7192.c:1335:
>>>>> +        return dev_err_probe(dev, ret, "Failed to get AVDD voltage\n");
>>>>> +    } else {
>>>>>
>>>>> Should I switch the last two branches to get rid of the warnings or just ignore them?
>>>>>
>>>>
>>>> In the other patches, I was able to reorder things to avoid this
>>>> warning, but since this one was more complicated, I just ignored
>>>> this warning.
>>>>
>>>> We can't just remove the else in this case because the return
>>>> is inside of an `else if`.
>>>
>>>          /* AVDD can optionally be used as reference voltage */
>>>          ret = devm_regulator_get_enable_read_voltage(dev, "avdd");
>>>          if (ret == -ENODEV || ret == -EINVAL) {
>>>                  /*
>>>                   * We get -EINVAL if avdd is a supply with unknown voltage. We
>>>                   * still need to enable it since it is also a power supply.
>>>                   */
>>>                  ret = devm_regulator_get_enable(dev, "avdd");
>>>                  if (ret)
>>>                          return dev_err_probe(dev, ret,
>>>                                               "Failed to enable AVDD supply\n");
>>>
>>>                  avdd_mv = 0;
>>>          } else if (ret >= 0) {
>>>                  avdd_mv = ret / MILLI;
>>>          } else {
>>>                  return dev_err_probe(dev, ret, "Failed to get AVDD voltage\n");
>>>          }
>>>
>>> Would switching the last two branches, in order to get rid of the
>>> warnings, make the code harder to understand?
>>>
>>
>> I did it in the other order because usually we like to handle the
>> error case first.
>>
>> To make it more like the other patches, we could do something like
>> this. The only thing i don't like about it is that `ret` on the very
>> last line could come from two different places. But it is logically
>> sound in the current form.
>>
>>      /* AVDD can optionally be used as reference voltage */
>>      ret = devm_regulator_get_enable_read_voltage(dev, "avdd");
>>      if (ret == -ENODEV || ret == -EINVAL) {
>>          /*
>>           * We get -EINVAL if avdd is a supply with unknown voltage. We
>>           * still need to enable it since it is also a power supply.
>>           */
>>          ret = devm_regulator_get_enable(dev, "avdd");
>>          if (ret)
>>              return dev_err_probe(dev, ret,
>>                           "Failed to enable AVDD supply\n");
>>      } else if (ret < 0) {
>>          return dev_err_probe(dev, ret, "Failed to get AVDD voltage\n");
>>      }
>>
>>      avdd_mv = ret <= 0 ? 0 : ret / MILLI;
> 
> Maybe this would make it a bit clearer, but yes, the ret == 0 could still come from two different places :(.
> 
> avdd_mv = ret == 0 ? 0 : ret / MILLI;
> 

We could make a ret2 local variable inside of the if block to avoid writing over ret.