RE: [PATCH 4/7] iio: adis16475: re-set max spi transfer

From: "Sa, Nuno" <Nuno.Sa@analog.com>
To: Alexandru Ardelean <ardeleanalex@gmail.com>
Cc: linux-iio <linux-iio@vger.kernel.org>,
	Jonathan Cameron <jic23@kernel.org>,
	"Hennerich, Michael" <Michael.Hennerich@analog.com>,
	Lars-Peter Clausen <lars@metafoo.de>
Subject: RE: [PATCH 4/7] iio: adis16475: re-set max spi transfer
Date: Thu, 15 Apr 2021 07:53:53 +0000	[thread overview]
Message-ID: <CY4PR03MB3112C342E901F4E308D0AFF0994D9@CY4PR03MB3112.namprd03.prod.outlook.com> (raw)
In-Reply-To: <CA+U=DsqeiRRGp+Q+yZ7OVOE719aBiyMoBLsFTZL3amGfWHtgkg@mail.gmail.com>

> -----Original Message-----
> From: Alexandru Ardelean <ardeleanalex@gmail.com>
> Sent: Wednesday, April 14, 2021 9:29 AM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> <jic23@kernel.org>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Lars-Peter Clausen
> <lars@metafoo.de>
> Subject: Re: [PATCH 4/7] iio: adis16475: re-set max spi transfer
> 
> [External]
> 
> On Tue, Apr 13, 2021 at 5:45 PM Nuno Sa <nuno.sa@analog.com>
> wrote:
> >
> > In case 'spi_sync()' fails, we would be left with a max spi transfer
> > which is not the one the user expects it to be. Hence, we need to re-
> set
> > it also in this error path.
> >
> > Fixes: fff7352bf7a3c ("iio: imu: Add support for adis16475")
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > ---
> >  drivers/iio/imu/adis16475.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/imu/adis16475.c b/drivers/iio/imu/adis16475.c
> > index 51b76444db0b..9dca7e506200 100644
> > --- a/drivers/iio/imu/adis16475.c
> > +++ b/drivers/iio/imu/adis16475.c
> > @@ -1067,8 +1067,10 @@ static irqreturn_t
> adis16475_trigger_handler(int irq, void *p)
> >         adis->spi->max_speed_hz = ADIS16475_BURST_MAX_SPEED;
> >
> >         ret = spi_sync(adis->spi, &adis->msg);
> 
> Purely stylistic here.
> But, the restore from the cached variable could be done here in a
> single line.
> So. just moving [1] here.

You mean also doing it in the label? I thought about that and the reason
why I didn't is that on a normal run, I want to reset the max freq as soon
as possible so that if someone concurrently tries to read 'direct mode' attrs
gets the max freq. This was my reasoning but I admit that it's not that
important so I will leave this to Jonathan's preference...

Hmm now that I spoke about the concurrently access to IIO attr and being paranoid about
the compiler, I wonder if we should not use
WRITE_ONCE(adis->spi->max_speed_hz, ADIS16475_BURST_MAX_SPEED)...