Re: [PATCH 2/2] iio: adc: ad717x: add AD717X driver
From: Andy Shevchenko
Date: Fri Aug 11 2023 - 04:47:49 EST
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