linux-kernel - Fwd: [PATCH v4] 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-next>] [day] [month] [year] [list]

Message-ID: <CANZih_SmneYr+jK9Apuif66vfCcpni7K+CzMk32-hZt-LRrZWw@mail.gmail.com>
Date: Sun, 11 May 2025 15:36:31 -0300
From: Andrew Ijano <andrew.ijano@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: andrew.lopes@...mni.usp.br, gustavobastos@....br, 
	David Lechner <dlechner@...libre.com>, nuno.sa@...log.com, andy@...nel.org, 
	jstephan@...libre.com, linux-iio <linux-iio@...r.kernel.org>, 
	linux-kernel@...r.kernel.org
Subject: Fwd: [PATCH v4] iio: accel: sca3000: replace usages of internal read
 data helpers by spi helpers

I noticed now that I replied again only to Jonathan. I'm forwarding
this to all stakeholders.

On Sun, May 11, 2025 at 8:34 AM Jonathan Cameron <jic23@...nel.org> wrote:
>
>
> A few things inline but clearly main thing is to reply to Andy's other
> points on v3.
>
Thanks! I replied to other Andy's points.

...

> > -static int sca3000_read_data_short(struct sca3000_state *st,
> > -                                u8 reg_address_high,
> > -                                int len)
> > +static int sca3000_read_data(struct sca3000_state *st,
> > +                          u8 reg_address_high,
> > +                          int len)
>
> I'd keep this where the original sca3000_read_data() is
> That will give a shorter, more obvious diff and puts the code near where it
> is called helping review.
Good idea! I'll do that then.

...

> >  error_ret:
> >       return ret;
> This shows the age of the code.  Just return in error paths above rather
> than a error_ret: label
>
> Might be a good idea to do a precursor patch tidying up any cases of this
> before the one doin gthe spi changes in ehre.

Ok! It makes sense. I'm really doing a lot of different changes in
this patch already.

>
> >  }
> > @@ -432,13 +434,13 @@ static int sca3000_print_rev(struct iio_dev *indio_dev)
> >       struct sca3000_state *st = iio_priv(indio_dev);
> >
> >       mutex_lock(&st->lock);
>
> Another patch to use guard(mutex)(&st->lock); etc would be help clean this
> up by allowing direct return in the error path.

Great! In this case, would you suggest the following order?
1. One patch for general style changes, like the removal of error_ret labels;
2. Another patch for spi changes;
3. And another one for using guard(mutex)(&st->lock).

> > -                     ret = sca3000_read_data_short(st, address, 2);
> > +                     ret = spi_w8r16(st->us, SCA3000_READ_REG(address));
>
> spi_w8r16be() then no need for the endian conversion below.
...
> > -                     ret = sca3000_read_data_short(st,
> > -                                                   SCA3000_REG_TEMP_MSB_ADDR,
> > -                                                   2);
> > +                     ret = spi_w8r16(st->us,
> > +                                             SCA3000_READ_REG(SCA3000_REG_TEMP_MSB_ADDR));
>
> As above. spi_w8r16be()

Great! I didn't know this one. I'll change that then.

> >               mutex_unlock(&st->lock);
>
> >
>
> As above. Put the new implementation of this here so we can easily see what
> changed.

Ok! Thanks for pointing it out.

--
Thanks,
Andrew