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

[Tomas Melin <tomas.melin@xxxxxxxxxxx>]

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