Re: [PATCH v2 2/2] iio: accel: Add driver support for ADXL355

[Jonathan Cameron <jic23@kernel.org>]

...
> > > +
> > > +static int adxl355_set_hpf_3db(struct adxl355_data *data,
> > > +                            enum adxl355_hpf_3db hpf)
> > > +{
> > > +     int ret = 0;
> > > +
> > > +     mutex_lock(&data->lock);
> > > +
> > > +     if (data->hpf_3db == hpf)
> > > +             goto out_unlock;
> > > +
> > > +     ret = adxl355_set_op_mode(data, ADXL355_STANDBY);
> > > +     if (ret < 0)
> > > +             goto out_unlock;
> > > +
> > > +     ret = regmap_update_bits(data->regmap, ADXL355_FILTER,
> > > +                              ADXL355_FILTER_HPF_MSK,
> > > +                              ADXL355_FILTER_HPF_MODE(hpf));
> > > +     if (!ret)
> > > +             data->hpf_3db = hpf;
> > > +
> > > +out_unlock:
> > > +     ret = adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
> > > +     mutex_unlock(&data->lock);
> > > +     return ret;
> > > +}
> > > +
> > > +static int adxl355_set_calibbias(struct adxl355_data *data,
> > > +                              int scan_index, int calibbias)
> > > +{
> > > +     int ret = 0;
> > > +     __be16 reg = cpu_to_be16(calibbias);  
> >
> > Hmm. I'm a bit in two minds on whether we can always rely on regmap
> > now copying these buffers and hence avoiding the need for DMA safe buffers
> > when used with SPI.  It seems like it now does but that's no documented
> > and a fairly recent development. Anyhow, I went with just asking Mark
> > Brown - see top of email.
> >  
> I will need to study this as I don't have knowledge about what you are saying.

It's a really 'interesting' but fiddly corner.  Following discussion with Mark
on the other branch of this thread the upshot is we should never use a buffer
on the stack for multiple register accesses in regmap because the underlying
bus driver (here SPI controller drivers are the ones that matters) may receive
the buffer directly for use in a scatter gather DMA access.

The guarantees around coherency of access to the data buffer only apply at the
size of a cacheline.  That means you can get evil things like..

1) DMA set up for the SPI transfer using this buffer.
2) Whilst DMA is going on, some unrelated bit of the driver writes a flag.
3) DMA completes and writes back the whole cacheline (some devices do this)
   and neatly rewrites the flag to it's value from before DMA started.
Having once encountered an SPI controller that could indeed do this I can tell
you it is very tricky to debug... 
https://www.youtube.com/watch?v=JDwaMClvV-s is a good presentation by Wolfram
that goes into some of these issues and why they are really hard to solve
in general (and hence why drivers still need to care!)

Anyhow, ensuring you don't share a cacheline that is used for DMA with 
anything else guarantees everything will be fine.
There would be ways of SPI controller drivers work around this, but reality
is data moves around system interconnects in cacheline sized blocks so it
fine in real systems.

Two ways of getting such data.  A heap allocation is always big enough
to ensure no sharing. IIO also has a trick given we hit this a lot.
The iio_priv() structure is cacheline aligned and on the heap.  So
if you put your buffer at the end of your driver structure that you
access via iio_priv and mark it ____cacheline_aligned then it will
start at the beginning of a new cacheline and that C requires structures
to be padded to the value of maximum element alignment means you
will be in a cacheline on your own.

A side note here is that is fine to share a cacheline for rx and tx
buffers if that makes sense as we can assume an SPI controller won't
trash it's own data.


> > > +
> > > +     mutex_lock(&data->lock);
> > > +
> > > +     ret = adxl355_set_op_mode(data, ADXL355_STANDBY);
> > > +     if (ret < 0)
> > > +             goto out_unlock;
> > > +

...

> >  
> > > +
> > > +static const struct iio_info adxl355_info = {
> > > +     .read_raw       = adxl355_read_raw,
> > > +     .write_raw      = adxl355_write_raw,
> > > +     .read_avail     = &adxl355_read_avail
> > > +};
> > > +
> > > +#define ADXL355_ACCEL_CHANNEL(index, reg, axis) {                    \
> > > +     .type = IIO_ACCEL,                                              \
> > > +     .address = reg,                                                 \
> > > +     .modified = 1,                                                  \
> > > +     .channel2 = IIO_MOD_##axis,                                     \
> > > +     .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |                  \
> > > +                           BIT(IIO_CHAN_INFO_CALIBBIAS),             \
> > > +     .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |          \
> > > +                                 BIT(IIO_CHAN_INFO_SAMP_FREQ) |      \
> > > +             BIT(IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY),      \
> > > +     .info_mask_shared_by_type_available =                           \
> > > +             BIT(IIO_CHAN_INFO_SAMP_FREQ) |                          \
> > > +             BIT(IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY),      \
> > > +     .scan_index = index,                                            \  
> >
> > Only makes sense if you are supporting buffered mode as otherwise there
> > isn't really any such thing as a 'scan'.  
> 
> I will be adding the support for buffered mode soon, is it fine if I
> leave these here?

Sure, that's fine though even better to do it in that patch ;)

Jonathan