linux-kernel - Re: [PATCH v2 2/6] iio: adc: ad4030: add driver for ad4030-24

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: <Z2eqOSN2Uk8SfTq1@debian-BULLSEYE-live-builder-AMD64>
Date: Sun, 22 Dec 2024 02:57:13 -0300
From: Marcelo Schmitt <marcelo.schmitt1@...il.com>
To: Esteban Blanc <eblanc@...libre.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>,
	Michael Hennerich <Michael.Hennerich@...log.com>,
	Nuno Sá <nuno.sa@...log.com>,
	Jonathan Cameron <jic23@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Jonathan Corbet <corbet@....net>, linux-iio@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-doc@...r.kernel.org
Subject: Re: [PATCH v2 2/6] iio: adc: ad4030: add driver for ad4030-24

Hello Esteban, some comments inline.

On 12/19, Esteban Blanc wrote:
> This adds a new driver for the Analog Devices INC. AD4030-24 ADC.
> 
> The driver implements basic support for the AD4030-24 1 channel
> differential ADC with hardware gain and offset control.
> 
> Signed-off-by: Esteban Blanc <eblanc@...libre.com>
> ---
[...]
> +
> +static int ad4030_spi_read(void *context, const void *reg, size_t reg_size,
> +			   void *val, size_t val_size)
> +{
> +	int ret;
> +	struct ad4030_state *st = context;
> +	struct spi_transfer xfer = {
> +		.tx_buf = st->tx_data,
> +		.rx_buf = st->rx_data.raw,
> +		.len = reg_size + val_size,
> +		.speed_hz = AD4030_SPI_MAX_REG_XFER_SPEED,
Is speed_hz really needed? What happens if the controller can't clock at 80MHz?

> +	};
> +
> +	if (xfer.len > ARRAY_SIZE(st->tx_data) ||
> +	    xfer.len > ARRAY_SIZE(st->rx_data.raw))
> +		return  -EINVAL;

Would it make sense to bring register configuration mode commands into the
regmap calls?
I mean, to do the ad4030_enter_config_mode() transfer here and the
ad4030_exit_config_mode() at the end of this function.
>From datasheet, it looks like both enter/exit config mode are required for reg
access so why not doing them in the regmap callbacks?
With that, I think it won't be needed to call register config mode functions
in ad4030_single_conversion() and in buffer enable/disable functions.
Might need implement regmap_config read and write callbacks to properly handle
regmap_bulk_read/write interface.


> +
> +	memset(st->tx_data, 0, ARRAY_SIZE(st->tx_data));
> +	memcpy(st->tx_data, reg, reg_size);
> +
> +	ret = spi_sync_transfer(st->spi, &xfer, 1);
> +	if (ret)
> +		return ret;
> +
> +	memcpy(val, &st->rx_data.raw[reg_size], val_size);
> +
> +	return ret;
> +}
> +
[...]
> +
> +static int ad4030_get_chan_calibscale(struct iio_dev *indio_dev,
> +				      struct iio_chan_spec const *chan,
> +				      int *val,
> +				      int *val2)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	u16 gain;
> +	int ret;
> +
> +	ret = regmap_bulk_read(st->regmap, AD4030_REG_GAIN_CHAN(chan->address),
> +			       st->rx_data.raw, AD4030_REG_GAIN_BYTES_NB);
> +	if (ret)
> +		return ret;
> +
> +	gain = get_unaligned_be16(st->rx_data.raw);
My impression is that it is a bit odd to handle endianness after/before
calling regmap_read/write(). Can you try set
.val_format_endian_default = REGMAP_ENDIAN_BIG, in ad4030_regmap_bus?
If that doesn't help, what about doing the get/set_unaligned stuff within
the bus read/write calls?

> +
> +	/* From datasheet: multiplied output = input × gain word/0x8000 */
> +	*val = gain / 0x8000;
Use a define to give a name to the gain constant?

> +	*val2 = mul_u64_u32_div(gain % 0x8000, NANO, 0x8000);
> +
> +	return IIO_VAL_INT_PLUS_NANO;
> +}
> +
[...]