Message-ID: <20241005183427.61cae54f@jic23-huawei>
Date: Sat, 5 Oct 2024 18:34:27 +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>, "Mike Looijmans" <mike.looijmans@...ic.nl>,
 Marius Cristea <marius.cristea@...rochip.com>, Dumitru Ceclan
 <mitrutzceclan@...il.com>, AngeloGioacchino Del Regno
 <angelogioacchino.delregno@...labora.com>, Alisa-Dariana Roman
 <alisadariana@...il.com>, Ivan Mikhaylov <fr0st61te@...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 v2 6/7] iio: adc: ad485x: add ad485x driver

On Fri, 4 Oct 2024 17:07:55 +0300
Antoniu Miclaus <antoniu.miclaus@...log.com> wrote:

> Add support for the AD485X DAS familiy.
As with the DT docs, it is good to have a little bit more info on the device
in this patch description. Just lets casual readers have some idea what
they are looking at!

I've left the packet_format ABI question for the next patch, but
I would like some info on the default.  The code doesn't set it currently 
but maybe for documentation purposes it should?

From the earlier discussion (ongoing) I think the oversampling control
requires particular formats. I'd like to see that in the driver
first with it overriding that format as necessary.

Then we can see whether we need the additional control

Various other minor comments inline.  With the exception of that
question of packet_format this is in a pretty good state.


Jonathan


> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@...log.com>
> ---
> changes in v2:
>  - update headers and include the missing ones, order them alphabetically.
>  - use clamp()
>  - use units macros where applies
>  - convert int to unsigned variables where applies.
>  - add trailing commas where applies.
>  - return FIELD_GET directly.
>  - shrink function header to one line where it fits.
>  - update scale table values arrangement - pow-of-two per line
>  - rename j to have a proper meaning.
>  - invert if (st->offsets[chan->channel] != val) and drop next lines indentation.
>  - drop whitespace from * val = ... (altough checkpatch complains about it)
>  - drop comma in the terminator lines for ext_info.
>  - fix inconsistency between chip_info structures.
>  - use devm_mutex_init
>  - return -ENOENT on max_cnt check.
>  - check both val and val2 for negative before converting to unsigned.
>  - remove val2 where not used.
>  - use dev_info() instead of dev_warn()
>  - add spaces after { and before } in ad485x_scale_table
> 
>  drivers/iio/adc/Kconfig  |   12 +
>  drivers/iio/adc/Makefile |    1 +
>  drivers/iio/adc/ad485x.c | 1094 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 1107 insertions(+)
>  create mode 100644 drivers/iio/adc/ad485x.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index f60fe85a30d5..83f55229d731 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -36,6 +36,18 @@ config AD4130
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called ad4130.
>  
> +config AD485X

No wild cards in drivers.  It goes wrong far too often!  Just
pick a part and name everything after that (files, functions etc).

For text, use 'and similar' to cover the range.

> +	tristate "Analog Device AD485x DAS Driver"
> +	depends on SPI
> +	select REGMAP_SPI
> +	select IIO_BACKEND
> +	help
> +	  Say yes here to build support for Analog Devices AD485x high speed
> +	  data acquisition system (DAS).

List all supported parts in here. Makes for easier grepping.

> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called ad485x.
> +
>  config AD7091R
>  	tristate
>  
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index d370e066544e..26c1670c8913 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -7,6 +7,7 @@
>  obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
>  obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
>  obj-$(CONFIG_AD4130) += ad4130.o
> +obj-$(CONFIG_AD485X) += ad485x.o
>  obj-$(CONFIG_AD7091R) += ad7091r-base.o
>  obj-$(CONFIG_AD7091R5) += ad7091r5.o
>  obj-$(CONFIG_AD7091R8) += ad7091r8.o
> diff --git a/drivers/iio/adc/ad485x.c b/drivers/iio/adc/ad485x.c
> new file mode 100644
> index 000000000000..faa10d56a791
> --- /dev/null
> +++ b/drivers/iio/adc/ad485x.c
> @@ -0,0 +1,1094 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Analog Devices AD485x DAS driver
> + *
> + * Copyright 2024 Analog Devices Inc.
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/bits.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/minmax.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/types.h>
> +#include <linux/units.h>
> +
> +#include <linux/iio/backend.h>
> +#include <linux/iio/iio.h>
> +
> +#include <asm/unaligned.h>

