Re: [PATCH v6 05/17] iio: adc: Add AD7768 and AD7768-4 core support

[Jonathan Cameron <jic23@kernel.org>]

> 
> commit cad3d6d13491f167da3c31d541a31ac850d8d3b5
> Author: Janani Sunil <janani.sunil@analog.com>
> 
> iio: adc: Add AD7768 and AD7768-4 core support
>     
> Add core support for the AD7768 and AD7768-4 simultaneous sampling ADCs.
> Configure supplies, clock and reset, use a custom regmap bus for the SPI
> protocol, and parse the enabled channels and input buffer settings from
> devicetree.

There was quite a bit of sashiko feedback on this one. I took a look
as getting close to merging the series. Please check my logic.

> 
> > diff --git a/drivers/iio/adc/ad7768.c b/drivers/iio/adc/ad7768.c
> > new file mode 100644
> > index 0000000000000..c80056a9edd8c
> > --- /dev/null
> > +++ b/drivers/iio/adc/ad7768.c
> > @@ -0,0 +1,1000 @@
> > +// SPDX-License-Identifier: GPL-2.0
> 
> [ ... ]
> 
> > +#define AD7768_REG_PRECHARGE_BUF1			0x11
> > +#define AD7768_REG_PRECHARGE_BUF2			0x12
> > +#define   AD7768_PREBUF_POS_EN(ch)			BIT((ch) * 2)
> > +#define   AD7768_PREBUF_NEG_EN(ch)			BIT(((ch) * 2) + 1)
> 
> [Severity: Medium]
> Are these bit assignments for the precharge buffers swapped? According to the
> AD7768 datasheet, the positive (POS) precharge buffers are assigned to odd bits,
> and the negative (NEG) precharge buffers are assigned to even bits.

No they aren't. I think sashiko didn't realize table starts at bit 7
and works down to bit 0.

> 
> > +
> > +#define AD7768_REG_REFP_BUF				0x13
> > +#define AD7768_REG_REFN_BUF				0x14
> 
> [ ... ]
> 
> > +static u8 ad7768_all_standby_mask(const struct ad7768_state *st)
> > +{
> > +	return GENMASK(st->chip_info->num_channels - 1, 0);
> > +}
> 
> [Severity: High]
> Does this generate the correct physical mask for the AD7768-4? The AD7768-4
> maps its 4 logical channels to physical channels 0, 1, 4, and 5. This returns
> GENMASK(3, 0) which is 0x0F, writing 0 to physical bits 4, 5, 6, and 7,
> leaving them fully active.
> 
> [ ... ]

This one looks plausible.  There is handling of the bits written to
deal with the mapping but not the mask I think. Please take a look.

