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