| Message ID | 20220701023030.2527019-2-Qing-wu.Li@leica-geosystems.com.cn (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | iio: accel: sca3300: Extend the trigger buffer | expand |
On Fri, 1 Jul 2022 02:30:30 +0000 LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn> 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 <Qing-wu.Li@leica-geosystems.com.cn> 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 <jic23@kernel.org> > Sent: Saturday, July 2, 2022 12:46 AM > To: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn> > Cc: lars@metafoo.de; tomas.melin@vaisala.com; nuno.sa@analog.com; > linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org; > GEO-CHHER-bsp-development <bsp-development.geo@leica-geosystems.com> > 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 > jic23@kernel.org. 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 <Qing-wu.Li@leica-geosystems.com.cn> 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 <Qing-wu.Li@leica-geosystems.com.cn> > > 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 <qing-wu.li@leica-geosystems.com.cn> wrote: > > -----Original Message----- > > From: Jonathan Cameron <jic23@kernel.org> > > Sent: Saturday, July 2, 2022 12:46 AM > > To: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn> > > Cc: lars@metafoo.de; tomas.melin@vaisala.com; nuno.sa@analog.com; > > linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org; > > GEO-CHHER-bsp-development <bsp-development.geo@leica-geosystems.com> > > 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 > > jic23@kernel.org. 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 <Qing-wu.Li@leica-geosystems.com.cn> 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 <Qing-wu.Li@leica-geosystems.com.cn> > > > > 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); >
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);
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 <Qing-wu.Li@leica-geosystems.com.cn> --- drivers/iio/accel/sca3300.c | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-)