Message-ID: <20240928184722.314b329b@jic23-huawei>
Date: Sat, 28 Sep 2024 18:47:22 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Antoniu Miclaus <antoniu.miclaus@...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>, Nuno Sa
 <nuno.sa@...log.com>, Olivier Moysan <olivier.moysan@...s.st.com>, Uwe
 Kleine-König <ukleinek@...nel.org>, Andy Shevchenko
 <andy@...nel.org>, David Lechner <dlechner@...libre.com>, Marcelo Schmitt
 <marcelo.schmitt@...log.com>, João Paulo Gonçalves <joao.goncalves@...adex.com>, Mike Looijmans
 <mike.looijmans@...ic.nl>, Dumitru Ceclan <mitrutzceclan@...il.com>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 Alisa-Dariana Roman <alisadariana@...il.com>, Sergiu Cuciurean
 <sergiu.cuciurean@...log.com>, Dragos Bogdan <dragos.bogdan@...log.com>,
 <linux-iio@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <devicetree@...r.kernel.org>, <linux-pwm@...r.kernel.org>
Subject: Re: [PATCH 6/7] iio: adc: ad485x: add ad485x driver

On Mon, 23 Sep 2024 13:10:23 +0300
Antoniu Miclaus <antoniu.miclaus@...log.com> wrote:

> Add support for the AD485X DAS familiy.
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@...log.com>

A few additional comments from me.

Thanks,

Jonathan

> diff --git a/drivers/iio/adc/ad485x.c b/drivers/iio/adc/ad485x.c
> new file mode 100644
> index 000000000000..3333cad9ed8f
> --- /dev/null
> +++ b/drivers/iio/adc/ad485x.c
> @@ -0,0 +1,1061 @@

> +static int ad485x_setup(struct ad485x_state *st)
> +{
> +	unsigned int product_id;
> +	int ret;
> +
> +	ret = ad485x_set_sampling_freq(st, 1000000);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD485X_REG_INTERFACE_CONFIG_A,
> +			   AD485X_SW_RESET);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD485X_REG_INTERFACE_CONFIG_B,
> +			   AD485X_SINGLE_INSTRUCTION);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD485X_REG_INTERFACE_CONFIG_A,
> +			   AD485X_SDO_ENABLE);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(st->regmap, AD485X_REG_PRODUCT_ID_L, &product_id);
> +	if (ret)
> +		return ret;
> +
> +	if (product_id != st->info->product_id)
> +		dev_warn(&st->spi->dev, "Unknown product ID: 0x%02X\n",
> +			 product_id);

dev_info() for this is probably better.  Might be fine afterall if
the new part is fully backwards compatible with this one.

> +
> +	ret = regmap_write(st->regmap, AD485X_REG_DEVICE_CTRL,
> +			   AD485X_ECHO_CLOCK_MODE);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(st->regmap, AD485X_REG_PACKET, 0);
> +}


> +
> +static int ad485x_get_packet_format(struct iio_dev *indio_dev,
> +				    const struct iio_chan_spec *chan)
> +{
> +	struct ad485x_state *st = iio_priv(indio_dev);
> +	unsigned int format;
> +	int ret;
> +
> +	ret = regmap_read(st->regmap, AD485X_REG_PACKET, &format);
> +	if (ret)
> +		return ret;
> +
> +	format = FIELD_GET(AD485X_PACKET_FORMAT_MASK, format);
> +
> +	return format;
	return FIELD_GET()
> +}

> +
> +static int ad485x_get_calibscale(struct ad485x_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, AD485X_REG_CHX_GAIN_MSB(ch),
> +			  &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	gain = (reg_val & 0xFF) << 8;
Use a byte array into which you write the regval then a get_unalignedb_be16
or similar to get the value.
> +
> +	ret = regmap_read(st->regmap, AD485X_REG_CHX_GAIN_LSB(ch),
> +			  &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	gain |= reg_val & 0xFF;
> +
> +	*val = gain;
> +	*val2 = 32768;
> +
> +	return IIO_VAL_FRACTIONAL;
> +}

> +static int ad485x_get_calibbias(struct ad485x_state *st, int ch, int *val,
> +				int *val2)
> +{
> +	unsigned int lsb, mid, msb;
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_MSB(ch),
> +			  &msb);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_MID(ch),
> +			  &mid);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(st->regmap, AD485X_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;
As below I'd use a byte array then you can do get_unaligned_be24 to
+ a right shift by 4 of the whole thing.

> +		*val = sign_extend32(*val, 19);
> +	}
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int ad485x_set_calibbias(struct ad485x_state *st, int ch, int val,
> +				int val2)
> +{
> +	unsigned int lsb, mid, msb;
> +	int ret;
> +
> +	if (st->info->resolution == 16) {
> +		lsb = 0;
> +		mid = val & 0xFF;
> +		msb = (val >> 8) & 0xFF;
> +	} else {
> +		lsb = (val << 4) & 0xFF;

Might as well mask by 0xF0 to make it really clear
nothing in bottom few bits.


> +		mid = (val >> 4) & 0xFF;
> +		msb = (val >> 12) & 0xFF;

I'd be tempted to shift the whole thing left 4 bits and
then use a put_unaligned_be24 on it into a byte array then pick
out the bytes from that array.  Will be easier to read.
Above 16 bit case would be a put_unaligned_be16

> +	}
> +
> +	guard(mutex)(&st->lock);
> +
> +	ret = regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_LSB(ch), lsb);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_MID(ch), mid);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_MSB(ch), msb);

