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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANZih_RTtcHHP80rtJ5gGkmkL1ohoctUBaGm-2Z2=Xo9VvT-Aw@mail.gmail.com>
Date: Sat, 14 Jun 2025 18:06:26 -0300
From: Andrew Ijano <andrew.ijano@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...el.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 v5 2/3] iio: accel: sca3000: replace usages of internal
 read data helpers by spi helpers

On Thu, Jun 12, 2025 at 10:22 AM Andy Shevchenko
<andriy.shevchenko@...el.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