Re: [PATCH 5/5] iio: adc: ad7405: add ad7405 driver
From: Andy Shevchenko
Date: Mon Mar 24 2025 - 06:21:33 EST
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@xxxxxxxxxx>
...
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