Message-ID: <Z_k3e1DfxmcJgQeu@debian-BULLSEYE-live-builder-AMD64>
Date: Fri, 11 Apr 2025 12:38:35 -0300
From: Marcelo Schmitt <marcelo.schmitt1@...il.com>
To: Nuno Sá <noname.nuno@...il.com>
Cc: Marcelo Schmitt <marcelo.schmitt@...log.com>, linux-iio@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	Ana-Maria Cusco <ana-maria.cusco@...log.com>, jic23@...nel.org,
	lars@...afoo.de, Michael.Hennerich@...log.com,
	dlechner@...libre.com, nuno.sa@...log.com, andy@...nel.org,
	robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org
Subject: Re: [PATCH v1 2/7] iio: adc: Add basic support for AD4170

Hi Nuno, thank you for your review.
I've already applied most of your suggestions.
Also providing answers to some questions inline.

Thanks

On 04/10, Nuno Sá wrote:
> Hi Marecelo,
> 
> First, superficial look...
> 
> On Wed, 2025-04-09 at 09:24 -0300, Marcelo Schmitt wrote:
> > From: Ana-Maria Cusco <ana-maria.cusco@...log.com>
> > 
> > Add support for the AD4170 ADC with the following features:
> > - Single-shot read.
> > - Analog front end PGA configuration.
> > - Digital filter and sampling frequency configuration.
> > - Calibration gain and offset configuration.
> > - Differential and pseudo-differential input configuration.
> > 
> > Signed-off-by: Ana-Maria Cusco <ana-maria.cusco@...log.com>
> > Co-developed-by: Marcelo Schmitt <marcelo.schmitt@...log.com>
> > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@...log.com>
> > ---
...
> > +	ret = ad4170_set_channel_enable(st, chan->address, true);
> > +	if (ret)
> > +		return ret;
> > +
> > +	reinit_completion(&st->completion);
> 
> I would do the above right before wait_for_completion_timeout()...

Ack. Changed accordingly.

...
> > +
> > +	setup->afe &= ~AD4170_AFE_PGA_GAIN_MSK;
> > +	setup->afe |= FIELD_PREP(AD4170_AFE_PGA_GAIN_MSK, pga);
> > 
> 
> ditto...

Ack.

