linux-kernel - Re: [PATCH 5/5] iio: adc: ad7405: add ad7405 driver

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: <Z-EyGWpu_hjgEUYO@smile.fi.intel.com>
Date: Mon, 24 Mar 2025 12:21:13 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Pop Ioan Daniel <pop.ioan-daniel@...log.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>,
	Michael Hennerich <Michael.Hennerich@...log.com>,
	Jonathan Cameron <jic23@...nel.org>, 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>,
	David Lechner <dlechner@...libre.com>,
	Javier Carrasco <javier.carrasco.cruz@...il.com>,
	Guillaume Stols <gstols@...libre.com>,
	Trevor Gamblin <tgamblin@...libre.com>,
	Dumitru Ceclan <mitrutzceclan@...il.com>,
	Matteo Martelli <matteomartelli3@...il.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	Alisa-Dariana Roman <alisadariana@...il.com>,
	Michael Walle <michael@...le.cc>,
	Herve Codina <herve.codina@...tlin.com>,
	Thomas Bonnefille <thomas.bonnefille@...tlin.com>,
	Dragos Bogdan <dragos.bogdan@...log.com>, linux-iio@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/5] iio: adc: ad7405: add ad7405 driver

On Mon, Mar 24, 2025 at 11:08:00AM +0200, Pop Ioan Daniel wrote:
> Add support for the AD7405/ADUM770x, a high performance isolated ADC,
> 1-channel, 16-bit with a second-order Σ-Δ modulator that converts an
> analog input signal into a high speed, single-bit data stream.

Datasheet: tag?

> Signed-off-by: Pop Ioan Daniel <pop.ioan-daniel@...log.com>

...

The list of headers is semi-random and unordered. Please, follow IWYU principle
and fix the ordering as well.

> +#include <linux/module.h>
> +#include <linux/log2.h>
> +#include <linux/clk.h>
> +#include <linux/platform_device.h>

> +#include <linux/of.h>

No of.h in the new code, please.

> +#include <linux/iio/iio.h>
> +#include <linux/iio/backend.h>

Move this group...

> +#include <linux/util_macros.h>
> +#include <linux/regulator/consumer.h>
> +

...somewhere here as other (recent) drivers do.

...

> +#define AD7405_DEFAULT_DEC_RATE 1024

What is the units?

...

> +const unsigned int ad7405_dec_rates[] = {
> +		4096, 2048, 1024, 512, 256, 128, 64, 32,

Too much TABbed.

> +};

...

> +struct ad7405_chip_info {
> +	const char *name;
> +	unsigned int num_channels;
> +	unsigned int max_rate;
> +	unsigned int min_rate;
> +	struct iio_chan_spec channel[3];
> +	const unsigned long *available_mask;

`pahole` has been run and this is the best choice, right?

> +};
> +
> +struct ad7405_state {

Ditto.

> +	struct iio_backend *back;
> +	struct clk *axi_clk_gen;
> +	/* lock to protect multiple accesses to the device registers */
> +	struct mutex lock;
> +	struct regmap *regmap;
> +	struct iio_info iio_info;
> +	const struct ad7405_chip_info *info;
> +	unsigned int sample_frequency_tbl[ARRAY_SIZE(ad7405_dec_rates)];
> +	unsigned int sample_frequency;
> +	unsigned int ref_frequency;
> +};

...

> +static void ad7405_fill_samp_freq_table(struct ad7405_state *st)
> +{
> +	int i;

Why signed.

> +
> +	for (i = 0; i < ARRAY_SIZE(ad7405_dec_rates); i++)
> +		st->sample_frequency_tbl[i] = DIV_ROUND_CLOSEST_ULL(st->ref_frequency, ad7405_dec_rates[i]);

This is too long even for relaxed mode...

> +}

...

> +static int ad7405_set_sampling_rate(struct iio_dev *indio_dev,
> +				    const struct iio_chan_spec *chan,
> +				    unsigned int samp_rate)
> +{
> +	struct ad7405_state *st = iio_priv(indio_dev);
> +	unsigned int dec_rate, idx;
> +	int ret;
> +
> +	dec_rate = DIV_ROUND_CLOSEST_ULL(st->ref_frequency, samp_rate);
> +
> +	idx = find_closest_descending(dec_rate, ad7405_dec_rates,
> +				      ARRAY_SIZE(ad7405_dec_rates));
> +
> +	    dec_rate = ad7405_dec_rates[idx];

Something happened there...

> +	ret = iio_backend_set_dec_rate(st->back, dec_rate);
> +	if (ret)
> +		return ret;
> +
> +	st->sample_frequency = DIV_ROUND_CLOSEST_ULL(st->ref_frequency, dec_rate);
> +
> +	return 0;
> +}

...

Okay, I'll stop here, this driver is not ready for upstream. Please, consult
with your colleagues and do round of internal review before sending a new
version.

-- 
With Best Regards,
Andy Shevchenko