On Thu, Aug 10, 2023 at 12:33:17PM +0300, Dumitru Ceclan wrote: > The AD717x 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 AD717x ADC. I believe checkpatch.pl expects at least 3 lines of help description. One of them may tell the user what will be the module name, if chosen to be built as a module. ... > +#include <linux/bitfield.h> > +#include <linux/delay.h> > +#include <linux/device.h> > +#include <linux/err.h> > +#include <linux/interrupt.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of.h> There is no user of the header. But missing bits.h, mod_devicetable.h, property.h, and might be more. So, revisit this block to see which ones are used and which one are missed, and which are redundant. > +#include <linux/sched.h> Is this used? How? > +#include <linux/slab.h> > +#include <linux/spi/spi.h> > +#include <linux/sysfs.h> > +#include <linux/units.h> > +#include <linux/gpio/driver.h> > +#include <linux/regulator/consumer.h> Can you keep the above sorted? ... > +#include <linux/iio/iio.h> > +#include <linux/iio/buffer.h> > +#include <linux/iio/sysfs.h> > +#include <linux/iio/trigger.h> > +#include <linux/iio/trigger_consumer.h> > +#include <linux/iio/triggered_buffer.h> Can you keep the above sorted? + Blank line > +#include <linux/iio/adc/ad_sigma_delta.h> ... > +#define AD717X_ID_MASK 0xfff0 GENMASK() > +#define AD717X_ADC_MODE_MODE_MASK 0x70 > +#define AD717X_ADC_MODE_CLOCKSEL_MASK 0xc You are using BIT(), and not GENMASK()... All can be converted. ... > +#define AD717X_SETUP_AREF_BUF (0x3 << 10) > +#define AD717X_SETUP_AIN_BUF (0x3 << 8) > +#define AD717X_SETUP_REF_SEL_MASK 0x30 Ditto for all. ... > +#define AD717X_SETUP_REF_SEL_AVDD1_AVSS 0x30 > +#define AD717X_SETUP_REF_SEL_INT_REF 0x20 > +#define AD717X_SETUP_REF_SEL_EXT_REF2 0x10 > +#define AD717X_SETUP_REF_SEL_EXT_REF 0x00 Seems to me like plain decimals with shift should be used #define AD717X_SETUP_REF_SEL_AVDD1_AVSS (3... #define AD717X_SETUP_REF_SEL_INT_REF (2... #define AD717X_SETUP_REF_SEL_EXT_REF2 (1... #define AD717X_SETUP_REF_SEL_EXT_REF (0 << 4) ... > +#define AD717X_FILTER_ODR0_MASK 0x1f GENMASK() ... > +static const struct ad717x_device_info ad717x_device_info[] = { > + [ID_AD7172_2] = { > + .clock = 2000000, > + }, > + [ID_AD7173_8] = { > + .clock = 2000000, > + }, > + [ID_AD7175_2] = { > + .clock = 16000000, > + }, > + [ID_AD7176_2] = { > + .clock = 16000000, > + }, > +}; HZ_PER_MHZ from units.h? ... > +static int ad717x_gpio_get(struct gpio_chip *chip, unsigned offset) > +{ > + struct ad717x_state *st = gpiochip_to_ad717x(chip); > + unsigned int mask; > + unsigned int value; > + int ret; > + > + switch (offset) { > + case 0: > + mask = AD717X_GPIO_GP_DATA0; > + break; > + case 1: > + mask = AD717X_GPIO_GP_DATA1; > + break; > + default: > + return -EINVAL; > + } > + > + ret = ad_sd_read_reg(&st->sd, AD717X_REG_GPIO, 2, &value); > + if (ret) > + return ret; > + > + return (bool)(value & mask); This is weird. You have int which you get from bool, wouldn't be better to use !!(...) as other GPIO drivers do? > +} ... > +static void ad717x_gpio_set(struct gpio_chip *chip, unsigned offset, int value) > +{ > + struct ad717x_state *st = gpiochip_to_ad717x(chip); > + unsigned int mask, set_mask, clr_mask; > + > + switch (offset) { > + case 0: > + mask = AD717X_GPIO_GP_DATA0; > + break; > + case 1: > + mask = AD717X_GPIO_GP_DATA1; > + break; > + case 2: > + mask = AD717X_GPIO_GP_DATA2; > + break; > + case 3: > + mask = AD717X_GPIO_GP_DATA3; > + break; > + default: > + return; > + } > + if (value) { > + set_mask = mask; > + clr_mask = 0; > + } else { > + set_mask = 0; > + clr_mask = mask; > + } > + > + ad717x_gpio_update(st, set_mask, clr_mask); It's too verbose, just if (value) ad717x_gpio_update(st, mask, 0); else ad717x_gpio_update(st, 0, mask); Would save a half a dozen LoCs. > +} ... > +static int ad717x_gpio_direction_input(struct gpio_chip *chip, unsigned offset) > +{ > + struct ad717x_state *st = gpiochip_to_ad717x(chip); > + unsigned int mask; > + > + switch (offset) { > + case 0: > + mask = AD717X_GPIO_IP_EN0; > + break; > + case 1: > + mask = AD717X_GPIO_IP_EN1; > + break; > + default: > + return -EINVAL; > + } > + return ad717x_gpio_update(st, mask, 0); Return directly from the switch case. > +} ... The GPIO methods are too verbose, I stopped looking into them here. The main question, why gpio-regmap is not used for this? ... > +static int ad717x_gpio_init(struct ad717x_state *st) > +{ struct device *dev = &st->sd.spi->dev; makes code neater here and elsewhere. > + st->gpiochip.label = dev_name(&st->sd.spi->dev); > + st->gpiochip.base = -1; > + if (st->info->has_gp23) > + st->gpiochip.ngpio = 4; > + else > + st->gpiochip.ngpio = 2; Instead of keeping a boolean flag in the info, why you not keep ngpio there (0 implies no GPIO)? > + st->gpiochip.parent = &st->sd.spi->dev; > + st->gpiochip.can_sleep = true; > + st->gpiochip.direction_input = ad717x_gpio_direction_input; > + st->gpiochip.direction_output = ad717x_gpio_direction_output; > + st->gpiochip.get = ad717x_gpio_get; > + st->gpiochip.set = ad717x_gpio_set; > + st->gpiochip.free = ad717x_gpio_free; > + st->gpiochip.owner = THIS_MODULE; > + > + return devm_gpiochip_add_data(&st->sd.spi->dev, &st->gpiochip, NULL); > +} ... > +static void ad717x_reset_usage_cnts(struct ad717x_state *st) > +{ > + int i; > + > + for (i = 0; i < st->info->num_configs; i++) > + (st->config_cnts)[i] = 0; What are the parentheses for? Wouldn't this a NIH one of memsetXX()? > + st->config_usage_counter = 0; > +} ... > + offset = offsetof(struct ad717x_channel_config, cfg_slot) + > + sizeof(cfg->cfg_slot); > + cmp_size = sizeof(*cfg) - offset; My gut's feeling this is some NIH one of macros from overflow.h. ... > + for (i = 0; i < st->num_channels; i++) { > + cfg_aux = &st->channels[i].cfg; > + > + if (cfg_aux->live && !memcmp(((void *)cfg) + offset, > + ((void *)cfg_aux) + offset, cmp_size)) My gosh! Explicit castings should be really rear. Can't you use proper struct / array pointers? > + return cfg_aux; > + } ... > + int ret = 0; How is this 0 used? > + if (!cfg->live) { > + live_cfg = ad717x_find_live_config(st, cfg); > + if (!live_cfg) { Why not positive check? > + ret = ad717x_load_config(st, cfg); > + if (ret < 0) > + return ret; > + } else { > + cfg->cfg_slot = live_cfg->cfg_slot; > + } > + } > + cfg->live = true; Can be moved inside the conditional. ... > + ret = ad717x_config_channel(st, channel); > + if (ret < 0) What is the meaning of > 0? Same Q to other similar checks. > + return ret; ... > +static int ad717x_setup(struct iio_dev *indio_dev) > +{ > + struct ad717x_state *st = iio_priv(indio_dev); > + unsigned int id; > + u8 *buf; > + int ret; > + > + /* reset the serial interface */ > + buf = kcalloc(8, sizeof(*buf), GFP_KERNEL); Magic number! > + if (!buf) > + return -ENOMEM; > + > + memset(buf, 0xff, 8); > + ret = spi_write(st->sd.spi, buf, 8); > + kfree(buf); > + if (ret < 0) > + return ret; I agree with comments by Nuno on this. > + /* datasheet recommends a delay of at least 500us after reset */ > + usleep_range(500, 1000); fsleep(500); > + ret = ad_sd_read_reg(&st->sd, AD717X_REG_ID, 2, &id); > + if (ret) > + return ret; > + > + id &= AD717X_ID_MASK; > + if (id != st->info->id) > + dev_warn(&st->sd.spi->dev, "Unexpected device id: %x, expected: %x\n", > + id, st->info->id); Wrong indentation. > + mutex_init(&st->cfgs_lock); > + st->adc_mode |= AD717X_ADC_MODE_REF_EN | AD717X_ADC_MODE_SING_CYC; > + st->interface_mode = 0x0; > + > + st->config_usage_counter = 0; > + st->config_cnts = devm_kzalloc(&indio_dev->dev, > + st->info->num_configs * sizeof(u64), No, let kernel do the right thing with this. > + GFP_KERNEL); > + if (!st->config_cnts) > + return -ENOMEM; > + > + /* All channels are enabled by default after a reset */ > + ret = ad717x_disable_all(&st->sd); > + if (ret < 0) > + return ret; > + > + return 0; > +} ... > +static int ad717x_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, int *val, int *val2, long info) > +{ > + struct ad717x_state *st = iio_priv(indio_dev); > + unsigned int reg; > + int ret; > + > + switch (info) { > + case IIO_CHAN_INFO_RAW: > + ret = ad_sigma_delta_single_conversion(indio_dev, chan, val); > + if (ret < 0) > + return ret; > + > + /* disable channel after single conversion */ > + ret = ad_sd_write_reg(&st->sd, AD717X_REG_CH(chan->address), 2, > + 0); This is much better on a single line. > + if (ret < 0) > + return ret; > + > + return IIO_VAL_INT; > + case IIO_CHAN_INFO_SCALE: > + if (chan->type == IIO_TEMP) { > + *val = 250000000; > + *val2 = 800273203; /* (2**24 * 477) / 10 */ Use mathematical notations (TeX like) (2^24 * 477) / 10 > + return IIO_VAL_FRACTIONAL; > + } else { > + *val = 2500; > + if (chan->differential) > + *val2 = 23; > + else > + *val2 = 24; > + return IIO_VAL_FRACTIONAL_LOG2; > + } > + case IIO_CHAN_INFO_OFFSET: > + if (chan->type == IIO_TEMP) { > + *val = -874379; > + } else { > + if (chan->differential) > + *val = -(1 << (chan->scan_type.realbits - 1)); > + else > + *val = 0; > + } > + return IIO_VAL_INT; > + case IIO_CHAN_INFO_SAMP_FREQ: > + reg = st->channels[chan->address].cfg.odr; > + > + *val = st->info->sinc5_data_rates[reg] / MILLI; > + *val2 = (st->info->sinc5_data_rates[reg] % MILLI) * MILLI; > + > + return IIO_VAL_INT_PLUS_MICRO; > + } > + return -EINVAL; > +} ... > +static int ad717x_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, int val, int val2, long info) > +{ > + int ret = 0; You won't need this if... > + mutex_lock(&st->cfgs_lock); ...you switch to use guarded mutex (see cleanup.h). Same applicable to many other functions. > + mutex_unlock(&st->cfgs_lock); > + return ret; > +} ... > +static int ad717x_debug(struct iio_dev *indio_dev, unsigned int reg, > + unsigned int writeval, unsigned int *readval) > +{ > + struct ad717x_state *st = iio_priv(indio_dev); > + u8 reg_size = 2; Better to make it an else branch... > + if (reg == 0) > + reg_size = 1; > + else if (reg == AD717X_REG_CRC || reg == AD717X_REG_DATA || > + reg >= AD717X_REG_OFFSET(0)) > + reg_size = 3; else reg_size = 2; > + if (readval) > + return ad_sd_read_reg(&st->sd, reg, reg_size, readval); > + > + return ad_sd_write_reg(&st->sd, reg, reg_size, writeval); > +} ... > +static int ad717x_of_parse_channel_config(struct iio_dev *indio_dev) of --> fw > +{ > + struct ad717x_state *st = iio_priv(indio_dev); > + struct ad717x_channel *channels_st_priv; > + struct fwnode_handle *child; > + struct device *dev = indio_dev->dev.parent; > + struct iio_chan_spec *chan; > + unsigned int num_channels = 0; How is this 0 being used? > + unsigned int ain[2], chan_index = 0; > + bool use_temp = false; No assignment needed here, see below. > + int ret; > + > + num_channels = device_get_child_node_count(dev); > + if (device_property_read_bool(dev, "adi,temp-channel")) { use_temp = device_property_... if (use_temp) { > + if (!st->info->has_temp) { > + dev_err(indio_dev->dev.parent, > + "Current chip does not support temperature channel"); > + return -EINVAL; return dev_err_probe(...); > + } > + > + num_channels++; > + use_temp = true; > + } > + st->num_channels = num_channels; I believe that it's already 0, so this can be moved... > + if (num_channels == 0) > + return 0; ...after this check. > + chan = devm_kcalloc(indio_dev->dev.parent, sizeof(*chan), num_channels, Why not use dev? > + GFP_KERNEL); > + if (!chan) > + return -ENOMEM; > + > + channels_st_priv = devm_kcalloc(indio_dev->dev.parent, num_channels, Ditto. > + sizeof(*channels_st_priv), GFP_KERNEL); > + if (!channels_st_priv) > + return -ENOMEM; > + > + indio_dev->channels = chan; > + indio_dev->num_channels = num_channels; > + st->channels = channels_st_priv; > + > + if (use_temp) { > + chan[chan_index] = ad717x_temp_iio_channel_template; > + channels_st_priv[chan_index].ain = > + AD717X_CH_ADDRESS(chan[chan_index].channel, chan[chan_index].channel2); > + channels_st_priv[chan_index].cfg.bipolar = false; > + channels_st_priv[chan_index].cfg.input_buf = true; > + chan_index++; > + } > + > + device_for_each_child_node(dev, child) { > + ret = fwnode_property_read_u32_array(child, "diff-channels", ain, 2); ARRAY_SIZE() ? > + if (ret) { > + fwnode_handle_put(child); > + return ret; > + } > + > + if (ain[0] >= st->info->num_inputs || > + ain[1] >= st->info->num_inputs) { > + dev_err(indio_dev->dev.parent, > + "Input pin number out of range for pair (%d %d).", ain[0], ain[1]); > + fwnode_handle_put(child); > + return -EINVAL; return dev_err_probe(...); > + } > + > + chan[chan_index] = ad717x_channel_template; > + chan[chan_index].address = chan_index; > + chan[chan_index].scan_index = chan_index; > + chan[chan_index].channel = ain[0]; > + chan[chan_index].channel2 = ain[1]; > + channels_st_priv[chan_index].ain = > + AD717X_CH_ADDRESS(ain[0], ain[1]); Why not one line here? > + channels_st_priv[chan_index].chan_reg = chan_index; > + channels_st_priv[chan_index].cfg.input_buf = true; > + channels_st_priv[chan_index].cfg.odr = 0; > + > + chan[chan_index].differential = fwnode_property_read_bool(child, "adi,bipolar"); > + if (chan[chan_index].differential) { > + chan[chan_index].info_mask_separate |= BIT(IIO_CHAN_INFO_OFFSET); > + channels_st_priv[chan_index].cfg.bipolar = true; > + } > + > + chan_index++; > + } > + > + return 0; > +} ... > + if (!spi->irq) { > + dev_err(&spi->dev, "No IRQ specified\n"); > + return -ENODEV; return dev_err_probe(...); > + } ... > +static const struct spi_device_id ad717x_id_table[] = { > + { "ad7172-2", }, > + { "ad7173-8", }, > + { "ad7175-2", }, > + { "ad7176-2", }, > + {} Missing driver_data here. > +}; -- With Best Regards, Andy Shevchenko