...
> > +static int ad4170_set_channel_freq(struct ad4170_state *st,
> > +				   struct iio_chan_spec const *chan, int val,
> > +				   int val2)
> > +{
...
> > +
> > +	old_filter_fs = setup->filter_fs;
> > +	if (f_type == AD4170_SINC5)
> > +		setup->filter_fs = ad4170_sinc5_filt_fs_tbl[i];
> > +	else
> > +		setup->filter_fs = ad4170_sinc3_filt_fs_tbl[i];
> > +
> > +	guard(mutex)(&st->lock);
> 
> Shouldn't the lock also protect the 'setup' struct?

Good catch. Changed to acquire the lock before reading and updating the setup in
all places.

...
> > +static int ad4170_parse_reference(struct ad4170_state *st,
> > +				  struct fwnode_handle *child,
> > +				  struct ad4170_setup *setup)
> > +{
> > +	struct device *dev = &st->spi->dev;
> > +	int ret;
> > +	u8 aux;
> > +
> > +	/* Positive reference buffer setup */
> > +	aux = AD4170_REF_BUF_PRE; /* Default to have precharge buffer enabled. */
> > +	ret = fwnode_property_read_u8(child, "adi,buffered-positive", &aux);
> > +	if (ret) {
> 
> Shouldn't this be if (!ret)?
> 
Yes, fixed for all off adi,buffered-positive/negative, adi,reference-select.
Thanks.

> > +		if (aux < AD4170_REF_BUF_PRE || aux > AD4170_REF_BUF_BYPASS)
> > +			return dev_err_probe(dev, -EINVAL,
> > +					     "Invalid adi,buffered-positive:
> > %u\n",
> > +					     aux);
> > +	}
> > +	setup->afe |= FIELD_PREP(AD4170_AFE_REF_BUF_P_MSK, aux);
> > +
> > +	/* Negative reference buffer setup */
> > +	aux = AD4170_REF_BUF_PRE; /* Default to have precharge buffer enabled. */
> > +	ret = fwnode_property_read_u8(child, "adi,buffered-negative", &aux);
> > +	if (ret) {
> 
> ditto
Ack

> 
> > +		if (aux < AD4170_REF_BUF_PRE || aux > AD4170_REF_BUF_BYPASS)
> > +			return dev_err_probe(dev, -EINVAL,
> > +					     "Invalid adi,buffered-negative:
> > %u\n",
> > +					     aux);
> > +	}
> > +	setup->afe |= FIELD_PREP(AD4170_AFE_REF_BUF_M_MSK, aux);
> > +
...
> > +
> > +static int ad4170_parse_adc_channel_type(struct device *dev,
> > +					 struct fwnode_handle *child,
> > +					 struct iio_chan_spec *chan)
> > +{
> > +	u32 pins[2];
> > +	int ret;
> > +
> > +	ret = fwnode_property_read_u32_array(child, "diff-channels", pins,
> > +					     ARRAY_SIZE(pins));
> > +	if (!ret) {
> > +		chan->differential = true;
> > +		chan->channel = pins[0];
> > +		chan->channel2 = pins[1];
> > +		return 0;
> > +	}
> > +	ret = fwnode_property_read_u32(child, "single-channel", &pins[0]);
> > +	if (!ret) {
> > +		chan->differential = false;
> > +		chan->channel = pins[0];
> > +
> > +		ret = fwnode_property_read_u32(child, "common-mode-channel",
> > +					       &pins[1]);
> > +		if (ret)
> > +			return dev_err_probe(dev, ret,
> > +				"single-ended channels must define common-mode-
> > channel\n");
> > +
> > +		chan->channel2 = pins[1];
> > +		return 0;
> > +	}
> 
> Kind of a nitpick but for the above I would flip the logic. First check for errors in
> the single-channel case and then you can have one less of level of indentation...

Okay, will do it.
> 
> > +	return dev_err_probe(dev, ret,
> > +		"Channel must define one of diff-channels or single-channel.\n");
> > +}
> > +
...
> > +
> > +	ret = fwnode_property_read_u32(child, "reg", &ch_reg);
> > +	if (ret)
> > +		return ret;
> > +
> 
> Could also deserve a log message?

Sure, added.

> 
> > +	if (ch_reg >= AD4170_MAX_CHANNELS)
> > +		return dev_err_probe(dev, -EINVAL,
> > +				     "Channel idx greater than no of channels\n");
> > 
> 
> ...
...
> > +
> > +	return request_irq(st->spi->irq, &ad4170_irq_handler, IRQF_ONESHOT,
> > +			   indio_dev->name, indio_dev);
> 
> devm_request_irq()...
> 
Done.

> > +}
> > +
> > +static int ad4170_regulator_setup(struct ad4170_state *st)
> > +{
> > +	struct device *dev = &st->spi->dev;
> > +	int ret;
> > +
> > +	/* Required regulators */
> > +	ret = devm_regulator_get_enable_read_voltage(dev, "avdd");
> > +	if (ret < 0)
> > +		return dev_err_probe(dev, ret, "Failed to get AVDD voltage.\n");
> > +
> > +	st->vrefs_uv[AD4170_AVDD_SUP] = ret;
> > +
> > +	ret = devm_regulator_get_enable_read_voltage(dev, "iovdd");
> > +	if (ret < 0)
> > +		return dev_err_probe(dev, ret, "Failed to get IOVDD voltage.\n");
> > +
> > +	st->vrefs_uv[AD4170_IOVDD_SUP] = ret;
> > +
> > +	/* Optional regulators */
> > +	ret = devm_regulator_get_enable_read_voltage(dev, "avss");
> > +	if (ret < 0 && ret != -ENODEV)
> > +		return dev_err_probe(dev, ret, "Failed to get AVSS voltage.\n");
> > +
> > +	/* Assume AVSS at GND (0V) if not provided */
> > +	st->vrefs_uv[AD4170_AVSS_SUP] = ret == -ENODEV ? 0 : -ret;
> > +
> > +	ret = devm_regulator_get_enable_read_voltage(dev, "refin1p");
> > +	if (ret < 0 && ret != -ENODEV)
> > +		return dev_err_probe(dev, ret, "Failed to get REFIN+ voltage.\n");
> > +
> > +	st->vrefs_uv[AD4170_REFIN1P_SUP] = ret;
> > +
> > +	ret = devm_regulator_get_enable_read_voltage(dev, "refin1n");
> > +	if (ret < 0 && ret != -ENODEV)
> > +		return dev_err_probe(dev, ret, "Failed to get REFIN- voltage.\n");
> > +
> > +	/* Negative supplies are assumed to provide negative voltage */
> > +	st->vrefs_uv[AD4170_REFIN1N_SUP] = ret == -ENODEV ? -ENODEV : -ret;
> 
> Maybe to early for me but the comment does not make it clear to me why the negation?
> Won't the regulator return a negative voltage?

devm_regulator_get_enable_read_voltage(), regulator_get_voltage(), and anything
about reading the regulator voltage returns either a positive voltage value or
a negative error code. I couldn't find out how to read a negative voltage with
regulator API. So, for now, this is making the simplifying assumption that
the negative end of external reference supplies is always below GND level (even
though they could be positive).

...
> > +
> > +	st = iio_priv(indio_dev);
> > +	devm_mutex_init(dev, &st->lock);
> 
> check for errors...
> 
Ack, done.

> > +
...
> > +	st->regmap24 = devm_regmap_init_spi(spi, &ad4170_regmap24_config);
> > +	if (IS_ERR(st->regmap24))
> > +		return dev_err_probe(dev, PTR_ERR(st->regmap24),
> > +				     "Failed to initialize regmap24\n");
> > +
> 
> Hmm, interesting idea... but I would expect an explanation on why can't we have bulk
> reads for the 16 and 24 bit cases? Without it, I have to ask why not?

Not sure how regmap_bulk_read() handles it from a quick look but CS can't be
taken high between multibyte register access. Anyway, will check out if that
can be used here.

>  
...
> > +
> > +	if (spi->irq) {
> > +		ret = ad4170_trigger_setup(indio_dev);
> > +		if (ret)
> > +			return dev_err_probe(dev, ret, "Failed to setup
> > trigger\n");
> 
> Typically it's better to log the errors inside ad4170_trigger_setup() unless you use
> it outside probe.

Ack.

> 
> - Nuno Sá
> > 

Thanks,
Marcelo

Powered by blists - more mailing lists