Re: [PATCH v4 02/11] iio: adc: Add basic support for AD4170

[Andy Shevchenko]

On Mon, Jun 02, 2025 at 08:36:47AM -0300, Marcelo Schmitt wrote:
> From: Ana-Maria Cusco <ana-maria.cusco@xxxxxxxxxx>
> 
> The AD4170 is a multichannel, low noise, 24-bit precision sigma-delta
> analog to digital converter. The AD4170 design offers a flexible data
> acquisition solution with crosspoint multiplexed analog inputs,
> configurable ADC voltage reference inputs, ultra-low noise integrated PGA,
> digital filtering, wide range of configurable output data rates, internal
> oscillator and temperature sensor, four GPIOs, and integrated features for
> interfacing with load cell weigh scales, RTD, and thermocouple sensors.
> 
> Add basic support for the AD4170 ADC with the following features:
> - Single-shot read.
> - Analog front end PGA configuration.
> - Differential and pseudo-differential input configuration.

...

+ array_size.h

> +#include <linux/bitfield.h>
> +#include <linux/bitmap.h>

> +#include <linux/bitops.h>
> +#include <linux/bits.h>

bitmap.h guarantees to include the two above, but it;s not a big deal to leave
them.

> +#include <linux/cleanup.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/iio/iio.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>

+ types.h

> +#include <linux/unaligned.h>
> +#include <linux/units.h>
> +#include <linux/util_macros.h>

...

> +static bool ad4170_setup_eq(struct ad4170_setup *a, struct ad4170_setup *b)
> +{
> +	/*
> +	 * The use of static_assert() here is to make sure that the comparison
> +	 * is adapted whenever struct ad4170_setup is changed.
> +	 */
> +	static_assert(sizeof(*a) ==
> +		      sizeof(struct {
> +				     u16 misc;
> +				     u16 afe;
> +				     u16 filter;
> +				     u16 filter_fs;
> +				     u32 offset;
> +				     u32 gain;
> +			     }));

I think it doesn't make much sense unless one uses memcpy().

> +	if (a->misc != b->misc ||
> +	    a->afe != b->afe ||
> +	    a->filter != b->filter ||
> +	    a->filter_fs != b->filter_fs ||
> +	    a->offset != b->offset ||
> +	    a->gain != b->gain)
> +		return false;
> +
> +	return true;
> +}

...

> +static int ad4170_soft_reset(struct ad4170_state *st)
> +{
> +	int ret;
> +
> +	ret = regmap_write(st->regmap, AD4170_CONFIG_A_REG,
> +			   AD4170_SW_RESET_MSK);
> +	if (ret)
> +		return ret;
> +
> +	/* AD4170-4 requires 1 ms between reset and any register access. */
> +	fsleep(MILLI);

1 * USEC_PER_MSEC

(need time.h)

...

> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);

+ blank line here.

> +	ret = devm_mutex_init(dev, &st->lock);
> +	if (ret)
> +		return ret;

> +	chip = spi_get_device_match_data(spi);
> +	if (!chip)
> +		return -EINVAL;
> +
> +	indio_dev->name = chip->name;
> +	indio_dev->info = &ad4170_info;

> +	st->spi = spi;

Can be grouped with above st = ...

> +	st->regmap = devm_regmap_init(dev, NULL, st, &ad4170_regmap_config);
> +	if (IS_ERR(st->regmap))
> +		return dev_err_probe(dev, PTR_ERR(st->regmap),
> +				     "Failed to initialize regmap\n");

-- 
With Best Regards,
Andy Shevchenko