linux-kernel - Re: [PATCH v3 4/8] iio: adc: Add support for ad4052

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c89f4b2f-0892-4f63-b9b4-5ae55b477c01@baylibre.com>
Date: Mon, 16 Jun 2025 09:54:52 -0500
From: David Lechner <dlechner@...libre.com>
To: Jonathan Cameron <jic23@...nel.org>,
 Jorge Marques <jorge.marques@...log.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>,
 Michael Hennerich <Michael.Hennerich@...log.com>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Jonathan Corbet <corbet@....net>,
 Nuno Sá <nuno.sa@...log.com>,
 Andy Shevchenko <andy@...nel.org>, Uwe Kleine-König
 <ukleinek@...nel.org>, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
 linux-doc@...r.kernel.org, linux-pwm@...r.kernel.org
Subject: Re: [PATCH v3 4/8] iio: adc: Add support for ad4052

On 6/14/25 5:08 AM, Jonathan Cameron wrote:
> On Tue, 10 Jun 2025 09:34:37 +0200
> Jorge Marques <jorge.marques@...log.com> wrote:
> 
>> The AD4052/AD4058/AD4050/AD4056 are versatile, 16-bit/12-bit, successive
>> approximation register (SAR) analog-to-digital converter (ADC) that
>> enables low-power, high-density data acquisition solutions without
>> sacrificing precision. This ADC offers a unique balance of performance
>> and power efficiency, plus innovative features for seamlessly switching
>> between high-resolution and low-power modes tailored to the immediate
>> needs of the system. The AD4052/AD4058/AD4050/AD4056 are ideal for
>> battery-powered, compact data acquisition and edge sensing applications.
>>

...

>> +static int ad4052_update_xfer_raw(struct iio_dev *indio_dev,
>> +				   struct iio_chan_spec const *chan)
>> +{
>> +	struct ad4052_state *st = iio_priv(indio_dev);
>> +	const struct iio_scan_type *scan_type;
>> +	struct spi_transfer *xfer = &st->xfer;
>> +
>> +	scan_type = iio_get_current_scan_type(indio_dev, chan);
>> +	if (IS_ERR(scan_type))
>> +		return PTR_ERR(scan_type);
>> +
>> +	xfer->rx_buf = st->raw;
>> +	xfer->bits_per_word = scan_type->realbits;
>> +	xfer->len = scan_type->realbits == 24 ? 4 : 2;
> 
> This is a little odd. I'm not sure what happens with len not dividing
> into a whole number of bits per word chunks.
> Maybe a comment?

Even better, there is now spi_bpw_to_bytes() for this.

> 
>> +	xfer->speed_hz = AD4052_SPI_MAX_ADC_XFER_SPEED(st->vio_uv);
>> +
>> +	return 0;
>> +}
> 
> 

...

> 
>> +static int __ad4052_read_chan_raw(struct ad4052_state *st, int *val)
>> +{
>> +	struct spi_device *spi = st->spi;
>> +	struct spi_transfer t_cnv = {};
>> +	int ret;
>> +
>> +	reinit_completion(&st->completion);
>> +
>> +	if (st->cnv_gp) {
>> +		gpiod_set_value_cansleep(st->cnv_gp, 1);
>> +		gpiod_set_value_cansleep(st->cnv_gp, 0);
>> +	} else {
>> +		ret = spi_sync_transfer(spi, &t_cnv, 1);
> 
> Add a comment for this.   I can't immediately spot documentation on what
> a content free transfer actually does.  I assume pulses the chip select?
> is that true for all SPI controllers?

Should be. Setting .delay in the xfer would also make it more
clear that this is doing.

> 
>> +		if (ret)
>> +			return ret;
>> +	}
>> +	/*
>> +	 * Single sample read should be used only for oversampling and
>> +	 * sampling frequency pairs that take less than 1 sec.
>> +	 */
>> +	if (st->gp1_irq) {
>> +		ret = wait_for_completion_timeout(&st->completion,
>> +						  msecs_to_jiffies(1000));
>> +		if (!ret)
>> +			return -ETIMEDOUT;
>> +	}
>> +
>> +	ret = spi_sync_transfer(spi, &st->xfer, 1);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (st->xfer.len == 2)
>> +		*val = sign_extend32(*(u16 *)(st->raw), 15);
>> +	else
>> +		*val = sign_extend32(*(u32 *)(st->raw), 23);
>> +
>> +	return ret;
>> +}
> 

...

>> +
>> +static int ad4052_debugfs_reg_access(struct iio_dev *indio_dev, unsigned int reg,
>> +				     unsigned int writeval, unsigned int *readval)
>> +{
>> +	struct ad4052_state *st = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	if (!iio_device_claim_direct(indio_dev))
> 
> For these guards in the debugfs callback, please add a comment on why they
> are needed.   We've had a lot of questions about these recently and I'd
> like it to be clear to people when they should cut and paste these and when
> not.

The reason I started doing this is that running the iio_info command attemps
to read register 0x00 via the debug attribute of every single iio device. So
if you run iio_info during a buffered read, and 0x00 is a valid register, it
would break things without this check.

Ideally, general purpose commands wouldn't be poking debug registers, but
that isn't the case. But I suppose we could "fix" iio_info instead.

> 
>> +		return -EBUSY;
>> +
>> +	if (readval)
>> +		ret = regmap_read(st->regmap, reg, readval);
>> +	else
>> +		ret = regmap_write(st->regmap, reg, writeval);
>> +	iio_device_release_direct(indio_dev);
>> +	return ret;
>> +}
>