Re: [PATCH v4 2/2] iio: adc: ad7173: add AD7173 driver

[Andy Shevchenko]

On Thu, Nov 16, 2023 at 03:46:55PM +0200, mitrutzceclan wrote:
> From: Dumitru Ceclan <mitrutzceclan@xxxxxxxxx>
> 
> The AD7173 family offer a complete integrated Sigma-Delta ADC solution
> which can be used in high precision, low noise single channel
> applications or higher speed multiplexed applications. The Sigma-Delta
> ADC is intended primarily for measurement of signals close to DC but also
> delivers outstanding performance with input bandwidths out to ~10kHz.

...

> +	help
> +	  Say yes here to build support for Analog Devices AD7173 and similar ADC
> +	  (currently supported: AD7172-2, AD7173-8, AD7175-2, AD7176-2).

This is hard to maintain, list it one model per a single line.

> +	  To compile this driver as a module, choose M here: the module will be
> +	  called ad7173.

...

+ array_size.h

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

> +#include <linux/bits.h>

This is guaranteed to be included by one from the above (don't remember
by heart which one or even both).

+ container_of.h

> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/interrupt.h>

> +#include <linux/kernel.h>

How is this being used (as not a proxy)?

> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/gpio/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +#include <linux/spi/spi.h>
> +#include <linux/units.h>

...

> +#define AD7173_CH_ADDRESS(pos, neg) \
> +	(FIELD_PREP(AD7173_CH_SETUP_AINPOS_MASK, pos) |\

Space before \ here and everywhere else in multi-line definitions.

> +	 FIELD_PREP(AD7173_CH_SETUP_AINNEG_MASK, neg))

...

> +#define AD7173_VOLTAGE_INT_REF_MICROV	2500000

MICROV --> uV (yes, with small letter), it's a common use for Amperes, Volts,
etc.

...

> +struct ad7173_channel_config {
> +	bool live;
> +	u8 cfg_slot;
> +	/* Following fields are used to compare equality. Bipolar must be first */
> +	bool bipolar;
> +	bool input_buf;
> +	u8 odr;
> +	u8 ref_sel;

If you group better by types, it might save a few bytes on the architectures /
compilers where bool != byte.

> +};

...

> +	st->reg_gpiocon_regmap = devm_regmap_init_spi(st->sd.spi, &ad7173_regmap_config);
> +	if (IS_ERR(st->reg_gpiocon_regmap)) {
> +		return dev_err_probe(dev, PTR_ERR(st->reg_gpiocon_regmap),
> +				     "Unable to init regmap\n");
> +	}

{} are not needed, can also be written as

	st->reg_gpiocon_regmap = devm_regmap_init_spi(st->sd.spi, &ad7173_regmap_config);
	ret = PTR_ERR_OR_ZERO(st->reg_gpiocon_regmap);
	if (ret)
		return dev_err_probe(dev, ret, "Unable to init regmap\n");

...

> +	st->gpio_regmap = devm_gpio_regmap_register(dev, &gpio_regmap);
> +	if (IS_ERR(st->gpio_regmap)) {
> +		return dev_err_probe(dev, PTR_ERR(st->gpio_regmap),
> +				     "Unable to init gpio-regmap\n");
> +	}

Ditto.

...

> +static struct ad7173_channel_config *ad7173_find_live_config
> +	(struct ad7173_state *st, struct ad7173_channel_config *cfg)

This is strange indentation.

Perhaps

static struct ad7173_channel_config *
ad7173_find_live_config(struct ad7173_state *st, struct ad7173_channel_config *cfg)

?

...

> +	offset = offsetof(struct ad7173_channel_config, cfg_slot) +
> +		 sizeof(cfg->cfg_slot);

Isn't it a offsetofend() from stddef.h?

> +	cmp_size = sizeof(*cfg) - offset;

sizeof_field() from the above mentioned header?

...

