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

[Tomas Melin <tomas.melin@vaisala.com>]

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@vaisala.com> 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@vaisala.com>
>
> ...
>
>> +/*
>> + * 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;
>> +}
>