Re: [PATCH v3 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer
On Tue, Apr 20, 2021 at 2:36 PM Tomas Melin wrote:
> On 4/20/21 1:47 PM, Andy Shevchenko wrote:
> > On Tue, Apr 20, 2021 at 11:50 AM Tomas Melin
> > wrote:
...
> + for_each_set_bit(bit, indio_dev->active_scan_mask,
> +indio_dev->masklength) {
> + ret = sca3300_read_reg(data,
> sca3300_channels[bit].address,
> + );
> + if (ret) {
> + dev_err(>spi->dev,
> + "failed to read register, error: %d\n",
> ret);
> + goto out;
> >>> Does it mean interrupt is handled in this case?
> >>> Perhaps a comment why it's okay to consider so?
> >> IRQ_HANDLED seemed more correct than IRQ_NONE.
> > Why? Care to explain?
>
> Thinking that IRQ was for the device and it was indeed handled. There
> were errors when handling
>
> it, but it was handled as much as possible.
>
> >
> >> Or did You have some
> >> other option in mind?
> >>
> >> How about something like:
> >>
> >> /* handled with errors */
> > But what if this is the very first interrupt (bit in the loop) that
> > failed? What about the rest?
>
> Aah, right. Other option could be to simply continue loop and set 'val'
> to e.g. 0 for
>
> readings with errors. But perhaps it is after all better to bail out,
> and only for cases
>
> when _all_ data is reliable, it is pushed to buffers(?)
>
> Comes to mind that perhaps better to have error message in this irq
> handler as
>
> dev_err_ratelimited(), to avoid possible flooding.
>
>
> So to conclude, proposing:
>
> *change to dev_err_ratelimited()
>
> * comment goto:
>
> /* handled, but bailing out this round due to errors */
>
> Would this be OK?
Sounds like a plan!
> >> goto out;
> >>
> + }
> + data->scan.channels[i++] = val;
> + }
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v3 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer
On 4/20/21 1:47 PM, Andy Shevchenko wrote:
On Tue, Apr 20, 2021 at 11:50 AM Tomas Melin wrote:
+ sca_data->txbuf[0] = 0x0 | (SCA3300_REG_STATUS << 2);
Seems you ignored my comment. What is this 0x0? What is the meaning of it?
Same for all the rest magic numbers in the code.
Sorry, not ignored but will remove this redundant 0x0 for next round.
Maybe it's not redundant after all (I noticed other magic numbers in
the same position)? Please, comment your intention case-by-case.
0x0 is for read operation, but since it's just or'd, end result should
be the same. It was there in v1 for clarity (also #defined). Basically
thinking perhaps it's cleaner to just leave it out.
Other magics should be cleaned up now.
...
+ for_each_set_bit(bit, indio_dev->active_scan_mask,
+indio_dev->masklength) {
+ ret = sca3300_read_reg(data, sca3300_channels[bit].address,
+ );
+ if (ret) {
+ dev_err(>spi->dev,
+ "failed to read register, error: %d\n", ret);
+ goto out;
Does it mean interrupt is handled in this case?
Perhaps a comment why it's okay to consider so?
IRQ_HANDLED seemed more correct than IRQ_NONE.
Why? Care to explain?
Thinking that IRQ was for the device and it was indeed handled. There
were errors when handling
it, but it was handled as much as possible.
Or did You have some
other option in mind?
How about something like:
/* handled with errors */
But what if this is the very first interrupt (bit in the loop) that
failed? What about the rest?
Aah, right. Other option could be to simply continue loop and set 'val'
to e.g. 0 for
readings with errors. But perhaps it is after all better to bail out,
and only for cases
when _all_ data is reliable, it is pushed to buffers(?)
Comes to mind that perhaps better to have error message in this irq
handler as
dev_err_ratelimited(), to avoid possible flooding.
So to conclude, proposing:
*change to dev_err_ratelimited()
* comment goto:
/* handled, but bailing out this round due to errors */
Would this be OK?
Thanks,
Tomas
goto out;
+ }
+ data->scan.channels[i++] = val;
+ }
Re: [PATCH v3 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer
On Tue, Apr 20, 2021 at 11:50 AM Tomas Melin wrote:
> On 4/19/21 4:55 PM, Andy Shevchenko wrote:
> > On Mon, Apr 19, 2021 at 4:26 PM Tomas Melin wrote:
...
> >> +#define SCA3300_MASK_STATUSGENMASK(8, 0)
> >> +#define SCA3300_MASK_RS_STATUS GENMASK(1, 0)
> > This feels like an orphan. Shouldn't you move it closer to the group
> > of corresponding register / etc definition?
>
> Tried to group these in alphabetical order, but IIUC preference would be
> towards grouping
Yes, alphabetical is about header block, and definition should be
understandable and HW represented.
> according to how they are used? Would this be clearer and acceptable?
1) with some amendments, see below.
> 1)
>
> /* Device mode register */
> #define SCA3300_REG_MODE0xd
> #define SCA3300_VALUE_SW_RESET0x20
SCA3300_MODE_SW_RESET
> /* Last register in map */
> #define SCA3300_REG_SELBANK0x1f
>
> /* Device status and related mask */
> #define SCA3300_REG_STATUS0x6
> #define SCA3300_MASK_STATUSGENMASK(8, 0)
SCA3300_STATUS_MASK
and so on (I guess you got the pattern)
> /* Device ID */
> #define SCA3300_REG_WHOAMI0x10
> #define SCA3300_VALUE_DEVICE_ID0x51
>
> /* Device return status and mask */
> #define SCA3300_VALUE_RS_ERROR0x3
> #define SCA3300_MASK_RS_STATUSGENMASK(1, 0)
...
> >> + * @txbuf: Transmit buffer
> >> + * @rxbuf: Receive buffer
> > Are the buffers subject to DMA? Shouldn't they have the proper alignment?
> Good point, I will add alignment.
Move them to the end of the structure to save few bytes,
...
> >> + sca_data->txbuf[0] = 0x0 | (SCA3300_REG_STATUS << 2);
> > Seems you ignored my comment. What is this 0x0? What is the meaning of it?
> > Same for all the rest magic numbers in the code.
>
> Sorry, not ignored but will remove this redundant 0x0 for next round.
Maybe it's not redundant after all (I noticed other magic numbers in
the same position)? Please, comment your intention case-by-case.
...
> >> + for_each_set_bit(bit, indio_dev->active_scan_mask,
> >> +indio_dev->masklength) {
> >> + ret = sca3300_read_reg(data, sca3300_channels[bit].address,
> >> + );
> >> + if (ret) {
> >> + dev_err(>spi->dev,
> >> + "failed to read register, error: %d\n",
> >> ret);
> >> + goto out;
> > Does it mean interrupt is handled in this case?
> > Perhaps a comment why it's okay to consider so?
>
> IRQ_HANDLED seemed more correct than IRQ_NONE.
Why? Care to explain?
> Or did You have some
> other option in mind?
>
> How about something like:
>
> /* handled with errors */
But what if this is the very first interrupt (bit in the loop) that
failed? What about the rest?
> goto out;
>
> >> + }
> >> + data->scan.channels[i++] = val;
> >> + }
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v3 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer
Hi Andy,
Thanks for further comments, see some answers/questions below.
thanks,
Tomas
On 4/19/21 4:55 PM, Andy Shevchenko wrote:
On Mon, Apr 19, 2021 at 4:26 PM Tomas Melin wrote:
Thanks for an update, it's getting better! My comments below.
Add initial support for Murata SCA3300 3-axis industrial
accelerometer with digital SPI interface. This device also
provides a temperature measurement.
First of all, you forgot Cc reviewer(s).
Ok, thanks for pointing this out, will add you as cc next round.
Datasheet:
https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.murata.com%2Fen-global%2Fproducts%2Fsensor%2Faccel%2Fsca3300data=04%7C01%7Ctomas.melin%40vaisala.com%7C5259ef3cd4b645f3a7d208d9033acdc5%7C6d7393e041f54c2e9b124c2be5da5c57%7C0%7C0%7C637544373362508656%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=BZue5RQjrHWtRzEOGZAw1Avb35QKLYu0ZOnXbyj9EE8%3Dreserved=0
No blank line in the tag block.
Signed-off-by: Tomas Melin
...
+/*
+ * Copyright (c) 2021 Vaisala Oyj. All rights reserved.
+ */
One line.
Opted for adding Description line to this header, thus planning to keep
as multi line.
...
+#define SCA3300_MASK_STATUSGENMASK(8, 0)
+#define SCA3300_MASK_RS_STATUS GENMASK(1, 0)
This feels like an orphan. Shouldn't you move it closer to the group
of corresponding register / etc definition?
Tried to group these in alphabetical order, but IIUC preference would be
towards grouping
according to how they are used? Would this be clearer and acceptable?
1)
/* Device mode register */
#define SCA3300_REG_MODE 0xd
#define SCA3300_VALUE_SW_RESET 0x20
/* Last register in map */
#define SCA3300_REG_SELBANK 0x1f
/* Device status and related mask */
#define SCA3300_REG_STATUS 0x6
#define SCA3300_MASK_STATUS GENMASK(8, 0)
/* Device ID */
#define SCA3300_REG_WHOAMI 0x10
#define SCA3300_VALUE_DEVICE_ID 0x51
/* Device return status and mask */
#define SCA3300_VALUE_RS_ERROR 0x3
#define SCA3300_MASK_RS_STATUS GENMASK(1, 0)
or then only adjusting current sorting with comments, like:
2)
/* Register mask values */
#define SCA3300_MASK_STATUS GENMASK(8, 0)
#define SCA3300_MASK_RS_STATUS GENMASK(1, 0)
/* Register index values */
#define SCA3300_REG_MODE 0xd
#define SCA3300_REG_SELBANK 0x1f
#define SCA3300_REG_STATUS 0x6
#define SCA3300_REG_WHOAMI 0x10
/* Register read/write values */
#define SCA3300_VALUE_DEVICE_ID 0x51
#define SCA3300_VALUE_RS_ERROR 0x3
#define SCA3300_VALUE_SW_RESET 0x20
+#define SCA3300_REG_MODE 0xd
+#define SCA3300_REG_SELBANK0x1f
+#define SCA3300_REG_STATUS 0x6
+#define SCA3300_REG_WHOAMI 0x10
+
+#define SCA3300_VALUE_DEVICE_ID0x51
+#define SCA3300_VALUE_RS_ERROR 0x3
+#define SCA3300_VALUE_SW_RESET 0x20
As above it doesn't shed a light for the relationship between
registers and these fields (?). I.o.w the names w/o properly grouped
(and probably commented) are confusing.
...
+/**
+ * struct sca3300_data - device data
+ * @spi: SPI device structure
+ * @lock: Data buffer lock
+ * @txbuf: Transmit buffer
+ * @rxbuf: Receive buffer
Are the buffers subject to DMA? Shouldn't they have the proper alignment?
Good point, I will add alignment.
+ * @scan: Triggered buffer. Four channel 16-bit data + 64-bit timestamp
+ */
+struct sca3300_data {
+ struct spi_device *spi;
+ struct mutex lock;
+ u8 txbuf[4];
+ u8 rxbuf[4];
+ struct {
+ s16 channels[4];
+ s64 ts __aligned(sizeof(s64));
+ } scan;
+};
...
+ struct spi_delay delay = {.value = 10, .unit = SPI_DELAY_UNIT_USECS};
Missed space.
...
+ sca_data->txbuf[0] = 0x0 | (SCA3300_REG_STATUS << 2);
Seems you ignored my comment. What is this 0x0? What is the meaning of it?
Same for all the rest magic numbers in the code.
Sorry, not ignored but will remove this redundant 0x0 for next round.
+ /*
+* return status error is cleared after reading status register once,
+* expect EINVAL here
/*
* Fix the style of all your multi-line comments.
* You may follow this example.
*/
Ok, will captialize sentence and add punctuation.
+*/
+ if (ret != -EINVAL) {
+ dev_err(_data->spi->dev,
+ "error reading device status: %d\n", ret);
+ return ret;
+ }
+
+ dev_err(_data->spi->dev, "device status: 0x%lx\n",
+ (val & SCA3300_MASK_STATUS));
Too many parentheses.
+ return 0;
+}
...
+static irqreturn_t sca3300_trigger_handler(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ struct sca3300_data *data = iio_priv(indio_dev);
+ int bit, ret, val, i = 0;
+
+ for_each_set_bit(bit, indio_dev->active_scan_mask,
+indio_dev->masklength) {
+ ret =
Re: [PATCH v3 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer
On Mon, Apr 19, 2021 at 4:26 PM Tomas Melin wrote:
Thanks for an update, it's getting better! My comments below.
> Add initial support for Murata SCA3300 3-axis industrial
> accelerometer with digital SPI interface. This device also
> provides a temperature measurement.
First of all, you forgot Cc reviewer(s).
> Datasheet: https://www.murata.com/en-global/products/sensor/accel/sca3300
>
No blank line in the tag block.
> Signed-off-by: Tomas Melin
...
> +/*
> + * Copyright (c) 2021 Vaisala Oyj. All rights reserved.
> + */
One line.
...
> +#define SCA3300_MASK_STATUSGENMASK(8, 0)
> +#define SCA3300_MASK_RS_STATUS GENMASK(1, 0)
This feels like an orphan. Shouldn't you move it closer to the group
of corresponding register / etc definition?
> +#define SCA3300_REG_MODE 0xd
> +#define SCA3300_REG_SELBANK0x1f
> +#define SCA3300_REG_STATUS 0x6
> +#define SCA3300_REG_WHOAMI 0x10
> +
> +#define SCA3300_VALUE_DEVICE_ID0x51
> +#define SCA3300_VALUE_RS_ERROR 0x3
> +#define SCA3300_VALUE_SW_RESET 0x20
As above it doesn't shed a light for the relationship between
registers and these fields (?). I.o.w the names w/o properly grouped
(and probably commented) are confusing.
...
> +/**
> + * struct sca3300_data - device data
> + * @spi: SPI device structure
> + * @lock: Data buffer lock
> + * @txbuf: Transmit buffer
> + * @rxbuf: Receive buffer
Are the buffers subject to DMA? Shouldn't they have the proper alignment?
> + * @scan: Triggered buffer. Four channel 16-bit data + 64-bit timestamp
> + */
> +struct sca3300_data {
> + struct spi_device *spi;
> + struct mutex lock;
> + u8 txbuf[4];
> + u8 rxbuf[4];
> + struct {
> + s16 channels[4];
> + s64 ts __aligned(sizeof(s64));
> + } scan;
> +};
...
> + struct spi_delay delay = {.value = 10, .unit = SPI_DELAY_UNIT_USECS};
Missed space.
...
> + sca_data->txbuf[0] = 0x0 | (SCA3300_REG_STATUS << 2);
Seems you ignored my comment. What is this 0x0? What is the meaning of it?
Same for all the rest magic numbers in the code.
> + /*
> +* return status error is cleared after reading status register once,
> +* expect EINVAL here
/*
* Fix the style of all your multi-line comments.
* You may follow this example.
*/
> +*/
> + if (ret != -EINVAL) {
> + dev_err(_data->spi->dev,
> + "error reading device status: %d\n", ret);
> + return ret;
> + }
> +
> + dev_err(_data->spi->dev, "device status: 0x%lx\n",
> + (val & SCA3300_MASK_STATUS));
Too many parentheses.
> + return 0;
> +}
...
> +static irqreturn_t sca3300_trigger_handler(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct sca3300_data *data = iio_priv(indio_dev);
> + int bit, ret, val, i = 0;
> +
> + for_each_set_bit(bit, indio_dev->active_scan_mask,
> +indio_dev->masklength) {
> + ret = sca3300_read_reg(data, sca3300_channels[bit].address,
> + );
> + if (ret) {
> + dev_err(>spi->dev,
> + "failed to read register, error: %d\n", ret);
> + goto out;
Does it mean interrupt is handled in this case?
Perhaps a comment why it's okay to consider so?
> + }
> + data->scan.channels[i++] = val;
> + }
> +
> + iio_push_to_buffers_with_timestamp(indio_dev, >scan,
> + iio_get_time_ns(indio_dev));
> +out:
> + iio_trigger_notify_done(indio_dev->trig);
> +
> + return IRQ_HANDLED;
> +}
...
> + /*
> +* wait 1ms after SW-reset command
> +* wait 15ms for settling of signal paths
> +*/
> + usleep_range(16e3, 50e3);
+ blank line
> + ret = sca3300_read_reg(sca_data, SCA3300_REG_WHOAMI, );
> + if (ret)
> + return ret;
> + ret = devm_iio_device_register(>dev, indio_dev);
> + if (ret) {
> + dev_err(>dev, "iio device register failed, error: %d\n",
> + ret);
> + return ret;
> + }
> +
> + return ret;
Deduplicate it.
Simply leave the latter one.
> +}
...
> +
No need for this blank line.
> + .probe = sca3300_probe,
> +};
> +
Ditto.
> +module_spi_driver(sca3300_driver);
--
With Best Regards,
Andy Shevchenko
