On 4/19/21 3:29 PM, Andy Shevchenko wrote:
On Mon, Apr 19, 2021 at 2:36 PM Tomas Melin <tomas.melin@xxxxxxxxxxx> wrote:
On 4/19/21 2:14 PM, Andy Shevchenko wrote:
On Mon, Apr 19, 2021 at 1:29 PM Tomas Melin <tomas.melin@xxxxxxxxxxx> wrote:
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.
...
+ 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, ®_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).
In such cases I would recommend introducing your own error space (with
positive numbers) or playing around with the number of transfers (but
this usually works only if you anticipate several of them in a row).
Something like
#define SCA3300_ERROR_FLAGS 1
...
if (ret > 0)
return error_handler(..., ret); // ret in case if you want to
convert the code to something in Linux error code space.
In version 1 of this patchset, positive return values were used for
indicating this situation.
One of the comments I got from Jonathan to that version was to change
to use appropriate standard error code rather than positive return value.
I would thus keep this as is, hopefully You are ok with current approach.
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.
+ }
