On Tue, Dec 5, 2023 at 3:46 PM Dumitru Ceclan <mitrutzceclan@xxxxxxxxx> wrote: > > 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. ... > +config AD7173 > + tristate "Analog Devices AD7173 driver" > + depends on SPI_MASTER > + select AD_SIGMA_DELTA > + select GPIO_REGMAP if GPIOLIB > + select REGMAP_SPI if GPIOLIB > + help > + Say yes here to build support for Analog Devices AD7173 and similar ADC > + Currently supported models: > + - AD7172-2, > + - AD7173-8, > + - AD7175-2, > + - AD7176-2 Drop commas (sorry if I was not clear enough). > +#include <linux/array_size.h> > +#include <linux/bitfield.h> > +#include <linux/bitmap.h> > +#include <linux/container_of.h> > +#include <linux/delay.h> > +#include <linux/device.h> > +#include <linux/err.h> > +#include <linux/gpio/driver.h> This... > +#include <linux/idr.h> > +#include <linux/interrupt.h> > +#include <linux/module.h> > +#include <linux/mod_devicetable.h> > +#include <linux/property.h> > +#include <linux/regmap.h> > +#include <linux/gpio/regmap.h> ...and this are too far from each other. I believe you should count on G order. > +#include <linux/regulator/consumer.h> > +#include <linux/slab.h> > +#include <linux/spi/spi.h> > +#include <linux/types.h> > +#include <linux/units.h> ... > +#define AD7173_GPO12_DATA(x) BIT(x) x + 0 ? > +#define AD7173_GPO23_DATA(x) BIT(x + 4) > +#define AD7173_GPO_DATA(x) (x < 2 ? AD7173_GPO12_DATA(x) : AD7173_GPO23_DATA(x)) ... > + dev_warn(&st->sd.spi->dev, Here... > + "Unexpected device id: 0x%04X, expected: 0x%04X\n", > + id, st->info->id); > + > + st->adc_mode |= AD7173_ADC_MODE_SING_CYC; > + st->interface_mode = 0x0; > + > + st->config_usage_counter = 0; > + st->config_cnts = devm_kcalloc(indio_dev->dev.parent, ...and here are different pointers in use, can you unify to use the physical device pointer (as per above) here as well? > + st->info->num_configs, sizeof(u64), > + GFP_KERNEL); > + if (!st->config_cnts) > + return -ENOMEM; ... > + case IIO_CHAN_INFO_SCALE: > + if (chan->type == IIO_TEMP) { > + *val = 250000000; MEGA? > + *val2 = 800273203; /* (2^24 * 477) / 10 */ Why not write it as is: *val2 = (BIT(24) * 477) / 10; ? It will be more robust as we don't expect compiler to give different results here > + return IIO_VAL_FRACTIONAL; > + } else { > + *val = ad7173_get_ref_voltage_milli(st, ch->cfg.ref_sel); > + *val2 = chan->scan_type.realbits - !!(ch->cfg.bipolar); > + return IIO_VAL_FRACTIONAL_LOG2; > + } ... > + /* If a regulator is not available, it will be set to a dummy regulator. > + * Each channel reference is checked with regulator_get_voltage() before > + * setting attributes so if any channel uses a dummy supply the driver > + * probe will fail. > + */ /* * Multi-line comments should follow this style. Find the * difference. */ ... > + ref_label = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_INT_REF]; > + > + fwnode_property_read_string(child, "adi,reference-select", > + &ref_label); > + ref_sel = match_string(ad7173_ref_sel_str, > + ARRAY_SIZE(ad7173_ref_sel_str), ref_label); > + if (ref_sel < 0) { Can we use fwnode_property_match_property_string()? > + fwnode_handle_put(child); > + return dev_err_probe(dev, -EINVAL, > + "Invalid channel reference name %s\n", > + ref_label); > + } -- With Best Regards, Andy Shevchenko