Re: [PATCH v3 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer
From: Tomas Melin
Date: Tue Apr 20 2021 - 04:50:45 EST
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 <tomas.melin@xxxxxxxxxxx> 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%2Fsca3300&data=04%7C01%7Ctomas.melin%40vaisala.com%7C5259ef3cd4b645f3a7d208d9033acdc5%7C6d7393e041f54c2e9b124c2be5da5c57%7C0%7C0%7C637544373362508656%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=BZue5RQjrHWtRzEOGZAw1Avb35QKLYu0ZOnXbyj9EE8%3D&reserved=0
No blank line in the tag block.
Signed-off-by: Tomas Melin <tomas.melin@xxxxxxxxxxx>
...
+/*
+ * 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_STATUS GENMASK(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_SELBANK 0x1f
+#define SCA3300_REG_STATUS 0x6
+#define SCA3300_REG_WHOAMI 0x10
+
+#define SCA3300_VALUE_DEVICE_ID 0x51
+#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(&sca_data->spi->dev,
+ "error reading device status: %d\n", ret);
+ return ret;
+ }
+
+ dev_err(&sca_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,
+ &val);
+ if (ret) {
+ dev_err(&data->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. Or did You have some
other option in mind?
How about something like:
/* handled with errors */
goto out;
+ }
+ data->scan.channels[i++] = val;
+ }
+
+ iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
+ iio_get_time_ns(indio_dev));
+out:
+ iio_trigger_notify_done(indio_dev->trig);
+
+ return IRQ_HANDLED;
+}