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@xxxxxxxxx> 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@xxxxxxxxx> wrote:
>>>>>>>
>>>>>>> On 15.06.2024 15:08, Jonathan Cameron wrote:
>>>>>>>> On Wed, 12 Jun 2024 16:03:05 -0500
>>>>>>>> David Lechner <dlechner@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
>>>>>>>>
>>>>>>>> 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.
