mbox series

[v6,00/10] iio: adc: ad7124: Various fixes

Message ID cover.1733504533.git.u.kleine-koenig@baylibre.com
Headers show
Series iio: adc: ad7124: Various fixes | expand

Message

Uwe Kleine-König Dec. 6, 2024, 5:28 p.m. UTC
Hello,

here comes v6 of this series. Compared to v5
(https://lore.kernel.org/linux-iio/20241203110019.1520071-12-u.kleine-koenig@baylibre.com)
the following things changed:

 - Rebased to v6.13-rc1 + 64612ec9b909. (No changes needed here.)

 - Add Ack from Conor to patch #3

 - Fixed how R̅D̅Y̅ is written. This was wrong before because the overline
   char must be added after the character that should get the overline,
   not before. I got that wrong because of
   https://bugs.debian.org/1089108. I would expect that now this is
   properly shown in most browsers, MUAs and editors.

   I guess Andy still doesn't agree to write it that way. Jonathan,
   would you decide here please? If you agree with Andy I suggest to
   replace it by #RDY. Andy suggested #RDY_N which I think is too far
   away from the original name. If you (also) like R̅D̅Y̅, just keep it as
   is.

 - Fix error handling in patch #8
   I just pasted "return ret" to all callers of
   ad_sigma_delta_clear_pending_event() before. Now the error handling
   matches the actual needs of the context.

 - s/irq controller's capabilities/irq controller capabilities/
   as suggested by Andy for patch #8

Best regards
Uwe

Uwe Kleine-König (10):
  iio: adc: ad7124: Don't create more channels than the driver can handle
  iio: adc: ad7124: Refuse invalid input specifiers
  dt-bindings: iio: adc: adi,ad7{124,173,192,780}: Allow specifications of a gpio for irq line
  iio: adc: ad_sigma_delta: Add support for reading irq status using a GPIO
  iio: adc: ad_sigma_delta: Handle CS assertion as intended in ad_sd_read_reg_raw()
  iio: adc: ad_sigma_delta: Fix a race condition
  iio: adc: ad_sigma_delta: Store information about reset sequence length
  iio: adc: ad_sigma_delta: Check for previous ready signals
  iio: adc: ad7124: Add error reporting during probe
  iio: adc: ad7124: Implement temperature measurement

 .../bindings/iio/adc/adi,ad7124.yaml          |  13 ++
 .../bindings/iio/adc/adi,ad7173.yaml          |  12 +
 .../bindings/iio/adc/adi,ad7192.yaml          |  15 ++
 .../bindings/iio/adc/adi,ad7780.yaml          |  11 +
 drivers/iio/adc/ad7124.c                      | 217 +++++++++++++-----
 drivers/iio/adc/ad7173.c                      |   1 +
 drivers/iio/adc/ad7192.c                      |   4 +-
 drivers/iio/adc/ad7791.c                      |   1 +
 drivers/iio/adc/ad7793.c                      |   3 +-
 drivers/iio/adc/ad_sigma_delta.c              | 194 +++++++++++++---
 include/linux/iio/adc/ad_sigma_delta.h        |   8 +-
 11 files changed, 390 insertions(+), 89 deletions(-)

base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37
prerequisite-patch-id: 617af17fc377a984762c61893b9f2a92ae62213a

Comments

Andy Shevchenko Dec. 6, 2024, 11:12 p.m. UTC | #1
On Fri, Dec 6, 2024 at 7:29 PM Uwe Kleine-König
<u.kleine-koenig@baylibre.com> wrote:
>
> Hello,
>
> here comes v6 of this series. Compared to v5
> (https://lore.kernel.org/linux-iio/20241203110019.1520071-12-u.kleine-koenig@baylibre.com)
> the following things changed:
>
>  - Rebased to v6.13-rc1 + 64612ec9b909. (No changes needed here.)
>
>  - Add Ack from Conor to patch #3
>
>  - Fixed how R̅D̅Y̅ is written. This was wrong before because the overline
>    char must be added after the character that should get the overline,
>    not before. I got that wrong because of
>    https://bugs.debian.org/1089108. I would expect that now this is
>    properly shown in most browsers, MUAs and editors.
>
>    I guess Andy still doesn't agree to write it that way.

Yeah, I prefer a solid overline which Unicode simply incapable of, but
HTML does.
In any case the representation in this version is much better than in
the previous version.
I leave this up to Jonathan, but as I said an electrical engineer in
me is not satisfied.

> Jonathan,
>    would you decide here please? If you agree with Andy I suggest to
>    replace it by #RDY. Andy suggested #RDY_N which I think is too far
>    away from the original name. If you (also) like R̅D̅Y̅, just keep it as
>    is.
>
>  - Fix error handling in patch #8
>    I just pasted "return ret" to all callers of
>    ad_sigma_delta_clear_pending_event() before. Now the error handling
>    matches the actual needs of the context.
>
>  - s/irq controller's capabilities/irq controller capabilities/
>    as suggested by Andy for patch #8
Jonathan Cameron Dec. 8, 2024, 12:42 p.m. UTC | #2
On Fri,  6 Dec 2024 18:28:38 +0100
Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

> The ad_sigma_delta driver helper uses irq_disable_nosync(). With that
> one it is possible that the irq handler still runs after the
> irq_disable_nosync() function call returns. Also to properly synchronize
> irq disabling in the different threads proper locking is needed and
> because it's unclear if the irq handler's irq_disable_nosync() call
> comes first or the one in the enabler's error path, all code locations
> that disable the irq must check for .irq_dis first to ensure there is
> exactly one disable call per enable call.
> 
> So add a spinlock to the struct ad_sigma_delta and use it to synchronize
> irq enabling and disabling. Also only act in the irq handler if the irq
> is still enabled.
> 
> Fixes: af3008485ea0 ("iio:adc: Add common code for ADI Sigma Delta devices")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>

From sparse.

drivers/iio/adc/ad_sigma_delta.c:205:13: warning: context imbalance in 'ad_sd_disable_irq' - wrong count at exit
drivers/iio/adc/ad_sigma_delta.c:218:13: warning: context imbalance in 'ad_sd_enable_irq' - wrong count at exit

I saw your discussion with Linus on this...

https://lore.kernel.org/all/CAHk-=wiVDZejo_1BhOaR33qb=pny7sWnYtP4JUbRTXkXCkW6jA@mail.gmail.com/

So I guess we just treat that as a false positive and move on.

> ---
>  drivers/iio/adc/ad_sigma_delta.c       | 56 ++++++++++++++++----------
>  include/linux/iio/adc/ad_sigma_delta.h |  1 +
>  2 files changed, 36 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
> index ff20fa61c293..a4c31baa9c9e 100644
> --- a/drivers/iio/adc/ad_sigma_delta.c
> +++ b/drivers/iio/adc/ad_sigma_delta.c
> @@ -202,6 +202,27 @@ int ad_sd_reset(struct ad_sigma_delta *sigma_delta,
>  }
>  EXPORT_SYMBOL_NS_GPL(ad_sd_reset, IIO_AD_SIGMA_DELTA);
>  
> +static bool ad_sd_disable_irq(struct ad_sigma_delta *sigma_delta)
> +{
> +	guard(spinlock_irqsave)(&sigma_delta->irq_lock);
> +
> +	/* It's already off, return false to indicate nothing was changed */
> +	if (sigma_delta->irq_dis)
> +		return false;
> +
> +	sigma_delta->irq_dis = true;
> +	disable_irq_nosync(sigma_delta->irq_line);
> +	return true;
> +}
> +
> +static void ad_sd_enable_irq(struct ad_sigma_delta *sigma_delta)
> +{
> +	guard(spinlock_irqsave)(&sigma_delta->irq_lock);
> +
> +	sigma_delta->irq_dis = false;
> +	enable_irq(sigma_delta->irq_line);
> +}
> +
>  int ad_sd_calibrate(struct ad_sigma_delta *sigma_delta,
>  	unsigned int mode, unsigned int channel)
>  {
> @@ -221,12 +242,10 @@ int ad_sd_calibrate(struct ad_sigma_delta *sigma_delta,
>  	if (ret < 0)
>  		goto out;
>  
> -	sigma_delta->irq_dis = false;
> -	enable_irq(sigma_delta->irq_line);
> +	ad_sd_enable_irq(sigma_delta);
>  	time_left = wait_for_completion_timeout(&sigma_delta->completion, 2 * HZ);
>  	if (time_left == 0) {
> -		sigma_delta->irq_dis = true;
> -		disable_irq_nosync(sigma_delta->irq_line);
> +		ad_sd_disable_irq(sigma_delta);
>  		ret = -EIO;
>  	} else {
>  		ret = 0;
> @@ -294,8 +313,7 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev,
>  
>  	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_SINGLE);
>  
> -	sigma_delta->irq_dis = false;
> -	enable_irq(sigma_delta->irq_line);
> +	ad_sd_enable_irq(sigma_delta);
>  	ret = wait_for_completion_interruptible_timeout(
>  			&sigma_delta->completion, HZ);
>  
> @@ -314,10 +332,7 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev,
>  		&raw_sample);
>  
>  out:
> -	if (!sigma_delta->irq_dis) {
> -		disable_irq_nosync(sigma_delta->irq_line);
> -		sigma_delta->irq_dis = true;
> -	}
> +	ad_sd_disable_irq(sigma_delta);
>  
>  	sigma_delta->keep_cs_asserted = false;
>  	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE);
> @@ -396,8 +411,7 @@ static int ad_sd_buffer_postenable(struct iio_dev *indio_dev)
>  	if (ret)
>  		goto err_unlock;
>  
> -	sigma_delta->irq_dis = false;
> -	enable_irq(sigma_delta->irq_line);
> +	ad_sd_enable_irq(sigma_delta);
>  
>  	return 0;
>  
> @@ -414,10 +428,7 @@ static int ad_sd_buffer_postdisable(struct iio_dev *indio_dev)
>  	reinit_completion(&sigma_delta->completion);
>  	wait_for_completion_timeout(&sigma_delta->completion, HZ);
>  
> -	if (!sigma_delta->irq_dis) {
> -		disable_irq_nosync(sigma_delta->irq_line);
> -		sigma_delta->irq_dis = true;
> -	}
> +	ad_sd_disable_irq(sigma_delta);
>  
>  	sigma_delta->keep_cs_asserted = false;
>  	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE);
> @@ -516,8 +527,7 @@ static irqreturn_t ad_sd_trigger_handler(int irq, void *p)
>  
>  irq_handled:
>  	iio_trigger_notify_done(indio_dev->trig);
> -	sigma_delta->irq_dis = false;
> -	enable_irq(sigma_delta->irq_line);
> +	ad_sd_enable_irq(sigma_delta);
>  
>  	return IRQ_HANDLED;
>  }
> @@ -551,11 +561,13 @@ static irqreturn_t ad_sd_data_rdy_trig_poll(int irq, void *private)
>  	 * So read the MOSI line as GPIO (if available) and only trigger the irq
>  	 * if the line is active. Without such a GPIO assume this is a valid
>  	 * interrupt.
> +	 *
> +	 * Also as disable_irq_nosync() is used to disable the irq, only act if
> +	 * the irq wasn't disabled before.
>  	 */
> -	if (!sigma_delta->rdy_gpiod || gpiod_get_value(sigma_delta->rdy_gpiod)) {
> +	if ((!sigma_delta->rdy_gpiod || gpiod_get_value(sigma_delta->rdy_gpiod)) &&
> +	    ad_sd_disable_irq(sigma_delta)) {
>  		complete(&sigma_delta->completion);
> -		disable_irq_nosync(irq);
> -		sigma_delta->irq_dis = true;
>  		iio_trigger_poll(sigma_delta->trig);
>  
>  		return IRQ_HANDLED;
> @@ -691,6 +703,8 @@ int ad_sd_init(struct ad_sigma_delta *sigma_delta, struct iio_dev *indio_dev,
>  		}
>  	}
>  
> +	spin_lock_init(&sigma_delta->irq_lock);
> +
>  	if (info->irq_line)
>  		sigma_delta->irq_line = info->irq_line;
>  	else
> diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
> index 126b187d70e9..f86eca6126b4 100644
> --- a/include/linux/iio/adc/ad_sigma_delta.h
> +++ b/include/linux/iio/adc/ad_sigma_delta.h
> @@ -86,6 +86,7 @@ struct ad_sigma_delta {
>  
>  /* private: */
>  	struct completion	completion;
> +	spinlock_t		irq_lock; /* protects .irq_dis and irq en/disable state */
>  	bool			irq_dis;
>  
>  	bool			bus_locked;
Jonathan Cameron Dec. 8, 2024, 12:44 p.m. UTC | #3
On Fri,  6 Dec 2024 18:28:32 +0100
Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

> Hello,
> 
> here comes v6 of this series. Compared to v5
> (https://lore.kernel.org/linux-iio/20241203110019.1520071-12-u.kleine-koenig@baylibre.com)
> the following things changed:
> 
>  - Rebased to v6.13-rc1 + 64612ec9b909. (No changes needed here.)
> 
>  - Add Ack from Conor to patch #3
> 
>  - Fixed how R̅D̅Y̅ is written. This was wrong before because the overline
>    char must be added after the character that should get the overline,
>    not before. I got that wrong because of
>    https://bugs.debian.org/1089108. I would expect that now this is
>    properly shown in most browsers, MUAs and editors.
> 
>    I guess Andy still doesn't agree to write it that way. Jonathan,
>    would you decide here please? If you agree with Andy I suggest to
>    replace it by #RDY. Andy suggested #RDY_N which I think is too far
>    away from the original name. If you (also) like R̅D̅Y̅, just keep it as
>    is.
> 
>  - Fix error handling in patch #8
>    I just pasted "return ret" to all callers of
>    ad_sigma_delta_clear_pending_event() before. Now the error handling
>    matches the actual needs of the context.
> 
>  - s/irq controller's capabilities/irq controller capabilities/
>    as suggested by Andy for patch #8
> 
> Best regards
> Uwe

Hi Uwe

Given the mix of fixes and other material (kind of fixes, but also kind
of new functionality), I've queued this for the next merge window in my
togreg branch.  If you think there are particular patches that need to
go sooner then I can handle them in a split fashion, but that does add
risk that the whole lot might no land depending on timings (particularly
given it's coming into vacation season).

Initially pushed out as testing - I assume we'll see that sparse warning.

Thanks,

Jonathan

> 
> Uwe Kleine-König (10):
>   iio: adc: ad7124: Don't create more channels than the driver can handle
>   iio: adc: ad7124: Refuse invalid input specifiers
>   dt-bindings: iio: adc: adi,ad7{124,173,192,780}: Allow specifications of a gpio for irq line
>   iio: adc: ad_sigma_delta: Add support for reading irq status using a GPIO
>   iio: adc: ad_sigma_delta: Handle CS assertion as intended in ad_sd_read_reg_raw()
>   iio: adc: ad_sigma_delta: Fix a race condition
>   iio: adc: ad_sigma_delta: Store information about reset sequence length
>   iio: adc: ad_sigma_delta: Check for previous ready signals
>   iio: adc: ad7124: Add error reporting during probe
>   iio: adc: ad7124: Implement temperature measurement
> 
>  .../bindings/iio/adc/adi,ad7124.yaml          |  13 ++
>  .../bindings/iio/adc/adi,ad7173.yaml          |  12 +
>  .../bindings/iio/adc/adi,ad7192.yaml          |  15 ++
>  .../bindings/iio/adc/adi,ad7780.yaml          |  11 +
>  drivers/iio/adc/ad7124.c                      | 217 +++++++++++++-----
>  drivers/iio/adc/ad7173.c                      |   1 +
>  drivers/iio/adc/ad7192.c                      |   4 +-
>  drivers/iio/adc/ad7791.c                      |   1 +
>  drivers/iio/adc/ad7793.c                      |   3 +-
>  drivers/iio/adc/ad_sigma_delta.c              | 194 +++++++++++++---
>  include/linux/iio/adc/ad_sigma_delta.h        |   8 +-
>  11 files changed, 390 insertions(+), 89 deletions(-)
> 
> base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37
> prerequisite-patch-id: 617af17fc377a984762c61893b9f2a92ae62213a
Jonathan Cameron Dec. 8, 2024, 12:46 p.m. UTC | #4
On Sat, 7 Dec 2024 01:12:56 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Dec 6, 2024 at 7:29 PM Uwe Kleine-König
> <u.kleine-koenig@baylibre.com> wrote:
> >
> > Hello,
> >
> > here comes v6 of this series. Compared to v5
> > (https://lore.kernel.org/linux-iio/20241203110019.1520071-12-u.kleine-koenig@baylibre.com)
> > the following things changed:
> >
> >  - Rebased to v6.13-rc1 + 64612ec9b909. (No changes needed here.)
> >
> >  - Add Ack from Conor to patch #3
> >
> >  - Fixed how R̅D̅Y̅ is written. This was wrong before because the overline
> >    char must be added after the character that should get the overline,
> >    not before. I got that wrong because of
> >    https://bugs.debian.org/1089108. I would expect that now this is
> >    properly shown in most browsers, MUAs and editors.
> >
> >    I guess Andy still doesn't agree to write it that way.  
> 
> Yeah, I prefer a solid overline which Unicode simply incapable of, but
> HTML does.
> In any case the representation in this version is much better than in
> the previous version.
> I leave this up to Jonathan, but as I said an electrical engineer in
> me is not satisfied.

Fully agree it's unsatisfying, but I think it is clear enough what
is intended to go ahead as is.

Jonathan

> 
> > Jonathan,
> >    would you decide here please? If you agree with Andy I suggest to
> >    replace it by #RDY. Andy suggested #RDY_N which I think is too far
> >    away from the original name. If you (also) like R̅D̅Y̅, just keep it as
> >    is.
> >
> >  - Fix error handling in patch #8
> >    I just pasted "return ret" to all callers of
> >    ad_sigma_delta_clear_pending_event() before. Now the error handling
> >    matches the actual needs of the context.
> >
> >  - s/irq controller's capabilities/irq controller capabilities/
> >    as suggested by Andy for patch #8  
>
Andy Shevchenko Dec. 8, 2024, 1:05 p.m. UTC | #5
On Sun, Dec 8, 2024 at 2:42 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Fri,  6 Dec 2024 18:28:38 +0100
> Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

...

> From sparse.
>
> drivers/iio/adc/ad_sigma_delta.c:205:13: warning: context imbalance in 'ad_sd_disable_irq' - wrong count at exit
> drivers/iio/adc/ad_sigma_delta.c:218:13: warning: context imbalance in 'ad_sd_enable_irq' - wrong count at exit
>
> I saw your discussion with Linus on this...
>
> https://lore.kernel.org/all/CAHk-=wiVDZejo_1BhOaR33qb=pny7sWnYtP4JUbRTXkXCkW6jA@mail.gmail.com/
>
> So I guess we just treat that as a false positive and move on.

I'm wondering if sparse annotation __acquire and __release may help here...
Jonathan Cameron Dec. 8, 2024, 6:23 p.m. UTC | #6
On Sun, 8 Dec 2024 15:05:38 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sun, Dec 8, 2024 at 2:42 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > On Fri,  6 Dec 2024 18:28:38 +0100
> > Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:  
> 
> ...
> 
> > From sparse.
> >
> > drivers/iio/adc/ad_sigma_delta.c:205:13: warning: context imbalance in 'ad_sd_disable_irq' - wrong count at exit
> > drivers/iio/adc/ad_sigma_delta.c:218:13: warning: context imbalance in 'ad_sd_enable_irq' - wrong count at exit
> >
> > I saw your discussion with Linus on this...
> >
> > https://lore.kernel.org/all/CAHk-=wiVDZejo_1BhOaR33qb=pny7sWnYtP4JUbRTXkXCkW6jA@mail.gmail.com/
> >
> > So I guess we just treat that as a false positive and move on.  
> 
> I'm wondering if sparse annotation __acquire and __release may help here...

The complaint is (I think) about guard(spinlock_irqsave)
so I'm not immediately sure how.
Andy Shevchenko Dec. 9, 2024, 12:52 a.m. UTC | #7
On Sun, Dec 8, 2024 at 8:23 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Sun, 8 Dec 2024 15:05:38 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Sun, Dec 8, 2024 at 2:42 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > > On Fri,  6 Dec 2024 18:28:38 +0100

...

> > > From sparse.
> > >
> > > drivers/iio/adc/ad_sigma_delta.c:205:13: warning: context imbalance in 'ad_sd_disable_irq' - wrong count at exit
> > > drivers/iio/adc/ad_sigma_delta.c:218:13: warning: context imbalance in 'ad_sd_enable_irq' - wrong count at exit
> > >
> > > I saw your discussion with Linus on this...
> > >
> > > https://lore.kernel.org/all/CAHk-=wiVDZejo_1BhOaR33qb=pny7sWnYtP4JUbRTXkXCkW6jA@mail.gmail.com/
> > >
> > > So I guess we just treat that as a false positive and move on.
> >
> > I'm wondering if sparse annotation __acquire and __release may help here...
>
> The complaint is (I think) about guard(spinlock_irqsave)
> so I'm not immediately sure how.

In cases like

 if (x)
   lock();
 ...do smth...
if (x)
  unlock()

Those annotations give sparse hints that locking is balanced. That is
why I was thinking it may help guard as well.
Uwe Kleine-König Dec. 9, 2024, 9:28 a.m. UTC | #8
Hello Jonathan,

On Fri, Dec 06, 2024 at 06:28:42PM +0100, Uwe Kleine-König wrote:
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> index 9405cb579324..d858bffd2628 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> [...]
> @@ -902,6 +941,37 @@ static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
>  		chan[channel].channel2 = ain[1];
>  	}
>  
> +	if (num_channels < AD7124_MAX_CHANNELS) {
> +		st->channels[num_channels] = (struct ad7124_channel) {
> +			.nr = num_channels,
> +			.ain = AD7124_CHANNEL_AINP(AD7124_INPUT_TEMPSENSOR) |
> +				AD7124_CHANNEL_AINM(AD7124_INPUT_AVSS),
> +			.cfg = {
> +				.bipolar = true,
> +			},
> +		};
> +
> +		chan[num_channels] = (struct iio_chan_spec) {
> +			.type = IIO_TEMP,
> +			.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET) |
> +				BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +			.scan_type = {
> +				/*
> +				 * You might find it strange that a bipolar
> +				 * measurement yields an unsigned value, but
> +				 * this matches the device's manual.
> +				 */
> +				.sign = 'u',
> +				.realbits = 24,
> +				.storagebits = 32,
> +				.endianness = IIO_BE,
> +			},
> +			.address = num_channels,
> +			.scan_index = num_channels,
> +		};
> +	};

The kernel build bot wailed about the ; here. Should I send a proper
patch, or can you still just rewrite your tree to drop it?

Best regards and thanks and sorry,
Uwe
Uwe Kleine-König Dec. 9, 2024, 9:47 a.m. UTC | #9
Hello Jonathan,

On Sun, Dec 08, 2024 at 12:44:27PM +0000, Jonathan Cameron wrote:
> Given the mix of fixes and other material (kind of fixes, but also kind
> of new functionality), I've queued this for the next merge window in my
> togreg branch.  If you think there are particular patches that need to
> go sooner then I can handle them in a split fashion, but that does add
> risk that the whole lot might no land depending on timings (particularly
> given it's coming into vacation season).

So you tend to not backport the rdy-gpios patches (i.e.

	dt-bindings: iio: adc: adi,ad7{124,173,192,780}: Allow specifications of a gpio for irq line
	iio: adc: ad_sigma_delta: Add support for reading irq status using a GPIO

)? I personally would want to have these backported, too, but I can
understand that you might decide that in a different way.

Cherry picking

	iio: adc: ad_sigma_delta: Fix a race condition
	iio: adc: ad_sigma_delta: Check for previous ready signals

isn't trivial without the rdy-gpios, but they could be reworked. Tell me
if you want a helping hand (or an eye judging your backport).

Best regards
Uwe
Jonathan Cameron Dec. 11, 2024, 7:23 p.m. UTC | #10
On Mon, 9 Dec 2024 10:28:17 +0100
Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

> Hello Jonathan,
> 
> On Fri, Dec 06, 2024 at 06:28:42PM +0100, Uwe Kleine-König wrote:
> > diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> > index 9405cb579324..d858bffd2628 100644
> > --- a/drivers/iio/adc/ad7124.c
> > +++ b/drivers/iio/adc/ad7124.c
> > [...]
> > @@ -902,6 +941,37 @@ static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
> >  		chan[channel].channel2 = ain[1];
> >  	}
> >  
> > +	if (num_channels < AD7124_MAX_CHANNELS) {
> > +		st->channels[num_channels] = (struct ad7124_channel) {
> > +			.nr = num_channels,
> > +			.ain = AD7124_CHANNEL_AINP(AD7124_INPUT_TEMPSENSOR) |
> > +				AD7124_CHANNEL_AINM(AD7124_INPUT_AVSS),
> > +			.cfg = {
> > +				.bipolar = true,
> > +			},
> > +		};
> > +
> > +		chan[num_channels] = (struct iio_chan_spec) {
> > +			.type = IIO_TEMP,
> > +			.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > +				BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET) |
> > +				BIT(IIO_CHAN_INFO_SAMP_FREQ),
> > +			.scan_type = {
> > +				/*
> > +				 * You might find it strange that a bipolar
> > +				 * measurement yields an unsigned value, but
> > +				 * this matches the device's manual.
> > +				 */
> > +				.sign = 'u',
> > +				.realbits = 24,
> > +				.storagebits = 32,
> > +				.endianness = IIO_BE,
> > +			},
> > +			.address = num_channels,
> > +			.scan_index = num_channels,
> > +		};
> > +	};  
> 
> The kernel build bot wailed about the ; here. Should I send a proper
> patch, or can you still just rewrite your tree to drop it?

I tweaked it in the tree

Thanks,

Jonathan

> 
> Best regards and thanks and sorry,
> Uwe
Jonathan Cameron Dec. 11, 2024, 7:24 p.m. UTC | #11
On Mon, 9 Dec 2024 10:47:29 +0100
Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

> Hello Jonathan,
> 
> On Sun, Dec 08, 2024 at 12:44:27PM +0000, Jonathan Cameron wrote:
> > Given the mix of fixes and other material (kind of fixes, but also kind
> > of new functionality), I've queued this for the next merge window in my
> > togreg branch.  If you think there are particular patches that need to
> > go sooner then I can handle them in a split fashion, but that does add
> > risk that the whole lot might no land depending on timings (particularly
> > given it's coming into vacation season).  
> 
> So you tend to not backport the rdy-gpios patches (i.e.
> 
> 	dt-bindings: iio: adc: adi,ad7{124,173,192,780}: Allow specifications of a gpio for irq line
> 	iio: adc: ad_sigma_delta: Add support for reading irq status using a GPIO
> 
> )? I personally would want to have these backported, too, but I can
> understand that you might decide that in a different way.

Yeah. If it were tiny amount of code I might have gone the other way, but
this just got a bit too complex. 

> 
> Cherry picking
> 
> 	iio: adc: ad_sigma_delta: Fix a race condition
> 	iio: adc: ad_sigma_delta: Check for previous ready signals
> 
> isn't trivial without the rdy-gpios, but they could be reworked. Tell me
> if you want a helping hand (or an eye judging your backport).
> 
A backport won't go anywhere until these are upstream.  At that point if you
want them, feel free to suggest backporting these and provide the code ;)

Thanks,

Jonathan

> Best regards
> Uwe
Uwe Kleine-König Dec. 12, 2024, 11:28 a.m. UTC | #12
On Wed, Dec 11, 2024 at 07:24:59PM +0000, Jonathan Cameron wrote:
> On Mon, 9 Dec 2024 10:47:29 +0100
> Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:
> 
> > Hello Jonathan,
> > 
> > On Sun, Dec 08, 2024 at 12:44:27PM +0000, Jonathan Cameron wrote:
> > > Given the mix of fixes and other material (kind of fixes, but also kind
> > > of new functionality), I've queued this for the next merge window in my
> > > togreg branch.  If you think there are particular patches that need to
> > > go sooner then I can handle them in a split fashion, but that does add
> > > risk that the whole lot might no land depending on timings (particularly
> > > given it's coming into vacation season).  
> > 
> > So you tend to not backport the rdy-gpios patches (i.e.
> > 
> > 	dt-bindings: iio: adc: adi,ad7{124,173,192,780}: Allow specifications of a gpio for irq line
> > 	iio: adc: ad_sigma_delta: Add support for reading irq status using a GPIO
> > 
> > )? I personally would want to have these backported, too, but I can
> > understand that you might decide that in a different way.
> 
> Yeah. If it were tiny amount of code I might have gone the other way, but
> this just got a bit too complex. 

I think it is easy to see that the changes for the rdy-gpios support has
zero impact if the device has no rdy-gpio. Then
devm_gpiod_get_optional() returns NULL and !sigma_delta->rdy_gpiod is
true and so nothing changes.

Of course it's subjective if you agree this to be easy to see, and also
if that matters for the backport.
 
> > Cherry picking
> > 
> > 	iio: adc: ad_sigma_delta: Fix a race condition
> > 	iio: adc: ad_sigma_delta: Check for previous ready signals
> > 
> > isn't trivial without the rdy-gpios, but they could be reworked. Tell me
> > if you want a helping hand (or an eye judging your backport).
>
> A backport won't go anywhere until these are upstream.  At that point if you
> want them, feel free to suggest backporting these and provide the code ;)

I've not given up hope that you agree to backport also the rdy-gpio
change yet. I won't invest the work without knowing it's used in the
end. So I'll wait until the changes are upstream and you made up your
mind. Then if the need arises, I will help.

Best regards
Uwe
Andy Shevchenko Dec. 12, 2024, 11:44 a.m. UTC | #13
On Thu, Dec 12, 2024 at 1:28 PM Uwe Kleine-König
<u.kleine-koenig@baylibre.com> wrote:
> On Wed, Dec 11, 2024 at 07:24:59PM +0000, Jonathan Cameron wrote:
> > On Mon, 9 Dec 2024 10:47:29 +0100
> > Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

...

> > A backport won't go anywhere until these are upstream.  At that point if you
> > want them, feel free to suggest backporting these and provide the code ;)
>
> I've not given up hope that you agree to backport also the rdy-gpio
> change yet. I won't invest the work without knowing it's used in the
> end. So I'll wait until the changes are upstream and you made up your
> mind. Then if the need arises, I will help.

AFAIK the process, any contributor may suggest a bacport of a few
patches if the justification is good enough, I don't understand how
Jonathan's view can be an impediment here (he may be a help, of
course).
Jonathan Cameron Dec. 14, 2024, 5:14 p.m. UTC | #14
On Thu, 12 Dec 2024 13:44:40 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Thu, Dec 12, 2024 at 1:28 PM Uwe Kleine-König
> <u.kleine-koenig@baylibre.com> wrote:
> > On Wed, Dec 11, 2024 at 07:24:59PM +0000, Jonathan Cameron wrote:  
> > > On Mon, 9 Dec 2024 10:47:29 +0100
> > > Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:  
> 
> ...
> 
> > > A backport won't go anywhere until these are upstream.  At that point if you
> > > want them, feel free to suggest backporting these and provide the code ;)  
> >
> > I've not given up hope that you agree to backport also the rdy-gpio
> > change yet. I won't invest the work without knowing it's used in the
> > end. So I'll wait until the changes are upstream and you made up your
> > mind. Then if the need arises, I will help.  
> 
> AFAIK the process, any contributor may suggest a bacport of a few
> patches if the justification is good enough, I don't understand how
> Jonathan's view can be an impediment here (he may be a help, of
> course).
> 

