Re: [PATCH v5 2/3] iio: accel: sca3000: replace usages of internal read data helpers by spi helpers

[Andrew Ijano <andrew.ijano@gmail.com>]

On Thu, Jun 12, 2025 at 10:22 AM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> ...
>
> > +     ret |= (mode & SCA3000_REG_MODE_MODE_MASK);
>
> Unneeded parentheses.
>
...
>
> > +                     ret = spi_w8r16be(st->us,
> > +                                             SCA3000_READ_REG(SCA3000_REG_TEMP_MSB_ADDR));
>
> Make it simply one line. The above formatting is ugly.

That's right! I'll fix them.

>
> >       struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> >       struct sca3000_state *st = iio_priv(indio_dev);
> > -     int len = 0, ret, val;
> > +     int len = 0, ret;
>
> Ideally it's better to split them and len should never be signed.

Nice! I can make this change.

> Moreover, the function  should be switched to sysfs_emit_at() if this is part
> of ABI.

Great! I didn't know that.

In this case, sca3000_read_av_freq() is described as a "sysfs function
to get available frequencies", so I guess it's the case, right?
Is your suggestion to replace cases of sprintf() by sysfs_emit_at()
then? If so, I could do that in a following patch, it seems that
sca3000_show_available_3db_freqs() is also using sprintf().

>
> >       mutex_lock(&st->lock);
> > -     ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
> > -     val = st->rx[0];
> > +     ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
> >       mutex_unlock(&st->lock);
> >       if (ret)
> >               return ret;
>
> ...
>
> >               }, {
> >                       .len = len,
> > -                     .rx_buf = rx,
> > +                     .rx_buf = st->rx,
> >               }
> >       };
>
> > -
>
> Stray change. Doesn't checkpatch complain on this?

I don't recall getting any warning from checkpatch but I can check
again for this next version.

> > -                     (st->rx[0] | SCA3000_REG_MODE_RING_BUF_ENABLE));
> > +                     (ret | SCA3000_REG_MODE_RING_BUF_ENABLE));
>
> > -                     (st->rx[0] & ~SCA3000_REG_MODE_RING_BUF_ENABLE));
> > +                     (ret & ~SCA3000_REG_MODE_RING_BUF_ENABLE));
>
> In the original code and still now too many parentheses.

Ok! I'll remove them.

Thanks,
Andrew