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). > 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@xxxxxxxxxxx> ... > +/* > + * 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
