| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <bea4dc56-b860-431c-a820-a482ce87743c@vaisala.com>
Date: Tue, 20 Apr 2021 11:50:37 +0300
From: Tomas Melin <tomas.melin@...sala.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: Jonathan Cameron <jic23@...nel.org>,
devicetree <devicetree@...r.kernel.org>,
linux-iio <linux-iio@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 2/2] iio: accel: Add driver for Murata SCA3300
accelerometer
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@...sala.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&data=04%7C01%7Ctomas.melin%40vaisala.com%7C5259ef3cd4b645f3a7d208d9033acdc5%7C6d7393e041f54c2e9b124c2be5da5c57%7C0%7C0%7C637544373362508656%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=BZue5RQjrHWtRzEOGZAw1Avb35QKLYu0ZOnXbyj9EE8%3D&reserved=0
> No blank line in the tag block.
>
>> Signed-off-by: Tomas Melin <tomas.melin@...sala.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;
>> +}
>
Powered by blists - more mailing lists