> +
> +#define AD4858_PROD_ID			0x60
> +#define AD4857_PROD_ID			0x61
> +#define AD4856_PROD_ID			0x62
> +#define AD4855_PROD_ID			0x63
> +#define AD4854_PROD_ID			0x64
> +#define AD4853_PROD_ID			0x65
> +#define AD4852_PROD_ID			0x66
> +#define AD4851_PROD_ID			0x67
> +#define AD4858I_PROD_ID			0x6F
These don't add anything over just using the numeric value
to fill the chip_info structure field.  That field clearly documents
what these magic numbers are anyway.

Note we used to have this code pattern of defines at the top
for PROD_ID in a load more drivers but have gotten rid of them
in favour of inline values.  May well be more to do!

> +
> +struct ad485x_chip_info {
> +	const char *name;
> +	unsigned int product_id;
> +	const unsigned int (*scale_table)[2];
> +	int num_scales;
As below.  Currently we only seem to have one option for the
scale parameters.  If that is so for now, drop these and encode
that option directly.

> +	const int *offset_table;
> +	int num_offset;
> +	const struct iio_chan_spec *channels;
> +	unsigned int num_channels;
> +	unsigned long throughput;
> +	unsigned int resolution;
> +};


> +
> +static const char *const ad4858_packet_fmts[] = {
> +	"20-bit", "24-bit", "32-bit",
> +};
> +
> +static const char *const ad4857_packet_fmts[] = {
> +	"16-bit", "24-bit",
I'll leave discussing these for the docs patch.

> +};
> +
> +static int ad485x_set_packet_format(struct iio_dev *indio_dev,
> +				    const struct iio_chan_spec *chan,
> +				    unsigned int format)
> +{
> +	struct ad485x_state *st = iio_priv(indio_dev);
> +	unsigned int val;
> +	int ret;
> +
> +	if (chan->scan_type.realbits == 20)

Perhaps a switch would be cleaner?

> +		switch (format) {
> +		case 0:
> +			val = 20;
> +			break;
> +		case 1:
> +			val = 24;
> +			break;
> +		case 2:
> +			val = 32;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +	else if (chan->scan_type.realbits == 16)
> +		switch (format) {
> +		case 0:
> +			val = 16;
> +			break;
> +		case 1:
> +			val = 24;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +	else
> +		return -EINVAL;
> +
> +	ret = iio_backend_data_size_set(st->back, val);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_update_bits(st->regmap, AD485X_REG_PACKET,
> +				  AD485X_PACKET_FORMAT_MASK, format);
> +}


> +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'm slowly standardizing across IIO on this mostly so we have a 'right'
answer rather than a messy mix of styles.  The choice was a random
pick rather than there being anything better about having a space.

> +};

> +
> +static const struct ad485x_chip_info ad4858_info = {
> +	.name = "ad4858",
> +	.product_id = AD4858_PROD_ID,
> +	.scale_table = ad485x_scale_table,

There only seems to be one scale table.
Unless you plan to very soon add more devices, drop it from
here for now and access it directly in the code.

Reduces complexity a little + it is easy to bring back the
entries in here when you need them.

If you are planning to send out support for more parts shortly
then it is fine to keep this as it is and reduce the churn.
I'm just not keen on speculative flexibility in drivers
where it may never use used!

> +	.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 = 1 * MEGA,
> +	.resolution = 20,
> +};
...


> +
> +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, },
> +	{}
	{ }
+ reorder as below.


> +};
> +
> +static const struct spi_device_id ad485x_spi_id[] = {
> +	{ "ad4858", (kernel_ulong_t)&ad4858_info },
> +	{ "ad4857", (kernel_ulong_t)&ad4857_info },
> +	{ "ad4856", (kernel_ulong_t)&ad4856_info },
> +	{ "ad4855", (kernel_ulong_t)&ad4855_info },
> +	{ "ad4854", (kernel_ulong_t)&ad4854_info },
> +	{ "ad4853", (kernel_ulong_t)&ad4853_info },
> +	{ "ad4852", (kernel_ulong_t)&ad4852_info },
> +	{ "ad4851", (kernel_ulong_t)&ad4851_info },
alphanumeric order.  So pretty much the reverse.


> +	{ "ad4858i", (kernel_ulong_t)&ad4858i_info },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, ad485x_spi_id);

Powered by blists - more mailing lists