[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAHp75Ve7-hMUxrmXQnkMjynhPUbD6R+K=-j+h0zELvcxZdy5nw@mail.gmail.com>
Date: Fri, 9 May 2025 12:06:15 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Andrew Ijano <andrew.ijano@...il.com>
Cc: jic23@...nel.org, andrew.lopes@...mni.usp.br, gustavobastos@....br,
dlechner@...libre.com, nuno.sa@...log.com, andy@...nel.org,
jstephan@...libre.com, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] iio: accel: sca3000: replace usages of internal read
data helpers by spi helpers
On Fri, May 9, 2025 at 4:39 AM Andrew Ijano <andrew.ijano@...il.com> wrote:
>
> Remove usages of sca3000_read_data() and sca3000_read_data_short()
> functions, replacing it by spi_w8r8() and spi_w8r16() helpers. Just
> one case that reads large buffers is left using an internal helper.
>
> This is an old driver that was not making full use of the newer
> infrastructure.
Suggested-by: ? (IIRC Jonathan suggested this, but ignore if I am mistaken)
...
> ret = sca3000_write_reg(st, SCA3000_REG_CTRL_SEL_ADDR, ctrl_reg);
> if (ret)
> goto error_ret;
> - ret = sca3000_read_data_short(st, SCA3000_REG_CTRL_DATA_ADDR, 1);
> +
> + ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_CTRL_DATA_ADDR));
> if (ret)
> goto error_ret;
> - return st->rx[0];
> + return ret;
> error_ret:
> return ret;
Doesn't feel like a good cleanup. Please, drop this error handling
completely, just return instead of goto above.
...
> - *val = sign_extend32(be16_to_cpup((__be16 *)st->rx) >>
> + *val = sign_extend32(be16_to_cpu((__be16) ret) >>
This doesn't look good, can you define a proper __be16 variable on
stack and use it instead of ret?
> chan->scan_type.shift,
With the above done, move this parameter to the previous line.
> chan->scan_type.realbits - 1);
> } else {
...
> - *val = (be16_to_cpup((__be16 *)st->rx) >>
> + *val = (be16_to_cpu((__be16) ret) >>
> chan->scan_type.shift) &
> GENMASK(chan->scan_type.realbits - 1, 0);
Ditto.
...
> /* if off and should be on */
> - if (state && !(st->rx[0] & SCA3000_REG_MODE_FREE_FALL_DETECT))
> + if (state && !(ret & SCA3000_REG_MODE_FREE_FALL_DETECT))
> return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
> - st->rx[0] | SCA3000_REG_MODE_FREE_FALL_DETECT);
> + ret | SCA3000_REG_MODE_FREE_FALL_DETECT);
> /* if on and should be off */
> - else if (!state && (st->rx[0] & SCA3000_REG_MODE_FREE_FALL_DETECT))
> + else if (!state && (ret & SCA3000_REG_MODE_FREE_FALL_DETECT))
Remove redundant 'else'
> return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
> - st->rx[0] & ~SCA3000_REG_MODE_FREE_FALL_DETECT);
> + ret & ~SCA3000_REG_MODE_FREE_FALL_DETECT);
> else
Ditto.
> return 0;
...
> ret = sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
> - (st->rx[0] & SCA3000_MODE_PROT_MASK));
> + (ret & SCA3000_MODE_PROT_MASK));
Remove unneeded parentheses.
...
> - ret = sca3000_read_data_short(st, SCA3000_REG_INT_MASK_ADDR, 1);
> + ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
> +
Stray blank line.
> if (ret)
> goto error_ret;
Perhaps you wanted it to be here?
> ret = sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
> - (st->rx[0] &
> + (ret &
> ~(SCA3000_REG_INT_MASK_RING_THREE_QUARTER |
> SCA3000_REG_INT_MASK_RING_HALF |
> SCA3000_REG_INT_MASK_ALL_INTS)));
Remove unneeded parentheses (outer for the last parameter).
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists