On Tue, 2024-10-22 at 09:59 +0300, Alexandru Ardelean wrote:
> On Mon, Oct 21, 2024 at 9:17 PM David Lechner <dlechner@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > > ---
> >
> > ...
> >
> >
> > > 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á
>
