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

[Tomas Melin <tomas.melin@vaisala.com>]

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, &reg_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.
>>>> +       }
>
>