From: Andy Shevchenko <andy.shevchenko@gmail.com>
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>
Subject: Re: [PATCH v2 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer
Date: Sat, 17 Apr 2021 15:39:12 +0300 [thread overview]
Message-ID: <CAHp75VcibWup79np=xeQpO2z+OGCFXPhL6vWL6aWRZ+G8+djwQ@mail.gmail.com> (raw)
In-Reply-To: <20210416134546.38475-3-tomas.melin@vaisala.com>
On Fri, Apr 16, 2021 at 5:21 PM Tomas Melin <tomas.melin@vaisala.com> wrote:
>
> Add initial support for Murata SCA3300 3-axis industrial
> accelerometer with digital SPI interface. This device also
> provides a temperature measurement.
>
> Device product page including datasheet can be found at:
> https://www.murata.com/en-global/products/sensor/accel/sca3300
Can you create a tag out of it, i.e.
Datasheet: <URL>
?
> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
...
> obj-$(CONFIG_SCA3000) += sca3000.o
> +obj-$(CONFIG_SCA3300) += sca3300.o
How much difference between them?
...
> +/*
> + * Copyright (c) 2021 Vaisala Oyj. All rights reserved.
> + */
One line
> +#include <asm/unaligned.h>
Usually asm/* goes after linux/*
> +#include <linux/bitops.h>
> +#include <linux/crc8.h>
> +#include <linux/delay.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
Can you move this below...
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/spi/spi.h>
...here.
So, you will have something like this at the end:
linux/*
asm/*
linux/iio*
...
> +#define SCA3300_REG_STATUS 0x6
> +#define SCA3300_REG_MODE 0xd
> +#define SCA3300_REG_WHOAMI 0x10
> +#define SCA3300_VALUE_SW_RESET 0x20
> +#define SCA3300_CRC8_POLYNOMIAL 0x1d
> +#define SCA3300_DEVICE_ID 0x51
> +#define SCA3300_RS_ERROR 0x3
> +#define SCA3300_SELBANK 0x1f
Is it mask or value or offset?
> +#define SCA3300_STATUS_MASK 0x1ff
GENMASK()
TAB indentation to all?
This all like an unordered pile of something. I can't guess the value
for which register and so on. Can you put an order here?
...
> +#define SCA3300_ACCEL_CHANNEL(index, reg, axis) { \
> + }
Something wrong with indentation here.
...
> +static const int sca3300_accel_scale[][2] = {{0, 370}, {0, 741}, {0, 185},
> + {0, 185}};
Put it all on one line.
...
> +static const unsigned long sca3300_scan_masks[] = {
> + BIT(SCA3300_ACC_X) | BIT(SCA3300_ACC_Y) | BIT(SCA3300_ACC_Z) |
> + BIT(SCA3300_TEMP),
> + 0};
Something wrong with indentation, i.e. }; should be on the following line.
...
> +static int sca3300_transfer(struct sca3300_data *sca_data, int *val)
> +{
> + struct spi_delay delay = {.value = 10, .unit = SPI_DELAY_UNIT_USECS};
> + int32_t ret;
> + int rs;
> + u8 crc;
> + struct spi_transfer xfers[2] = {
> + {
> + .tx_buf = sca_data->txbuf,
> + .rx_buf = NULL,
> + .len = ARRAY_SIZE(sca_data->txbuf),
> + .delay = delay,
> + .cs_change = 1,
> + },
> + {
> + .tx_buf = NULL,
> + .rx_buf = sca_data->rxbuf,
> + .len = ARRAY_SIZE(sca_data->rxbuf),
> + .delay = delay,
> + .cs_change = 0,
> + }
> + };
> +
> + /* inverted crc value as described in device data sheet */
> + crc = ~crc8(sca3300_crc_table, &sca_data->txbuf[0], 3, CRC8_INIT_VALUE);
> + sca_data->txbuf[3] = crc;
> +
> + ret = spi_sync_transfer(sca_data->spi, xfers, ARRAY_SIZE(xfers));
> + if (ret < 0) {
> + dev_err(&sca_data->spi->dev,
> + "transfer error, error: %d\n", ret);
> + return -EIO;
Why shadowing error code?
> + }
> +
> + crc = ~crc8(sca3300_crc_table, &sca_data->rxbuf[0], 3, CRC8_INIT_VALUE);
> + if (sca_data->rxbuf[3] != crc) {
> + dev_err(&sca_data->spi->dev, "CRC checksum mismatch");
> + return -EIO;
> + }
> +
> + /* get return status */
> + rs = sca_data->rxbuf[0] & 0x03;
What's 0x3? Shouldn't it be defined with a descriptive name?
> + if (rs == SCA3300_RS_ERROR)
> + ret = -EINVAL;
> +
> + *val = sign_extend32(get_unaligned_be16(&sca_data->rxbuf[1]), 15);
> +
> + return ret;
> +}
...
> +static int sca3300_error_handler(struct sca3300_data *sca_data)
> +{
> + int ret;
> + int val;
> +
> + mutex_lock(&sca_data->lock);
> + sca_data->txbuf[0] = 0x0 | (SCA3300_REG_STATUS << 2);
> + ret = sca3300_transfer(sca_data, &val);
> + mutex_unlock(&sca_data->lock);
> + /* return status is cleared after reading status register */
> + if (ret != -EINVAL) {
Why this? Comment above is confusing, see also the comments below.
> + dev_err(&sca_data->spi->dev,
> + "error reading device status: %d\n", ret);
> + return ret;
> + }
> + dev_err(&sca_data->spi->dev, "device status: 0x%x\n",
> + (u16)(val & SCA3300_STATUS_MASK));
Why casting here?!
> + return 0;
> +}
...
> +static int sca3300_read_reg(struct sca3300_data *sca_data, u8 reg, int *val)
> +{
> + int ret;
> +
> + mutex_lock(&sca_data->lock);
> + sca_data->txbuf[0] = 0x0 | (reg << 2);
What is the meaning of the 0x0? Comment, please.
> + ret = sca3300_transfer(sca_data, val);
> + mutex_unlock(&sca_data->lock);
> + if (ret == -EINVAL)
This needs a good comment.
> + ret = sca3300_error_handler(sca_data);
> +
> + return ret;
if (ret != -EINVAL)
return ret;
return _error_handler(...);
> +}
> +
> +static int sca3300_write_reg(struct sca3300_data *sca_data, u8 reg, int val)
> +{
> + int reg_val = 0;
> + int ret;
> +
> + mutex_lock(&sca_data->lock);
> + sca_data->txbuf[0] = BIT(7) | (reg << 2);
> + put_unaligned_be16(val, &sca_data->txbuf[1]);
> + ret = sca3300_transfer(sca_data, ®_val);
> + mutex_unlock(&sca_data->lock);
> + if (ret == -EINVAL)
> + ret = sca3300_error_handler(sca_data);
> +
> + return ret;
> +}
As per above.
...
> + for (i = 0; i < ARRAY_SIZE(sca3300_accel_scale); i++) {
> + if (val2 == sca3300_accel_scale[i][1]) {
> + idx = i;
Redundant variable. Refactor w/o using it. It's easy.
> + break;
> + }
> + }
> + if (idx == -1)
> + return -EINVAL;
...
> + /* freq. change is possible only for mode 3 and 4 */
> + if (reg_val == 2 && val == sca3300_lp_freq[3])
> + return sca3300_write_reg(data, SCA3300_REG_MODE, 3);
> + else if (reg_val == 3 && val == sca3300_lp_freq[2])
> + return sca3300_write_reg(data, SCA3300_REG_MODE, 2);
> + else
> + return -EINVAL;
Two times redundant 'else'
...
> +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 < 0) {
> + dev_err(&data->spi->dev,
> + "failed to read register, error: %d\n", ret);
> + goto out;
> + }
> + ((s16 *)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;
> +}
> +
...
> + ret = sca3300_write_reg(sca_data, SCA3300_REG_MODE,
> + SCA3300_VALUE_SW_RESET);
> + if (ret != 0)
if (ret)
Everywhere in the code.
> + return ret;
+ blank line
> + /* wait at least 1ms after SW-reset command */
> + usleep_range(1e3, 10e3);
This doesn't make any difference. You may simply drop it and unify the comments.
> + /* wait 15ms for settling of signal paths */
> + usleep_range(15e3, 50e3);
...
> + if (value != SCA3300_DEVICE_ID) {
> + dev_err(&sca_data->spi->dev,
> + "device id not expected value, %d != %u\n",
> + value, SCA3300_DEVICE_ID);
> + return -EIO;
-ENODEV ?
> + }
...
> +static int sca3300_debugfs_reg_access(struct iio_dev *indio_dev,
> + unsigned int reg, unsigned int writeval,
> + unsigned int *readval)
> +{
> + struct sca3300_data *data = iio_priv(indio_dev);
> + int value;
> + int ret;
> +
> + if (reg > SCA3300_SELBANK)
> + return -EINVAL;
> +
> + if (!readval)
> + return sca3300_write_reg(data, reg, writeval);
> + ret = sca3300_read_reg(data, reg, &value);
> + if (ret < 0)
> + return ret;
> + *readval = (unsigned int)value;
While casting?
> + return 0;
> +}
If you switch to use regmap, the read part will be a bonus automatically.
...
> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*sca_data));
> + if (!indio_dev) {
> + dev_err(&spi->dev,
> + "failed to allocate memory for iio device\n");
Noice. You will get the same from user space.
> + return -ENOMEM;
> + }
...
> + indio_dev->dev.parent = &spi->dev;
This is done by IIO core.
...
> + ret = devm_iio_device_register(&spi->dev, indio_dev);
> + if (ret < 0) {
> + dev_err(&spi->dev, "iio device register failed, error: %d\n",
> + ret);
> + return ret;
> + }
> +
> + return 0;
return ret;
> +}
...
> +static const struct of_device_id sca3300_dt_ids[] = {
> + { .compatible = "murata,sca3300"},
> + {},
Drop comma. No need for a terminator line.
> +};
...
> +static struct spi_driver sca3300_driver = {
> + .driver = {
> + .name = SCA3300_ALIAS,
> + .owner = THIS_MODULE,
Redundant.
> + .of_match_table = of_match_ptr(sca3300_dt_ids),
Drop of_match_ptr(). In 99% its use is wrong.
> + },
> +
> + .probe = sca3300_probe,
> +};
--
With Best Regards,
Andy Shevchenko
next prev parent reply other threads:[~2021-04-17 12:39 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-16 13:45 iio: accel: sca3300: Accelerometer support and binding docs Tomas Melin
2021-04-16 13:45 ` [PATCH v2 1/2] dt-bindings: iio: accel: Add SCA3300 documentation Tomas Melin
2021-04-16 13:45 ` [PATCH v2 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer Tomas Melin
2021-04-17 12:39 ` Andy Shevchenko [this message]
2021-04-18 11:26 ` Jonathan Cameron
2021-04-19 10:29 ` Tomas Melin
2021-04-19 11:14 ` Andy Shevchenko
2021-04-19 11:36 ` Tomas Melin
2021-04-19 12:29 ` Andy Shevchenko
2021-04-19 12:48 ` Tomas Melin
2021-04-18 12:08 ` Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAHp75VcibWup79np=xeQpO2z+OGCFXPhL6vWL6aWRZ+G8+djwQ@mail.gmail.com' \
--to=andy.shevchenko@gmail.com \
--cc=devicetree@vger.kernel.org \
--cc=jic23@kernel.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=tomas.melin@vaisala.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.