After added inclination angle channels, the trigger buffer size is
insufficient. Extend the buffer size from 16 to 32 bytes, and change
the trigger buffer from the struct to a u8 array to adapt the sensor
with/without inclination angles output.
New trigger buffer data:
- SCA3300: 3 accel channels, temp, and timestamp.
- SCL3300: 3 accel channels, temp, 3 incli channels, and timestamp.
Readjustment the scan index to make it consistent with the buffer data.
Signed-off-by: LI Qingwu <[email protected]>
---
drivers/iio/accel/sca3300.c | 29 ++++++++++++++++++-----------
1 file changed, 18 insertions(+), 11 deletions(-)
diff --git a/drivers/iio/accel/sca3300.c b/drivers/iio/accel/sca3300.c
index 3c4827bfef53..820dfb635bf1 100644
--- a/drivers/iio/accel/sca3300.c
+++ b/drivers/iio/accel/sca3300.c
@@ -47,12 +47,20 @@
#define SCL3300_REG_ANG_CTRL 0x0C
#define SCL3300_ANG_ENABLE 0x1F
+/*
+ * Buffer size max case:
+ * Three accel channels, two bytes per channel.
+ * Temperature channel, two bytes.
+ * Three incli channels, two bytes per channel.
+ * Timestamp channel, eight bytes.
+ */
+#define SCA3300_MAX_BUFFER_SIZE (ALIGN(2 * 7, sizeof(s64)) + sizeof(s64))
+
enum sca3300_scan_indexes {
SCA3300_ACC_X = 0,
SCA3300_ACC_Y,
SCA3300_ACC_Z,
SCA3300_TEMP,
- SCA3300_TIMESTAMP,
SCA3300_INCLI_X,
SCA3300_INCLI_Y,
SCA3300_INCLI_Z,
@@ -140,10 +148,10 @@ static const struct iio_chan_spec scl3300_channels[] = {
SCA3300_ACCEL_CHANNEL(SCA3300_ACC_Y, 0x2, Y),
SCA3300_ACCEL_CHANNEL(SCA3300_ACC_Z, 0x3, Z),
SCA3300_TEMP_CHANNEL(SCA3300_TEMP, 0x05),
- IIO_CHAN_SOFT_TIMESTAMP(4),
SCA3300_INCLI_CHANNEL(SCA3300_INCLI_X, 0x09, X),
SCA3300_INCLI_CHANNEL(SCA3300_INCLI_Y, 0x0A, Y),
SCA3300_INCLI_CHANNEL(SCA3300_INCLI_Z, 0x0B, Z),
+ IIO_CHAN_SOFT_TIMESTAMP(7),
};
static const unsigned long sca3300_scan_masks[] = {
@@ -184,7 +192,9 @@ struct sca3300_chip_info {
* @spi: SPI device structure
* @lock: Data buffer lock
* @chip: Sensor chip specific information
- * @scan: Triggered buffer. Four channel 16-bit data + 64-bit timestamp
+ * @buffer: Triggered buffer:
+ * -SCA3300: 4 channel 16-bit data + 64-bit timestamp
+ * -SCL3300: 7 channel 16-bit data + 64-bit timestamp
* @txbuf: Transmit buffer
* @rxbuf: Receive buffer
*/
@@ -192,10 +202,7 @@ struct sca3300_data {
struct spi_device *spi;
struct mutex lock;
const struct sca3300_chip_info *chip;
- struct {
- s16 channels[4];
- s64 ts __aligned(sizeof(s64));
- } scan;
+ u8 buffer[SCA3300_MAX_BUFFER_SIZE] __aligned(sizeof(s64));
u8 txbuf[4] __aligned(IIO_DMA_MINALIGN);
u8 rxbuf[4];
};
@@ -484,21 +491,21 @@ static irqreturn_t sca3300_trigger_handler(int irq, void *p)
struct iio_dev *indio_dev = pf->indio_dev;
struct sca3300_data *data = iio_priv(indio_dev);
int bit, ret, val, i = 0;
+ s16 *channels = (s16 *)data->buffer;
for_each_set_bit(bit, indio_dev->active_scan_mask,
indio_dev->masklength) {
- ret = sca3300_read_reg(data, sca3300_channels[bit].address,
- &val);
+ ret = sca3300_read_reg(data, indio_dev->channels[bit].address, &val);
if (ret) {
dev_err_ratelimited(&data->spi->dev,
"failed to read register, error: %d\n", ret);
/* handled, but bailing out due to errors */
goto out;
}
- data->scan.channels[i++] = val;
+ channels[i++] = val;
}
- iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
+ iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
iio_get_time_ns(indio_dev));
out:
iio_trigger_notify_done(indio_dev->trig);
--
2.25.1
On Fri, 1 Jul 2022 02:30:30 +0000
LI Qingwu <[email protected]> wrote:
> After added inclination angle channels, the trigger buffer size is
> insufficient. Extend the buffer size from 16 to 32 bytes, and change
> the trigger buffer from the struct to a u8 array to adapt the sensor
> with/without inclination angles output.
> New trigger buffer data:
> - SCA3300: 3 accel channels, temp, and timestamp.
> - SCL3300: 3 accel channels, temp, 3 incli channels, and timestamp.
> Readjustment the scan index to make it consistent with the buffer data.
>
> Signed-off-by: LI Qingwu <[email protected]>
Hi.
Looks good. A trivial suggestion inline to make the code a little more
'self documenting'. It's a minor change so if you are happy with
the suggestion I can tweak that whilst applying.
Thanks,
Jonathan
> ---
> drivers/iio/accel/sca3300.c | 29 ++++++++++++++++++-----------
> 1 file changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/iio/accel/sca3300.c b/drivers/iio/accel/sca3300.c
> index 3c4827bfef53..820dfb635bf1 100644
> --- a/drivers/iio/accel/sca3300.c
> +++ b/drivers/iio/accel/sca3300.c
> @@ -47,12 +47,20 @@
> #define SCL3300_REG_ANG_CTRL 0x0C
> #define SCL3300_ANG_ENABLE 0x1F
>
> +/*
> + * Buffer size max case:
> + * Three accel channels, two bytes per channel.
> + * Temperature channel, two bytes.
> + * Three incli channels, two bytes per channel.
> + * Timestamp channel, eight bytes.
> + */
> +#define SCA3300_MAX_BUFFER_SIZE (ALIGN(2 * 7, sizeof(s64)) + sizeof(s64))
Instead of the 2 use sizeof(s16)
Also now you don't have timestamp in your enum sca3000_scan_indexes
you could add a 'tail' element to the enum such as SCA3000_SCAN_MAX then
use that instead of the 7 here. Hopefully that would make
this more self documenting.
> +
> enum sca3300_scan_indexes {
> SCA3300_ACC_X = 0,
> SCA3300_ACC_Y,
> SCA3300_ACC_Z,
> SCA3300_TEMP,
> - SCA3300_TIMESTAMP,
> SCA3300_INCLI_X,
> SCA3300_INCLI_Y,
> SCA3300_INCLI_Z,
> @@ -140,10 +148,10 @@ static const struct iio_chan_spec scl3300_channels[] = {
> SCA3300_ACCEL_CHANNEL(SCA3300_ACC_Y, 0x2, Y),
> SCA3300_ACCEL_CHANNEL(SCA3300_ACC_Z, 0x3, Z),
> SCA3300_TEMP_CHANNEL(SCA3300_TEMP, 0x05),
> - IIO_CHAN_SOFT_TIMESTAMP(4),
> SCA3300_INCLI_CHANNEL(SCA3300_INCLI_X, 0x09, X),
> SCA3300_INCLI_CHANNEL(SCA3300_INCLI_Y, 0x0A, Y),
> SCA3300_INCLI_CHANNEL(SCA3300_INCLI_Z, 0x0B, Z),
> + IIO_CHAN_SOFT_TIMESTAMP(7),
> };
>
> static const unsigned long sca3300_scan_masks[] = {
> @@ -184,7 +192,9 @@ struct sca3300_chip_info {
> * @spi: SPI device structure
> * @lock: Data buffer lock
> * @chip: Sensor chip specific information
> - * @scan: Triggered buffer. Four channel 16-bit data + 64-bit timestamp
> + * @buffer: Triggered buffer:
> + * -SCA3300: 4 channel 16-bit data + 64-bit timestamp
> + * -SCL3300: 7 channel 16-bit data + 64-bit timestamp
> * @txbuf: Transmit buffer
> * @rxbuf: Receive buffer
> */
> @@ -192,10 +202,7 @@ struct sca3300_data {
> struct spi_device *spi;
> struct mutex lock;
> const struct sca3300_chip_info *chip;
> - struct {
> - s16 channels[4];
> - s64 ts __aligned(sizeof(s64));
> - } scan;
> + u8 buffer[SCA3300_MAX_BUFFER_SIZE] __aligned(sizeof(s64));
> u8 txbuf[4] __aligned(IIO_DMA_MINALIGN);
> u8 rxbuf[4];
> };
> @@ -484,21 +491,21 @@ static irqreturn_t sca3300_trigger_handler(int irq, void *p)
> struct iio_dev *indio_dev = pf->indio_dev;
> struct sca3300_data *data = iio_priv(indio_dev);
> int bit, ret, val, i = 0;
> + s16 *channels = (s16 *)data->buffer;
>
> for_each_set_bit(bit, indio_dev->active_scan_mask,
> indio_dev->masklength) {
> - ret = sca3300_read_reg(data, sca3300_channels[bit].address,
> - &val);
> + ret = sca3300_read_reg(data, indio_dev->channels[bit].address, &val);
> if (ret) {
> dev_err_ratelimited(&data->spi->dev,
> "failed to read register, error: %d\n", ret);
> /* handled, but bailing out due to errors */
> goto out;
> }
> - data->scan.channels[i++] = val;
> + channels[i++] = val;
> }
>
> - iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
> + iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
> iio_get_time_ns(indio_dev));
> out:
> iio_trigger_notify_done(indio_dev->trig);
> -----Original Message-----
> From: Jonathan Cameron <[email protected]>
> Sent: Saturday, July 2, 2022 12:46 AM
> To: LI Qingwu <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> GEO-CHHER-bsp-development <[email protected]>
> Subject: Re: [PATCH V1 1/1] iio: accel: sca3300: Extend the trigger buffer from
> 16 to 32 bytes
>
> [Some people who received this message don't often get email from
> [email protected]. Learn why this is important at
> https://aka.ms/LearnAboutSenderIdentification ]
>
> This email is not from Hexagon's Office 365 instance. Please be careful while
> clicking links, opening attachments, or replying to this email.
>
>
> On Fri, 1 Jul 2022 02:30:30 +0000
> LI Qingwu <[email protected]> wrote:
>
> > After added inclination angle channels, the trigger buffer size is
> > insufficient. Extend the buffer size from 16 to 32 bytes, and change
> > the trigger buffer from the struct to a u8 array to adapt the sensor
> > with/without inclination angles output.
> > New trigger buffer data:
> > - SCA3300: 3 accel channels, temp, and timestamp.
> > - SCL3300: 3 accel channels, temp, 3 incli channels, and timestamp.
> > Readjustment the scan index to make it consistent with the buffer data.
> >
> > Signed-off-by: LI Qingwu <[email protected]>
>
> Hi.
>
> Looks good. A trivial suggestion inline to make the code a little more 'self
> documenting'. It's a minor change so if you are happy with the suggestion I
> can tweak that whilst applying.
>
> Thanks,
>
> Jonathan
>
Yes, sure, absolutely agree, Thank you!
LI Qingwu
> > ---
> > drivers/iio/accel/sca3300.c | 29 ++++++++++++++++++-----------
> > 1 file changed, 18 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/iio/accel/sca3300.c b/drivers/iio/accel/sca3300.c
> > index 3c4827bfef53..820dfb635bf1 100644
> > --- a/drivers/iio/accel/sca3300.c
> > +++ b/drivers/iio/accel/sca3300.c
> > @@ -47,12 +47,20 @@
> > #define SCL3300_REG_ANG_CTRL 0x0C
> > #define SCL3300_ANG_ENABLE 0x1F
> >
> > +/*
> > + * Buffer size max case:
> > + * Three accel channels, two bytes per channel.
> > + * Temperature channel, two bytes.
> > + * Three incli channels, two bytes per channel.
> > + * Timestamp channel, eight bytes.
> > + */
> > +#define SCA3300_MAX_BUFFER_SIZE (ALIGN(2 * 7, sizeof(s64)) +
> > +sizeof(s64))
>
> Instead of the 2 use sizeof(s16)
> Also now you don't have timestamp in your enum sca3000_scan_indexes you
> could add a 'tail' element to the enum such as SCA3000_SCAN_MAX then use
> that instead of the 7 here. Hopefully that would make this more self
> documenting.
>
> > +
> > enum sca3300_scan_indexes {
> > SCA3300_ACC_X = 0,
> > SCA3300_ACC_Y,
> > SCA3300_ACC_Z,
> > SCA3300_TEMP,
> > - SCA3300_TIMESTAMP,
> > SCA3300_INCLI_X,
> > SCA3300_INCLI_Y,
> > SCA3300_INCLI_Z,
> > @@ -140,10 +148,10 @@ static const struct iio_chan_spec scl3300_channels[]
> = {
> > SCA3300_ACCEL_CHANNEL(SCA3300_ACC_Y, 0x2, Y),
> > SCA3300_ACCEL_CHANNEL(SCA3300_ACC_Z, 0x3, Z),
> > SCA3300_TEMP_CHANNEL(SCA3300_TEMP, 0x05),
> > - IIO_CHAN_SOFT_TIMESTAMP(4),
> > SCA3300_INCLI_CHANNEL(SCA3300_INCLI_X, 0x09, X),
> > SCA3300_INCLI_CHANNEL(SCA3300_INCLI_Y, 0x0A, Y),
> > SCA3300_INCLI_CHANNEL(SCA3300_INCLI_Z, 0x0B, Z),
> > + IIO_CHAN_SOFT_TIMESTAMP(7),
> > };
> >
> > static const unsigned long sca3300_scan_masks[] = { @@ -184,7 +192,9
> > @@ struct sca3300_chip_info {
> > * @spi: SPI device structure
> > * @lock: Data buffer lock
> > * @chip: Sensor chip specific information
> > - * @scan: Triggered buffer. Four channel 16-bit data + 64-bit
> > timestamp
> > + * @buffer: Triggered buffer:
> > + * -SCA3300: 4 channel 16-bit data + 64-bit timestamp
> > + * -SCL3300: 7 channel 16-bit data + 64-bit timestamp
> > * @txbuf: Transmit buffer
> > * @rxbuf: Receive buffer
> > */
> > @@ -192,10 +202,7 @@ struct sca3300_data {
> > struct spi_device *spi;
> > struct mutex lock;
> > const struct sca3300_chip_info *chip;
> > - struct {
> > - s16 channels[4];
> > - s64 ts __aligned(sizeof(s64));
> > - } scan;
> > + u8 buffer[SCA3300_MAX_BUFFER_SIZE] __aligned(sizeof(s64));
> > u8 txbuf[4] __aligned(IIO_DMA_MINALIGN);
> > u8 rxbuf[4];
> > };
> > @@ -484,21 +491,21 @@ static irqreturn_t sca3300_trigger_handler(int irq,
> void *p)
> > struct iio_dev *indio_dev = pf->indio_dev;
> > struct sca3300_data *data = iio_priv(indio_dev);
> > int bit, ret, val, i = 0;
> > + s16 *channels = (s16 *)data->buffer;
> >
> > for_each_set_bit(bit, indio_dev->active_scan_mask,
> > indio_dev->masklength) {
> > - ret = sca3300_read_reg(data,
> sca3300_channels[bit].address,
> > - &val);
> > + ret = sca3300_read_reg(data,
> > + indio_dev->channels[bit].address, &val);
> > if (ret) {
> > dev_err_ratelimited(&data->spi->dev,
> > "failed to read register, error: %d\n",
> ret);
> > /* handled, but bailing out due to errors */
> > goto out;
> > }
> > - data->scan.channels[i++] = val;
> > + channels[i++] = val;
> > }
> >
> > - iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
> > + iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
> >
> iio_get_time_ns(indio_dev));
> > out:
> > iio_trigger_notify_done(indio_dev->trig);
On Mon, 4 Jul 2022 01:01:46 +0000
LI Qingwu <[email protected]> wrote:
> > -----Original Message-----
> > From: Jonathan Cameron <[email protected]>
> > Sent: Saturday, July 2, 2022 12:46 AM
> > To: LI Qingwu <[email protected]>
> > Cc: [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected];
> > GEO-CHHER-bsp-development <[email protected]>
> > Subject: Re: [PATCH V1 1/1] iio: accel: sca3300: Extend the trigger buffer from
> > 16 to 32 bytes
> >
> > [Some people who received this message don't often get email from
> > [email protected]. Learn why this is important at
> > https://aka.ms/LearnAboutSenderIdentification ]
> >
> > This email is not from Hexagon's Office 365 instance. Please be careful while
> > clicking links, opening attachments, or replying to this email.
> >
> >
> > On Fri, 1 Jul 2022 02:30:30 +0000
> > LI Qingwu <[email protected]> wrote:
> >
> > > After added inclination angle channels, the trigger buffer size is
> > > insufficient. Extend the buffer size from 16 to 32 bytes, and change
> > > the trigger buffer from the struct to a u8 array to adapt the sensor
> > > with/without inclination angles output.
> > > New trigger buffer data:
> > > - SCA3300: 3 accel channels, temp, and timestamp.
> > > - SCL3300: 3 accel channels, temp, 3 incli channels, and timestamp.
> > > Readjustment the scan index to make it consistent with the buffer data.
> > >
> > > Signed-off-by: LI Qingwu <[email protected]>
> >
> > Hi.
> >
> > Looks good. A trivial suggestion inline to make the code a little more 'self
> > documenting'. It's a minor change so if you are happy with the suggestion I
> > can tweak that whilst applying.
> >
> > Thanks,
> >
> > Jonathan
> >
>
> Yes, sure, absolutely agree, Thank you!
Looks like I should be able to sneak in a last pull request for this cycle
before the merge window (it was delayed a week). Hence I've applied this
to the togreg branch of iio.git
Thanks,
Jonathan
>
> LI Qingwu
>
> > > ---
> > > drivers/iio/accel/sca3300.c | 29 ++++++++++++++++++-----------
> > > 1 file changed, 18 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/iio/accel/sca3300.c b/drivers/iio/accel/sca3300.c
> > > index 3c4827bfef53..820dfb635bf1 100644
> > > --- a/drivers/iio/accel/sca3300.c
> > > +++ b/drivers/iio/accel/sca3300.c
> > > @@ -47,12 +47,20 @@
> > > #define SCL3300_REG_ANG_CTRL 0x0C
> > > #define SCL3300_ANG_ENABLE 0x1F
> > >
> > > +/*
> > > + * Buffer size max case:
> > > + * Three accel channels, two bytes per channel.
> > > + * Temperature channel, two bytes.
> > > + * Three incli channels, two bytes per channel.
> > > + * Timestamp channel, eight bytes.
> > > + */
> > > +#define SCA3300_MAX_BUFFER_SIZE (ALIGN(2 * 7, sizeof(s64)) +
> > > +sizeof(s64))
> >
> > Instead of the 2 use sizeof(s16)
> > Also now you don't have timestamp in your enum sca3000_scan_indexes you
> > could add a 'tail' element to the enum such as SCA3000_SCAN_MAX then use
> > that instead of the 7 here. Hopefully that would make this more self
> > documenting.
>
>
> >
> > > +
> > > enum sca3300_scan_indexes {
> > > SCA3300_ACC_X = 0,
> > > SCA3300_ACC_Y,
> > > SCA3300_ACC_Z,
> > > SCA3300_TEMP,
> > > - SCA3300_TIMESTAMP,
> > > SCA3300_INCLI_X,
> > > SCA3300_INCLI_Y,
> > > SCA3300_INCLI_Z,
> > > @@ -140,10 +148,10 @@ static const struct iio_chan_spec scl3300_channels[]
> > = {
> > > SCA3300_ACCEL_CHANNEL(SCA3300_ACC_Y, 0x2, Y),
> > > SCA3300_ACCEL_CHANNEL(SCA3300_ACC_Z, 0x3, Z),
> > > SCA3300_TEMP_CHANNEL(SCA3300_TEMP, 0x05),
> > > - IIO_CHAN_SOFT_TIMESTAMP(4),
> > > SCA3300_INCLI_CHANNEL(SCA3300_INCLI_X, 0x09, X),
> > > SCA3300_INCLI_CHANNEL(SCA3300_INCLI_Y, 0x0A, Y),
> > > SCA3300_INCLI_CHANNEL(SCA3300_INCLI_Z, 0x0B, Z),
> > > + IIO_CHAN_SOFT_TIMESTAMP(7),
> > > };
> > >
> > > static const unsigned long sca3300_scan_masks[] = { @@ -184,7 +192,9
> > > @@ struct sca3300_chip_info {
> > > * @spi: SPI device structure
> > > * @lock: Data buffer lock
> > > * @chip: Sensor chip specific information
> > > - * @scan: Triggered buffer. Four channel 16-bit data + 64-bit
> > > timestamp
> > > + * @buffer: Triggered buffer:
> > > + * -SCA3300: 4 channel 16-bit data + 64-bit timestamp
> > > + * -SCL3300: 7 channel 16-bit data + 64-bit timestamp
> > > * @txbuf: Transmit buffer
> > > * @rxbuf: Receive buffer
> > > */
> > > @@ -192,10 +202,7 @@ struct sca3300_data {
> > > struct spi_device *spi;
> > > struct mutex lock;
> > > const struct sca3300_chip_info *chip;
> > > - struct {
> > > - s16 channels[4];
> > > - s64 ts __aligned(sizeof(s64));
> > > - } scan;
> > > + u8 buffer[SCA3300_MAX_BUFFER_SIZE] __aligned(sizeof(s64));
> > > u8 txbuf[4] __aligned(IIO_DMA_MINALIGN);
> > > u8 rxbuf[4];
> > > };
> > > @@ -484,21 +491,21 @@ static irqreturn_t sca3300_trigger_handler(int irq,
> > void *p)
> > > struct iio_dev *indio_dev = pf->indio_dev;
> > > struct sca3300_data *data = iio_priv(indio_dev);
> > > int bit, ret, val, i = 0;
> > > + s16 *channels = (s16 *)data->buffer;
> > >
> > > for_each_set_bit(bit, indio_dev->active_scan_mask,
> > > indio_dev->masklength) {
> > > - ret = sca3300_read_reg(data,
> > sca3300_channels[bit].address,
> > > - &val);
> > > + ret = sca3300_read_reg(data,
> > > + indio_dev->channels[bit].address, &val);
> > > if (ret) {
> > > dev_err_ratelimited(&data->spi->dev,
> > > "failed to read register, error: %d\n",
> > ret);
> > > /* handled, but bailing out due to errors */
> > > goto out;
> > > }
> > > - data->scan.channels[i++] = val;
> > > + channels[i++] = val;
> > > }
> > >
> > > - iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
> > > + iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
> > >
> > iio_get_time_ns(indio_dev));
> > > out:
> > > iio_trigger_notify_done(indio_dev->trig);
>