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 for Android: free password hash cracker in your pocket

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

Message-ID: <9083e7ff5471e9f8c478a10a67a7c606b81c6287.camel@gmail.com>
Date: Tue, 22 Oct 2024 10:28:40 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: Alexandru Ardelean <aardelean@...libre.com>, 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 Tue, 2024-10-22 at 09:59 +0300, Alexandru Ardelean wrote:
> 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).

Agreed...

> 
> 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.
> 

Maybe it's also due to .read_avail() not being around when the driver was first
added (but not sure about that).

- Nuno Sá

>