>
> > > +struct bmi323_data {
> > > + struct device *dev;
> > > + struct regmap *regmap;
> > > + struct iio_mount_matrix orientation;
> > > + enum bmi323_irq_pin irq_pin;
> > > + struct iio_trigger *trig;
> > > + bool drdy_trigger_enabled;
> > > + enum bmi323_state state;
> > > + s64 fifo_tstamp, old_fifo_tstamp;
> > > + u32 odrns[2];
> > > + u32 odrhz[2];
> > > + unsigned int feature_events;
> > > +
> > > + /*
> > > + * Lock to protect the members of device's private data from concurrent
> > > + * access and also to serialize the access of extended registers.
> > > + * See bmi323_write_ext_reg(..) for more info.
> > > + */
> > > + struct mutex mutex;
> > > + int watermark;
> > > + __le16 fifo_buff[BMI323_FIFO_FULL_IN_WORDS] __aligned(IIO_DMA_MINALIGN);
> > > + struct {
> > > + __le16 channels[6];
> > > + s64 ts __aligned(8);
> >
> > Hopefully Andy's aligned_s64 set will land soon and we can tidy this up.
> > I'm a bit unsure of this, but can you overlap some of these buffers or are
> > they used concurrently? (if they are then we have problems with DMA safety.)
> >
> > Perhaps an anonymous union is appropriate?
>
> Yes both buffers are used at the same time. In fifo_flush
> fifo_buff is used to store all fifo data, and buffer is
> used to push a single data frame to iio buffers, overlapping
> will corrupt the data, so I used separate buffers for both.
Ah. So the structure is used in 2 ways.
1. As a target for DMA, which means it should live in the cacheline we
are saving for that purpsoe.
2. As a place to build up data.
In general we should be careful with doing 2 as that could race with
DMA and end up with data corruption, however you only use it that
way in flush_fifo where both the DMA and this usage under under
the mutex. Hence I think you are fine.
>
> > > +static IIO_DEVICE_ATTR_RW(in_accel_gyro_averaging, 0);
> > > +static IIO_CONST_ATTR(in_accel_gyro_averaging_available, "2 4 8 16 32 64");
> > > +
> > > +static struct attribute *bmi323_attributes[] = {
> > > + &iio_dev_attr_in_accel_gyro_averaging.dev_attr.attr,
> > > + &iio_const_attr_in_accel_gyro_averaging_available.dev_attr.attr,
> >
> > So averaging often maps directly to oversampling. Kind of different names
> > for the same thing. Perhaps that standard ABI can be used?
> > It tends to make sampling frequency reporting need to take it into account
> > though as that drops as divided by oversampling ratio.
>
> Yes, oversampling can be used, but changing the average
> value doesn't alter the sampling frequency. The sampling
> frequency is same even with the increase in averaging value.
Ok. That's unusual so good to know.
> > > +static int bmi323_feature_engine_enable(struct bmi323_data *data, bool en)
> > > +{
> > > + unsigned int feature_status;
> > > + int ret, i;
> > > +
> > > + if (en) {
> > > + ret = regmap_write(data->regmap, BMI323_FEAT_IO2_REG,
> > > + 0x012c);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = regmap_write(data->regmap, BMI323_FEAT_IO_STATUS_REG,
> > > + BMI323_FEAT_IO_STATUS_MSK);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = regmap_write(data->regmap, BMI323_FEAT_CTRL_REG,
> > > + BMI323_FEAT_ENG_EN_MSK);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + i = 5;
> >
> > Why 5?
>
> No specific reason, during testing the feature engine was
> taking around 4 milliseconds, so I thought of checking
> every 2 milliseconds and max of 5 trials.
That's a good reason. Just add a comment to that say that.
>
> > > + * From BMI323 datasheet section 4: Notes on the Serial Interface Support.
> > > + * Each SPI register read operation requires to read one dummy byte before
> > > + * the actual payload.
> > > + */
> > > +static int bmi323_regmap_spi_read(void *context, const void *reg_buf,
> > > + size_t reg_size, void *val_buf,
> > > + size_t val_size)
> > > +{
> > > + struct spi_device *spi = context;
> > > + u8 reg, *buff = NULL;
> > > + int ret;
> > > +
> > > + buff = kmalloc(val_size + BMI323_SPI_DUMMY, GFP_KERNEL);
> >
> > Hmm. Regmap has pad_bits (which can be multiple bytes) but this case
> > is unusual in that they only apply to reads.
> >
> > I wonder if we can make this cheaper though rather than having
> > to handle either some context or having dynamic allocations in here.
> >
> > How about making the write bigger? Does that have any effect?
> > Looks like don't care state in Figure 31. If that's the case,
> > send some zeros on that as it's known fixed size (2 bytes including
> > the padding) and then you can directly use the read buffer without
> > yet another memcpy.
>
> For spi with pad_bits=8 and without any custom read and
> write functions, regmap_read() works but regmap_write()
> does not. Write is also adding 8 bits of padding and
> the device is treating it as data.
> (7.2.3 SPI Protocol Figure 30)
Understood. I looked it up too before suggesting this local hack.
You'll still need a custom regmap, but at least this trick would
allow you to avoid allocating a buffer in the read function.
Jonathan
