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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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@analog.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