> 
> > +static int ad7768_regmap_read(void *context, const void *reg_buf,
> > +			      size_t reg_size, void *val_buf, size_t val_size)
> > +{
> > +	struct ad7768_state *st = spi_get_drvdata(context);
> > +	struct spi_device *spi = context;
> > +	struct spi_transfer t[] = {
> > +		{
> > +			.tx_buf = &st->d16,
> > +			.len = sizeof(st->d16),
> > +			.cs_change = 1,
> > +		}, {
> > +			/*
> > +			 * Register responses are delayed by one CS frame. While
> > +			 * receiving the response to this read, the device also
> > +			 * decodes another command on SDI. Repeat the read
> > +			 * command to avoid sending an unspecified dummy
> > +			 * command.
> > +			 */
> > +			.tx_buf = &st->d16,
> > +			.rx_buf = &st->d16,
> > +			.len = sizeof(st->d16),
> > +		},
> > +	};
> 
> [Severity: High]
> Can this violate the DMA API? Both transfers use the same memory address
> (&st->d16) for their buffers. When the SPI subsystem maps all transfers for
> DMA concurrently, attempting to map the same memory region twice concurrently
> (DMA_TO_DEVICE and DMA_BIDIRECTIONAL) can cause cache coherency issues on
> non-coherent architectures.

So this one I'm fairly sure on, but feel free to check.

There is nothing stopping SPI using one buffer for both directions. If it's a
problem for a particular setup I think they have to bounce it.

> 
> [ ... ]
> 
> > +static int ad7768_update_scan_mode(struct iio_dev *indio_dev,
> > +				   const unsigned long *scan_mask)
> > +{
> > +	struct ad7768_state *st = iio_priv(indio_dev);
> > +	unsigned long channel_mask;
> > +	unsigned long standby_mask;
> > +	int ret;
> > +
> > +	channel_mask = ad7768_all_standby_mask(st);
> > +	standby_mask = channel_mask & ~*scan_mask;
> > +
> > +	/*
> > +	 * Crystal excitation requires channel 4 on AD7768 or channel 2 on
> > +	 * AD7768-4 to remain active.
> > +	 */
> > +	if (st->clock_source == AD7768_CLOCK_SOURCE_XTAL)
> > +		__clear_bit(st->chip_info->num_channels / 2, &standby_mask);
> > +
> > +	ret = regmap_update_bits(st->regmap, AD7768_REG_CH_STANDBY,
> > +				 channel_mask, standby_mask);
> > +	if (ret)
> > +		return ret;
> > +
> > +	for (unsigned int c = 0; c < st->chip_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;
> > +	}
> 
> [Severity: Medium]
> Will this leave the backend hardware out of sync if an error occurs? If one
> operation fails, it returns the error immediately without unwinding the
> previously modified channels.

This is one of those error cases where unwinding may just be impractical.
Real question is whether there is anything that means trying again won't
end up in a sane state or that anything particularly bad will happen
as a result of the bad state in the meantime. I think this is fine
but please check.

> 
> > +
> > +	return 0;
> > +}
> 
> [ ... ]
> 
> > +static int ad7768_buffer_postdisable(struct iio_dev *indio_dev)
> > +{
> > +	struct ad7768_state *st = iio_priv(indio_dev);
> > +
> > +	pm_runtime_put_autosuspend(regmap_get_device(st->regmap));
> > +	return 0;
> > +}
> 
> [Severity: Medium]
> Could this leave the hardware powered on after device removal?
> 
> If the driver is unbound while the IIO buffer is active,
> devm_iio_device_register unwinds first, calling iio_device_unregister(),
> which disables the buffer and triggers ad7768_buffer_postdisable().
> 
> Immediately after, devm_pm_runtime_set_active_enabled unwinds, calling
> pm_runtime_disable() which cancels the pending autosuspend work. The
> driver should ensure synchronous suspend if the buffer is active during
> removal.

Ah.  Interesting corner case.  There are probably lots of other
paths to the power happening to be on at shut down but I can see
maybe we ultimately want a flag to say we are going down in that
buffer disable path.  Anyhow, you should be careful to ensure it
doesn't matter what state we are in wrt to runtime pm, after we've
turned that off we should make sure we turn off the device by hand.
Afterall maybe runtime pm was never enabled in the first place
(or even built!)

> 
> [ ... ]
> 
> > +static int ad7768_configure_precharge_buffers(struct iio_dev *indio_dev,
> > +					      struct ad7768_precharge_config *precharge_cfg)
> > +{
> > +	struct ad7768_state *st = iio_priv(indio_dev);
> > +	u8 prebuf1_val, prebuf2_val;
> > +	u16 prebuf_mask = 0;
> > +	u8 refbufp_val = 0;
> > +	u8 refbufn_val = 0;
> > +	int ret;
> > +
> > +	for (unsigned int ch = 0; ch < indio_dev->num_channels; ch++) {
> > +		u8 channel = indio_dev->channels[ch].channel;
> > +
> > +		if (precharge_cfg[channel].prebufp_en)
> > +			prebuf_mask |= AD7768_PREBUF_POS_EN(channel);
> > +
> > +		if (precharge_cfg[channel].prebufn_en)
> > +			prebuf_mask |= AD7768_PREBUF_NEG_EN(channel);
> > +
> > +		if (precharge_cfg[channel].refbufp)
> > +			refbufp_val |= ad7768_channel_mask(st, channel);
> > +
> > +		if (precharge_cfg[channel].refbufn)
> > +			refbufn_val |= ad7768_channel_mask(st, channel);
> > +	}
> 
> [Severity: Medium]
> Does this apply the incorrect polarity for the reference buffer configuration?
> When adi,refbuf-pos-enable or adi,refbuf-neg-enable are provided in the device
> tree, the driver sets the corresponding bit to 1. According to the AD7768
> datasheet, setting a bit to 1 in the REFP_BUF and REFN_BUF registers powers down
> the buffer.

I'm not sure what datasheet sashiko has.  Mine says 1 is power up.
I'm going to guess it's assuming layout of values that aren't true.

> 
> [ ... ]
> 
> > +	device_for_each_named_child_node_scoped(dev, child, "channel") {
> > +		u32 channel;
> > +
> > +		ret = fwnode_property_read_u32(child, "reg", &channel);
> > +		if (ret)
> > +			return dev_err_probe(dev, ret,
> > +					     "Failed to parse reg of %pfwP\n",
> > +					     child);
> > +
> > +		if (channel >= st->chip_info->num_channels)
> > +			return dev_err_probe(dev, -ECHRNG,
> > +					     "Invalid channel %u in firmware\n",
> > +					     channel);
> > +
> > +		ret = regmap_clear_bits(st->regmap, AD7768_REG_CH_STANDBY,
> > +					BIT(channel));
> 
> [Severity: High]
> Does this use logical indices instead of physical bits for the AD7768-4? The
> AD7768-4 uses physical channels 0, 1, 4, and 5. Using BIT(channel) will update
> physical bits 0-3 instead of the mapped hardware bits.

This seems plausible. Please check.

> 
> > +		if (ret)
> > +			return ret;
> > +
> 
> [ ... ]
> 
> > +	pm_runtime_set_autosuspend_delay(dev, 2000);
> > +	pm_runtime_use_autosuspend(dev);
> > +	ret = devm_pm_runtime_set_active_enabled(dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return devm_iio_device_register(dev, indio_dev);
> > +}
> 
> [Severity: Medium]
> Does this leave the device permanently active? This configures autosuspend and
> enables runtime PM, which transitions the device to RPM_ACTIVE with a usage
> count of 0, but fails to call pm_request_idle(dev). As a result, the autosuspend
> timer is never triggered, leaving the device fully powered indefinitely until
> a user explicitly accesses it.
Nope.  This is usual thing of sashiko failing to notice the stuff
in base/dd.c that always checks if the reference count is 0 and hence
it can put it to sleep. This happens just after probe.

So a definite mixed bag for sashiko on this one.

Jonathan