linux-kernel - Re: [PATCH v2 1/5] iio: adc: ad7192: use devm_regulator_get_enable_read

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMknhBE=fEDdYPe1VeZwWWuvqf5TcUdM_LQGOugHCxGhSGP8-w@mail.gmail.com>
Date: Mon, 17 Jun 2024 10:28:42 -0500
From: David Lechner <dlechner@...libre.com>
To: Alisa-Dariana Roman <alisadariana@...il.com>
Cc: Jonathan Cameron <jic23@...nel.org>, Marcelo Schmitt <marcelo.schmitt1@...il.com>, 
	Nuno Sá <nuno.sa@...log.com>, 
	Michael Hennerich <Michael.Hennerich@...log.com>, Mark Brown <broonie@...nel.org>, 
	Liam Girdwood <lgirdwood@...il.com>, linux-iio@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/5] iio: adc: ad7192: use devm_regulator_get_enable_read_voltage

On Mon, Jun 17, 2024 at 9:10 AM Alisa-Dariana Roman
<alisadariana@...il.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@...il.com> wrote:
> >>>>
> >>>> On 15.06.2024 15:08, Jonathan Cameron wrote:
> >>>>> On Wed, 12 Jun 2024 16:03:05 -0500
> >>>>> David Lechner <dlechner@...libre.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@...libre.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;