linux-kernel - Re: [PATCH v2 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <91ea3aba-854e-30f1-1236-733debfcf5dc@vaisala.com>
Date:   Mon, 19 Apr 2021 13:29:41 +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 v2 2/2] iio: accel: Add driver for Murata SCA3300
 accelerometer

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@...sala.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&amp;data=04%7C01%7Ctomas.melin%40vaisala.com%7C7b536921ec37446568bf08d9019dd944%7C6d7393e041f54c2e9b124c2be5da5c57%7C0%7C0%7C637542599730610545%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=VulPc9WnmQMJJZpYZfHmAuFPeDhclwt%2F5uPw7iYx8fU%3D&amp;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 */
>