Re: [PATCH v3] iio: accel: sca3000: replace usages of internal read data helpers by spi helpers

[Andy Shevchenko]

On Fri, May 9, 2025 at 4:39 AM Andrew Ijano <andrew.ijano@xxxxxxxxx> 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