I think you already got this question, but consider bulk writes.

> +}
> +
> +static const unsigned int ad485x_scale_table[][2] = {
> +	{2500, 0x0}, {5000, 0x1}, {5000, 0x2}, {10000, 0x3}, {6250, 0x04},
> +	{12500, 0x5}, {10000, 0x6}, {20000, 0x7}, {12500, 0x8}, {25000, 0x9},
> +	{20000, 0xA}, {40000, 0xB}, {25000, 0xC}, {50000, 0xD}, {40000, 0xE},
> +	{80000, 0xF}

Spaces after { and before } preferred.
Maybe just have this as one item per line. I think that is more readable.

> +};

> +
> +static int ad485x_read_raw(struct iio_dev *indio_dev,
> +			   const struct iio_chan_spec *chan,
> +			   int *val, int *val2, long info)
> +{
> +	struct ad485x_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 ad485x_get_calibscale(st, chan->channel, val, val2);
> +	case IIO_CHAN_INFO_SCALE:
> +		return ad485x_get_scale(st, chan, val, val2);
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		return ad485x_get_calibbias(st, chan->channel, val, val2);
> +	case IIO_CHAN_INFO_OFFSET:
> +		scoped_guard(mutex, &st->lock)
> +			* val = st->offsets[chan->channel];

			*val = ...

> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +
> +#define AD485X_IIO_CHANNEL(index, real, storage, info)			\
> +{									\
> +	.type = IIO_VOLTAGE,						\
> +	.ext_info = info,						\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_CALIBSCALE) |		\
> +		BIT(IIO_CHAN_INFO_CALIBBIAS) |				\
> +		BIT(IIO_CHAN_INFO_SCALE) |				\
> +		BIT(IIO_CHAN_INFO_OFFSET),				\
> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
> +	.info_mask_shared_by_type_available =				\
> +		BIT(IIO_CHAN_INFO_SCALE) |				\
> +		BIT(IIO_CHAN_INFO_OFFSET),				\

Maybe add avail for calibscale and calibbias as well.


> +
> +static struct iio_chan_spec_ext_info ad4858_ext_info[] = {
> +	IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> +	IIO_ENUM_AVAILABLE("packet_format",
> +			   IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> +	{},
> +};
> +
> +static struct iio_chan_spec_ext_info ad4857_ext_info[] = {
> +	IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
> +	IIO_ENUM_AVAILABLE("packet_format",
> +			   IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
> +	{},
I think Andy pointed these out. No trailing comma on terminators.

However, I'm not sure this ABI is practical currently.
Basically I have no idea what it is or how to use it!
> +};

> +
> +static const struct ad485x_chip_info ad4858i_info = {
> +	.name = "ad4858i",
> +	.product_id = AD4858I_PROD_ID,
> +	.scale_table = ad485x_scale_table,
> +	.num_scales = ARRAY_SIZE(ad485x_scale_table),
> +	.offset_table = ad4858_offset_table,
> +	.num_offset = ARRAY_SIZE(ad4858_offset_table),
> +	.channels = ad4858_channels,
> +	.num_channels = ARRAY_SIZE(ad4858_channels),
> +	.throughput = 1000000,

Odd you use MEGA above, but not here.

> +	.resolution = 20,
> +};

> +static int ad485x_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct ad485x_state *st;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->spi = spi;
> +
> +	mutex_init(&st->lock);

Ever so slight preference for the new option of.
	ret = devm_mutex_init(dev, &st->lock);
	if (ret)
		return ret;

...

> +}
> +
> +static const struct of_device_id ad485x_of_match[] = {
> +	{ .compatible = "adi,ad4858", .data = &ad4858_info, },
> +	{ .compatible = "adi,ad4857", .data = &ad4857_info, },
> +	{ .compatible = "adi,ad4856", .data = &ad4856_info, },
> +	{ .compatible = "adi,ad4855", .data = &ad4855_info, },
> +	{ .compatible = "adi,ad4854", .data = &ad4854_info, },
> +	{ .compatible = "adi,ad4853", .data = &ad4853_info, },
> +	{ .compatible = "adi,ad4852", .data = &ad4852_info, },
> +	{ .compatible = "adi,ad4851", .data = &ad4851_info, },
> +	{ .compatible = "adi,ad4858i", .data = &ad4858i_info, },
> +	{}
	{ }
> +};
> +

Powered by blists - more mailing lists