Re: [PATCH v4 2/3] iio: accel: add ADXL380 driver

[Jonathan Cameron <jic23@kernel.org>]

*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);
> > +}