Re: [PATCH v2 1/2] iio: adc: ad7124: allow 16 channels

From: Jonathan Cameron
Date: Sat Feb 06 2021 - 10:38:03 EST

Next message: Eyal Birger: "Re: [PATCH net-next] net/packet: Improve the comment about LL header visibility criteria"
Previous message: Jonathan Cameron: "Re: [PATCH v2 0/2] iio: adc: ad7124: allow 16 channels"
In reply to: alexandru.tachici: "[PATCH v2 1/2] iio: adc: ad7124: allow 16 channels"
Next in thread: alexandru.tachici: "[PATCH v2 2/2] dt-bindings: iio: adc: ad7124: add config nodes"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, 4 Feb 2021 13:35:50 +0200
<alexandru.tachici@xxxxxxxxxx> wrote:

> From: Alexandru Tachici <alexandru.tachici@xxxxxxxxxx>
>
> AD7124-8 can have up to 16 pseudo-differential channels
> enabled simultaneously and only 8 configurations. In this
> scenario we cannot assign one configuration per channel,
> some channels will have to share configurations like, ODR,
> gain and filter parameters.
>
> This patch allows the user to specify channels and configurations
> separately in device-tree and assign, if needed, the same
> configuration to multiple channels.
>
> Signed-off-by: Alexandru Tachici <alexandru.tachici@xxxxxxxxxx>
A couple of minor comments, but the big question is whether this is a
good approach to take in general - discussion on that in reply
to the cover letter.

Jonathan

> ---
> drivers/iio/adc/ad7124.c | 183 +++++++++++++++++++++++----------------
> 1 file changed, 109 insertions(+), 74 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> index 766c73333604..0df88bea336f 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> @@ -86,6 +86,12 @@
> #define AD7124_SINC3_FILTER 2
> #define AD7124_SINC4_FILTER 0
>
> +#define AD7124_CONF_ADDR_OFFSET 20
> +#define AD7124_MAX_CONFIGS 8
> +#define AD7124_MAX_CHANNELS 16
> +
> +#define AD7124_REG_NO 57

What's this for?

