On Tue, 2024-02-20 at 17:34 +0200, Dumitru Ceclan wrote:
> This adds support for LTC6373 36 V Fully-Differential Programmable-Gain
> Instrumentation Amplifier with 25 pA Input Bias Current.
> The user can program the gain to one of seven available settings through
> a 3-bit parallel interface (A2 to A0).
>
> Signed-off-by: Dumitru Ceclan <[email protected]>
> ---
Just one minor comment. With that:
Reviewed-by: Nuno Sa <[email protected]>
> drivers/iio/amplifiers/hmc425a.c | 124 ++++++++++++++++++++++++++++++-
> 1 file changed, 120 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iio/amplifiers/hmc425a.c b/drivers/iio/amplifiers/hmc425a.c
> index 77872e2dfdfe..50c86c2d28d7 100644
> --- a/drivers/iio/amplifiers/hmc425a.c
> +++ b/drivers/iio/amplifiers/hmc425a.c
> @@ -2,9 +2,10 @@
> /*
> * HMC425A and similar Gain Amplifiers
> *
> - * Copyright 2020 Analog Devices Inc.
> + * Copyright 2020, 2024 Analog Devices Inc.
> */
..
>
>
> +static ssize_t ltc6373_read_powerdown(struct iio_dev *indio_dev,
> + uintptr_t private,
> + const struct iio_chan_spec *chan,
> + char *buf)
> +{
> + struct hmc425a_state *st = iio_priv(indio_dev);
> +
> + return sysfs_emit(buf, "%d\n", st->powerdown);
Well, in theory the read should also be protected with the lock...
- Nuno Sá
On Wed, 21 Feb 2024 14:23:51 +0100
Nuno Sá <[email protected]> wrote:
> On Tue, 2024-02-20 at 17:34 +0200, Dumitru Ceclan wrote:
> > This adds support for LTC6373 36 V Fully-Differential Programmable-Gain
> > Instrumentation Amplifier with 25 pA Input Bias Current.
> > The user can program the gain to one of seven available settings through
> > a 3-bit parallel interface (A2 to A0).
> >
> > Signed-off-by: Dumitru Ceclan <[email protected]>
> > ---
>
> Just one minor comment. With that:
>
> Reviewed-by: Nuno Sa <[email protected]>
>
> > drivers/iio/amplifiers/hmc425a.c | 124 ++++++++++++++++++++++++++++++-
> > 1 file changed, 120 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/iio/amplifiers/hmc425a.c b/drivers/iio/amplifiers/hmc425a.c
> > index 77872e2dfdfe..50c86c2d28d7 100644
> > --- a/drivers/iio/amplifiers/hmc425a.c
> > +++ b/drivers/iio/amplifiers/hmc425a.c
> > @@ -2,9 +2,10 @@
> > /*
> > * HMC425A and similar Gain Amplifiers
> > *
> > - * Copyright 2020 Analog Devices Inc.
> > + * Copyright 2020, 2024 Analog Devices Inc.
> > */
>
> ...
>
> >
> >
> > +static ssize_t ltc6373_read_powerdown(struct iio_dev *indio_dev,
> > + uintptr_t private,
> > + const struct iio_chan_spec *chan,
> > + char *buf)
> > +{
> > + struct hmc425a_state *st = iio_priv(indio_dev);
> > +
> > + return sysfs_emit(buf, "%d\n", st->powerdown);
>
> Well, in theory the read should also be protected with the lock...
Only reason I can think of for that is potential read tearing.
If that happens on a bool we are going to be in a mess so I think this
is in practice fine without, though paranoia might suggest locking.
It can race against it being powered down but that effectively happens
even if we do have a lock as we either see the value before or after
a racing power down event and we have no way of knowing which.
Applied rest series to iio.git togreg branch and pushed out as testing.
Duitru, if you can figure out what happened to your message thread before
sending more patches that would be great. The IDs for patches 0-5 go
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
Which really confuses my email client and patchwork.
https://patchwork.kernel.org/project/linux-iio/list/?series=827901
>
> - Nuno Sá
>
On Sat, 2024-02-24 at 17:54 +0000, Jonathan Cameron wrote:
> On Wed, 21 Feb 2024 14:23:51 +0100
> Nuno Sá <[email protected]> wrote:
>
> > On Tue, 2024-02-20 at 17:34 +0200, Dumitru Ceclan wrote:
> > > This adds support for LTC6373 36 V Fully-Differential Programmable-Gain
> > > Instrumentation Amplifier with 25 pA Input Bias Current.
> > > The user can program the gain to one of seven available settings through
> > > a 3-bit parallel interface (A2 to A0).
> > >
> > > Signed-off-by: Dumitru Ceclan <[email protected]>
> > > ---
> >
> > Just one minor comment. With that:
> >
> > Reviewed-by: Nuno Sa <[email protected]>
> >
> > > drivers/iio/amplifiers/hmc425a.c | 124 ++++++++++++++++++++++++++++++-
> > > 1 file changed, 120 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/iio/amplifiers/hmc425a.c
> > > b/drivers/iio/amplifiers/hmc425a.c
> > > index 77872e2dfdfe..50c86c2d28d7 100644
> > > --- a/drivers/iio/amplifiers/hmc425a.c
> > > +++ b/drivers/iio/amplifiers/hmc425a.c
> > > @@ -2,9 +2,10 @@
> > > /*
> > > * HMC425A and similar Gain Amplifiers
> > > *
> > > - * Copyright 2020 Analog Devices Inc.
> > > + * Copyright 2020, 2024 Analog Devices Inc.
> > > */
> >
> > ...
> >
> > >
> > >
> > > +static ssize_t ltc6373_read_powerdown(struct iio_dev *indio_dev,
> > > + uintptr_t private,
> > > + const struct iio_chan_spec *chan,
> > > + char *buf)
> > > +{
> > > + struct hmc425a_state *st = iio_priv(indio_dev);
> > > +
> > > + return sysfs_emit(buf, "%d\n", st->powerdown);
> >
> > Well, in theory the read should also be protected with the lock...
>
> Only reason I can think of for that is potential read tearing.
> If that happens on a bool we are going to be in a mess so I think this
> is in practice fine without, though paranoia might suggest locking.
Yeah, also mentioned it for correctness. I mean, in theory, read_once() should be
more that enough in here but I often find that too much for using in "simple" drivers
where a lock is surely easier to understand for someone reading the code.
Now, about your bool comment, I used to think like that until I saw the LF rcu
mentorship video from Paul. I'm fairly sure he comes up with some "crazy" possibility
about the CPU/compiler screwing you even on a char (IIRC, he was also arguing about
not using read_once() on a bool).
Now, practically speaking, I tend to agree that for the archs we care this will
likely never be an issue but yeah, not 100% correct kernel code IMO.
- Nuno Sá