*grumpy*
Alexandru, crop out the irrelevant parts!
I initially only spotted the comma one due to too much scrolling.
Good to have your review though.
Jonathan
> > +#define ADXL380_FIFO_SAMPLES 315UL
> > +
> > +enum adxl380_channels {
> > + ADXL380_ACCEL_X,
> > + ADXL380_ACCEL_Y,
> > + ADXL380_ACCEL_Z,
> > + ADXL380_TEMP,
> > + ADXL380_CH_NUM,
>
> nitpick: If ADXL380_CH_NUM is the number of channels, then a trailing
> comma is not needed.
> Fine to also leave it.
> > +static int adxl380_buffer_postenable(struct iio_dev *indio_dev)
> > +{
> > + struct adxl380_state *st = iio_priv(indio_dev);
> > + int i;
> > + int ret;
> > +
> > + guard(mutex)(&st->lock);
> > +
> > + ret = adxl380_set_measure_en(st, false);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_update_bits(st->regmap,
> > + st->int_map[0],
> > + ADXL380_INT_MAP0_FIFO_WM_INT0_MSK,
> > + FIELD_PREP(ADXL380_INT_MAP0_FIFO_WM_INT0_MSK, 1));
> > + if (ret)
> > + return ret;
> > +
> > + for_each_clear_bit(i, indio_dev->active_scan_mask, ADXL380_CH_NUM) {
>
> Would this need to be?:
> for_each_set_bit(i, indio_dev->active_scan_mask, indio_dev->masklength)
>
> Or, is the logic intended to go over the cleared bits here?
>
> Depending on what's needed here, this could make use of
> "iio_for_each_active_channel()" later.
>
>
> > + ret = regmap_update_bits(st->regmap, ADXL380_DIG_EN_REG,
> > + ADXL380_CHAN_EN_MSK(i),
> > + 0 << (4 + i));
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + st->fifo_set_size = bitmap_weight(indio_dev->active_scan_mask,
> > + indio_dev->masklength);
>
> Depending on Nuno's series (and if that gets accepted first), this
> might need to use the new iio_get_masklength() wrapper.
> That's not a reason against this going in first though.
There will be a bunch of races with that, so I'm not worried if drivers
use it yet. We'll fix them all up along with the existing cases before
taking the masklength private.
I have applied Nuno's initial series and some drivers that will need
converting today :)
...
> > +static int adxl380_write_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int val, int val2, long info)
> > +{
> > + struct adxl380_state *st = iio_priv(indio_dev);
> > + int odr_index, lpf_index, hpf_index, range_index;
> > +
> > + switch (info) {
> > + case IIO_CHAN_INFO_SAMP_FREQ:
> > + odr_index = adxl380_find_match_1d_tbl(st->chip_info->samp_freq_tbl,
> > + ARRAY_SIZE(st->chip_info->samp_freq_tbl),
> > + val);
> > + return adxl380_set_odr(st, odr_index);
> > + case IIO_CHAN_INFO_CALIBBIAS:
> > + return adxl380_write_calibbias_value(st, chan->scan_index, val);
> > + case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> > + lpf_index = adxl380_find_match_1d_tbl(st->lpf_tbl,
> > + ARRAY_SIZE(st->lpf_tbl),
> > + val);
> > + if (lpf_index < 0)
> > + return lpf_index;
>
> The way I see adxl380_find_match_1d_tbl(), it will never return negative.
>
> > + return adxl380_set_lpf(st, lpf_index);
> > + case IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY:
> > + hpf_index = adxl380_find_match_2d_tbl(st->hpf_tbl,
> > + ARRAY_SIZE(st->hpf_tbl),
> > + val, val2);
> > + if (hpf_index < 0)
> > + return hpf_index;
> > + return adxl380_set_hpf(st, hpf_index);
> > + case IIO_CHAN_INFO_SCALE:
> > + range_index = adxl380_find_match_2d_tbl(st->chip_info->scale_tbl,
> > + ARRAY_SIZE(st->chip_info->scale_tbl),
> > + val, val2);
> > + if (range_index < 0)
> > + return range_index;
> > + return adxl380_set_range(st, range_index);
> > + default:
> > + return -EINVAL;
> > + }
> > +}
>
> > +int adxl380_probe(struct device *dev, struct regmap *regmap,
> > + const struct adxl380_chip_info *chip_info)
> > +{
> > + struct iio_dev *indio_dev;
> > + struct adxl380_state *st;
> > + int ret;
> > +
> > + indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + st = iio_priv(indio_dev);
> > +
> > + st->dev = dev;
> > + st->regmap = regmap;
> > + st->chip_info = chip_info;
> > +
> > + mutex_init(&st->lock);
> > +
> > + indio_dev->channels = adxl380_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(adxl380_channels);
> > + indio_dev->name = chip_info->name;
> > + indio_dev->info = &adxl380_info;
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > + ret = devm_regulator_get_enable(dev, "vddio");
> > + if (ret)
> > + return dev_err_probe(st->dev, ret,
> > + "Failed to get vddio regulator\n");
> > +
> > + ret = devm_regulator_get_enable(st->dev, "vsupply");
> > + if (ret)
> > + return dev_err_probe(st->dev, ret,
> > + "Failed to get vsupply regulator\n");
> > +
> > + st->irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT0");
> > + if (st->irq > 0) {
> > + st->int_map[0] = ADXL380_INT0_MAP0_REG;
> > + st->int_map[1] = ADXL380_INT0_MAP1_REG;
> > + } else {
> > + st->irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT1");
> > + if (st->irq > 0)
> > + return dev_err_probe(dev, -ENODEV,
> > + "no interrupt name specified");
> > + st->int_map[0] = ADXL380_INT1_MAP0_REG;
> > + st->int_map[1] = ADXL380_INT1_MAP1_REG;
> > + }
>
> Would it make sense fo this interrupt-register setup to go into
> "adxl380_config_irq()"?
>
> > +
> > + ret = adxl380_setup(indio_dev);
> > + if (ret)
> > + return ret;
> > +
> > + ret = devm_iio_kfifo_buffer_setup_ext(st->dev, indio_dev,
> > + &adxl380_buffer_ops,
> > + adxl380_fifo_attributes);
> > + if (ret)
> > + return ret;
> > +
> > + return devm_iio_device_register(dev, indio_dev);
> > +}
