linux-kernel - Re: [PATCH 4/6] iio: adc: ad7606: rework scale-available to be static

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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CA+GgBR_tT5J8Lq7DmSw9GdETiRScXzPzcf5UkDk32GLP8tSrQw@mail.gmail.com>
Date: Tue, 22 Oct 2024 09:59:16 +0300
From: Alexandru Ardelean <aardelean@...libre.com>
To: David Lechner <dlechner@...libre.com>
Cc: linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, 
	devicetree@...r.kernel.org, jic23@...nel.org, krzk+dt@...nel.org, 
	robh@...nel.org, lars@...afoo.de, michael.hennerich@...log.com, 
	gstols@...libre.com
Subject: Re: [PATCH 4/6] iio: adc: ad7606: rework scale-available to be static

On Mon, Oct 21, 2024 at 9:17 PM David Lechner <dlechner@...libre.com> wrote:
>
> On 10/21/24 8:02 AM, Alexandru Ardelean wrote:
> > The main driver for this change is the AD7607 part, which has a scale of
> > "1.220703" for the ±10V range. The AD7607 has a resolution of 14-bits.
> >
> > So, just adding the scale-available list for that part would require some
> > quirks to handle just that scale value.
> > But to do it more neatly, the best approach is to rework the scale
> > available lists to have the same format as it is returned to userspace.
> > That way, we can also get rid of the allocation for the 'scale_avail_show'
> > array.
> >
> > Signed-off-by: Alexandru Ardelean <aardelean@...libre.com>
> > ---
>
> ...
>
>
> >  static ssize_t in_voltage_scale_available_show(struct device *dev,
> >                                              struct device_attribute *attr,
> >                                              char *buf)
> > @@ -703,8 +690,16 @@ static ssize_t in_voltage_scale_available_show(struct device *dev,
> >       struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> >       struct ad7606_state *st = iio_priv(indio_dev);
> >       struct ad7606_chan_scale *cs = &st->chan_scales[0];
> > +     const unsigned int (*vals)[2] = cs->scale_avail;
> > +     unsigned int i;
> > +     size_t len = 0;
> >
> > -     return ad7606_show_avail(buf, cs->scale_avail, cs->num_scales, true);
> > +     for (i = 0; i < cs->num_scales; i++)
> > +             len += scnprintf(buf + len, PAGE_SIZE - len, "%u.%06u ",
> > +                              vals[i][0], vals[i][1]);
> > +     buf[len - 1] = '\n';
> > +
> > +     return len;
> >  }
> >
> >  static IIO_DEVICE_ATTR_RO(in_voltage_scale_available, 0);
>
> Probably a reason for this that I forgot, but why is this handled in a
> custom attribute instead of ad7606_read_avail()?

[1]
So, this is a quirk of this driver that would require a bigger cleanup.
It could be done as a different series.
Or (otherwise said) I would do it in a different series (unless
requested otherwise).

These device-level attributes are attached in the probe() of this
driver, based on the GPIO configurations provided via DT.
There's that bit of code

        if (st->gpio_os) {
                if (st->gpio_range)
                        indio_dev->info = &ad7606_info_os_and_range;
                else
                        indio_dev->info = &ad7606_info_os;
        } else {
                if (st->gpio_range)
                        indio_dev->info = &ad7606_info_range;
                else
                        indio_dev->info = &ad7606_info_no_os_or_range;
        }

The "range" there refers to "scale_available", which is only attached
like this, for HW mode.
A rework of HW-mode would be a good idea.

>
> > @@ -742,6 +737,7 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
> >                           long mask)
> >  {
> >       struct ad7606_state *st = iio_priv(indio_dev);
> > +     unsigned int scale_avail[AD760X_MAX_SCALES];
>
> Calling this scale_avail_uv would make the code easier for me to understand.

Ack.

>
> >       struct ad7606_chan_scale *cs;
> >       int i, ret, ch = 0;
> >
> > @@ -752,7 +748,12 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
> >               if (st->sw_mode_en)
> >                       ch = chan->address;
> >               cs = &st->chan_scales[ch];
> > -             i = find_closest(val2, cs->scale_avail, cs->num_scales);
> > +             for (i = 0; i < cs->num_scales; i++) {
> > +                     scale_avail[i] = cs->scale_avail[i][0] * 1000000 +
>
>                                                                  MICRO

Ack.

>
> > +                                      cs->scale_avail[i][1];
> > +             }
> > +             val = val * 1000000 + val2;
> > +             i = find_closest(val, scale_avail, cs->num_scales);
> >               ret = st->write_scale(indio_dev, ch, i + cs->reg_offset);
> >               if (ret < 0)
> >                       return ret;
> > @@ -788,9 +789,15 @@ static ssize_t ad7606_oversampling_ratio_avail(struct device *dev,
> >  {
> >       struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> >       struct ad7606_state *st = iio_priv(indio_dev);
> > +     const unsigned int *vals = st->oversampling_avail;
> > +     unsigned int i;
> > +     size_t len = 0;
> >
> > -     return ad7606_show_avail(buf, st->oversampling_avail,
> > -                              st->num_os_ratios, false);
> > +     for (i = 0; i < st->num_os_ratios; i++)
> > +             len += scnprintf(buf + len, PAGE_SIZE - len, "%u ", vals[i]);
> > +     buf[len - 1] = '\n';
> > +
> > +     return len;
> >  }
> >
> >  static IIO_DEVICE_ATTR(oversampling_ratio_available, 0444,
>
> Same question about ad7606_read_avail() instead for this one.

See [1]

>