* iio: accel: sca3300: Accelerometer support and binding docs
@ 2021-04-16 13:45 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
0 siblings, 2 replies; 11+ messages in thread
From: Tomas Melin @ 2021-04-16 13:45 UTC (permalink / raw)
To: jic23, devicetree; +Cc: linux-iio, linux-kernel
Hi,
Patch series adds driver and dt-bindings for Murata SCA3300 accelerometer.
Device is a 3-axis Accelerometer with digital SPI interface.
Patches are tested and based on linux-next.
Changes V2:
- Add manufacturer name to dt-binding file
- Update spdx license statement for dt-bindings
- Remove murata,opmode devicetree property in favor of driver scale + frequency properties
- Add copyright year for driver
- Remove X_READ and X_WRITE defines
- Add _available sysfs attributes
- Fix errors in documentation formatting
- Use ARRAY_SIZE where applicable
- Use get/put_unaligned_be16() helpers where applicable
- Factor out error handling to separate function
- Return only negative values from transfer function
- Fix INFO_SCALE to return multiplier instead of inverted value
- Change INFO_SCALE values to be actual scale instead of mode value
- Do not provide INFO_PROCESSED values for user space
- Add error message for failed data read in irq handler
- Move trigger handler timestamping as part of pushing data to buffers
- Document startup sequence, providing reference to data sheet section
- Convert iio_triggered_buffer_setup to devm_iio_triggered_buffer_setup,
making remove() obsolete
Tomas Melin (2):
dt-bindings: iio: accel: Add SCA3300 documentation
iio: accel: Add driver for Murata SCA3300 accelerometer
Documentation/devicetree/bindings/iio/accel/murata,sca3300.yaml | 44 ++++++++++++++++++
drivers/iio/accel/Kconfig | 13 ++++++
drivers/iio/accel/Makefile | 1 +
drivers/iio/accel/sca3300.c | 468 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 526 insertions(+)
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH v2 1/2] dt-bindings: iio: accel: Add SCA3300 documentation 2021-04-16 13:45 iio: accel: sca3300: Accelerometer support and binding docs Tomas Melin @ 2021-04-16 13:45 ` Tomas Melin 2021-04-16 13:45 ` [PATCH v2 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer Tomas Melin 1 sibling, 0 replies; 11+ messages in thread From: Tomas Melin @ 2021-04-16 13:45 UTC (permalink / raw) To: jic23, devicetree; +Cc: linux-iio, linux-kernel, Tomas Melin Initial DT bindings for Murata SCA3300 Accelerometer. Signed-off-by: Tomas Melin <tomas.melin@vaisala.com> --- .../bindings/iio/accel/murata,sca3300.yaml | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/accel/murata,sca3300.yaml diff --git a/Documentation/devicetree/bindings/iio/accel/murata,sca3300.yaml b/Documentation/devicetree/bindings/iio/accel/murata,sca3300.yaml new file mode 100644 index 000000000000..55fd3548e3b6 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/accel/murata,sca3300.yaml @@ -0,0 +1,44 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/accel/murata,sca3300.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Murata SCA3300 Accelerometer + +description: | + 3-axis industrial accelerometer with digital SPI interface + https://www.murata.com/en-global/products/sensor/accel/sca3300 + +maintainers: + - Tomas Melin <tomas.melin@vaisala.com> + +properties: + compatible: + enum: + - murata,sca3300 + + reg: + maxItems: 1 + + spi-max-frequency: + maximum: 8000000 + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + spi { + #address-cells = <1>; + #size-cells = <0>; + accelerometer@0 { + compatible = "murata,sca3300"; + reg = <0x0>; + spi-max-frequency = <4000000>; + }; + }; +... -- 2.21.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer 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 ` Tomas Melin 2021-04-17 12:39 ` Andy Shevchenko 2021-04-18 12:08 ` Jonathan Cameron 1 sibling, 2 replies; 11+ messages in thread From: Tomas Melin @ 2021-04-16 13:45 UTC (permalink / raw) To: jic23, devicetree; +Cc: linux-iio, linux-kernel, Tomas Melin 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 Signed-off-by: Tomas Melin <tomas.melin@vaisala.com> --- drivers/iio/accel/Kconfig | 13 + drivers/iio/accel/Makefile | 1 + drivers/iio/accel/sca3300.c | 468 ++++++++++++++++++++++++++++++++++++ 3 files changed, 482 insertions(+) create mode 100644 drivers/iio/accel/sca3300.c diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig index cceda3cecbcf..0dbf7b648e8a 100644 --- a/drivers/iio/accel/Kconfig +++ b/drivers/iio/accel/Kconfig @@ -450,6 +450,19 @@ config SCA3000 To compile this driver as a module, say M here: the module will be called sca3000. +config SCA3300 + tristate "Murata SCA3300 3-Axis Accelerometer Driver" + depends on SPI + select CRC8 + select IIO_BUFFER + select IIO_TRIGGERED_BUFFER + help + Say yes here to build support for Murata SCA3300 3-Axis + accelerometer. + + To compile this driver as a module, choose M here: the module will be + called sca3300. + config STK8312 tristate "Sensortek STK8312 3-Axis Accelerometer Driver" depends on I2C diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile index 32cd1342a31a..4b56527a2b97 100644 --- a/drivers/iio/accel/Makefile +++ b/drivers/iio/accel/Makefile @@ -50,6 +50,7 @@ obj-$(CONFIG_MXC4005) += mxc4005.o obj-$(CONFIG_MXC6255) += mxc6255.o obj-$(CONFIG_SCA3000) += sca3000.o +obj-$(CONFIG_SCA3300) += sca3300.o obj-$(CONFIG_STK8312) += stk8312.o obj-$(CONFIG_STK8BA50) += stk8ba50.o diff --git a/drivers/iio/accel/sca3300.c b/drivers/iio/accel/sca3300.c new file mode 100644 index 000000000000..79c3c60f9fab --- /dev/null +++ b/drivers/iio/accel/sca3300.c @@ -0,0 +1,468 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (c) 2021 Vaisala Oyj. All rights reserved. + */ +#include <asm/unaligned.h> +#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> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/spi/spi.h> + +#define SCA3300_ALIAS "sca3300" + +#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 +#define SCA3300_STATUS_MASK 0x1ff + +enum sca3300_scan_indexes { + SCA3300_ACC_X = 0, + SCA3300_ACC_Y, + SCA3300_ACC_Z, + SCA3300_TEMP, + SCA3300_TIMESTAMP, +}; + +#define SCA3300_ACCEL_CHANNEL(index, reg, axis) { \ + .type = IIO_ACCEL, \ + .address = reg, \ + .modified = 1, \ + .channel2 = IIO_MOD_##axis, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = \ + BIT(IIO_CHAN_INFO_SCALE) | \ + BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY), \ + .info_mask_shared_by_type_available = \ + BIT(IIO_CHAN_INFO_SCALE) | \ + BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY), \ + .scan_index = index, \ + .scan_type = { \ + .sign = 's', \ + .realbits = 16, \ + .storagebits = 16, \ + .shift = 0, \ + .endianness = IIO_CPU, \ + }, \ + } + +static const struct iio_chan_spec sca3300_channels[] = { + SCA3300_ACCEL_CHANNEL(SCA3300_ACC_X, 0x1, X), + SCA3300_ACCEL_CHANNEL(SCA3300_ACC_Y, 0x2, Y), + SCA3300_ACCEL_CHANNEL(SCA3300_ACC_Z, 0x3, Z), + { + .type = IIO_TEMP, + .address = 0x5, + .scan_index = SCA3300_TEMP, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), + .scan_type = { + .sign = 's', + .realbits = 16, + .storagebits = 16, + .shift = 0, + .endianness = IIO_CPU, + }, + }, + IIO_CHAN_SOFT_TIMESTAMP(4), +}; + +static const int sca3300_lp_freq[] = {70, 70, 70, 10}; +static const int sca3300_accel_scale[][2] = {{0, 370}, {0, 741}, {0, 185}, + {0, 185}}; + +static const unsigned long sca3300_scan_masks[] = { + BIT(SCA3300_ACC_X) | BIT(SCA3300_ACC_Y) | BIT(SCA3300_ACC_Z) | + BIT(SCA3300_TEMP), + 0}; + +/** + * struct sca3300_data - device data + * @spi: SPI device structure + * @lock: Data buffer lock + * @txbuf: Transmit buffer + * @rxbuf: Receive buffer + * @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; +}; + +DECLARE_CRC8_TABLE(sca3300_crc_table); + +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; + } + + 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; + 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) { + 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)); + 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); + ret = sca3300_transfer(sca_data, val); + mutex_unlock(&sca_data->lock); + if (ret == -EINVAL) + ret = sca3300_error_handler(sca_data); + + return ret; +} + +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; +} + +static int sca3300_write_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int val, int val2, long mask) +{ + struct sca3300_data *data = iio_priv(indio_dev); + int reg_val; + int ret; + int i; + int idx = -1; + + switch (mask) { + case IIO_CHAN_INFO_SCALE: + if (val) + return -EINVAL; + + for (i = 0; i < ARRAY_SIZE(sca3300_accel_scale); i++) { + if (val2 == sca3300_accel_scale[i][1]) { + idx = i; + break; + } + } + if (idx == -1) + return -EINVAL; + + return sca3300_write_reg(data, SCA3300_REG_MODE, idx); + case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY: + ret = sca3300_read_reg(data, SCA3300_REG_MODE, ®_val); + if (ret < 0) + return ret; + /* 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; + default: + return -EINVAL; + } +} + +static int sca3300_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int *val, int *val2, long mask) +{ + struct sca3300_data *data = iio_priv(indio_dev); + int ret; + int reg_val; + + switch (mask) { + case IIO_CHAN_INFO_RAW: + ret = sca3300_read_reg(data, chan->address, val); + if (ret < 0) + return ret; + return IIO_VAL_INT; + case IIO_CHAN_INFO_SCALE: + ret = sca3300_read_reg(data, SCA3300_REG_MODE, ®_val); + if (ret < 0) + return ret; + *val = 0; + *val2 = sca3300_accel_scale[reg_val][1]; + return IIO_VAL_INT_PLUS_MICRO; + case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY: + ret = sca3300_read_reg(data, SCA3300_REG_MODE, ®_val); + if (ret < 0) + return ret; + *val = sca3300_lp_freq[reg_val]; + return IIO_VAL_INT; + default: + return -EINVAL; + } +} + +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; +} + +/* + * sca3300_init - Device init sequence. See datasheet rev 2 section + * 4.2 Start-Up Sequence for details. + */ +static int sca3300_init(struct sca3300_data *sca_data, + struct iio_dev *indio_dev) +{ + int ret; + int value = 0; + + ret = sca3300_write_reg(sca_data, SCA3300_REG_MODE, + SCA3300_VALUE_SW_RESET); + if (ret != 0) + return ret; + /* wait at least 1ms after SW-reset command */ + usleep_range(1e3, 10e3); + + /* wait 15ms for settling of signal paths */ + usleep_range(15e3, 50e3); + ret = sca3300_read_reg(sca_data, SCA3300_REG_WHOAMI, &value); + if (ret != 0) + return ret; + + 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; + } + return 0; +} + +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; + + return 0; +} + +static int sca3300_read_avail(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + const int **vals, + int *type, + int *length, + long mask) +{ + switch (mask) { + case IIO_CHAN_INFO_SCALE: + *vals = (const int *)sca3300_accel_scale; + *length = ARRAY_SIZE(sca3300_accel_scale) * 2 - 2; + *type = IIO_VAL_INT_PLUS_MICRO; + return IIO_AVAIL_LIST; + case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY: + *vals = &sca3300_lp_freq[2]; + *length = 2; + *type = IIO_VAL_INT; + return IIO_AVAIL_LIST; + default: + return -EINVAL; + } +} + +static const struct iio_info sca3300_info = { + .read_raw = sca3300_read_raw, + .write_raw = sca3300_write_raw, + .debugfs_reg_access = &sca3300_debugfs_reg_access, + .read_avail = sca3300_read_avail, +}; + +static int sca3300_probe(struct spi_device *spi) +{ + struct sca3300_data *sca_data; + struct iio_dev *indio_dev; + int ret; + + 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"); + return -ENOMEM; + } + + sca_data = iio_priv(indio_dev); + mutex_init(&sca_data->lock); + sca_data->spi = spi; + + crc8_populate_msb(sca3300_crc_table, SCA3300_CRC8_POLYNOMIAL); + + indio_dev->dev.parent = &spi->dev; + indio_dev->info = &sca3300_info; + indio_dev->name = SCA3300_ALIAS; + indio_dev->modes = INDIO_DIRECT_MODE; + indio_dev->channels = sca3300_channels; + indio_dev->num_channels = ARRAY_SIZE(sca3300_channels); + indio_dev->available_scan_masks = sca3300_scan_masks; + + ret = sca3300_init(sca_data, indio_dev); + if (ret < 0) { + dev_err(&spi->dev, "failed to init device, error: %d\n", ret); + return ret; + } + + ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev, + iio_pollfunc_store_time, + sca3300_trigger_handler, NULL); + if (ret < 0) { + dev_err(&spi->dev, + "iio triggered buffer setup failed, error: %d\n", ret); + return ret; + } + + 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; +} + +static const struct of_device_id sca3300_dt_ids[] = { + { .compatible = "murata,sca3300"}, + {}, +}; +MODULE_DEVICE_TABLE(of, sca3300_dt_ids); + +static struct spi_driver sca3300_driver = { + .driver = { + .name = SCA3300_ALIAS, + .owner = THIS_MODULE, + .of_match_table = of_match_ptr(sca3300_dt_ids), + }, + + .probe = sca3300_probe, +}; + +module_spi_driver(sca3300_driver); + +MODULE_AUTHOR("Tomas Melin <tomas.melin@vaisala.com>"); +MODULE_DESCRIPTION("Murata SCA3300 SPI Accelerometer"); +MODULE_LICENSE("GPL v2"); -- 2.21.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer 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 2021-04-18 11:26 ` Jonathan Cameron 2021-04-19 10:29 ` Tomas Melin 2021-04-18 12:08 ` Jonathan Cameron 1 sibling, 2 replies; 11+ messages in thread From: Andy Shevchenko @ 2021-04-17 12:39 UTC (permalink / raw) To: Tomas Melin Cc: Jonathan Cameron, devicetree, linux-iio, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer 2021-04-17 12:39 ` Andy Shevchenko @ 2021-04-18 11:26 ` Jonathan Cameron 2021-04-19 10:29 ` Tomas Melin 1 sibling, 0 replies; 11+ messages in thread From: Jonathan Cameron @ 2021-04-18 11:26 UTC (permalink / raw) To: Andy Shevchenko Cc: Tomas Melin, devicetree, linux-iio, Linux Kernel Mailing List On Sat, 17 Apr 2021 15:39:12 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > 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? Enormous :) The sca3000 series were much more exciting parts with fifos and other fun a long time before anyone else had them. That part drove a lot of the weirder corners of IIO in the early days and had features that didn't show up anywhere else for quite a few years. The sca3100 onwards were much simpler (from software point of view) - I guess Murata have a different focus than VTI did. Jonathan ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer 2021-04-17 12:39 ` Andy Shevchenko 2021-04-18 11:26 ` Jonathan Cameron @ 2021-04-19 10:29 ` Tomas Melin 2021-04-19 11:14 ` Andy Shevchenko 1 sibling, 1 reply; 11+ messages in thread From: Tomas Melin @ 2021-04-19 10:29 UTC (permalink / raw) To: Andy Shevchenko Cc: Jonathan Cameron, devicetree, linux-iio, Linux Kernel Mailing List Hi Andy, Thanks for the comments. One reply below, otherwise implementing these improvements as suggested to next version. Thanks. Tomas On 4/17/21 3:39 PM, Andy Shevchenko wrote: > 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://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%7C7b536921ec37446568bf08d9019dd944%7C6d7393e041f54c2e9b124c2be5da5c57%7C0%7C0%7C637542599730610545%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=VulPc9WnmQMJJZpYZfHmAuFPeDhclwt%2F5uPw7iYx8fU%3D&reserved=0 > ... >> +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? Returning EIO here to have full control over the return value from this function. As return value of this is used for testing for possible status error (EINVAL), feels more confident to have it like this to avoid any confusion. And atleast spi_sync_transfer() return value would be visible in error message. > >> + } >> + >> + 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 */ > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer 2021-04-19 10:29 ` Tomas Melin @ 2021-04-19 11:14 ` Andy Shevchenko 2021-04-19 11:36 ` Tomas Melin 0 siblings, 1 reply; 11+ messages in thread From: Andy Shevchenko @ 2021-04-19 11:14 UTC (permalink / raw) To: Tomas Melin Cc: Jonathan Cameron, devicetree, linux-iio, Linux Kernel Mailing List On Mon, Apr 19, 2021 at 1:29 PM Tomas Melin <tomas.melin@vaisala.com> wrote: > On 4/17/21 3:39 PM, Andy Shevchenko wrote: > > 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. ... > >> + 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? > > Returning EIO here to have full control over the return value from this > function. As return value of this is used for testing Care to show what kind of testing requires this? Also why can't it be refactored to accept all error codes? > for possible status error (EINVAL), feels more confident to have it like > this to avoid any confusion. And atleast spi_sync_transfer() return value > > would be visible in error message. > >> + } -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer 2021-04-19 11:14 ` Andy Shevchenko @ 2021-04-19 11:36 ` Tomas Melin 2021-04-19 12:29 ` Andy Shevchenko 0 siblings, 1 reply; 11+ messages in thread From: Tomas Melin @ 2021-04-19 11:36 UTC (permalink / raw) To: Andy Shevchenko Cc: Jonathan Cameron, devicetree, linux-iio, Linux Kernel Mailing List Hi, On 4/19/21 2:14 PM, Andy Shevchenko wrote: > On Mon, Apr 19, 2021 at 1:29 PM Tomas Melin <tomas.melin@vaisala.com> wrote: >> On 4/17/21 3:39 PM, Andy Shevchenko wrote: >>> 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. > ... > >>>> + 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? >> Returning EIO here to have full control over the return value from this >> function. As return value of this is used for testing > Care to show what kind of testing requires this? > Also why can't it be refactored to accept all error codes? I was referring to this: +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); + ret = sca3300_transfer(sca_data, val); + mutex_unlock(&sca_data->lock); + if (ret == -EINVAL) + ret = sca3300_error_handler(sca_data); + + return ret; +} + +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; +} So this goes into error handling only when transfer indicates EINVAL (which happens when transfer otherwise is good, but device return status has error flags set i message). Thanks, Tomas > >> for possible status error (EINVAL), feels more confident to have it like >> this to avoid any confusion. And atleast spi_sync_transfer() return value >> >> would be visible in error message. >>>> + } > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer 2021-04-19 11:36 ` Tomas Melin @ 2021-04-19 12:29 ` Andy Shevchenko 2021-04-19 12:48 ` Tomas Melin 0 siblings, 1 reply; 11+ messages in thread From: Andy Shevchenko @ 2021-04-19 12:29 UTC (permalink / raw) To: Tomas Melin Cc: Jonathan Cameron, devicetree, linux-iio, Linux Kernel Mailing List On Mon, Apr 19, 2021 at 2:36 PM Tomas Melin <tomas.melin@vaisala.com> wrote: > On 4/19/21 2:14 PM, Andy Shevchenko wrote: > > On Mon, Apr 19, 2021 at 1:29 PM Tomas Melin <tomas.melin@vaisala.com> wrote: > >> On 4/17/21 3:39 PM, Andy Shevchenko wrote: > >>> 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. > > ... > > > >>>> + 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? > >> Returning EIO here to have full control over the return value from this > >> function. As return value of this is used for testing > > Care to show what kind of testing requires this? > > Also why can't it be refactored to accept all error codes? > > I was referring to this: > > +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); > + ret = sca3300_transfer(sca_data, val); > + mutex_unlock(&sca_data->lock); > + if (ret == -EINVAL) > + ret = sca3300_error_handler(sca_data); > + > + return ret; > +} > + > +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; > +} > > So this goes into error handling only when transfer indicates EINVAL > (which happens when > > transfer otherwise is good, but device return status has error flags set > i message). In such cases I would recommend introducing your own error space (with positive numbers) or playing around with the number of transfers (but this usually works only if you anticipate several of them in a row). Something like #define SCA3300_ERROR_FLAGS 1 ... if (ret > 0) return error_handler(..., ret); // ret in case if you want to convert the code to something in Linux error code space. > >> for possible status error (EINVAL), feels more confident to have it like > >> this to avoid any confusion. And atleast spi_sync_transfer() return value > >> > >> would be visible in error message. > >>>> + } -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer 2021-04-19 12:29 ` Andy Shevchenko @ 2021-04-19 12:48 ` Tomas Melin 0 siblings, 0 replies; 11+ messages in thread From: Tomas Melin @ 2021-04-19 12:48 UTC (permalink / raw) To: Andy Shevchenko Cc: Jonathan Cameron, devicetree, linux-iio, Linux Kernel Mailing List On 4/19/21 3:29 PM, Andy Shevchenko wrote: > On Mon, Apr 19, 2021 at 2:36 PM Tomas Melin <tomas.melin@vaisala.com> wrote: >> On 4/19/21 2:14 PM, Andy Shevchenko wrote: >>> On Mon, Apr 19, 2021 at 1:29 PM Tomas Melin <tomas.melin@vaisala.com> wrote: >>>> On 4/17/21 3:39 PM, Andy Shevchenko wrote: >>>>> 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. >>> ... >>> >>>>>> + 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? >>>> Returning EIO here to have full control over the return value from this >>>> function. As return value of this is used for testing >>> Care to show what kind of testing requires this? >>> Also why can't it be refactored to accept all error codes? >> I was referring to this: >> >> +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); >> + ret = sca3300_transfer(sca_data, val); >> + mutex_unlock(&sca_data->lock); >> + if (ret == -EINVAL) >> + ret = sca3300_error_handler(sca_data); >> + >> + return ret; >> +} >> + >> +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; >> +} >> >> So this goes into error handling only when transfer indicates EINVAL >> (which happens when >> >> transfer otherwise is good, but device return status has error flags set >> i message). > In such cases I would recommend introducing your own error space (with > positive numbers) or playing around with the number of transfers (but > this usually works only if you anticipate several of them in a row). > > Something like > > #define SCA3300_ERROR_FLAGS 1 > ... > > if (ret > 0) > return error_handler(..., ret); // ret in case if you want to > convert the code to something in Linux error code space. In version 1 of this patchset, positive return values were used for indicating this situation. One of the comments I got from Jonathan to that version was to change to use appropriate standard error code rather than positive return value. I would thus keep this as is, hopefully You are ok with current approach. Thanks, Tomas > >>>> for possible status error (EINVAL), feels more confident to have it like >>>> this to avoid any confusion. And atleast spi_sync_transfer() return value >>>> >>>> would be visible in error message. >>>>>> + } ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer 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 @ 2021-04-18 12:08 ` Jonathan Cameron 1 sibling, 0 replies; 11+ messages in thread From: Jonathan Cameron @ 2021-04-18 12:08 UTC (permalink / raw) To: Tomas Melin; +Cc: devicetree, linux-iio, linux-kernel On Fri, 16 Apr 2021 16:45:46 +0300 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 > > Signed-off-by: Tomas Melin <tomas.melin@vaisala.com> A few things inline. Mostly looks pretty good Jonathan > --- > drivers/iio/accel/Kconfig | 13 + > drivers/iio/accel/Makefile | 1 + > drivers/iio/accel/sca3300.c | 468 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 482 insertions(+) > create mode 100644 drivers/iio/accel/sca3300.c > > diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig > index cceda3cecbcf..0dbf7b648e8a 100644 > --- a/drivers/iio/accel/Kconfig > +++ b/drivers/iio/accel/Kconfig > @@ -450,6 +450,19 @@ config SCA3000 > To compile this driver as a module, say M here: the module will be > called sca3000. > > +config SCA3300 > + tristate "Murata SCA3300 3-Axis Accelerometer Driver" > + depends on SPI > + select CRC8 > + select IIO_BUFFER > + select IIO_TRIGGERED_BUFFER > + help > + Say yes here to build support for Murata SCA3300 3-Axis > + accelerometer. > + > + To compile this driver as a module, choose M here: the module will be > + called sca3300. > + > config STK8312 > tristate "Sensortek STK8312 3-Axis Accelerometer Driver" > depends on I2C > diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile > index 32cd1342a31a..4b56527a2b97 100644 > --- a/drivers/iio/accel/Makefile > +++ b/drivers/iio/accel/Makefile > @@ -50,6 +50,7 @@ obj-$(CONFIG_MXC4005) += mxc4005.o > obj-$(CONFIG_MXC6255) += mxc6255.o > > obj-$(CONFIG_SCA3000) += sca3000.o > +obj-$(CONFIG_SCA3300) += sca3300.o > > obj-$(CONFIG_STK8312) += stk8312.o > obj-$(CONFIG_STK8BA50) += stk8ba50.o > diff --git a/drivers/iio/accel/sca3300.c b/drivers/iio/accel/sca3300.c > new file mode 100644 > index 000000000000..79c3c60f9fab > --- /dev/null > +++ b/drivers/iio/accel/sca3300.c > @@ -0,0 +1,468 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2021 Vaisala Oyj. All rights reserved. > + */ > +#include <asm/unaligned.h> Slight preference for 'specific' includes such asm after the generic linux ones. > +#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> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/spi/spi.h> > + > +#define SCA3300_ALIAS "sca3300" > + > +#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 > +#define SCA3300_STATUS_MASK 0x1ff > + > +enum sca3300_scan_indexes { > + SCA3300_ACC_X = 0, > + SCA3300_ACC_Y, > + SCA3300_ACC_Z, > + SCA3300_TEMP, > + SCA3300_TIMESTAMP, > +}; > + > +#define SCA3300_ACCEL_CHANNEL(index, reg, axis) { \ > + .type = IIO_ACCEL, \ > + .address = reg, \ > + .modified = 1, \ > + .channel2 = IIO_MOD_##axis, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .info_mask_shared_by_type = \ > + BIT(IIO_CHAN_INFO_SCALE) | \ > + BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY), \ > + .info_mask_shared_by_type_available = \ > + BIT(IIO_CHAN_INFO_SCALE) | \ > + BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY), \ > + .scan_index = index, \ > + .scan_type = { \ > + .sign = 's', \ > + .realbits = 16, \ > + .storagebits = 16, \ > + .shift = 0, \ > + .endianness = IIO_CPU, \ > + }, \ > + } Alignment done something odd here and above. Editors get confused by macros so you might need to tweak this carefully! > + > +static const struct iio_chan_spec sca3300_channels[] = { > + SCA3300_ACCEL_CHANNEL(SCA3300_ACC_X, 0x1, X), > + SCA3300_ACCEL_CHANNEL(SCA3300_ACC_Y, 0x2, Y), > + SCA3300_ACCEL_CHANNEL(SCA3300_ACC_Z, 0x3, Z), > + { > + .type = IIO_TEMP, > + .address = 0x5, > + .scan_index = SCA3300_TEMP, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), > + .scan_type = { > + .sign = 's', > + .realbits = 16, > + .storagebits = 16, > + .shift = 0, Obviously default, so no need to specify a shift of 0. > + .endianness = IIO_CPU, > + }, > + }, > + IIO_CHAN_SOFT_TIMESTAMP(4), > +}; > + > +static const int sca3300_lp_freq[] = {70, 70, 70, 10}; > +static const int sca3300_accel_scale[][2] = {{0, 370}, {0, 741}, {0, 185}, > + {0, 185}}; > + > +static const unsigned long sca3300_scan_masks[] = { > + BIT(SCA3300_ACC_X) | BIT(SCA3300_ACC_Y) | BIT(SCA3300_ACC_Z) | > + BIT(SCA3300_TEMP), > + 0}; > + > +/** > + * struct sca3300_data - device data > + * @spi: SPI device structure > + * @lock: Data buffer lock > + * @txbuf: Transmit buffer > + * @rxbuf: Receive buffer > + * @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; > +}; > + > +DECLARE_CRC8_TABLE(sca3300_crc_table); > + > +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, Obvious default, don't bother specifying these buffers are NULL. > + .rx_buf = sca_data->rxbuf, > + .len = ARRAY_SIZE(sca_data->rxbuf), > + .delay = delay, > + .cs_change = 0, That should be obvious default, so don't bother specifying it. > + } > + }; > + > + /* 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; > + } > + > + 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; > + 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) { > + 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 the cast? Not sure it does anything useful. > + 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); > + ret = sca3300_transfer(sca_data, val); > + mutex_unlock(&sca_data->lock); > + if (ret == -EINVAL) > + ret = sca3300_error_handler(sca_data); > + > + return ret; > +} > + > +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; > +} > + > +static int sca3300_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int val, int val2, long mask) > +{ > + struct sca3300_data *data = iio_priv(indio_dev); > + int reg_val; > + int ret; > + int i; > + int idx = -1; > + > + switch (mask) { > + case IIO_CHAN_INFO_SCALE: > + if (val) > + return -EINVAL; > + > + for (i = 0; i < ARRAY_SIZE(sca3300_accel_scale); i++) { > + if (val2 == sca3300_accel_scale[i][1]) { > + idx = i; > + break; > + } > + } > + if (idx == -1) > + return -EINVAL; > + > + return sca3300_write_reg(data, SCA3300_REG_MODE, idx); > + case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY: > + ret = sca3300_read_reg(data, SCA3300_REG_MODE, ®_val); > + if (ret < 0) > + return ret; > + /* 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; > + default: > + return -EINVAL; > + } > +} > + > +static int sca3300_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, int *val2, long mask) > +{ > + struct sca3300_data *data = iio_priv(indio_dev); > + int ret; > + int reg_val; > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + ret = sca3300_read_reg(data, chan->address, val); > + if (ret < 0) > + return ret; > + return IIO_VAL_INT; > + case IIO_CHAN_INFO_SCALE: > + ret = sca3300_read_reg(data, SCA3300_REG_MODE, ®_val); > + if (ret < 0) > + return ret; > + *val = 0; > + *val2 = sca3300_accel_scale[reg_val][1]; > + return IIO_VAL_INT_PLUS_MICRO; > + case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY: > + ret = sca3300_read_reg(data, SCA3300_REG_MODE, ®_val); > + if (ret < 0) > + return ret; > + *val = sca3300_lp_freq[reg_val]; > + return IIO_VAL_INT; > + default: > + return -EINVAL; > + } > +} > + > +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; Why do you need the cast? > + } > + > + 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; > +} > + > +/* > + * sca3300_init - Device init sequence. See datasheet rev 2 section > + * 4.2 Start-Up Sequence for details. > + */ > +static int sca3300_init(struct sca3300_data *sca_data, > + struct iio_dev *indio_dev) > +{ > + int ret; > + int value = 0; > + > + ret = sca3300_write_reg(sca_data, SCA3300_REG_MODE, > + SCA3300_VALUE_SW_RESET); > + if (ret != 0) > + return ret; > + /* wait at least 1ms after SW-reset command */ > + usleep_range(1e3, 10e3); > + > + /* wait 15ms for settling of signal paths */ > + usleep_range(15e3, 50e3); > + ret = sca3300_read_reg(sca_data, SCA3300_REG_WHOAMI, &value); > + if (ret != 0) For error checks we tend to just use if (ret) Same in a few other places. > + return ret; > + > + 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; > + } > + return 0; > +} > + > +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; > + > + return 0; > +} > + > +static int sca3300_read_avail(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + const int **vals, You could wrap this a little less without hurting readability I think. > + int *type, > + int *length, > + long mask) > +{ > + switch (mask) { > + case IIO_CHAN_INFO_SCALE: > + *vals = (const int *)sca3300_accel_scale; > + *length = ARRAY_SIZE(sca3300_accel_scale) * 2 - 2; > + *type = IIO_VAL_INT_PLUS_MICRO; > + return IIO_AVAIL_LIST; > + case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY: > + *vals = &sca3300_lp_freq[2]; > + *length = 2; > + *type = IIO_VAL_INT; > + return IIO_AVAIL_LIST; > + default: > + return -EINVAL; > + } > +} > + > +static const struct iio_info sca3300_info = { > + .read_raw = sca3300_read_raw, > + .write_raw = sca3300_write_raw, > + .debugfs_reg_access = &sca3300_debugfs_reg_access, > + .read_avail = sca3300_read_avail, > +}; > + > +static int sca3300_probe(struct spi_device *spi) > +{ > + struct sca3300_data *sca_data; > + struct iio_dev *indio_dev; > + int ret; > + > + 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"); > + return -ENOMEM; > + } > + > + sca_data = iio_priv(indio_dev); > + mutex_init(&sca_data->lock); > + sca_data->spi = spi; > + > + crc8_populate_msb(sca3300_crc_table, SCA3300_CRC8_POLYNOMIAL); > + > + indio_dev->dev.parent = &spi->dev; This should now be set by the IIO core IIRC. > + indio_dev->info = &sca3300_info; > + indio_dev->name = SCA3300_ALIAS; > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->channels = sca3300_channels; > + indio_dev->num_channels = ARRAY_SIZE(sca3300_channels); > + indio_dev->available_scan_masks = sca3300_scan_masks; > + > + ret = sca3300_init(sca_data, indio_dev); > + if (ret < 0) { > + dev_err(&spi->dev, "failed to init device, error: %d\n", ret); > + return ret; > + } > + > + ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev, > + iio_pollfunc_store_time, > + sca3300_trigger_handler, NULL); > + if (ret < 0) { > + dev_err(&spi->dev, > + "iio triggered buffer setup failed, error: %d\n", ret); > + return ret; > + } > + > + 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; > +} > + > +static const struct of_device_id sca3300_dt_ids[] = { > + { .compatible = "murata,sca3300"}, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, sca3300_dt_ids); > + > +static struct spi_driver sca3300_driver = { > + .driver = { > + .name = SCA3300_ALIAS, > + .owner = THIS_MODULE, > + .of_match_table = of_match_ptr(sca3300_dt_ids), > + }, > + > + .probe = sca3300_probe, > +}; > + > +module_spi_driver(sca3300_driver); > + > +MODULE_AUTHOR("Tomas Melin <tomas.melin@vaisala.com>"); > +MODULE_DESCRIPTION("Murata SCA3300 SPI Accelerometer"); > +MODULE_LICENSE("GPL v2"); ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-04-19 12:49 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 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
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.