> +
> enum ad7124_ids {
> ID_AD7124_4,
> ID_AD7124_8,
> @@ -136,21 +142,28 @@ struct ad7124_chip_info {
> };
>
> struct ad7124_channel_config {
> + bool enable;
> + unsigned int nr;
> enum ad7124_ref_sel refsel;
> bool bipolar;
> bool buf_positive;
> bool buf_negative;
> - unsigned int ain;
> unsigned int vref_mv;
> unsigned int pga_bits;
> unsigned int odr;
> unsigned int filter_type;
> };
>
> +struct ad7124_channel {
> + struct ad7124_channel_config *cfg;
> + unsigned int ain;
> +};
> +
> struct ad7124_state {
> const struct ad7124_chip_info *chip_info;
> struct ad_sigma_delta sd;
> - struct ad7124_channel_config *channel_config;
> + struct ad7124_channel channels[AD7124_MAX_CHANNELS];
> + struct ad7124_channel_config configs[AD7124_MAX_CONFIGS];
> struct regulator *vref[4];
> struct clk *mclk;
> unsigned int adc_control;
> @@ -243,8 +256,8 @@ static int ad7124_set_channel(struct ad_sigma_delta *sd, unsigned int channel)
> struct ad7124_state *st = container_of(sd, struct ad7124_state, sd);
> unsigned int val;
>
> - val = st->channel_config[channel].ain | AD7124_CHANNEL_EN(1) |
> - AD7124_CHANNEL_SETUP(channel);
> + val = st->channels[channel].ain | AD7124_CHANNEL_EN(1) |
> + AD7124_CHANNEL_SETUP(st->channels[channel].cfg->nr);
>
> return ad_sd_write_reg(&st->sd, AD7124_CHANNEL(channel), 2, val);
> }
> @@ -280,14 +293,13 @@ static int ad7124_set_channel_odr(struct ad7124_state *st,
> else if (odr_sel_bits > 2047)
> odr_sel_bits = 2047;
>
> - ret = ad7124_spi_write_mask(st, AD7124_FILTER(channel),
> + ret = ad7124_spi_write_mask(st, AD7124_FILTER(st->channels[channel].cfg->nr),
> AD7124_FILTER_FS_MSK,
> AD7124_FILTER_FS(odr_sel_bits), 3);
> if (ret < 0)
> return ret;
> /* fADC = fCLK / (FS[10:0] x 32) */
> - st->channel_config[channel].odr =
> - DIV_ROUND_CLOSEST(fclk, odr_sel_bits * 32);
> + st->channels[channel].cfg->odr = DIV_ROUND_CLOSEST(fclk, odr_sel_bits * 32);
>
> return 0;
> }
> @@ -301,13 +313,13 @@ static int ad7124_set_channel_gain(struct ad7124_state *st,
>
> res = ad7124_find_closest_match(ad7124_gain,
> ARRAY_SIZE(ad7124_gain), gain);
> - ret = ad7124_spi_write_mask(st, AD7124_CONFIG(channel),
> + ret = ad7124_spi_write_mask(st, AD7124_CONFIG(st->channels[channel].cfg->nr),
> AD7124_CONFIG_PGA_MSK,
> AD7124_CONFIG_PGA(res), 2);
> if (ret < 0)
> return ret;
>
> - st->channel_config[channel].pga_bits = res;
> + st->channels[channel].cfg->pga_bits = res;
>
> return 0;
> }
> @@ -317,9 +329,9 @@ static int ad7124_get_3db_filter_freq(struct ad7124_state *st,
> {
> unsigned int fadc;
>
> - fadc = st->channel_config[channel].odr;
> + fadc = st->channels[channel].cfg->odr;
>
> - switch (st->channel_config[channel].filter_type) {
> + switch (st->channels[channel].cfg->filter_type) {
> case AD7124_SINC3_FILTER:
> return DIV_ROUND_CLOSEST(fadc * 230, 1000);
> case AD7124_SINC4_FILTER:
> @@ -349,11 +361,11 @@ static int ad7124_set_3db_filter_freq(struct ad7124_state *st,
> new_odr = sinc3_3db_odr;
> }
>
> - if (st->channel_config[channel].filter_type != new_filter) {
> + if (st->channels[channel].cfg->filter_type != new_filter) {
> int ret;
>
> - st->channel_config[channel].filter_type = new_filter;
> - ret = ad7124_spi_write_mask(st, AD7124_FILTER(channel),
> + st->channels[channel].cfg->filter_type = new_filter;
> + ret = ad7124_spi_write_mask(st, AD7124_FILTER(st->channels[channel].cfg->nr),
> AD7124_FILTER_TYPE_MSK,
> AD7124_FILTER_TYPE_SEL(new_filter),
> 3);
> @@ -380,30 +392,30 @@ static int ad7124_read_raw(struct iio_dev *indio_dev,
> /* After the conversion is performed, disable the channel */
> ret = ad_sd_write_reg(&st->sd,
> AD7124_CHANNEL(chan->address), 2,
> - st->channel_config[chan->address].ain |
> + st->channels[chan->address].ain |
> AD7124_CHANNEL_EN(0));
> if (ret < 0)
> return ret;
>
> return IIO_VAL_INT;
> case IIO_CHAN_INFO_SCALE:
> - idx = st->channel_config[chan->address].pga_bits;
> - *val = st->channel_config[chan->address].vref_mv;
> - if (st->channel_config[chan->address].bipolar)
> + idx = st->channels[chan->address].cfg->pga_bits;
> + *val = st->channels[chan->address].cfg->vref_mv;
> + if (st->channels[chan->address].cfg->bipolar)
> *val2 = chan->scan_type.realbits - 1 + idx;
> else
> *val2 = chan->scan_type.realbits + idx;
>
> return IIO_VAL_FRACTIONAL_LOG2;
> case IIO_CHAN_INFO_OFFSET:
> - if (st->channel_config[chan->address].bipolar)
> + if (st->channels[chan->address].cfg->bipolar)
> *val = -(1 << (chan->scan_type.realbits - 1));
> else
> *val = 0;
>
> return IIO_VAL_INT;
> case IIO_CHAN_INFO_SAMP_FREQ:
> - *val = st->channel_config[chan->address].odr;
> + *val = st->channels[chan->address].cfg->odr;
>
> return IIO_VAL_INT;
> case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> @@ -431,12 +443,12 @@ static int ad7124_write_raw(struct iio_dev *indio_dev,
> if (val != 0)
> return -EINVAL;
>
> - if (st->channel_config[chan->address].bipolar)
> + if (st->channels[chan->address].cfg->bipolar)
> full_scale = 1 << (chan->scan_type.realbits - 1);
> else
> full_scale = 1 << chan->scan_type.realbits;
>
> - vref = st->channel_config[chan->address].vref_mv * 1000000LL;
> + vref = st->channels[chan->address].cfg->vref_mv * 1000000LL;
> res = DIV_ROUND_CLOSEST(vref, full_scale);
> gain = DIV_ROUND_CLOSEST(res, val2);
>
> @@ -550,7 +562,7 @@ static int ad7124_check_chip_id(struct ad7124_state *st)
> static int ad7124_init_channel_vref(struct ad7124_state *st,
> unsigned int channel_number)
> {
> - unsigned int refsel = st->channel_config[channel_number].refsel;
> + unsigned int refsel = st->channels[channel_number].cfg->refsel;
>
> switch (refsel) {
> case AD7124_REFIN1:
> @@ -562,13 +574,13 @@ static int ad7124_init_channel_vref(struct ad7124_state *st,
> ad7124_ref_names[refsel]);
> return PTR_ERR(st->vref[refsel]);
> }
> - st->channel_config[channel_number].vref_mv =
> + st->channels[channel_number].cfg->vref_mv =
> regulator_get_voltage(st->vref[refsel]);
> /* Conversion from uV to mV */
> - st->channel_config[channel_number].vref_mv /= 1000;
> + st->channels[channel_number].cfg->vref_mv /= 1000;
> break;
> case AD7124_INT_REF:
> - st->channel_config[channel_number].vref_mv = 2500;
> + st->channels[channel_number].cfg->vref_mv = 2500;
> st->adc_control &= ~AD7124_ADC_CTRL_REF_EN_MSK;
> st->adc_control |= AD7124_ADC_CTRL_REF_EN(1);
> return ad_sd_write_reg(&st->sd, AD7124_ADC_CONTROL,
> @@ -587,14 +599,40 @@ static int ad7124_of_parse_channel_config(struct iio_dev *indio_dev,
> struct ad7124_state *st = iio_priv(indio_dev);
> struct device_node *child;
> struct iio_chan_spec *chan;
> - struct ad7124_channel_config *chan_config;
> - unsigned int ain[2], channel = 0, tmp;
> + unsigned int ain[2], config_nr = 0, channel = 0, tmp;
> int ret;
>
> - st->num_channels = of_get_available_child_count(np);
> - if (!st->num_channels) {
> - dev_err(indio_dev->dev.parent, "no channel children\n");
> - return -ENODEV;
> + /* parse configuration nodes */
> + for_each_available_child_of_node(np, child) {
> + ret = of_property_read_u32_array(child, "diff-channels", ain, 2);

Can we do something based on the child node name rather than a field potentially
within it?

> + if (!ret) {
> + st->num_channels++;
> + continue;
> + }
> +
> + if (ret == -EINVAL) {
> + ret = of_property_read_u32(child, "reg", &config_nr);
> + if (ret)
> + goto err;
> +
> + config_nr -= AD7124_CONF_ADDR_OFFSET;
> + st->configs[config_nr].enable = true;
> + st->configs[config_nr].nr = config_nr;
> + st->configs[config_nr].bipolar = of_property_read_bool(child, "bipolar");
> +
> + ret = of_property_read_u32(child, "adi,reference-select", &tmp);
> + if (ret)
> + st->configs[config_nr].refsel = AD7124_INT_REF;
> + else
> + st->configs[config_nr].refsel = tmp;
> +
> + st->configs[config_nr].buf_positive =
> + of_property_read_bool(child, "adi,buffered-positive");
> + st->configs[config_nr].buf_negative =
> + of_property_read_bool(child, "adi,buffered-negative");
> + } else {
> + goto err;
> + }
> }
>
> chan = devm_kcalloc(indio_dev->dev.parent, st->num_channels,
> @@ -602,46 +640,43 @@ static int ad7124_of_parse_channel_config(struct iio_dev *indio_dev,
> if (!chan)
> return -ENOMEM;
>
> - chan_config = devm_kcalloc(indio_dev->dev.parent, st->num_channels,
> - sizeof(*chan_config), GFP_KERNEL);
> - if (!chan_config)
> - return -ENOMEM;
> -
> indio_dev->channels = chan;
> indio_dev->num_channels = st->num_channels;
> - st->channel_config = chan_config;
>
> + /* parse channel nodes */
> for_each_available_child_of_node(np, child) {
> - ret = of_property_read_u32(child, "reg", &channel);
> - if (ret)
> - goto err;
> -
> - ret = of_property_read_u32_array(child, "diff-channels",
> - ain, 2);
> - if (ret)
> - goto err;
> -
> - st->channel_config[channel].ain = AD7124_CHANNEL_AINP(ain[0]) |
> - AD7124_CHANNEL_AINM(ain[1]);
> - st->channel_config[channel].bipolar =
> - of_property_read_bool(child, "bipolar");
> -
> - ret = of_property_read_u32(child, "adi,reference-select", &tmp);
> - if (ret)
> - st->channel_config[channel].refsel = AD7124_INT_REF;
> - else
> - st->channel_config[channel].refsel = tmp;
> -
> - st->channel_config[channel].buf_positive =
> - of_property_read_bool(child, "adi,buffered-positive");
> - st->channel_config[channel].buf_negative =
> - of_property_read_bool(child, "adi,buffered-negative");
> -
> - chan[channel] = ad7124_channel_template;
> - chan[channel].address = channel;
> - chan[channel].scan_index = channel;
> - chan[channel].channel = ain[0];
> - chan[channel].channel2 = ain[1];
> + ret = of_property_read_u32_array(child, "diff-channels", ain, 2);
> + if (!ret) {
> + ret = of_property_read_u32(child, "reg", &channel);
> + if (ret)
> + goto err;
> +
> + ret = of_property_read_u32_array(child, "diff-channels", ain, 2);
> + if (ret)
> + goto err;
> +
> + st->channels[channel].ain = AD7124_CHANNEL_AINP(ain[0]) |
> + AD7124_CHANNEL_AINM(ain[1]);
> +
> + ret = of_property_read_u32(child, "adi,configuration", &config_nr);
> + if (ret)
> + goto err;
> +
> + config_nr -= AD7124_CONF_ADDR_OFFSET;
> + if (!st->configs[config_nr].enable) {
> + dev_err(&st->sd.spi->dev, "Configuration %u not specified in DT.\n",
> + config_nr);
> + return -EINVAL;
> + }
> +
> + st->channels[channel].cfg = &st->configs[config_nr];
> +
> + chan[channel] = ad7124_channel_template;
> + chan[channel].address = channel;
> + chan[channel].scan_index = channel;
> + chan[channel].channel = ain[0];
> + chan[channel].channel2 = ain[1];
> + }
> }
>
> return 0;
> @@ -678,7 +713,7 @@ static int ad7124_setup(struct ad7124_state *st)
> return ret;
>
> for (i = 0; i < st->num_channels; i++) {
> - val = st->channel_config[i].ain | AD7124_CHANNEL_SETUP(i);
> + val = st->channels[i].ain | AD7124_CHANNEL_SETUP(i);
> ret = ad_sd_write_reg(&st->sd, AD7124_CHANNEL(i), 2, val);
> if (ret < 0)
> return ret;
> @@ -687,13 +722,13 @@ static int ad7124_setup(struct ad7124_state *st)
> if (ret < 0)
> return ret;
>
> - tmp = (st->channel_config[i].buf_positive << 1) +
> - st->channel_config[i].buf_negative;
> + tmp = (st->channels[i].cfg->buf_positive << 1) +
> + st->channels[i].cfg->buf_negative;
>
> - val = AD7124_CONFIG_BIPOLAR(st->channel_config[i].bipolar) |
> - AD7124_CONFIG_REF_SEL(st->channel_config[i].refsel) |
> + val = AD7124_CONFIG_BIPOLAR(st->channels[i].cfg->bipolar) |
> + AD7124_CONFIG_REF_SEL(st->channels[i].cfg->refsel) |
> AD7124_CONFIG_IN_BUFF(tmp);
> - ret = ad_sd_write_reg(&st->sd, AD7124_CONFIG(i), 2, val);
> + ret = ad_sd_write_reg(&st->sd, AD7124_CONFIG(st->channels[i].cfg->nr), 2, val);
> if (ret < 0)
> return ret;
> /*

Next message: Eyal Birger: "Re: [PATCH net-next] net/packet: Improve the comment about LL header visibility criteria"
Previous message: Jonathan Cameron: "Re: [PATCH v2 0/2] iio: adc: ad7124: allow 16 channels"
In reply to: alexandru.tachici: "[PATCH v2 1/2] iio: adc: ad7124: allow 16 channels"
Next in thread: alexandru.tachici: "[PATCH v2 2/2] dt-bindings: iio: adc: ad7124: add config nodes"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]