| Message ID | 20241021130221.1469099-5-aardelean@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: ad7606: add support for AD760{7,8,9} parts | expand |
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@baylibre.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()? > @@ -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. > 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 > + 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.
On Mon, Oct 21, 2024 at 9:17 PM David Lechner <dlechner@baylibre.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@baylibre.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] >
On Tue, 2024-10-22 at 09:59 +0300, Alexandru Ardelean wrote: > On Mon, Oct 21, 2024 at 9:17 PM David Lechner <dlechner@baylibre.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@baylibre.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á >
diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c index a1f0c2feb04a..115c27ae02f3 100644 --- a/drivers/iio/adc/ad7606.c +++ b/drivers/iio/adc/ad7606.c @@ -33,42 +33,44 @@ /* * Scales are computed as 5000/32768 and 10000/32768 respectively, - * so that when applied to the raw values they provide mV values + * so that when applied to the raw values they provide mV values. + * The scale arrays are kept as IIO_VAL_INT_PLUS_MICRO, so index + * X is the integer part and X + 1 is the fractional part. */ -static const unsigned int ad7606_16bit_hw_scale_avail[2] = { - 152588, 305176 +static const unsigned int ad7606_16bit_hw_scale_avail[2][2] = { + { 0, 152588 }, { 0, 305176 } }; -static const unsigned int ad7606_18bit_hw_scale_avail[2] = { - 38147, 76294 +static const unsigned int ad7606_18bit_hw_scale_avail[2][2] = { + { 0, 38147 }, { 0, 76294 } }; -static const unsigned int ad7606c_16bit_single_ended_unipolar_scale_avail[3] = { - 76294, 152588, 190735, +static const unsigned int ad7606c_16bit_single_ended_unipolar_scale_avail[3][2] = { + { 0, 76294 }, { 0, 152588 }, { 0, 190735 } }; -static const unsigned int ad7606c_16bit_single_ended_bipolar_scale_avail[5] = { - 76294, 152588, 190735, 305176, 381470 +static const unsigned int ad7606c_16bit_single_ended_bipolar_scale_avail[5][2] = { + { 0, 76294 }, { 0, 152588 }, { 0, 190735 }, { 0, 305176 }, { 0, 381470 } }; -static const unsigned int ad7606c_16bit_differential_bipolar_scale_avail[4] = { - 152588, 305176, 381470, 610352 +static const unsigned int ad7606c_16bit_differential_bipolar_scale_avail[4][2] = { + { 0, 152588 }, { 0, 305176 }, { 0, 381470 }, { 0, 610352 } }; -static const unsigned int ad7606c_18bit_single_ended_unipolar_scale_avail[3] = { - 19073, 38147, 47684 +static const unsigned int ad7606c_18bit_single_ended_unipolar_scale_avail[3][2] = { + { 0, 19073 }, { 0, 38147 }, { 0, 47684 } }; -static const unsigned int ad7606c_18bit_single_ended_bipolar_scale_avail[5] = { - 19073, 38147, 47684, 76294, 95367 +static const unsigned int ad7606c_18bit_single_ended_bipolar_scale_avail[5][2] = { + { 0, 19073 }, { 0, 38147 }, { 0, 47684 }, { 0, 76294 }, { 0, 95367 } }; -static const unsigned int ad7606c_18bit_differential_bipolar_scale_avail[4] = { - 38147, 76294, 95367, 152588 +static const unsigned int ad7606c_18bit_differential_bipolar_scale_avail[4][2] = { + { 0, 38147 }, { 0, 76294 }, { 0, 95367 }, { 0, 152588 } }; -static const unsigned int ad7606_16bit_sw_scale_avail[3] = { - 76293, 152588, 305176 +static const unsigned int ad7606_16bit_sw_scale_avail[3][2] = { + { 0, 76293 }, { 0, 152588 }, { 0, 305176 } }; static const unsigned int ad7606_oversampling_avail[7] = { @@ -663,8 +665,8 @@ static int ad7606_read_raw(struct iio_dev *indio_dev, if (st->sw_mode_en) ch = chan->address; cs = &st->chan_scales[ch]; - *val = 0; - *val2 = cs->scale_avail[cs->range]; + *val = cs->scale_avail[cs->range][0]; + *val2 = cs->scale_avail[cs->range][1]; return IIO_VAL_INT_PLUS_MICRO; case IIO_CHAN_INFO_OVERSAMPLING_RATIO: *val = st->oversampling; @@ -681,21 +683,6 @@ static int ad7606_read_raw(struct iio_dev *indio_dev, return -EINVAL; } -static ssize_t ad7606_show_avail(char *buf, const unsigned int *vals, - unsigned int n, bool micros) -{ - size_t len = 0; - int i; - - for (i = 0; i < n; i++) { - len += scnprintf(buf + len, PAGE_SIZE - len, - micros ? "0.%06u " : "%u ", vals[i]); - } - buf[len - 1] = '\n'; - - return len; -} - 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); @@ -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]; 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 + + 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, @@ -944,8 +951,8 @@ static int ad7606_read_avail(struct iio_dev *indio_dev, ch = chan->address; cs = &st->chan_scales[ch]; - *vals = cs->scale_avail_show; - *length = cs->num_scales * 2; + *vals = (int *)cs->scale_avail; + *length = cs->num_scales; *type = IIO_VAL_INT_PLUS_MICRO; return IIO_AVAIL_LIST; @@ -1068,24 +1075,9 @@ static int ad7606_chan_scales_setup(struct iio_dev *indio_dev) indio_dev->channels = chans; for (ch = 0; ch < num_channels; ch++) { - struct ad7606_chan_scale *cs; - int i; - ret = st->chip_info->scale_setup_cb(st, &chans[ch + 1], ch); if (ret) return ret; - - cs = &st->chan_scales[ch]; - - if (cs->num_scales * 2 > AD760X_MAX_SCALE_SHOW) - return dev_err_probe(st->dev, -ERANGE, - "Driver error: scale range too big"); - - /* Generate a scale_avail list for showing to userspace */ - for (i = 0; i < cs->num_scales; i++) { - cs->scale_avail_show[i * 2] = 0; - cs->scale_avail_show[i * 2 + 1] = cs->scale_avail[i]; - } } return 0; diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h index 2c629a15cc33..32c6f776c5df 100644 --- a/drivers/iio/adc/ad7606.h +++ b/drivers/iio/adc/ad7606.h @@ -103,8 +103,6 @@ struct ad7606_chip_info { /** * struct ad7606_chan_scale - channel scale configuration * @scale_avail pointer to the array which stores the available scales - * @scale_avail_show a duplicate of 'scale_avail' which is readily formatted - * such that it can be read via the 'read_avail' hook * @num_scales number of elements stored in the scale_avail array * @range voltage range selection, selects which scale to apply * @reg_offset offset for the register value, to be applied when @@ -112,9 +110,7 @@ struct ad7606_chip_info { */ struct ad7606_chan_scale { #define AD760X_MAX_SCALES 16 -#define AD760X_MAX_SCALE_SHOW (AD760X_MAX_SCALES * 2) - const unsigned int *scale_avail; - int scale_avail_show[AD760X_MAX_SCALE_SHOW]; + const unsigned int (*scale_avail)[2]; unsigned int num_scales; unsigned int range; unsigned int reg_offset;
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@baylibre.com> --- drivers/iio/adc/ad7606.c | 106 ++++++++++++++++++--------------------- drivers/iio/adc/ad7606.h | 6 +-- 2 files changed, 50 insertions(+), 62 deletions(-)