Message-ID:
 <CY4PR03MB339993CDE9BA8DD3976CF2F29B5C2@CY4PR03MB3399.namprd03.prod.outlook.com>
Date: Thu, 7 Nov 2024 10:51:49 +0000
From: "Miclaus, Antoniu" <Antoniu.Miclaus@...log.com>
To: David Lechner <dlechner@...libre.com>,
        "jic23@...nel.org"
	<jic23@...nel.org>,
        "conor+dt@...nel.org" <conor+dt@...nel.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-pwm@...r.kernel.org" <linux-pwm@...r.kernel.org>
Subject: RE: [PATCH v5 6/6] iio: adc: ad4851: add ad485x driver

> > +	if (osr == 1) {
> > +		ret = regmap_update_bits(st->regmap, AD4851_REG_PACKET,
> > +					 AD4851_PACKET_FORMAT_MASK,
> 0);
> 
> regmap_clear_bits()
> 
> > +		if (ret)
> > +			return ret;
> > +
> > +		st->resolution_boost_enabled = false;
> > +	} else {
> > +		ret = regmap_update_bits(st->regmap, AD4851_REG_PACKET,
> > +					 AD4851_PACKET_FORMAT_MASK,
> 1);
> 
> regmap_set_bits()
Packet format is 2 bits wide. Not sure how can I write 1 if I use regmap set_bits
Should I do 2 separate masks?
> 
> > +		if (ret)
> > +			return ret;
> > +
> > +		st->resolution_boost_enabled = true;
> 
> Technically speaking, 16-bit chips don't have resolution boost. And
> selecting PACKET_FORMAT = 1 here would enable extra status bits that
> we aren't using. I don't think that is what we want.
> 
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int ad4851_get_oversampling_ratio(struct ad4851_state *st,
> unsigned int *val)
> > +{
> > +	unsigned int osr;
> > +	int ret;
> 
> 	guard(mutex)(&st->lock);
> > +
> > +	ret = regmap_read(st->regmap, AD4851_REG_OVERSAMPLE, &osr);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (!FIELD_GET(AD4851_OS_EN_MSK, osr))
> > +		*val = 1;
> > +	else
> > +		*val =
> ad4851_oversampling_ratios[FIELD_GET(AD4851_OS_RATIO_MSK, osr)];
> > +
> > +	return IIO_VAL_INT;
> > +}
> > +
> > +static void ad4851_reg_disable(void *data)
> > +{
> > +	regulator_disable(data);
> > +}
> > +
> > +static int ad4851_setup(struct ad4851_state *st)
> > +{
> > +	unsigned int product_id;
> > +	int ret;
> > +
> > +	if (st->pd_gpio) {
> > +		gpiod_set_value(st->pd_gpio, GPIOD_OUT_HIGH);
> > +		fsleep(1);
> > +		gpiod_set_value(st->pd_gpio, GPIOD_OUT_LOW);
> > +		fsleep(1);
> > +		gpiod_set_value(st->pd_gpio, GPIOD_OUT_HIGH);
> > +		fsleep(1);
> > +		gpiod_set_value(st->pd_gpio, GPIOD_OUT_LOW);
> 
> The GPIOD_OUT_* macros are not valid here. Just use 0 and 1.
> Also, we can use gpiod_set_value_cansleep() here.
> 
> > +		fsleep(1000);
> > +	}
> > +
> > +	if (!IS_ERR(st->vrefbuf)) {
> > +		ret = regmap_update_bits(st->regmap,
> AD4851_REG_DEVICE_CTRL,
> > +					 AD4851_REFBUF_PD,
> AD4851_REFBUF_PD);
> 
> Can be simplified to regmap_set_bits().
> 
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = regulator_enable(st->vrefbuf);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = devm_add_action_or_reset(&st->spi->dev,
> ad4851_reg_disable,
> > +					       st->vrefbuf);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	if (!IS_ERR(st->vrefio)) {
> > +		ret = regmap_update_bits(st->regmap,
> AD4851_REG_DEVICE_CTRL,
> > +					 AD4851_REFSEL_PD,
> AD4851_REFSEL_PD);
> 
> Can be simplified to regmap_set_bits().
> 
> 
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = regulator_enable(st->vrefio);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = devm_add_action_or_reset(&st->spi->dev,
> ad4851_reg_disable,
> > +					       st->vrefio);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	ret = ad4851_set_sampling_freq(st, HZ_PER_MHZ);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_write(st->regmap,
> AD4851_REG_INTERFACE_CONFIG_A,
> > +			   AD4851_SW_RESET);
> > +	if (ret)
> > +		return ret;
> 
> Probably need a time delay after reset. Also we should not need to
> call this if we were able to reset using the PD gpio since the chip
> was already reset.
> 
> Also, also, this needs to be moved before other register writes
> above, otherwise it will reset those registers.
> 
> > +
> > +	ret = regmap_write(st->regmap,
> AD4851_REG_INTERFACE_CONFIG_B,
> > +			   AD4851_SINGLE_INSTRUCTION);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_write(st->regmap,
> AD4851_REG_INTERFACE_CONFIG_A,
> > +			   AD4851_SDO_ENABLE);
> > +	if (ret)
> > +		return ret;
> 
> This one also needs to be done before any regmap reads (including
> update_bits, set_bits, clear_bits that implicitly read).
> 
> > +
> > +	ret = regmap_read(st->regmap, AD4851_REG_PRODUCT_ID_L,
> &product_id);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (product_id != st->info->product_id)
> > +		dev_info(&st->spi->dev, "Unknown product ID: 0x%02X\n",
> > +			 product_id);
> > +
> > +	ret = regmap_write(st->regmap, AD4851_REG_DEVICE_CTRL,
> > +			   AD4851_ECHO_CLOCK_MODE);
> 
> Doing regmap_write here will set all other bits to 0, which will
> clear previous config done above. Should be regmap_set_bits().
> 
> Other regmap_write() calls seem OK, but it's always nice to have a
> comment explaining why it is OK, otherwise it looks suspicios, like
> it could be hiding a bug like we have here.
> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	return regmap_write(st->regmap, AD4851_REG_PACKET, 0);
> > +}
> > +
> > +static int ad4851_find_opt(bool *field, u32 size, u32 *ret_start)
> > +{
> > +	unsigned int i, cnt = 0, max_cnt = 0, max_start = 0;
> > +	int start;
> > +
> > +	for (i = 0, start = -1; i < size; i++) {
> > +		if (field[i] == 0) {
> > +			if (start == -1)
> > +				start = i;
> > +			cnt++;
> > +		} else {
> > +			if (cnt > max_cnt) {
> > +				max_cnt = cnt;
> > +				max_start = start;
> > +			}
> > +			start = -1;
> > +			cnt = 0;
> > +		}
> > +	}
> > +	/*
> > +	 * Find the longest consecutive sequence of false values from field
> > +	 * and return starting index.
> > +	 */
> > +	if (cnt > max_cnt) {
> > +		max_cnt = cnt;
> > +		max_start = start;
> > +	}
> > +
> > +	if (!max_cnt)
> > +		return -ENOENT;
> > +
> > +	*ret_start = max_start;
> > +
> > +	return max_cnt;
> > +}
> > +
> > +static int ad4851_calibrate(struct ad4851_state *st)
> > +{
> > +	unsigned int opt_delay, lane_num, delay, i, s, c;
> > +	enum iio_backend_interface_type interface_type;
> > +	DECLARE_BITMAP(pn_status, AD4851_MAX_LANES *
> AD4851_MAX_IODELAY);
> > +	bool status;
> > +	int ret;
> > +
> > +	ret = iio_backend_interface_type_get(st->back, &interface_type);
> > +	if (ret)
> > +		return ret;
> > +
> > +	switch (interface_type) {
> > +	case IIO_BACKEND_INTERFACE_SERIAL_CMOS:
> > +		lane_num = st->info->num_channels;
> > +		break;
> > +	case IIO_BACKEND_INTERFACE_SERIAL_LVDS:
> > +		lane_num = 1;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}> +
> > +	if (st->info->resolution == 16) {
> > +		ret = iio_backend_data_size_set(st->back, 24);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = regmap_write(st->regmap, AD4851_REG_PACKET,
> > +				   AD4851_TEST_PAT |
> AD4857_PACKET_SIZE_24);
> > +		if (ret)
> > +			return ret;
> > +	} else {
> > +		ret = iio_backend_data_size_set(st->back, 32);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = regmap_write(st->regmap, AD4851_REG_PACKET,
> > +				   AD4851_TEST_PAT |
> AD4858_PACKET_SIZE_32);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	for (i = 0; i < st->info->num_channels; i++) {
> > +		ret = regmap_write(st->regmap, AD4851_REG_TESTPAT_0(i),
> > +				   AD4851_TESTPAT_0_DEFAULT);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = regmap_write(st->regmap, AD4851_REG_TESTPAT_1(i),
> > +				   AD4851_TESTPAT_1_DEFAULT);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = regmap_write(st->regmap, AD4851_REG_TESTPAT_2(i),
> > +				   AD4851_TESTPAT_2_DEFAULT);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = regmap_write(st->regmap, AD4851_REG_TESTPAT_3(i),
> > +				   AD4851_TESTPAT_3_DEFAULT(i));
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = iio_backend_chan_enable(st->back, i);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	for (i = 0; i < lane_num; i++) {
> > +		for (delay = 0; delay < AD4851_MAX_IODELAY; delay++) {
> > +			ret = iio_backend_iodelay_set(st->back, i, delay);
> > +			if (ret)
> > +				return ret;
> > +			ret = iio_backend_chan_status(st->back, i, &status);
> > +			if (ret)
> > +				return ret;
> > +
> > +			if (status)
> > +				set_bit(i * AD4851_MAX_IODELAY + delay,
> pn_status);
> > +			else
> > +				clear_bit(i * AD4851_MAX_IODELAY + delay,
> pn_status);
> > +		}
> > +	}
> > +
> > +	for (i = 0; i < lane_num; i++) {
> > +		status = test_bit(i * AD4851_MAX_IODELAY, pn_status);
> > +		c = ad4851_find_opt(&status, AD4851_MAX_IODELAY, &s);
> > +		if (c < 0)
> > +			return c;
> > +
> > +		opt_delay = s + c / 2;
> > +		ret = iio_backend_iodelay_set(st->back, i, opt_delay);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	for (i = 0; i < st->info->num_channels; i++) {
> > +		ret = iio_backend_chan_disable(st->back, i);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	ret = iio_backend_data_size_set(st->back, 20);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return regmap_write(st->regmap, AD4851_REG_PACKET, 0);
> > +}
> > +
> > +static int ad4851_get_calibscale(struct ad4851_state *st, int ch, int *val, int
> *val2)
> > +{
> > +	unsigned int reg_val;
> > +	int gain;
> > +	int ret;
> > +
> > +	guard(mutex)(&st->lock);
> > +
> > +	ret = regmap_read(st->regmap, AD4851_REG_CHX_GAIN_MSB(ch),
> > +			  &reg_val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	gain = reg_val << 8;
> > +
> > +	ret = regmap_read(st->regmap, AD4851_REG_CHX_GAIN_LSB(ch),
> > +			  &reg_val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	gain |= reg_val;
> > +
> > +	*val = gain;
> > +	*val2 = 32768;
> > +
> > +	return IIO_VAL_FRACTIONAL;
> > +}
> > +
> > +static int ad4851_set_calibscale(struct ad4851_state *st, int ch, int val,
> > +				 int val2)
> > +{
> > +	u64 gain;
> > +	u8 buf[0];
> > +	int ret;
> > +
> > +	if (val < 0 || val2 < 0)
> > +		return -EINVAL;
> > +
> > +	gain = val * MICRO + val2;
> > +	gain = DIV_U64_ROUND_CLOSEST(gain * 32768, MICRO);
> > +
> > +	put_unaligned_be16(gain, buf);
> > +
> > +	guard(mutex)(&st->lock);
> > +
> > +	ret = regmap_write(st->regmap, AD4851_REG_CHX_GAIN_MSB(ch),
> > +			   buf[0]);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return regmap_write(st->regmap, AD4851_REG_CHX_GAIN_LSB(ch),
> > +			    buf[1]);
> > +}
> > +
> 
> I'm pretty sure that calibscale and calibbias also need to take into
> account if resolution boost is enabled or not.

Can you please detail a bit on this topic? I am not sure what I should do.

> > +static int ad4851_get_calibbias(struct ad4851_state *st, int ch, int *val)
> > +{
> > +	unsigned int lsb, mid, msb;
> > +	int ret;
> > +
> > +	guard(mutex)(&st->lock);
> > +
> > +	ret = regmap_read(st->regmap, AD4851_REG_CHX_OFFSET_MSB(ch),
> > +			  &msb);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_read(st->regmap, AD4851_REG_CHX_OFFSET_MID(ch),
> > +			  &mid);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_read(st->regmap, AD4851_REG_CHX_OFFSET_LSB(ch),
> > +			  &lsb);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (st->info->resolution == 16) {
> > +		*val = msb << 8;
> > +		*val |= mid;
> > +		*val = sign_extend32(*val, 15);
> > +	} else {
> > +		*val = msb << 12;
> > +		*val |= mid << 4;
> > +		*val |= lsb >> 4;
> > +		*val = sign_extend32(*val, 19);
> > +	}
> > +
> > +	return IIO_VAL_INT;
> > +}
> > +
> > +static int ad4851_set_calibbias(struct ad4851_state *st, int ch, int val)
> > +{
> > +	u8 buf[3] = { 0 };
> > +	int ret;
> > +
> > +	if (val < 0)
> > +		return -EINVAL;
> > +
> > +	if (st->info->resolution == 16)
> > +		put_unaligned_be16(val, buf);
> > +	else
> > +		put_unaligned_be24(val << 4, buf);
> > +
> > +	guard(mutex)(&st->lock);
> > +
> > +	ret = regmap_write(st->regmap, AD4851_REG_CHX_OFFSET_LSB(ch),
> buf[2]);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_write(st->regmap, AD4851_REG_CHX_OFFSET_MID(ch),
> buf[1]);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return regmap_write(st->regmap,
> AD4851_REG_CHX_OFFSET_MSB(ch), buf[0]);
> > +}
> > +
> 
> nit: a comment mentioning this is mapping voltage ranges to register
> values would be helpful
> 
> > +static const unsigned int ad4851_scale_table[][2] = {
> > +	{ 2500, 0x0 },
> > +	{ 5000, 0x1 },
> > +	{ 5000, 0x2 },
> > +	{ 10000, 0x3 },
> > +	{ 6250, 0x04 },
> 
> nit: drop the extra 0
> 
> > +	{ 12500, 0x5 },
> > +	{ 10000, 0x6 },
> > +	{ 20000, 0x7 },
> > +	{ 12500, 0x8 },
> > +	{ 25000, 0x9 },
> > +	{ 20000, 0xA },
> > +	{ 40000, 0xB },
> > +	{ 25000, 0xC },
> > +	{ 50000, 0xD },
> > +	{ 40000, 0xE },
> > +	{ 80000, 0xF },
> > +};
> 
> I'm not sure how this table is supposed to work since there are
> multiple entries with the same voltage value. Probably better
> would be to just have the entries for the unipolar/unsigned ranges.
> Then if applying this to a differential/signed channel, just add
> 1 to resulting register value before writing it to the register.
> Or make two different tables, one for unsigned and one for signed
> channels.

It is stated in the set_scale function comment how this table works.
This table contains range-register value pair.
Always the second value corresponds to the single ended mode.
> 
> > +
> > +static const int ad4857_offset_table[] = {
> > +	0, -32768,
> > +};
> > +
> > +static const int ad4858_offset_table[] = {
> > +	0, -524288,
> > +};
> 
> These are no longer used.
> 
> > +
> > +static const unsigned int ad4851_scale_avail[] = {
> > +	2500, 5000,
> > +	10000, 6250,
> > +	12500, 20000,
> > +	25000, 40000,
> > +	50000, 80000,
> > +};
> 
> This should only go up to 40000. Or we need two different tables, one
> for signed channels and one for unsigned channels. Although it would
> probably be simpler to just multiply values from this table by 2 if
> .differential = 1 rather than having a second table.
> 
> > +
> > +static void __ad4851_get_scale(struct ad4851_state *st, int scale_tbl,
> > +			       unsigned int *val, unsigned int *val2)
> > +{
> > +	const struct ad4851_chip_info *info = st->info;
> > +	const struct iio_chan_spec *chan = &info->channels[0];
> > +	unsigned int tmp;
> > +
> > +	tmp = ((unsigned long long)scale_tbl * MICRO) >> chan-
> >scan_type.realbits;
> > +	*val = tmp / MICRO;
> > +	*val2 = tmp % MICRO;
> > +}
> > +
> > +static int ad4851_set_scale(struct ad4851_state *st,
> > +			    const struct iio_chan_spec *chan, int val, int val2)
> > +{
> > +	unsigned int scale_val[2];
> > +	unsigned int i;
> > +	bool single_ended = false;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(ad4851_scale_table); i++) {
> > +		__ad4851_get_scale(st, ad4851_scale_table[i][0],
> > +				   &scale_val[0], &scale_val[1]);
> > +		if (scale_val[0] != val || scale_val[1] != val2)
> > +			continue;
> > +
> > +		/*
> > +		 * Adjust the softspan value (differential or single ended)
> > +		 * based on the scale value selected channel type.
> > +		 *
> > +		 * If the channel is not differential then continue iterations
> > +		 * until the next matching scale value which always
> corresponds
> > +		 * to the single ended mode.
> > +		 */
> > +		if (!chan->differential && !single_ended) {
> > +			single_ended = true;
> > +			continue;
> > +		}
> > +
> > +		return regmap_write(st->regmap,
> > +				    AD4851_REG_CHX_SOFTSPAN(chan-
> >channel),
> > +				    ad4851_scale_table[i][1]);
> 
> Since we don't know if we are using single-ended or differential
> until we actually read data, we should not be setting the softspan
> register here. Instead, we should just save the selected scale
> to st->scale. Then when we do a buffered read, we can set the
> softspan correctly for each channel depending on which one is
> enabled.
> 
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static int ad4851_get_scale(struct ad4851_state *st,
> > +			    const struct iio_chan_spec *chan, int *val,
> > +			    int *val2)
> > +{
> > +	int i, softspan_val;
> > +	int ret;
> > +
> > +	ret = regmap_read(st->regmap, AD4851_REG_CHX_SOFTSPAN(chan-
> >channel),
> > +			  &softspan_val);
> > +	if (ret)
> > +		return ret;
> 
> As above, we can just return the value save in st->scale instead of
> reading the register.
> 
> > +
> > +	for (i = 0; i < ARRAY_SIZE(ad4851_scale_table); i++) {
> > +		if (softspan_val == ad4851_scale_table[i][1])
> > +			break;
> > +	}
> > +
> > +	if (i == ARRAY_SIZE(ad4851_scale_table))
> > +		return -EIO;
> > +
> > +	__ad4851_get_scale(st, ad4851_scale_table[i][0], val, val2);
> 
> If resolution boost is in effect because of oversampling, we need
> to take that into account here as well.
> 
> > +
> > +	return IIO_VAL_INT_PLUS_MICRO;
> > +}
> > +
> > +static int ad4851_scale_fill(struct ad4851_state *st)
> > +{
> > +	unsigned int i, val1, val2;
> > +
> > +	st->scales = devm_kmalloc_array(&st->spi->dev,
> ARRAY_SIZE(ad4851_scale_avail),
> > +					sizeof(*st->scales), GFP_KERNEL);
> > +	if (!st->scales)
> > +		return -ENOMEM;
> 
> It looks like this is a fixed size, so we could just include that
> size in struct ad4851_state instead of doing a kmalloc here.
> 
> > +
> > +	for (i = 0; i < ARRAY_SIZE(ad4851_scale_avail); i++) {
> > +		__ad4851_get_scale(st, ad4851_scale_avail[i], &val1, &val2);
> > +		st->scales[i][0] = val1;
> > +		st->scales[i][1] = val2;
> > +	}
> 
> Maybe simpler to make st->scales a 1-dimintional array and just
> store the index of the values in ad4851_scale_avail instead of
> copying the values?
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int ad4851_read_raw(struct iio_dev *indio_dev,
> > +			   const struct iio_chan_spec *chan,
> > +			   int *val, int *val2, long info)
> > +{
> > +	struct ad4851_state *st = iio_priv(indio_dev);
> > +
> > +	switch (info) {
> > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > +		*val = st->sampling_freq;
> > +		return IIO_VAL_INT;
> > +	case IIO_CHAN_INFO_CALIBSCALE:
> > +		return ad4851_get_calibscale(st, chan->channel, val, val2);
> > +	case IIO_CHAN_INFO_SCALE:
> > +		return ad4851_get_scale(st, chan, val, val2);
> > +	case IIO_CHAN_INFO_CALIBBIAS:
> > +		return ad4851_get_calibbias(st, chan->channel, val);
> > +		return IIO_VAL_INT;
> 
> Unreachable return.
> 
> > +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > +		return ad4851_get_oversampling_ratio(st, val);
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static int ad4851_write_raw(struct iio_dev *indio_dev,
> > +			    struct iio_chan_spec const *chan,
> > +			    int val, int val2, long info)
> > +{
> > +	struct ad4851_state *st = iio_priv(indio_dev);
> > +
> > +	switch (info) {
> > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > +		return ad4851_set_sampling_freq(st, val);
> > +	case IIO_CHAN_INFO_SCALE:
> > +		return ad4851_set_scale(st, chan, val, val2);
> > +	case IIO_CHAN_INFO_CALIBSCALE:
> > +		return ad4851_set_calibscale(st, chan->channel, val, val2);
> > +	case IIO_CHAN_INFO_CALIBBIAS:
> > +		return ad4851_set_calibbias(st, chan->channel, val);
> > +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > +		return ad4851_set_oversampling_ratio(st, chan, val);
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static int ad4851_update_scan_mode(struct iio_dev *indio_dev,
> > +				   const unsigned long *scan_mask)
> > +{
> > +	struct ad4851_state *st = iio_priv(indio_dev);
> > +	unsigned int c;
> > +	int ret;
> > +
> 
> This is where we should also write the SoftSpan register to ensure
> the correct unipolar or bipolar range is selected depending on
> which channels are enabled.
> 
> Also, we will need to check here and return error if both the
> single-ended and differential channel for any physical input
> is enabled.
> 
> > +	for (c = 0; c < st->info->num_channels; c++) {
> > +		if (test_bit(c, scan_mask))
> > +			ret = iio_backend_chan_enable(st->back, c);
> > +		else
> > +			ret = iio_backend_chan_disable(st->back, c);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int ad4851_read_avail(struct iio_dev *indio_dev,
> > +			     struct iio_chan_spec const *chan,
> > +			     const int **vals, int *type, int *length,
> > +			     long mask)
> > +{
> > +	struct ad4851_state *st = iio_priv(indio_dev);
> > +
> > +	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 = ARRAY_SIZE(ad4851_scale_avail) * 2;
> > +		return IIO_AVAIL_LIST;
> > +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > +		*vals = ad4851_oversampling_ratios;
> > +		*length = ARRAY_SIZE(ad4851_oversampling_ratios);
> > +		*type = IIO_VAL_INT;
> > +		return IIO_AVAIL_LIST;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static const struct iio_scan_type ad4851_scan_type_16[] = {
> > +	[AD4851_SCAN_TYPE_NORMAL] = {
> > +		.sign = 's',
> > +		.realbits = 16,
> > +		.storagebits = 16,
> 
> Is storagebits really 16 here? The HDL engineers mentioned that on
> other projects, they were trying to standardize on 32-bit storage
> size everywhere, even when data is <= 16 bit.
> 
> > +	},
> > +	[AD4851_SCAN_TYPE_RESOLUTION_BOOST] = {
> > +		.sign = 's',
> > +		.realbits = 16,
> > +		.storagebits = 16,
> > +	},
> > +};
> 
> NORMAL and RESOLUTION_BOOST are the same on 16-bit chips, so we
> don't actually need to implement ext_scan_type for those chips.
> 
> > +
> > +static const struct iio_scan_type ad4851_scan_type_20[] = {
> > +	[AD4851_SCAN_TYPE_NORMAL] = {
> > +		.sign = 's',
> > +		.realbits = 20,
> > +		.storagebits = 32,
> > +	},
> > +	[AD4851_SCAN_TYPE_RESOLUTION_BOOST] = {
> > +		.sign = 's',
> > +		.realbits = 24,
> > +		.storagebits = 32,
> > +	},
> > +};
> 
> The single-ended channels (differential = 0) have unsigned data, so we
> will need different structs to handle that case.
> 
> > +
> > +static int ad4851_get_current_scan_type(const struct iio_dev *indio_dev,
> > +					const struct iio_chan_spec *chan)
> > +{
> > +	struct ad4851_state *st = iio_priv(indio_dev);
> > +
> > +	return st->resolution_boost_enabled ?
> AD4851_SCAN_TYPE_RESOLUTION_BOOST
> > +					    : AD4851_SCAN_TYPE_NORMAL;
> > +}
> > +
> > +#define AD4851_IIO_CHANNEL(index, diff, real)
> 	\
> 
> nit: "bits" would make more sense to me than "real"
> 
> > +{									\
> > +	.type = IIO_VOLTAGE,						\
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_CALIBSCALE) |
> 	\
> > +		BIT(IIO_CHAN_INFO_CALIBBIAS) |
> 	\
> > +		BIT(IIO_CHAN_INFO_SCALE),				\
> > +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> 	\
> > +		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
> 	\
> > +	.info_mask_shared_by_type_available =
> 	\
> > +		BIT(IIO_CHAN_INFO_SCALE) |				\
> 
> Scale needs to be info_mask_separate_available to match
> IIO_CHAN_INFO_SCALE
> flag on info_mask_separate.
> 
> > +		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
> 	\
> > +	.indexed = 1,							\
> > +	.differential = diff,						\
> > +	.channel = index,						\
> > +	.channel2 = index + diff * 8,					\
> > +	.scan_index = index + diff * 8,					\
> > +	.ext_scan_type = ad4851_scan_type_##real,			\
> > +	.num_ext_scan_type =						\
> > +		ARRAY_SIZE(ad4851_scan_type_##real),		\
> > +}
> > +
> > +static const struct iio_chan_spec ad4858_channels[] = {
> > +	AD4851_IIO_CHANNEL(0, 0, 20),
> > +	AD4851_IIO_CHANNEL(1, 0, 20),
> > +	AD4851_IIO_CHANNEL(2, 0, 20),
> > +	AD4851_IIO_CHANNEL(3, 0, 20),
> > +	AD4851_IIO_CHANNEL(4, 0, 20),
> > +	AD4851_IIO_CHANNEL(5, 0, 20),
> > +	AD4851_IIO_CHANNEL(6, 0, 20),
> > +	AD4851_IIO_CHANNEL(7, 0, 20),
> > +	AD4851_IIO_CHANNEL(0, 1, 20),
> > +	AD4851_IIO_CHANNEL(1, 1, 20),
> > +	AD4851_IIO_CHANNEL(2, 1, 20),
> > +	AD4851_IIO_CHANNEL(3, 1, 20),
> > +	AD4851_IIO_CHANNEL(4, 1, 20),
> > +	AD4851_IIO_CHANNEL(5, 1, 20),
> > +	AD4851_IIO_CHANNEL(6, 1, 20),
> > +	AD4851_IIO_CHANNEL(7, 1, 20),
> > +};
> > +
> > +static const struct iio_chan_spec ad4857_channels[] = {
> > +	AD4851_IIO_CHANNEL(0, 0, 16),
> > +	AD4851_IIO_CHANNEL(1, 0, 16),
> > +	AD4851_IIO_CHANNEL(2, 0, 16),
> > +	AD4851_IIO_CHANNEL(3, 0, 16),
> > +	AD4851_IIO_CHANNEL(4, 0, 16),
> > +	AD4851_IIO_CHANNEL(5, 0, 16),
> > +	AD4851_IIO_CHANNEL(6, 0, 16),
> > +	AD4851_IIO_CHANNEL(7, 0, 16),
> > +	AD4851_IIO_CHANNEL(0, 1, 16),
> > +	AD4851_IIO_CHANNEL(1, 1, 16),
> > +	AD4851_IIO_CHANNEL(2, 1, 16),
> > +	AD4851_IIO_CHANNEL(3, 1, 16),
> > +	AD4851_IIO_CHANNEL(4, 1, 16),
> > +	AD4851_IIO_CHANNEL(5, 1, 16),
> > +	AD4851_IIO_CHANNEL(6, 1, 16),
> > +	AD4851_IIO_CHANNEL(7, 1, 16),
> > +};
> 
> I don't think it is valid for two channels to have the same scan_index.
> And since this is simultaneous sampling and we don't have control over
> the order in which the data is received from the backend, to get the
> ordering correct, we will likely have to make this:
> 
I am not sure which of these channels have the same index.
scan_index is index + diff * 8 in the channel definition.

> #define AD4851_IIO_CHANNEL(scan_index, channel, diff, bits) \
> ...
> 
> 	AD4851_IIO_CHANNEL(0, 0, 0, 16),
> 	AD4851_IIO_CHANNEL(1, 0, 1, 16),
> 	AD4851_IIO_CHANNEL(2, 1, 0, 16),
> 	AD4851_IIO_CHANNEL(3, 1, 1, 16),
> 	AD4851_IIO_CHANNEL(4, 2, 0, 16),
> 	AD4851_IIO_CHANNEL(5, 2, 1, 16),
> 	AD4851_IIO_CHANNEL(6, 3, 0, 16),
> 	AD4851_IIO_CHANNEL(7, 3, 1, 16),
> 	AD4851_IIO_CHANNEL(8, 4, 0, 16),
> 	AD4851_IIO_CHANNEL(9, 4, 1, 16),
> 	AD4851_IIO_CHANNEL(10, 5, 0, 16),
> 	AD4851_IIO_CHANNEL(11, 5, 1, 16),
> 	AD4851_IIO_CHANNEL(12, 6, 0, 16),
> 	AD4851_IIO_CHANNEL(13, 6, 1, 16),
> 	AD4851_IIO_CHANNEL(14, 7, 0, 16),
> 	AD4851_IIO_CHANNEL(15, 7, 1, 16),
> 
> 
> > +
> > +static const struct ad4851_chip_info ad4851_info = {
> > +	.name = "ad4851",
> > +	.product_id = 0x67,
> > +	.channels = ad4857_channels,
> > +	.num_channels = ARRAY_SIZE(ad4857_channels),
> > +	.throughput = 250 * KILO,

Powered by blists - more mailing lists