Messages in this thread | | | Date | Mon, 20 Nov 2023 15:00:41 +0200 | From | Andy Shevchenko <> | Subject | Re: [PATCH v4 2/2] iio: adc: ad7173: add AD7173 driver |
| |
On Thu, Nov 16, 2023 at 03:46:55PM +0200, mitrutzceclan wrote: > From: Dumitru Ceclan <mitrutzceclan@gmail.com> > > 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, ®); > + 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
| |