Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210419132159.4450-1-tomas.melin@vaisala.com> <20210419132159.4450-3-tomas.melin@vaisala.com>
In-Reply-To: <20210419132159.4450-3-tomas.melin@vaisala.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Mon, 19 Apr 2021 16:55:16 +0300
Message-ID: <CAHp75VdApCk_Ydt2W_WWJ_wme4d1ocrrnvo+TjZcQ62RG6uOUA@mail.gmail.com>
Subject: Re: [PATCH v3 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer
To:     Tomas Melin <tomas.melin@vaisala.com>
Cc:     Jonathan Cameron <jic23@kernel.org>,
        devicetree <devicetree@vger.kernel.org>,
        linux-iio <linux-iio@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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).

> Datasheet: https://www.murata.com/en-global/products/sensor/accel/sca3300

>

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.

...

> +#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?

> +#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?

> + * @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(&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?

> +               }
> +               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;
> +}

...

> +       /*
> +        * 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, &value);
> +       if (ret)
> +               return ret;

> +       ret = devm_iio_device_register(&spi->dev, indio_dev);
> +       if (ret) {
> +               dev_err(&spi->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