On Sun, 2023-12-10 at 11:27 +0000, Jonathan Cameron wrote:
> On Thu, 07 Dec 2023 11:02:36 +0100
> Nuno Sá <noname.nuno@xxxxxxxxx> wrote:
>
> > On Wed, 2023-12-06 at 17:01 -0600, David Lechner wrote:
> > > On Tue, Dec 5, 2023 at 11:06 AM Nuno Sa via B4 Relay
> > > <devnull+nuno.sa.analog.com@xxxxxxxxxx> wrote:
> > > >
> > > > From: Nuno Sa <nuno.sa@xxxxxxxxxx>
> > > >
> > > > When reading in_voltage_scale we can get something like:
> > > >
> > > > root@analog:/sys/bus/iio/devices/iio:device2# cat in_voltage_scale
> > > > 0.038146
> > > >
> > > > However, when reading the available options:
> > > >
> > > > root@analog:/sys/bus/iio/devices/iio:device2# cat
> > > > in_voltage_scale_available
> > > > 2000.000000 2100.000006 2200.000007 2300.000008 2400.000009 2500.000010
> > > >
> > > > which does not make sense. Moreover, when trying to set a new scale we
> > > > get an error because there's no call to __ad9467_get_scale() to give us
> > > > values as given when reading in_voltage_scale. Fix it by computing the
> > > > available scales during probe and properly pass the list when
> > > > .read_available() is called.
> > > >
> > > > While at it, change to use .read_available() from iio_info. Also note
> > > > that to properly fix this, adi-axi-adc.c has to be changed accordingly.
> > > >
> > > > Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
> > > > Signed-off-by: Nuno Sa <nuno.sa@xxxxxxxxxx>
> > > > ---
> > > > drivers/iio/adc/ad9467.c | 47 +++++++++++++++++++++++
> > > > drivers/iio/adc/adi-axi-adc.c | 74 ++++++++----------------------
> > > > -------
> > > > include/linux/iio/adc/adi-axi-adc.h | 4 ++
> > > > 3 files changed, 66 insertions(+), 59 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> > > > index badbef2ce9f8..3c8bd6c821a4 100644
> > > > --- a/drivers/iio/adc/ad9467.c
> > > > +++ b/drivers/iio/adc/ad9467.c
> > > > @@ -120,6 +120,7 @@ struct ad9467_state {
> > > > struct spi_device *spi;
> > > > struct clk *clk;
> > > > unsigned int output_mode;
> > > > + unsigned int (*scales)[2];
> > > >
> > > > struct gpio_desc *pwrdown_gpio;
> > > > /* ensure consistent state obtained on multiple related accesses
> > > > */
> > > > @@ -216,6 +217,7 @@ static void __ad9467_get_scale(struct
> > > > adi_axi_adc_conv *conv,
> > > > int index,
> > > > .channel = _chan,
> > > > \
> > > > .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |
> > > > \
> > > > BIT(IIO_CHAN_INFO_SAMP_FREQ),
> > > > \
> > > > + .info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE),
> > > > \
> > > > .scan_index = _si,
> > > > \
> > > > .scan_type = {
> > > > \
> > > > .sign = _sign,
> > > > \
> > > > @@ -370,6 +372,26 @@ static int ad9467_write_raw(struct adi_axi_adc_conv
> > > > *conv,
> > > > }
> > > > }
> > > >
> > > > +static int ad9467_read_avail(struct adi_axi_adc_conv *conv,
> > > > + struct iio_chan_spec const *chan,
> > > > + const int **vals, int *type, int *length,
> > > > + long mask)
> > > > +{
> > > > + const struct adi_axi_adc_chip_info *info = conv->chip_info;
> > > > + struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
> > > > +
> > > > + switch (mask) {
> > > > + case IIO_CHAN_INFO_SCALE:
> > > > + *vals = (const int *)st->scales;
> > > > + *type = IIO_VAL_INT_PLUS_MICRO;
> > > > + /* Values are stored in a 2D matrix */
> > > > + *length = info->num_scales * 2;
> > >
> > > Maybe use ARRAY_SIZE(*info->scales) here instead of hard-coding 2?
> >
> > Not to keen on that as the comment should already say everything about the
> > hardcoded
> > 2.
> >
> I don't mind either way, but the comment only says it's a 2D matrix, not that
> the dimension
> is 2 * num_scales - it could easily be 14 * num_scales
Oops, totally right! For some reason I was seeing the comment as "2 columns" in
the matrix which is obviously not true (but I suspect that was the intent of the
comment)
- Nuno Sá