> +	for (i = 0; i < st->num_channels; i++) {
> +		cfg_aux = &st->channels[i].cfg;
> +
> +		if (cfg_aux->live && !memcmp(&cfg->bipolar, &cfg_aux->bipolar,
> +					     cmp_size))

I would split this on logic operator, it will be easier to read.

> +			return cfg_aux;
> +	}

...

> +	free_cfg_slot = find_first_zero_bit(st->cfg_slots_status,
> +					    st->info->num_configs);
> +	if (free_cfg_slot == st->info->num_configs)
> +		free_cfg_slot = ad7173_free_config_slot_lru(st);
> +
> +	set_bit(free_cfg_slot, st->cfg_slots_status);
> +	cfg->cfg_slot = free_cfg_slot;

Looks like reinvention of IDA.

...

> +	struct ad7173_state *st = iio_priv(indio_dev);
> +	unsigned int id;
> +	u8 buf[AD7173_RESET_LENGTH];
> +	int ret;

Reversed xmas tree order?

	struct ad7173_state *st = iio_priv(indio_dev);
	u8 buf[AD7173_RESET_LENGTH];
	unsigned int id;
	int ret;

...

> +	return vref / (MICRO/MILLI);

What does the denominator mean and why you can't simply use MILL?

...

> +			if (ch->cfg.bipolar)
> +				/* (1<<31) is UB for a 32bit channel */
> +				*val = (chan->scan_type.realbits == 32) ?
> +					INT_MIN :
> +					-(1 << (chan->scan_type.realbits - 1));

So, what's the issue to use BIT() which has no such issue with UB?

> +			else
> +				*val = 0;

...

> +		*val = st->info->sinc5_data_rates[reg] / (MICRO/MILLI);
> +		*val2 = (st->info->sinc5_data_rates[reg] % MILLI) * (MICRO/MILLI);

Same Q about denominator.

...

> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		freq = val * MILLI + val2 / MILLI;

> +

Unneeded blank line.

> +		for (i = 0; i < st->info->num_sinc5_data_rates - 1; i++) {
> +			if (freq >= st->info->sinc5_data_rates[i])
> +				break;
> +		}
> +
> +		cfg = &st->channels[chan->address].cfg;
> +		cfg->odr = i;
> +
> +		if (!cfg->live)
> +			break;
> +
> +		ret = ad_sd_read_reg(&st->sd, AD7173_REG_FILTER(cfg->cfg_slot), 2, &reg);
> +		if (ret)
> +			break;
> +		reg &= ~AD7173_FILTER_ODR0_MASK;
> +		reg |= FIELD_PREP(AD7173_FILTER_ODR0_MASK, i);
> +		ret = ad_sd_write_reg(&st->sd, AD7173_REG_FILTER(cfg->cfg_slot), 2, reg);
> +		break;

...

> +static int ad7173_update_scan_mode(struct iio_dev *indio_dev,
> +				   const unsigned long *scan_mask)
> +{
> +	struct ad7173_state *st = iio_priv(indio_dev);

> +	int i, ret = 0;

Use the 0 directly...

> +
> +	for (i = 0; i < indio_dev->num_channels; i++) {
> +		if (test_bit(i, scan_mask))
> +			ret = ad7173_set_channel(&st->sd, i);
> +		else
> +			ret = ad_sd_write_reg(&st->sd, AD7173_REG_CH(i), 2, 0);
> +
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return ret;

...here.

> +}

> +	chan_arr = devm_kcalloc(dev, sizeof(*chan_arr), num_channels,
> +				GFP_KERNEL);

One line.

> +	if (!chan_arr)
> +		return -ENOMEM;

...

> +		if (fwnode_property_read_u32(child, "adi,reference-select", &ref_sel))
> +			ref_sel = AD7173_SETUP_REF_SEL_INT_REF;

if is redundant.

		ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
		fwnode_property_read_u32(child, "adi,reference-select", &ref_sel);

-- 
With Best Regards,
Andy Shevchenko