I could in theory object (as could any reviewer!) but on this one I
won't because I think it is justified, just not something to rush
into without some soaking time in upstream.

Thanks,

Jonathan
Uwe Kleine-König Dec. 17, 2024, 10:23 a.m. UTC | #15
Hello Jonathan,

On Fri, Dec 06, 2024 at 06:28:40PM +0100, Uwe Kleine-König wrote:
> diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
> index 101cf30f4458..d32102b25530 100644
> --- a/drivers/iio/adc/ad_sigma_delta.c
> +++ b/drivers/iio/adc/ad_sigma_delta.c
> [...]
> @@ -222,6 +225,86 @@ static void ad_sd_enable_irq(struct ad_sigma_delta *sigma_delta)
>  	enable_irq(sigma_delta->irq_line);
>  }
>  
> +#define AD_SD_CLEAR_DATA_BUFLEN 9
> +
> +/* Called with `sigma_delta->bus_locked == true` only. */
> +static int ad_sigma_delta_clear_pending_event(struct ad_sigma_delta *sigma_delta)
> +{
> +	bool pending_event;
> +	unsigned int data_read_len = BITS_TO_BYTES(sigma_delta->info->num_resetclks);
> +	u8 *data;
> +	struct spi_transfer t[] = {
> +		{
> +			.len = 1,
> +		}, {
> +			.len = data_read_len,
> +		}
> +	};
> +	struct spi_message m;
> +	int ret;
> +
> +	/*
> +	 * Read R̅D̅Y̅ pin (if possible) or status register to check if there is an
> +	 * old event.
> +	 */
> +	if (sigma_delta->rdy_gpiod) {
> +		pending_event = gpiod_get_value(sigma_delta->rdy_gpiod);
> +	} else {
> +		unsigned status_reg;

While backporting this patch to a vendor tree I noticed checkpatch criticising:

	WARNING: Prefer 'unsigned int' to bare use of 'unsigned'

for the above line. Would you fixup an "int" into that line please, or
is that already too late?

Something like:

	git checkout togreg
	sed -i 's/unsigned status_reg;/unsigned int status_reg;/' drivers/iio/adc/ad_sigma_delta.c
	git commit --fixup=132d44dc6966 drivers/iio/adc/ad_sigma_delta.c
	git rebase -r --autosquash 132d44dc6966^

If it's already to late, I can prepare a proper patch for that.

Best regards
Uwe