Re: [PATCH v3 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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&amp;data=04%7C01%7Ctomas.melin%40vaisala.com%7C5259ef3cd4b645f3a7d208d9033acdc5%7C6d7393e041f54c2e9b124c2be5da5c57%7C0%7C0%7C637544373362508656%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=BZue5RQjrHWtRzEOGZAw1Avb35QKLYu0ZOnXbyj9EE8%3D&amp;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;
+}




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux