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

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: <aD27cobHWeBX8o30@smile.fi.intel.com>
Date: Mon, 2 Jun 2025 17:55:46 +0300
From: Andy Shevchenko <andy@...nel.org>
To: Marcelo Schmitt <marcelo.schmitt@...log.com>
Cc: linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
	linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
	Ana-Maria Cusco <ana-maria.cusco@...log.com>, jic23@...nel.org,
	lars@...afoo.de, Michael.Hennerich@...log.com,
	dlechner@...libre.com, nuno.sa@...log.com, robh@...nel.org,
	krzk+dt@...nel.org, conor+dt@...nel.org, linus.walleij@...aro.org,
	brgl@...ev.pl, marcelo.schmitt1@...il.com
Subject: Re: [PATCH v4 02/11] iio: adc: Add basic support for AD4170

On Mon, Jun 02, 2025 at 08:36:47AM -0300, Marcelo Schmitt wrote:
> From: Ana-Maria Cusco <ana-maria.cusco@...log.com>
> 
> 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