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

[Alisa-Dariana Roman <alisadariana@gmail.com>]

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;