[PATCH v3 00/10] iio: adc: ad7124: Various fixes

[PATCH v3 00/10] iio: adc: ad7124: Various fixes

[Uwe Kleine-König]

Hello,

this series contains all the yet unapplied patches I created to improve
support for the ad7124. I hope collecting them all in a single series is
fine and simplifies application. It superseeds the following series:

	https://lore.kernel.org/linux-iio/cover.1731404695.git.u.kleine-koenig@baylibre.com
	https://lore.kernel.org/linux-iio/20241119183611.56820-2-u.kleine-koenig@baylibre.com
	https://lore.kernel.org/linux-iio/20241028160748.489596-6-u.kleine-koenig@baylibre.com

It also benefits the other ADCs making use of the ad_sigma_delta
helpers. v6.12 is used as a base plus a fix that is already in next as
64612ec9b909b699293b7220c634f67a9fc12e06.

Changes since the previous submissions:

 - Rebase to v6.12 (was v6.12-rc1 before)
 - Rewording of some commit logs to match the actual changes
 - The last five patches are new.

The rdy-gpio patch was discussed controversially before, but I'm still
convinced that it's correct to do it this way. IMHO this is also
confirmed in the discussion with tglx at
https://lore.kernel.org/all/io53lznz3qp3jd5rohqsjhosnmdzd6d44sdbwu5jcfrs3rz2a2@orquwgflrtyc/
. Also while this fix doesn't work on all platforms (because not all irq
controllers can expose the DOUT/̅R̅D̅Y line as a GPIO in addition to
triggering the irq), it is definitively useful on the platforms where it
does work. The alternative is polling instead of using the irq which I'm
sure comes at a performance cost. (And even for polling, having the GPIO
reading the DOUT/̅R̅D̅Y line is beneficial as polling a GPIO is easier and
cheaper than polling a register bit via spi.)

The patch "Fix a race condition" is supposed to cure the following
warning that triggered twice for me:

	[   86.807282] ------------[ cut here ]------------
	[   86.811902] WARNING: CPU: 1 PID: 291 at kernel/irq/manage.c:790 __enable_irq+0x68/0x8c
	[   86.819819] Unbalanced enable for IRQ 35
	[   86.823729] Modules linked in:
	[   86.826784] CPU: 1 UID: 0 PID: 291 Comm: iio_readdev Not tainted 6.12.0-rc1+ #63
	[   86.834158] Hardware name: Altera SOCFPGA
	[   86.838156] Call trace: 
	[   86.838166]  unwind_backtrace from show_stack+0x18/0x1c
	[   86.845924]  show_stack from dump_stack_lvl+0x54/0x68
	[   86.850985]  dump_stack_lvl from __warn+0x88/0x11c
	[   86.855792]  __warn from warn_slowpath_fmt+0x128/0x194
	[   86.860937]  warn_slowpath_fmt from __enable_irq+0x68/0x8c
	[   86.866430]  __enable_irq from enable_irq+0x54/0xac
	[   86.871313]  enable_irq from ad_sd_buffer_postenable+0x120/0x18c
	[   86.877321]  ad_sd_buffer_postenable from __iio_update_buffers+0x688/0xa24
	[   86.884187]  __iio_update_buffers from enable_store+0x8c/0xd4
	[   86.889925]  enable_store from kernfs_fop_write_iter+0x130/0x1fc
	[   86.895928]  kernfs_fop_write_iter from vfs_write+0x258/0x404
	[   86.901671]  vfs_write from ksys_write+0x78/0xfc
	[   86.906287]  ksys_write from ret_fast_syscall+0x0/0x1c
	[   86.911420] Exception stack(0xf0a45fa8 to 0xf0a45ff0)
	[   86.916460] 5fa0:                   00000002 00b98f80 00000005 00b98f80 00000002 00000000
	[   86.924611] 5fc0: 00000002 00b98f80 00000005 00000004 b6ee3b94 00b97960 00000002 bee7442c
	[   86.932758] 5fe0: 00000004 bee73f20 b6e613e9 b6dd2776
	[   86.937793] ---[ end trace 0000000000000000 ]---

While I didn't find a way to reproduce this problem, I'm convinced the
patch fixes real race conditions like the one above.

The patch "Check for previous ready signals" fixes a similar problem as
the rdy-gpios patch. In that case however the immediate irq is triggered
by a previous measurement that didn't complete in time (e.g. because the
sysfs read was Ctrl-C'd). This one cannot be detected with the rdy-gpio
approach because in this case the device actually reports a pending irq.

The last patch isn't a fix but provides (part of) the motivation for
this series. Adding temperature support in half a work day was my
initial plan, all the fixes were found while implementing and testing
that one on a de10-nano board.

I didn't mark the patches for application to stable, because I think
this should be a maintainer decision, but if you ask me: backport all
but the last one. (Patches 3 and 7 are not fixes, but prerequisites for
the following patches.)

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,ad7124: 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          |   8 +
 drivers/iio/adc/ad7124.c                      | 219 +++++++++++++-----
 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              | 184 ++++++++++++---
 include/linux/iio/adc/ad_sigma_delta.h        |   7 +-
 8 files changed, 338 insertions(+), 89 deletions(-)

base-commit: adc218676eef25575469234709c2d87185ca223a
prerequisite-patch-id: 617af17fc377a984762c61893b9f2a92ae62213a
-- 
2.45.2

[PATCH v3 01/10] iio: adc: ad7124: Don't create more channels than the driver can handle

[Uwe Kleine-König]

The ad7124-4 and ad7124-8 both support 16 channel registers and assigns
each channel defined in dt statically such a register. While the driver
could be a bit more clever about this, it currently isn't and specifying
more than 16 channels yields broken behaviour. So just refuse to bind in
this situation.

Fixes: b3af341bbd96 ("iio: adc: Add ad7124 support")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/iio/adc/ad7124.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 8d94bc2b1cac..5352b26bb391 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -821,6 +821,16 @@ static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
 	if (!st->num_channels)
 		return dev_err_probe(dev, -ENODEV, "no channel children\n");
 
+	/*
+	 * The driver assigns each logical channel defined in the device tree
+	 * statically one channel register. So only accept 16 such logical
+	 * channels to not treat CONFIG_0 (i.e. the register following
+	 * CHANNEL_15) as an additional channel register. The driver could be
+	 * improved to lift this limitation.
+	 */
+	if (st->num_channels > AD7124_MAX_CHANNELS)
+		return dev_err_probe(dev, -EINVAL, "Too many channels defined\n");
+
 	chan = devm_kcalloc(indio_dev->dev.parent, st->num_channels,
 			    sizeof(*chan), GFP_KERNEL);
 	if (!chan)
-- 
2.45.2

Re: [PATCH v3 01/10] iio: adc: ad7124: Don't create more channels than the driver can handle

[David Lechner]

On 11/22/24 5:33 AM, Uwe Kleine-König wrote:
> The ad7124-4 and ad7124-8 both support 16 channel registers and assigns
> each channel defined in dt statically such a register. While the driver
> could be a bit more clever about this, it currently isn't and specifying
> more than 16 channels yields broken behaviour. So just refuse to bind in
> this situation.

The ad7124-4 datasheet I am looking at says that it only has registers
CONFIG_0 to CONFIG_7, so do we need to limit those chips to 8 channels?
> 
> Fixes: b3af341bbd96 ("iio: adc: Add ad7124 support")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---
>  drivers/iio/adc/ad7124.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> index 8d94bc2b1cac..5352b26bb391 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> @@ -821,6 +821,16 @@ static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
>  	if (!st->num_channels)
>  		return dev_err_probe(dev, -ENODEV, "no channel children\n");
>  
> +	/*
> +	 * The driver assigns each logical channel defined in the device tree
> +	 * statically one channel register. So only accept 16 such logical
> +	 * channels to not treat CONFIG_0 (i.e. the register following
> +	 * CHANNEL_15) as an additional channel register. The driver could be
> +	 * improved to lift this limitation.
> +	 */
> +	if (st->num_channels > AD7124_MAX_CHANNELS)
> +		return dev_err_probe(dev, -EINVAL, "Too many channels defined\n");
> +
>  	chan = devm_kcalloc(indio_dev->dev.parent, st->num_channels,
>  			    sizeof(*chan), GFP_KERNEL);
>  	if (!chan)

Re: [PATCH v3 01/10] iio: adc: ad7124: Don't create more channels than the driver can handle

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 968 bytes --]

On Fri, Nov 22, 2024 at 09:14:18AM -0600, David Lechner wrote:
> On 11/22/24 5:33 AM, Uwe Kleine-König wrote:
> > The ad7124-4 and ad7124-8 both support 16 channel registers and assigns
> > each channel defined in dt statically such a register. While the driver
> > could be a bit more clever about this, it currently isn't and specifying
> > more than 16 channels yields broken behaviour. So just refuse to bind in
> > this situation.
> 
> The ad7124-4 datasheet I am looking at says that it only has registers
> CONFIG_0 to CONFIG_7, so do we need to limit those chips to 8 channels?

These could be reused for different channels if the settings match. I'm
unsure what happens if the 16 channels use more than 8 different
configs and you want to bulk read them. Single channel use should work
fine I think. If that is a problem I might have to extend this series of
fixes, but this is something orthogonal to this patch I think.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 01/10] iio: adc: ad7124: Don't create more channels than the driver can handle

[David Lechner]

On 11/22/24 9:54 AM, Uwe Kleine-König wrote:
> On Fri, Nov 22, 2024 at 09:14:18AM -0600, David Lechner wrote:
>> On 11/22/24 5:33 AM, Uwe Kleine-König wrote:
>>> The ad7124-4 and ad7124-8 both support 16 channel registers and assigns
>>> each channel defined in dt statically such a register. While the driver
>>> could be a bit more clever about this, it currently isn't and specifying
>>> more than 16 channels yields broken behaviour. So just refuse to bind in
>>> this situation.
>>
>> The ad7124-4 datasheet I am looking at says that it only has registers
>> CONFIG_0 to CONFIG_7, so do we need to limit those chips to 8 channels?
> 
> These could be reused for different channels if the settings match. I'm
> unsure what happens if the 16 channels use more than 8 different
> configs and you want to bulk read them. Single channel use should work
> fine I think. If that is a problem I might have to extend this series of
> fixes, but this is something orthogonal to this patch I think.
> 
> Best regards
> Uwe

Oh, oops, you are right. I was mixing up _channel_ registers and
_config_ registers.

[PATCH v3 02/10] iio: adc: ad7124: Refuse invalid input specifiers

[Uwe Kleine-König]

The ad7124-4 has 8 analog inputs; the input select values 8 to 15 are
reserved and not to be used. These are fine for ad7124-8. For both
ad7124-4 and ad7124-8 values bigger than 15 are internal channels that
might appear as inputs in the channels specified in the device
description according to the description of commit f1794fd7bdf7 ("iio:
adc: ad7124: Remove input number limitation"), values bigger than 31
don't fit into the respective register bit field and the driver masked
them to smaller values.

Check for these invalid input specifiers and fail to probe if one is
found.

Fixes: f1794fd7bdf7 ("iio: adc: ad7124: Remove input number limitation")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/iio/adc/ad7124.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 5352b26bb391..1f3342373f1c 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -807,6 +807,19 @@ static int ad7124_check_chip_id(struct ad7124_state *st)
 	return 0;
 }
 
+/*
+ * Input specifiers 8 - 15 are explicitly reserved for ad7124-4
+ * while they are fine for ad7124-8. Values above 31 don't fit
+ * into the register field and so are invalid for sure.
+ */
+static bool ad7124_valid_input_select(unsigned int ain, const struct ad7124_chip_info *info)
+{
+	if (ain >= info->num_inputs && ain < 16)
+		return false;
+
+	return ain <= FIELD_MAX(AD7124_CHANNEL_AINM_MSK);
+}
+
 static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
 				       struct device *dev)
 {
@@ -859,6 +872,11 @@ static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
 		if (ret)
 			return ret;
 
+		if (!ad7124_valid_input_select(ain[0], st->chip_info) ||
+		    !ad7124_valid_input_select(ain[1], st->chip_info))
+			return dev_err_probe(dev, -EINVAL,
+					     "diff-channels property of %pfwP contains invalid data\n", child);
+
 		st->channels[channel].nr = channel;
 		st->channels[channel].ain = AD7124_CHANNEL_AINP(ain[0]) |
 						  AD7124_CHANNEL_AINM(ain[1]);
-- 
2.45.2

[PATCH v3 03/10] dt-bindings: iio: adc: adi,ad7124: Allow specifications of a gpio for irq line

[Uwe Kleine-König]

For the AD7124 chip the logical irq line (̅R̅D̅Y) is physically on the same
pin as the spi MISO output (DOUT) and so reading a register might
trigger an interrupt. For correct operation it's critical that the
actual state of the pin can be read to judge if an interrupt event is a
real one or just a spurious one triggered by toggling the line in its
MISO mode.

Allow specification of an "rdy-gpios" property that references a GPIO
that can be used for that purpose. While this is typically the same GPIO
also used (implicitly) as interrupt source, it is still supposed that
the interrupt is specified as before and usual.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
index 35ed04350e28..ebe77cbe87ff 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
@@ -37,6 +37,12 @@ properties:
     description: IRQ line for the ADC
     maxItems: 1
 
+  rdy-gpios:
+    description: |
+      GPIO reading the ̅R̅D̅Y line. Useful to reliably detect the interrupt
+      condition.
+    maxItems: 1
+
   '#address-cells':
     const: 1
 
@@ -111,6 +117,7 @@ unevaluatedProperties: false
 
 examples:
   - |
+    #include <dt-bindings/gpio/gpio.h>
     spi {
       #address-cells = <1>;
       #size-cells = <0>;
@@ -121,6 +128,7 @@ examples:
         spi-max-frequency = <5000000>;
         interrupts = <25 2>;
         interrupt-parent = <&gpio>;
+        rdy-gpios = <&gpio 25 GPIO_ACTIVE_LOW>;
         refin1-supply = <&adc_vref>;
         clocks = <&ad7124_mclk>;
         clock-names = "mclk";
-- 
2.45.2

Re: [PATCH v3 03/10] dt-bindings: iio: adc: adi,ad7124: Allow specifications of a gpio for irq line

[Rob Herring]

On Fri, Nov 22, 2024 at 12:33:22PM +0100, Uwe Kleine-König wrote:
> For the AD7124 chip the logical irq line (̅R̅D̅Y) is physically on the same
> pin as the spi MISO output (DOUT) and so reading a register might
> trigger an interrupt. For correct operation it's critical that the
> actual state of the pin can be read to judge if an interrupt event is a
> real one or just a spurious one triggered by toggling the line in its
> MISO mode.
> 
> Allow specification of an "rdy-gpios" property that references a GPIO
> that can be used for that purpose. While this is typically the same GPIO
> also used (implicitly) as interrupt source, it is still supposed that
> the interrupt is specified as before and usual.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---
>  Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml | 8 ++++++++
>  1 file changed, 8 insertions(+)

Comment and R-by on v2 still apply.

Rob

Re: [PATCH v3 03/10] dt-bindings: iio: adc: adi,ad7124: Allow specifications of a gpio for irq line

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1172 bytes --]

On Wed, Nov 27, 2024 at 07:41:28AM -0600, Rob Herring wrote:
> On Fri, Nov 22, 2024 at 12:33:22PM +0100, Uwe Kleine-König wrote:
> > For the AD7124 chip the logical irq line (̅R̅D̅Y) is physically on the same
> > pin as the spi MISO output (DOUT) and so reading a register might
> > trigger an interrupt. For correct operation it's critical that the
> > actual state of the pin can be read to judge if an interrupt event is a
> > real one or just a spurious one triggered by toggling the line in its
> > MISO mode.
> > 
> > Allow specification of an "rdy-gpios" property that references a GPIO
> > that can be used for that purpose. While this is typically the same GPIO
> > also used (implicitly) as interrupt source, it is still supposed that
> > the interrupt is specified as before and usual.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> > ---
> >  Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> 
> Comment and R-by on v2 still apply.

Oh dang I missed that and only see this when I just sent out v4. I'll
add a comment there.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v3 04/10] iio: adc: ad_sigma_delta: Add support for reading irq status using a GPIO

[Uwe Kleine-König]

Some of the ADCs by Analog signal their irq condition on the MISO line.
So typically that line is connected to an SPI controller and a GPIO. The
GPIO is used as input and the respective interrupt is enabled when the
last SPI transfer is completed.

Depending on the GPIO controller the toggling MISO line might make the
interrupt pending even while it's masked. In that case the irq handler
is called immediately after irq_enable() and so before the device
actually pulls that line low which results in non-sense values being
reported to the upper layers.

The only way to find out if the line was actually pulled low is to read
the GPIO. (There is a flag in AD7124's status register that also signals
if an interrupt was asserted, but reading that register toggles the MISO
line and so might trigger another spurious interrupt.)

Add the possibility to specify an interrupt GPIO in the machine
description in addition to the plain interrupt. This GPIO is used then
to check if the irq line is actually active in the irq handler.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/iio/adc/ad_sigma_delta.c       | 35 ++++++++++++++++++++++----
 include/linux/iio/adc/ad_sigma_delta.h |  1 +
 2 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index ea4aabd3960a..4c8d986b6609 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -539,12 +539,29 @@ static irqreturn_t ad_sd_data_rdy_trig_poll(int irq, void *private)
 {
 	struct ad_sigma_delta *sigma_delta = private;
 
-	complete(&sigma_delta->completion);
-	disable_irq_nosync(irq);
-	sigma_delta->irq_dis = true;
-	iio_trigger_poll(sigma_delta->trig);
+	/*
+	 * AD7124 and a few others use the same physical line for interrupt
+	 * reporting (nRDY) and MISO.
+	 * As MISO toggles when reading a register, this likely results in a
+	 * pending interrupt. This has two consequences: a) The irq might
+	 * trigger immediately after it's enabled even though the conversion
+	 * isn't done yet; and b) checking the STATUS register's nRDY flag is
+	 * off-limits as reading that would trigger another irq event.
+	 *
+	 * So read the MOSI line as GPIO (if available) and only trigger the irq
+	 * if the line is active.
+	 */
 
-	return IRQ_HANDLED;
+	if (!sigma_delta->rdy_gpiod || gpiod_get_value(sigma_delta->rdy_gpiod)) {
+		complete(&sigma_delta->completion);
+		disable_irq_nosync(irq);
+		sigma_delta->irq_dis = true;
+		iio_trigger_poll(sigma_delta->trig);
+
+		return IRQ_HANDLED;
+	} else {
+		return IRQ_NONE;
+	}
 }
 
 /**
@@ -679,6 +696,14 @@ int ad_sd_init(struct ad_sigma_delta *sigma_delta, struct iio_dev *indio_dev,
 	else
 		sigma_delta->irq_line = spi->irq;
 
+	sigma_delta->rdy_gpiod = devm_gpiod_get_optional(&spi->dev, "rdy", GPIOD_IN);
+	if (IS_ERR(sigma_delta->rdy_gpiod))
+		return dev_err_probe(&spi->dev, PTR_ERR(sigma_delta->rdy_gpiod),
+				     "Failed to find rdy gpio\n");
+
+	if (sigma_delta->rdy_gpiod && !sigma_delta->irq_line)
+		sigma_delta->irq_line = gpiod_to_irq(sigma_delta->rdy_gpiod);
+
 	iio_device_set_drvdata(indio_dev, sigma_delta);
 
 	return 0;
diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
index f8c1d2505940..866b4c21794b 100644
--- a/include/linux/iio/adc/ad_sigma_delta.h
+++ b/include/linux/iio/adc/ad_sigma_delta.h
@@ -96,6 +96,7 @@ struct ad_sigma_delta {
 	unsigned int		active_slots;
 	unsigned int		current_slot;
 	unsigned int		num_slots;
+	struct gpio_desc	*rdy_gpiod;
 	int		irq_line;
 	bool			status_appended;
 	/* map slots to channels in order to know what to expect from devices */
-- 
2.45.2

Re: [PATCH v3 04/10] iio: adc: ad_sigma_delta: Add support for reading irq status using a GPIO

[Andy Shevchenko]

On Fri, Nov 22, 2024 at 1:34 PM Uwe Kleine-König
<u.kleine-koenig@baylibre.com> wrote:
>
> Some of the ADCs by Analog signal their irq condition on the MISO line.
> So typically that line is connected to an SPI controller and a GPIO. The
> GPIO is used as input and the respective interrupt is enabled when the
> last SPI transfer is completed.
>
> Depending on the GPIO controller the toggling MISO line might make the
> interrupt pending even while it's masked. In that case the irq handler
> is called immediately after irq_enable() and so before the device
> actually pulls that line low which results in non-sense values being
> reported to the upper layers.
>
> The only way to find out if the line was actually pulled low is to read
> the GPIO. (There is a flag in AD7124's status register that also signals
> if an interrupt was asserted, but reading that register toggles the MISO
> line and so might trigger another spurious interrupt.)
>
> Add the possibility to specify an interrupt GPIO in the machine
> description in addition to the plain interrupt. This GPIO is used then
> to check if the irq line is actually active in the irq handler.

...

> +       if (!sigma_delta->rdy_gpiod || gpiod_get_value(sigma_delta->rdy_gpiod)) {
> +               complete(&sigma_delta->completion);
> +               disable_irq_nosync(irq);
> +               sigma_delta->irq_dis = true;
> +               iio_trigger_poll(sigma_delta->trig);
> +
> +               return IRQ_HANDLED;

> +       } else {

Redundant 'else'.

> +               return IRQ_NONE;
> +       }

Can we actually invert the conditional?

>  }

...

> +       if (sigma_delta->rdy_gpiod && !sigma_delta->irq_line)

Do you need the first check? (I haven't remember what gpiod_to_irq()
will return on NULL, though)

> +               sigma_delta->irq_line = gpiod_to_irq(sigma_delta->rdy_gpiod);

...

> --- a/include/linux/iio/adc/ad_sigma_delta.h
> +++ b/include/linux/iio/adc/ad_sigma_delta.h
> @@ -96,6 +96,7 @@ struct ad_sigma_delta {
>         unsigned int            active_slots;
>         unsigned int            current_slot;
>         unsigned int            num_slots;
> +       struct gpio_desc        *rdy_gpiod;

Do you need a type forward declaration?

>         int             irq_line;
>         bool                    status_appended;


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 04/10] iio: adc: ad_sigma_delta: Add support for reading irq status using a GPIO

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3159 bytes --]

On Fri, Nov 22, 2024 at 09:16:24PM +0200, Andy Shevchenko wrote:
> On Fri, Nov 22, 2024 at 1:34 PM Uwe Kleine-König
> <u.kleine-koenig@baylibre.com> wrote:
> >
> > Some of the ADCs by Analog signal their irq condition on the MISO line.
> > So typically that line is connected to an SPI controller and a GPIO. The
> > GPIO is used as input and the respective interrupt is enabled when the
> > last SPI transfer is completed.
> >
> > Depending on the GPIO controller the toggling MISO line might make the
> > interrupt pending even while it's masked. In that case the irq handler
> > is called immediately after irq_enable() and so before the device
> > actually pulls that line low which results in non-sense values being
> > reported to the upper layers.
> >
> > The only way to find out if the line was actually pulled low is to read
> > the GPIO. (There is a flag in AD7124's status register that also signals
> > if an interrupt was asserted, but reading that register toggles the MISO
> > line and so might trigger another spurious interrupt.)
> >
> > Add the possibility to specify an interrupt GPIO in the machine
> > description in addition to the plain interrupt. This GPIO is used then
> > to check if the irq line is actually active in the irq handler.
> 
> ...
> 
> > +       if (!sigma_delta->rdy_gpiod || gpiod_get_value(sigma_delta->rdy_gpiod)) {
> > +               complete(&sigma_delta->completion);
> > +               disable_irq_nosync(irq);
> > +               sigma_delta->irq_dis = true;
> > +               iio_trigger_poll(sigma_delta->trig);
> > +
> > +               return IRQ_HANDLED;
> 
> > +       } else {
> 
> Redundant 'else'.
> 
> > +               return IRQ_NONE;
> > +       }
> 
> Can we actually invert the conditional?

I thought about that and I prefer it that way because like this is
better matches the code comment before the if. I dropped the 'else'
though for the next submission.

> >  }
> 
> ...
> 
> > +       if (sigma_delta->rdy_gpiod && !sigma_delta->irq_line)
> 
> Do you need the first check? (I haven't remember what gpiod_to_irq()
> will return on NULL, though)
> 
> > +               sigma_delta->irq_line = gpiod_to_irq(sigma_delta->rdy_gpiod);

gpiod_to_irq() returns -EINVAL then. I added an error path for that
condition.

> > --- a/include/linux/iio/adc/ad_sigma_delta.h
> > +++ b/include/linux/iio/adc/ad_sigma_delta.h
> > @@ -96,6 +96,7 @@ struct ad_sigma_delta {
> >         unsigned int            active_slots;
> >         unsigned int            current_slot;
> >         unsigned int            num_slots;
> > +       struct gpio_desc        *rdy_gpiod;
> 
> Do you need a type forward declaration?

That would indeed be more robust. It compiles fine currently because all
consumers of linux/iio/adc/ad_sigma_delta.h also include linux/spi/spi.h
before which in turn includes linux/gpio/consumer.h and so knows about
struct gpio_desc. I'll add a declaration to be on the safe side.

> >         int             irq_line;
> >         bool                    status_appended;

Thanks for your feedback!
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v3 05/10] iio: adc: ad_sigma_delta: Handle CS assertion as intended in ad_sd_read_reg_raw()

[Uwe Kleine-König]

When struct ad_sigma_delta::keep_cs_asserted was introduced only
register writing was adapted to honor this new flag. Also respect it
when reading a register.

Fixes: df1d80aee963 ("iio: ad_sigma_delta: Properly handle SPI bus locking vs CS assertion")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/iio/adc/ad_sigma_delta.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index 4c8d986b6609..5e7e5cb908d8 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -109,7 +109,7 @@ static int ad_sd_read_reg_raw(struct ad_sigma_delta *sigma_delta,
 		}, {
 			.rx_buf = val,
 			.len = size,
-			.cs_change = sigma_delta->bus_locked,
+			.cs_change = sigma_delta->keep_cs_asserted,
 		},
 	};
 	struct spi_message m;
-- 
2.45.2

Re: [PATCH v3 05/10] iio: adc: ad_sigma_delta: Handle CS assertion as intended in ad_sd_read_reg_raw()

[Trevor Gamblin]

On 2024-11-22 06:33, Uwe Kleine-König wrote:
> When struct ad_sigma_delta::keep_cs_asserted was introduced only
> register writing was adapted to honor this new flag. Also respect it
> when reading a register.
>
> Fixes: df1d80aee963 ("iio: ad_sigma_delta: Properly handle SPI bus locking vs CS assertion")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
Reviewed-by: Trevor Gamblin <tgamblin@baylibre.com>

Re: [PATCH v3 05/10] iio: adc: ad_sigma_delta: Handle CS assertion as intended in ad_sd_read_reg_raw()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 691 bytes --]

Hello Trevor,

On Fri, Nov 22, 2024 at 10:16:07AM -0500, Trevor Gamblin wrote:
> On 2024-11-22 06:33, Uwe Kleine-König wrote:
> > When struct ad_sigma_delta::keep_cs_asserted was introduced only
> > register writing was adapted to honor this new flag. Also respect it
> > when reading a register.
> > 
> > Fixes: df1d80aee963 ("iio: ad_sigma_delta: Properly handle SPI bus locking vs CS assertion")
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> Reviewed-by: Trevor Gamblin <tgamblin@baylibre.com>

Oh, you already provided your Reviewed-by tag on the initial submission.
Sorry to have missed to add it myself and thanks to have provided it again.

Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v3 06/10] iio: adc: ad_sigma_delta: Fix a race condition

[Uwe Kleine-König]

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 per enable.

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>
---
 drivers/iio/adc/ad_sigma_delta.c       | 57 ++++++++++++++++----------
 include/linux/iio/adc/ad_sigma_delta.h |  1 +
 2 files changed, 36 insertions(+), 22 deletions(-)

diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index 5e7e5cb908d8..fd3560beb56b 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);
+
+	if (!sigma_delta->irq_dis) {
+		sigma_delta->irq_dis = true;
+		disable_irq_nosync(sigma_delta->irq_line);
+		return true;
+	} else {
+		return false;
+	}
+}
+
+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;
 }
@@ -550,12 +560,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.
+	 *
+	 * Also as only 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 +702,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 866b4c21794b..b3e4d89f2d06 100644
--- a/include/linux/iio/adc/ad_sigma_delta.h
+++ b/include/linux/iio/adc/ad_sigma_delta.h
@@ -85,6 +85,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;
-- 
2.45.2

Re: [PATCH v3 06/10] iio: adc: ad_sigma_delta: Fix a race condition

[Andy Shevchenko]

On Fri, Nov 22, 2024 at 1:34 PM 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 per enable.

"...exactly one disable call per one 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.

...

> +static bool ad_sd_disable_irq(struct ad_sigma_delta *sigma_delta)
> +{
> +       guard(spinlock_irqsave)(&sigma_delta->irq_lock);
> +
> +       if (!sigma_delta->irq_dis) {

Why not positive conditional?

  if (->irq_dis)
    return false;
  ...
  return true;

> +               sigma_delta->irq_dis = true;
> +               disable_irq_nosync(sigma_delta->irq_line);
> +               return true;
> +       } else {
> +               return false;
> +       }
> +}

...

>  /* private: */

Consider at some point marking the below members with __private.

>         struct completion       completion;
> +       spinlock_t              irq_lock; /* protects .irq_dis and irq en/disable state */
>         bool                    irq_dis;

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 07/10] iio: adc: ad_sigma_delta: Store information about reset sequence length

[Uwe Kleine-König]

The various chips can be reset using a sequence of SPI transfers with
MOSI = 1. The length of such a sequence varies from chip to chip. Store
that length in struct ad_sigma_delta_info and replace the respective
parameter to ad_sd_reset() with it.

Note the ad7192 used to pass 48 as length but the documentation
specifies 40 as the required length. Assuming the latter is right.
(Using a too long sequence doesn't hurt apart from using a longer spi
transfer than necessary, so this is no relevant fix.)

The motivation for storing this information is that this is useful to
clear a pending RDY signal in the next change.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/iio/adc/ad7124.c               | 3 ++-
 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       | 7 +++----
 include/linux/iio/adc/ad_sigma_delta.h | 5 +++--
 7 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 1f3342373f1c..b17c3dbeaeba 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -571,6 +571,7 @@ static const struct ad_sigma_delta_info ad7124_sigma_delta_info = {
 	.data_reg = AD7124_DATA,
 	.num_slots = 8,
 	.irq_flags = IRQF_TRIGGER_FALLING,
+	.num_resetclks = 64,
 };
 
 static int ad7124_read_raw(struct iio_dev *indio_dev,
@@ -756,7 +757,7 @@ static int ad7124_soft_reset(struct ad7124_state *st)
 	unsigned int readval, timeout;
 	int ret;
 
-	ret = ad_sd_reset(&st->sd, 64);
+	ret = ad_sd_reset(&st->sd);
 	if (ret < 0)
 		return ret;
 
diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index 0702ec71aa29..2550194efee8 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -744,6 +744,7 @@ static struct ad_sigma_delta_info ad7173_sigma_delta_info = {
 	.read_mask = BIT(6),
 	.status_ch_mask = GENMASK(3, 0),
 	.data_reg = AD7173_REG_DATA,
+	.num_resetclks = 64,
 };
 
 static int ad7173_setup(struct iio_dev *indio_dev)
diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 7042ddfdfc03..c4dd48edd8d9 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -361,6 +361,7 @@ static const struct ad_sigma_delta_info ad7192_sigma_delta_info = {
 	.status_ch_mask = GENMASK(3, 0),
 	.num_slots = 4,
 	.irq_flags = IRQF_TRIGGER_FALLING,
+	.num_resetclks = 40,
 };
 
 static const struct ad_sigma_delta_info ad7194_sigma_delta_info = {
@@ -373,6 +374,7 @@ static const struct ad_sigma_delta_info ad7194_sigma_delta_info = {
 	.read_mask = BIT(6),
 	.status_ch_mask = GENMASK(3, 0),
 	.irq_flags = IRQF_TRIGGER_FALLING,
+	.num_resetclks = 40,
 };
 
 static const struct ad_sd_calib_data ad7192_calib_arr[8] = {
@@ -565,7 +567,7 @@ static int ad7192_setup(struct iio_dev *indio_dev, struct device *dev)
 	int i, ret, id;
 
 	/* reset the serial interface */
-	ret = ad_sd_reset(&st->sd, 48);
+	ret = ad_sd_reset(&st->sd);
 	if (ret < 0)
 		return ret;
 	usleep_range(500, 1000); /* Wait for at least 500us */
diff --git a/drivers/iio/adc/ad7791.c b/drivers/iio/adc/ad7791.c
index 86effe8501b4..c7509b911835 100644
--- a/drivers/iio/adc/ad7791.c
+++ b/drivers/iio/adc/ad7791.c
@@ -254,6 +254,7 @@ static const struct ad_sigma_delta_info ad7791_sigma_delta_info = {
 	.addr_shift = 4,
 	.read_mask = BIT(3),
 	.irq_flags = IRQF_TRIGGER_FALLING,
+	.num_resetclks = 32,
 };
 
 static int ad7791_read_raw(struct iio_dev *indio_dev,
diff --git a/drivers/iio/adc/ad7793.c b/drivers/iio/adc/ad7793.c
index abebd519cafa..0767c56bb442 100644
--- a/drivers/iio/adc/ad7793.c
+++ b/drivers/iio/adc/ad7793.c
@@ -206,6 +206,7 @@ static const struct ad_sigma_delta_info ad7793_sigma_delta_info = {
 	.addr_shift = 3,
 	.read_mask = BIT(6),
 	.irq_flags = IRQF_TRIGGER_FALLING,
+	.num_resetclks = 32,
 };
 
 static const struct ad_sd_calib_data ad7793_calib_arr[6] = {
@@ -265,7 +266,7 @@ static int ad7793_setup(struct iio_dev *indio_dev,
 		return ret;
 
 	/* reset the serial interface */
-	ret = ad_sd_reset(&st->sd, 32);
+	ret = ad_sd_reset(&st->sd);
 	if (ret < 0)
 		goto out;
 	usleep_range(500, 2000); /* Wait for at least 500us */
diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index fd3560beb56b..19cb9b7b62c6 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -182,14 +182,13 @@ EXPORT_SYMBOL_NS_GPL(ad_sd_read_reg, IIO_AD_SIGMA_DELTA);
  *
  * Returns 0 on success, an error code otherwise.
  **/
-int ad_sd_reset(struct ad_sigma_delta *sigma_delta,
-	unsigned int reset_length)
+int ad_sd_reset(struct ad_sigma_delta *sigma_delta)
 {
+	unsigned int reset_length = sigma_delta->info->num_resetclks;
+	unsigned int size = DIV_ROUND_UP(reset_length, 8);
 	uint8_t *buf;
-	unsigned int size;
 	int ret;
 
-	size = DIV_ROUND_UP(reset_length, 8);
 	buf = kcalloc(size, sizeof(*buf), GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
index b3e4d89f2d06..39bb3a6dd9cd 100644
--- a/include/linux/iio/adc/ad_sigma_delta.h
+++ b/include/linux/iio/adc/ad_sigma_delta.h
@@ -53,6 +53,7 @@ struct iio_dev;
  * @irq_flags: flags for the interrupt used by the triggered buffer
  * @num_slots: Number of sequencer slots
  * @irq_line: IRQ for reading conversions. If 0, spi->irq will be used
+ * @num_resetclks: Number of SPI clk cycles with MOSI=1 to reset the chip.
  */
 struct ad_sigma_delta_info {
 	int (*set_channel)(struct ad_sigma_delta *, unsigned int channel);
@@ -69,6 +70,7 @@ struct ad_sigma_delta_info {
 	unsigned long irq_flags;
 	unsigned int num_slots;
 	int irq_line;
+	unsigned int num_resetclks;
 };
 
 /**
@@ -180,8 +182,7 @@ int ad_sd_write_reg(struct ad_sigma_delta *sigma_delta, unsigned int reg,
 int ad_sd_read_reg(struct ad_sigma_delta *sigma_delta, unsigned int reg,
 	unsigned int size, unsigned int *val);
 
-int ad_sd_reset(struct ad_sigma_delta *sigma_delta,
-	unsigned int reset_length);
+int ad_sd_reset(struct ad_sigma_delta *sigma_delta);
 
 int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev,
 	const struct iio_chan_spec *chan, int *val);
-- 
2.45.2

Re: [PATCH v3 07/10] iio: adc: ad_sigma_delta: Store information about reset sequence length

[Andy Shevchenko]

On Fri, Nov 22, 2024 at 1:34 PM Uwe Kleine-König
<u.kleine-koenig@baylibre.com> wrote:
>
> The various chips can be reset using a sequence of SPI transfers with
> MOSI = 1. The length of such a sequence varies from chip to chip. Store
> that length in struct ad_sigma_delta_info and replace the respective
> parameter to ad_sd_reset() with it.
>
> Note the ad7192 used to pass 48 as length but the documentation
> specifies 40 as the required length. Assuming the latter is right.
> (Using a too long sequence doesn't hurt apart from using a longer spi
> transfer than necessary, so this is no relevant fix.)
>
> The motivation for storing this information is that this is useful to
> clear a pending RDY signal in the next change.

...

> @@ -182,14 +182,13 @@ EXPORT_SYMBOL_NS_GPL(ad_sd_read_reg, IIO_AD_SIGMA_DELTA);
>   *
>   * Returns 0 on success, an error code otherwise.

>   **/

JFYI: unneeded asterisk.

...

> +       unsigned int size = DIV_ROUND_UP(reset_length, 8);
>         uint8_t *buf;
> -       unsigned int size;
>         int ret;
>
> -       size = DIV_ROUND_UP(reset_length, 8);
>         buf = kcalloc(size, sizeof(*buf), GFP_KERNEL);
>         if (!buf)
>                 return -ENOMEM;

This is somewhat like unrelated refactoring.

I would suggest avoiding doing it here and instead add a change that
either uses BITS_TO_BYTES() or even bitmap_zalloc(), whoever the
latter might require more changes in the code.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 07/10] iio: adc: ad_sigma_delta: Store information about reset sequence length

[kernel test robot]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=unknown-8bit, Size: 3533 bytes --]

Hi Uwe,

kernel test robot noticed the following build warnings:

[auto build test WARNING on adc218676eef25575469234709c2d87185ca223a]

url:    https://github.com/intel-lab-lkp/linux/commits/Uwe-Kleine-K-nig/iio-adc-ad7124-Don-t-create-more-channels-than-the-driver-can-handle/20241125-104725
base:   adc218676eef25575469234709c2d87185ca223a
patch link:    https://lore.kernel.org/r/20241122113322.242875-19-u.kleine-koenig%40baylibre.com
patch subject: [PATCH v3 07/10] iio: adc: ad_sigma_delta: Store information about reset sequence length
config: i386-buildonly-randconfig-003-20241125 (https://download.01.org/0day-ci/archive/20241125/202411251711.ZE0hl0cg-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241125/202411251711.ZE0hl0cg-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411251711.ZE0hl0cg-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/iio/adc/ad_sigma_delta.c:186: warning: Excess function parameter 'reset_length' description in 'ad_sd_reset'


vim +186 drivers/iio/adc/ad_sigma_delta.c

af3008485ea037 Lars-Peter Clausen 2012-08-10  176  
7fc10de8d49a74 Dragos Bogdan      2017-09-05  177  /**
7fc10de8d49a74 Dragos Bogdan      2017-09-05  178   * ad_sd_reset() - Reset the serial interface
7fc10de8d49a74 Dragos Bogdan      2017-09-05  179   *
7fc10de8d49a74 Dragos Bogdan      2017-09-05  180   * @sigma_delta: The sigma delta device
7fc10de8d49a74 Dragos Bogdan      2017-09-05  181   * @reset_length: Number of SCLKs with DIN = 1
7fc10de8d49a74 Dragos Bogdan      2017-09-05  182   *
7fc10de8d49a74 Dragos Bogdan      2017-09-05  183   * Returns 0 on success, an error code otherwise.
7fc10de8d49a74 Dragos Bogdan      2017-09-05  184   **/
066265377484bc Uwe Kleine-König   2024-11-22  185  int ad_sd_reset(struct ad_sigma_delta *sigma_delta)
7fc10de8d49a74 Dragos Bogdan      2017-09-05 @186  {
066265377484bc Uwe Kleine-König   2024-11-22  187  	unsigned int reset_length = sigma_delta->info->num_resetclks;
066265377484bc Uwe Kleine-König   2024-11-22  188  	unsigned int size = DIV_ROUND_UP(reset_length, 8);
7fc10de8d49a74 Dragos Bogdan      2017-09-05  189  	uint8_t *buf;
7fc10de8d49a74 Dragos Bogdan      2017-09-05  190  	int ret;
7fc10de8d49a74 Dragos Bogdan      2017-09-05  191  
7fc10de8d49a74 Dragos Bogdan      2017-09-05  192  	buf = kcalloc(size, sizeof(*buf), GFP_KERNEL);
7fc10de8d49a74 Dragos Bogdan      2017-09-05  193  	if (!buf)
7fc10de8d49a74 Dragos Bogdan      2017-09-05  194  		return -ENOMEM;
7fc10de8d49a74 Dragos Bogdan      2017-09-05  195  
7fc10de8d49a74 Dragos Bogdan      2017-09-05  196  	memset(buf, 0xff, size);
7fc10de8d49a74 Dragos Bogdan      2017-09-05  197  	ret = spi_write(sigma_delta->spi, buf, size);
7fc10de8d49a74 Dragos Bogdan      2017-09-05  198  	kfree(buf);
7fc10de8d49a74 Dragos Bogdan      2017-09-05  199  
7fc10de8d49a74 Dragos Bogdan      2017-09-05  200  	return ret;
7fc10de8d49a74 Dragos Bogdan      2017-09-05  201  }
ef807729767fb7 Jonathan Cameron   2022-01-30  202  EXPORT_SYMBOL_NS_GPL(ad_sd_reset, IIO_AD_SIGMA_DELTA);
7fc10de8d49a74 Dragos Bogdan      2017-09-05  203  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v3 08/10] iio: adc: ad_sigma_delta: Check for previous ready signals

[Uwe Kleine-König]

It can happen if a previous conversion was aborted the ADC pulls down
the ̅R̅D̅Y line but the event wasn't handled before. In that case enabling
the irq might immediatly fire (depending on the irq controller's
capabilities) and even with a rdy-gpio isn't identified as an unrelated
one.

To cure that problem check for a pending event before the measurement is
started and clear it if needed.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/iio/adc/ad_sigma_delta.c | 91 ++++++++++++++++++++++++++++++++
 1 file changed, 91 insertions(+)

diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index 19cb9b7b62c6..8bc652b71019 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -29,8 +29,11 @@
 #define AD_SD_COMM_CHAN_MASK	0x3
 
 #define AD_SD_REG_COMM		0x00
+#define AD_SD_REG_STATUS	0x00
 #define AD_SD_REG_DATA		0x03
 
+#define AD_SD_REG_STATUS_RDY	0x80
+
 /**
  * ad_sd_set_comm() - Set communications register
  *
@@ -222,6 +225,82 @@ static void ad_sd_enable_irq(struct ad_sigma_delta *sigma_delta)
 	enable_irq(sigma_delta->irq_line);
 }
 
+/* Called with `sigma_delta->bus_locked == true` only.  */
+static int ad_sigma_delta_clear_pending_event(struct ad_sigma_delta *sigma_delta)
+{
+	int ret = 0;
+	bool pending_event;
+
+	/*
+	 * read RDY 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;
+
+		ret = ad_sd_read_reg(sigma_delta, AD_SD_REG_STATUS, 1, &status_reg);
+		if (ret)
+			return ret;
+
+		pending_event = !(status_reg & AD_SD_REG_STATUS_RDY);
+	}
+
+	if (pending_event) {
+		/*
+		 * In general the size of the data register is unknown. It
+		 * varies from device to device, might be one byte longer if
+		 * CONTROL.DATA_STATUS is set and even varies on some devices
+		 * depending on which input is selected. So send one byte to
+		 * start reading the data register and then just clock for some
+		 * bytes with DIN (aka MOSI) high to not confuse the register
+		 * access state machine after the data register was completely
+		 * read. Note however that the sequence length must be shorter
+		 * than the reset procedure.
+		 */
+		unsigned int data_read_len = DIV_ROUND_UP(sigma_delta->info->num_resetclks, 8);
+		uint8_t data[9];
+		struct spi_transfer t[] = {
+			{
+				.tx_buf = data,
+				.len = 1,
+			}, {
+				.tx_buf = data + 1,
+				.len = data_read_len,
+			}
+		};
+		struct spi_message m;
+
+		/* Oh, back out instead of overflowing data[] */
+		if (data_read_len > sizeof(data) - 1)
+			return -EINVAL;
+
+		spi_message_init(&m);
+		if (sigma_delta->info->has_registers) {
+			unsigned int data_reg = sigma_delta->info->data_reg ?: AD_SD_REG_DATA;
+
+			data[0] = data_reg << sigma_delta->info->addr_shift;
+			data[0] |= sigma_delta->info->read_mask;
+			data[0] |= sigma_delta->comm;
+			spi_message_add_tail(&t[0], &m);
+		}
+
+		/*
+		 * The first transferred byte is part of the real data register,
+		 * so this doesn't need to be 0xff. In the remaining
+		 * `data_read_len - 1` bytes are less than $num_resetclks ones.
+		 */
+		data[1] = 0x00;
+		memset(data + 2, 0xff, data_read_len - 1);
+		spi_message_add_tail(&t[1], &m);
+
+		ret = spi_sync_locked(sigma_delta->spi, &m);
+	}
+
+	return ret;
+}
+
 int ad_sd_calibrate(struct ad_sigma_delta *sigma_delta,
 	unsigned int mode, unsigned int channel)
 {
@@ -237,6 +316,10 @@ int ad_sd_calibrate(struct ad_sigma_delta *sigma_delta,
 	sigma_delta->keep_cs_asserted = true;
 	reinit_completion(&sigma_delta->completion);
 
+	ret = ad_sigma_delta_clear_pending_event(sigma_delta);
+	if (ret)
+		return ret;
+
 	ret = ad_sigma_delta_set_mode(sigma_delta, mode);
 	if (ret < 0)
 		goto out;
@@ -310,6 +393,10 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev,
 	sigma_delta->keep_cs_asserted = true;
 	reinit_completion(&sigma_delta->completion);
 
+	ret = ad_sigma_delta_clear_pending_event(sigma_delta);
+	if (ret)
+		return ret;
+
 	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_SINGLE);
 
 	ad_sd_enable_irq(sigma_delta);
@@ -406,6 +493,10 @@ static int ad_sd_buffer_postenable(struct iio_dev *indio_dev)
 	sigma_delta->bus_locked = true;
 	sigma_delta->keep_cs_asserted = true;
 
+	ret = ad_sigma_delta_clear_pending_event(sigma_delta);
+	if (ret)
+		return ret;
+
 	ret = ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_CONTINUOUS);
 	if (ret)
 		goto err_unlock;
-- 
2.45.2

Re: [PATCH v3 08/10] iio: adc: ad_sigma_delta: Check for previous ready signals

[Andy Shevchenko]

On Fri, Nov 22, 2024 at 1:34 PM Uwe Kleine-König
<u.kleine-koenig@baylibre.com> wrote:
>
> It can happen if a previous conversion was aborted the ADC pulls down
> the ̅R̅D̅Y line but the event wasn't handled before. In that case enabling
> the irq might immediatly fire (depending on the irq controller's

immediately

> capabilities) and even with a rdy-gpio isn't identified as an unrelated
> one.
>
> To cure that problem check for a pending event before the measurement is
> started and clear it if needed.

...

> +static int ad_sigma_delta_clear_pending_event(struct ad_sigma_delta *sigma_delta)
> +{
> +       int ret = 0;

Unneeded assignment, see below.

> +       bool pending_event;
> +
> +       /*
> +        * read RDY 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;
> +
> +               ret = ad_sd_read_reg(sigma_delta, AD_SD_REG_STATUS, 1, &status_reg);
> +               if (ret)
> +                       return ret;

Side note: in the initial logic the 0 assigned above is overwritten by
0 here. While it's not a technical problem, it might affect the
workflow in the future.

> +               pending_event = !(status_reg & AD_SD_REG_STATUS_RDY);
> +       }
> +
> +       if (pending_event) {

So, check for the error condition first pattern?

  if (!pending_event)
    return 0;

This among other benefits makes the below code less indented and hence
less LoCs to be occupied.

> +               /*
> +                * In general the size of the data register is unknown. It
> +                * varies from device to device, might be one byte longer if
> +                * CONTROL.DATA_STATUS is set and even varies on some devices
> +                * depending on which input is selected. So send one byte to
> +                * start reading the data register and then just clock for some
> +                * bytes with DIN (aka MOSI) high to not confuse the register
> +                * access state machine after the data register was completely
> +                * read. Note however that the sequence length must be shorter
> +                * than the reset procedure.
> +                */
> +               unsigned int data_read_len = DIV_ROUND_UP(sigma_delta->info->num_resetclks, 8);

BITS_TO_BYTES()

> +               uint8_t data[9];

Why not u8?

> +               struct spi_transfer t[] = {
> +                       {
> +                               .tx_buf = data,
> +                               .len = 1,
> +                       }, {
> +                               .tx_buf = data + 1,
> +                               .len = data_read_len,
> +                       }
> +               };
> +               struct spi_message m;
> +
> +               /* Oh, back out instead of overflowing data[] */
> +               if (data_read_len > sizeof(data) - 1)
> +                       return -EINVAL;
> +
> +               spi_message_init(&m);
> +               if (sigma_delta->info->has_registers) {
> +                       unsigned int data_reg = sigma_delta->info->data_reg ?: AD_SD_REG_DATA;
> +
> +                       data[0] = data_reg << sigma_delta->info->addr_shift;
> +                       data[0] |= sigma_delta->info->read_mask;
> +                       data[0] |= sigma_delta->comm;
> +                       spi_message_add_tail(&t[0], &m);
> +               }
> +
> +               /*
> +                * The first transferred byte is part of the real data register,
> +                * so this doesn't need to be 0xff. In the remaining
> +                * `data_read_len - 1` bytes are less than $num_resetclks ones.
> +                */
> +               data[1] = 0x00;
> +               memset(data + 2, 0xff, data_read_len - 1);

> +               spi_message_add_tail(&t[1], &m);

Instead you can also use

  if (...)
    spi_message_init_with_transfers(..., 2);
  else
    spi_message_init_with_transfers(..., 1);


> +               ret = spi_sync_locked(sigma_delta->spi, &m);
> +       }
> +
> +       return ret;

return spi_sync_locked(...);

> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 08/10] iio: adc: ad_sigma_delta: Check for previous ready signals

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 791 bytes --]

Hello Andy,

On Fri, Nov 22, 2024 at 10:17:51PM +0200, Andy Shevchenko wrote:
> On Fri, Nov 22, 2024 at 1:34 PM Uwe Kleine-König
> <u.kleine-koenig@baylibre.com> wrote:
> > +static int ad_sigma_delta_clear_pending_event(struct ad_sigma_delta *sigma_delta)
> > +{
> > +       int ret = 0;
> 
> Unneeded assignment, see below.

With the changes you suggested below this can get a more local scope,
too. That's what I did for v4.
 
> > +               spi_message_add_tail(&t[1], &m);
> 
> Instead you can also use
> 
>   if (...)
>     spi_message_init_with_transfers(..., 2);
>   else
>     spi_message_init_with_transfers(..., 1);

I kept it as is to match the register read function.

Implemented your other suggestions, stay tuned for v4.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v3 09/10] iio: adc: ad7124: Add error reporting during probe

[Uwe Kleine-König]

A driver that silently fails to probe is annoying and hard to debug. So
add messages in the error paths of the probe function.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/iio/adc/ad7124.c | 78 +++++++++++++++++++++-------------------
 1 file changed, 42 insertions(+), 36 deletions(-)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index b17c3dbeaeba..fdbe2806bf11 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -360,20 +360,21 @@ static int ad7124_find_free_config_slot(struct ad7124_state *st)
 	return free_cfg_slot;
 }
 
+/* Only called during probe, so dev_err_probe() can be used */
 static int ad7124_init_config_vref(struct ad7124_state *st, struct ad7124_channel_config *cfg)
 {
 	unsigned int refsel = cfg->refsel;
+	struct device *dev = &st->sd.spi->dev;
 
 	switch (refsel) {
 	case AD7124_REFIN1:
 	case AD7124_REFIN2:
 	case AD7124_AVDD_REF:
-		if (IS_ERR(st->vref[refsel])) {
-			dev_err(&st->sd.spi->dev,
-				"Error, trying to use external voltage reference without a %s regulator.\n",
-				ad7124_ref_names[refsel]);
-			return PTR_ERR(st->vref[refsel]);
-		}
+		if (IS_ERR(st->vref[refsel]))
+			return dev_err_probe(dev, PTR_ERR(st->vref[refsel]),
+					     "Error, trying to use external voltage reference without a %s regulator.\n",
+					     ad7124_ref_names[refsel]);
+
 		cfg->vref_mv = regulator_get_voltage(st->vref[refsel]);
 		/* Conversion from uV to mV */
 		cfg->vref_mv /= 1000;
@@ -384,8 +385,7 @@ static int ad7124_init_config_vref(struct ad7124_state *st, struct ad7124_channe
 		st->adc_control |= AD7124_ADC_CTRL_REF_EN(1);
 		return 0;
 	default:
-		dev_err(&st->sd.spi->dev, "Invalid reference %d\n", refsel);
-		return -EINVAL;
+		return dev_err_probe(dev, -EINVAL, "Invalid reference %d\n", refsel);
 	}
 }
 
@@ -752,6 +752,7 @@ static const struct iio_info ad7124_info = {
 	.attrs = &ad7124_attrs_group,
 };
 
+/* Only called during probe, so dev_err_probe() can be used */
 static int ad7124_soft_reset(struct ad7124_state *st)
 {
 	unsigned int readval, timeout;
@@ -766,7 +767,7 @@ static int ad7124_soft_reset(struct ad7124_state *st)
 	do {
 		ret = ad_sd_read_reg(&st->sd, AD7124_STATUS, 1, &readval);
 		if (ret < 0)
-			return ret;
+			return dev_err_probe(&st->sd.spi->dev, ret, "Error reading status register\n");
 
 		if (!(readval & AD7124_STATUS_POR_FLAG_MSK))
 			return 0;
@@ -775,9 +776,7 @@ static int ad7124_soft_reset(struct ad7124_state *st)
 		usleep_range(100, 2000);
 	} while (--timeout);
 
-	dev_err(&st->sd.spi->dev, "Soft reset failed\n");
-
-	return -EIO;
+	return dev_err_probe(&st->sd.spi->dev, -EIO, "Soft reset failed\n");
 }
 
 static int ad7124_check_chip_id(struct ad7124_state *st)
@@ -787,23 +786,20 @@ static int ad7124_check_chip_id(struct ad7124_state *st)
 
 	ret = ad_sd_read_reg(&st->sd, AD7124_ID, 1, &readval);
 	if (ret < 0)
-		return ret;
+		return dev_err_probe(&st->sd.spi->dev, ret,
+				     "Failure to read ID register\n");
 
 	chip_id = AD7124_DEVICE_ID_GET(readval);
 	silicon_rev = AD7124_SILICON_REV_GET(readval);
 
-	if (chip_id != st->chip_info->chip_id) {
-		dev_err(&st->sd.spi->dev,
-			"Chip ID mismatch: expected %u, got %u\n",
-			st->chip_info->chip_id, chip_id);
-		return -ENODEV;
-	}
+	if (chip_id != st->chip_info->chip_id)
+		return dev_err_probe(&st->sd.spi->dev, -ENODEV,
+				     "Chip ID mismatch: expected %u, got %u\n",
+				     st->chip_info->chip_id, chip_id);
 
-	if (silicon_rev == 0) {
-		dev_err(&st->sd.spi->dev,
-			"Silicon revision empty. Chip may not be present\n");
-		return -ENODEV;
-	}
+	if (silicon_rev == 0)
+		return dev_err_probe(&st->sd.spi->dev, -ENODEV,
+				     "Silicon revision empty. Chip may not be present\n");
 
 	return 0;
 }
@@ -862,16 +858,18 @@ static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
 	device_for_each_child_node_scoped(dev, child) {
 		ret = fwnode_property_read_u32(child, "reg", &channel);
 		if (ret)
-			return ret;
+			return dev_err_probe(dev, ret,
+					     "Failed to parse reg property of %pfwP\n", child);
 
 		if (channel >= indio_dev->num_channels)
 			return dev_err_probe(dev, -EINVAL,
-				"Channel index >= number of channels\n");
+					     "Channel index >= number of channels in %pfwP\n", child);
 
 		ret = fwnode_property_read_u32_array(child, "diff-channels",
 						     ain, 2);
 		if (ret)
-			return ret;
+			return dev_err_probe(dev, ret,
+					     "Failed to parse diff-channels property of %pfwP\n", child);
 
 		if (!ad7124_valid_input_select(ain[0], st->chip_info) ||
 		    !ad7124_valid_input_select(ain[1], st->chip_info))
@@ -909,11 +907,12 @@ static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
 static int ad7124_setup(struct ad7124_state *st)
 {
 	unsigned int fclk, power_mode;
+	struct device *dev = &st->sd.spi->dev;
 	int i, ret;
 
 	fclk = clk_get_rate(st->mclk);
 	if (!fclk)
-		return -EINVAL;
+		return dev_err_probe(dev, -EINVAL, "Failed to get mclk rate\n");
 
 	/* The power mode changes the master clock frequency */
 	power_mode = ad7124_find_closest_match(ad7124_master_clk_freq_hz,
@@ -922,7 +921,7 @@ static int ad7124_setup(struct ad7124_state *st)
 	if (fclk != ad7124_master_clk_freq_hz[power_mode]) {
 		ret = clk_set_rate(st->mclk, fclk);
 		if (ret)
-			return ret;
+			return dev_err_probe(dev, ret, "Failed to set mclk rate\n");
 	}
 
 	/* Set the power mode */
@@ -953,7 +952,7 @@ static int ad7124_setup(struct ad7124_state *st)
 
 	ret = ad_sd_write_reg(&st->sd, AD7124_ADC_CONTROL, 2, st->adc_control);
 	if (ret < 0)
-		return ret;
+		return dev_err_probe(dev, ret, "Failed to setup CONTROL register\n");
 
 	return ret;
 }
@@ -968,11 +967,12 @@ static int ad7124_probe(struct spi_device *spi)
 	const struct ad7124_chip_info *info;
 	struct ad7124_state *st;
 	struct iio_dev *indio_dev;
+	struct device *dev = &spi->dev;
 	int i, ret;
 
 	info = spi_get_device_match_data(spi);
 	if (!info)
-		return -ENODEV;
+		return dev_err_probe(dev, -ENODEV, "Failed to get match data\n");
 
 	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
 	if (!indio_dev)
@@ -1007,36 +1007,42 @@ static int ad7124_probe(struct spi_device *spi)
 
 		ret = regulator_enable(st->vref[i]);
 		if (ret)
-			return ret;
+			return dev_err_probe(dev, ret, "Failed to enable regulator #%d\n", i);
 
 		ret = devm_add_action_or_reset(&spi->dev, ad7124_reg_disable,
 					       st->vref[i]);
 		if (ret)
-			return ret;
+			return dev_err_probe(dev, ret, "Failed to register disable handler for regulator #%d\n", i);
 	}
 
 	st->mclk = devm_clk_get_enabled(&spi->dev, "mclk");
 	if (IS_ERR(st->mclk))
-		return PTR_ERR(st->mclk);
+		return dev_err_probe(dev, PTR_ERR(st->mclk), "Failed to get mclk\n");
 
 	ret = ad7124_soft_reset(st);
 	if (ret < 0)
+		/* ad7124_soft_reset() already emitted an error message */
 		return ret;
 
 	ret = ad7124_check_chip_id(st);
 	if (ret)
+		/* ad7124_check_chip_id() already emitted an error message */
 		return ret;
 
 	ret = ad7124_setup(st);
 	if (ret < 0)
+		/* ad7124_setup() already emitted an error message */
 		return ret;
 
 	ret = devm_ad_sd_setup_buffer_and_trigger(&spi->dev, indio_dev);
 	if (ret < 0)
-		return ret;
+		return dev_err_probe(dev, ret, "Failed to setup triggers\n");
 
-	return devm_iio_device_register(&spi->dev, indio_dev);
+	ret = devm_iio_device_register(&spi->dev, indio_dev);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to register iio device\n");
 
+	return 0;
 }
 
 static const struct of_device_id ad7124_of_match[] = {
-- 
2.45.2

Re: [PATCH v3 09/10] iio: adc: ad7124: Add error reporting during probe

[Trevor Gamblin]

On 2024-11-22 06:33, Uwe Kleine-König wrote:
> A driver that silently fails to probe is annoying and hard to debug. So
> add messages in the error paths of the probe function.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
Reviewed-by: Trevor Gamblin <tgamblin@baylibre.com>

Re: [PATCH v3 09/10] iio: adc: ad7124: Add error reporting during probe

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 551 bytes --]

On Fri, Nov 22, 2024 at 11:44:32AM -0500, Trevor Gamblin wrote:
> 
> On 2024-11-22 06:33, Uwe Kleine-König wrote:
> > A driver that silently fails to probe is annoying and hard to debug. So
> > add messages in the error paths of the probe function.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> Reviewed-by: Trevor Gamblin <tgamblin@baylibre.com>

With the changes that Andy suggested I didn't add your tag yet. So if
you miss it in v4, *this time* it was a concious choice. :-)

Best regards and thanks,
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 09/10] iio: adc: ad7124: Add error reporting during probe

[Andy Shevchenko]

On Fri, Nov 22, 2024 at 1:34 PM Uwe Kleine-König
<u.kleine-koenig@baylibre.com> wrote:
>
> A driver that silently fails to probe is annoying and hard to debug. So
> add messages in the error paths of the probe function.

...

> +/* Only called during probe, so dev_err_probe() can be used */

It's a harmless comment, but I think dev_err_probe() name is good
enough to give such a hint.

...

> +/* Only called during probe, so dev_err_probe() can be used */

Ditto.

...

>         do {
>                 ret = ad_sd_read_reg(&st->sd, AD7124_STATUS, 1, &readval);
>                 if (ret < 0)
> -                       return ret;
> +                       return dev_err_probe(&st->sd.spi->dev, ret, "Error reading status register\n");
>
>                 if (!(readval & AD7124_STATUS_POR_FLAG_MSK))
>                         return 0;

>                 usleep_range(100, 2000);

Side note 1: fsleep() ?

>         } while (--timeout);

Side note 2: maybe using read_poll_timeout() from iopoll.h makes this
better looking?

...

>  static int ad7124_check_chip_id(struct ad7124_state *st)

>         ret = ad_sd_read_reg(&st->sd, AD7124_ID, 1, &readval);
>         if (ret < 0)
> -               return ret;
> +               return dev_err_probe(&st->sd.spi->dev, ret,
> +                                    "Failure to read ID register\n");

Why not temporary for the struct device, will be the same LoCs now,
but might help in the future if more callers will need this parameter.

>
>         chip_id = AD7124_DEVICE_ID_GET(readval);
>         silicon_rev = AD7124_SILICON_REV_GET(readval);
>
> -       if (chip_id != st->chip_info->chip_id) {
> -               dev_err(&st->sd.spi->dev,
> -                       "Chip ID mismatch: expected %u, got %u\n",
> -                       st->chip_info->chip_id, chip_id);
> -               return -ENODEV;
> -       }
> +       if (chip_id != st->chip_info->chip_id)
> +               return dev_err_probe(&st->sd.spi->dev, -ENODEV,
> +                                    "Chip ID mismatch: expected %u, got %u\n",
> +                                    st->chip_info->chip_id, chip_id);
>
> -       if (silicon_rev == 0) {
> -               dev_err(&st->sd.spi->dev,
> -                       "Silicon revision empty. Chip may not be present\n");
> -               return -ENODEV;
> -       }
> +       if (silicon_rev == 0)
> +               return dev_err_probe(&st->sd.spi->dev, -ENODEV,
> +                                    "Silicon revision empty. Chip may not be present\n");
>
>         return 0;
>  }

...

>         ret = ad_sd_write_reg(&st->sd, AD7124_ADC_CONTROL, 2, st->adc_control);
>         if (ret < 0)
> -               return ret;
> +               return dev_err_probe(dev, ret, "Failed to setup CONTROL register\n");
>
>         return ret;

Side note 3: return 0;

...

>         ret = ad7124_soft_reset(st);
>         if (ret < 0)

> +               /* ad7124_soft_reset() already emitted an error message */

To me it looks like an almost useless comment.

>                 return ret;
>
>         ret = ad7124_check_chip_id(st);
>         if (ret)
> +               /* ad7124_check_chip_id() already emitted an error message */
>                 return ret;
>
>         ret = ad7124_setup(st);
>         if (ret < 0)
> +               /* ad7124_setup() already emitted an error message */
>                 return ret;

Ditto.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 09/10] iio: adc: ad7124: Add error reporting during probe

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1090 bytes --]

On Fri, Nov 22, 2024 at 10:25:43PM +0200, Andy Shevchenko wrote:
> On Fri, Nov 22, 2024 at 1:34 PM Uwe Kleine-König
> <u.kleine-koenig@baylibre.com> wrote:
> >
> > A driver that silently fails to probe is annoying and hard to debug. So
> > add messages in the error paths of the probe function.
> 
> ...
> 
> > +/* Only called during probe, so dev_err_probe() can be used */
> 
> It's a harmless comment, but I think dev_err_probe() name is good
> enough to give such a hint.

Seems to be subjective. I guess I found already too many functions using
dev_err_probe() that are called also after .probe().

> >         ret = ad7124_soft_reset(st);
> >         if (ret < 0)
> 
> > +               /* ad7124_soft_reset() already emitted an error message */
> 
> To me it looks like an almost useless comment.

Same as above. If one of the first thoughts when you look at this code
is: "Huh, no error message in this exit code", it helps.

I ignored your side notes for now as they wouldn't affect this patch. I
made a note for later however.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v3 10/10] iio: adc: ad7124: Implement temperature measurement

[Uwe Kleine-König]

If the maximal count of channels the driver supports isn't fully
utilized, add an attribute providing the internal temperature.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/iio/adc/ad7124.c | 112 +++++++++++++++++++++++++++++++--------
 1 file changed, 91 insertions(+), 21 deletions(-)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index fdbe2806bf11..3eff156b9c00 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -95,6 +95,10 @@
 #define AD7124_MAX_CONFIGS	8
 #define AD7124_MAX_CHANNELS	16
 
+/* AD7124 input sources */
+#define AD7124_INPUT_TEMPSENSOR	16
+#define AD7124_INPUT_AVSS	17
+
 enum ad7124_ids {
 	ID_AD7124_4,
 	ID_AD7124_8,
@@ -588,39 +592,75 @@ static int ad7124_read_raw(struct iio_dev *indio_dev,
 			return ret;
 
 		return IIO_VAL_INT;
+
 	case IIO_CHAN_INFO_SCALE:
-		mutex_lock(&st->cfgs_lock);
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			mutex_lock(&st->cfgs_lock);
 
-		idx = st->channels[chan->address].cfg.pga_bits;
-		*val = st->channels[chan->address].cfg.vref_mv;
-		if (st->channels[chan->address].cfg.bipolar)
-			*val2 = chan->scan_type.realbits - 1 + idx;
-		else
-			*val2 = chan->scan_type.realbits + idx;
+			idx = st->channels[chan->address].cfg.pga_bits;
+			*val = st->channels[chan->address].cfg.vref_mv;
+			if (st->channels[chan->address].cfg.bipolar)
+				*val2 = chan->scan_type.realbits - 1 + idx;
+			else
+				*val2 = chan->scan_type.realbits + idx;
+
+			mutex_unlock(&st->cfgs_lock);
+			return IIO_VAL_FRACTIONAL_LOG2;
+
+		case IIO_TEMP:
+			/*
+			 * According to the data sheet
+			 *   Temperature (°C)
+			 * = ((Conversion − 0x800000)/13584) − 272.5
+			 * = (Conversion − 0x800000 - 13584 * 272.5) / 13584
+			 * = (Conversion − 12090248) / 13584
+			 * So scale with 1000/13584 to yield °mC. Reduce by 8 to
+			 * 125/1698.
+			 */
+			*val = 125;
+			*val2 = 1698;
+			return IIO_VAL_FRACTIONAL;
+
+		default:
+			return -EINVAL;
+		}
 
-		mutex_unlock(&st->cfgs_lock);
-		return IIO_VAL_FRACTIONAL_LOG2;
 	case IIO_CHAN_INFO_OFFSET:
-		mutex_lock(&st->cfgs_lock);
-		if (st->channels[chan->address].cfg.bipolar)
-			*val = -(1 << (chan->scan_type.realbits - 1));
-		else
-			*val = 0;
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			mutex_lock(&st->cfgs_lock);
+			if (st->channels[chan->address].cfg.bipolar)
+				*val = -(1 << (chan->scan_type.realbits - 1));
+			else
+				*val = 0;
+
+			mutex_unlock(&st->cfgs_lock);
+			return IIO_VAL_INT;
+
+		case IIO_TEMP:
+			/* see calculation above */
+			*val = -12090248;
+			return IIO_VAL_INT;
+
+		default:
+			return -EINVAL;
+		}
 
-		mutex_unlock(&st->cfgs_lock);
-		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		mutex_lock(&st->cfgs_lock);
 		*val = st->channels[chan->address].cfg.odr;
 		mutex_unlock(&st->cfgs_lock);
 
 		return IIO_VAL_INT;
+
 	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
 		mutex_lock(&st->cfgs_lock);
 		*val = ad7124_get_3db_filter_freq(st, chan->scan_index);
 		mutex_unlock(&st->cfgs_lock);
 
 		return IIO_VAL_INT;
+
 	default:
 		return -EINVAL;
 	}
@@ -645,6 +685,7 @@ static int ad7124_write_raw(struct iio_dev *indio_dev,
 
 		ad7124_set_channel_odr(st, chan->address, val);
 		break;
+
 	case IIO_CHAN_INFO_SCALE:
 		if (val != 0) {
 			ret = -EINVAL;
@@ -666,6 +707,7 @@ static int ad7124_write_raw(struct iio_dev *indio_dev,
 
 		st->channels[chan->address].cfg.pga_bits = res;
 		break;
+
 	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
 		if (val2 != 0) {
 			ret = -EINVAL;
@@ -825,11 +867,10 @@ static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
 	struct ad7124_channel *channels;
 	struct iio_chan_spec *chan;
 	unsigned int ain[2], channel = 0, tmp;
+	unsigned int num_channels;
 	int ret;
 
-	st->num_channels = device_get_child_node_count(dev);
-	if (!st->num_channels)
-		return dev_err_probe(dev, -ENODEV, "no channel children\n");
+	num_channels = device_get_child_node_count(dev);
 
 	/*
 	 * The driver assigns each logical channel defined in the device tree
@@ -838,9 +879,12 @@ static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
 	 * CHANNEL_15) as an additional channel register. The driver could be
 	 * improved to lift this limitation.
 	 */
-	if (st->num_channels > AD7124_MAX_CHANNELS)
+	if (num_channels > AD7124_MAX_CHANNELS)
 		return dev_err_probe(dev, -EINVAL, "Too many channels defined\n");
 
+	/* Add one for temperature */
+	st->num_channels = min(num_channels + 1, AD7124_MAX_CHANNELS);
+
 	chan = devm_kcalloc(indio_dev->dev.parent, st->num_channels,
 			    sizeof(*chan), GFP_KERNEL);
 	if (!chan)
@@ -861,7 +905,7 @@ static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
 			return dev_err_probe(dev, ret,
 					     "Failed to parse reg property of %pfwP\n", child);
 
-		if (channel >= indio_dev->num_channels)
+		if (channel >= num_channels)
 			return dev_err_probe(dev, -EINVAL,
 					     "Channel index >= number of channels in %pfwP\n", child);
 
@@ -901,6 +945,32 @@ 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 = {
+				.sign = 'u',
+				.realbits = 24,
+				.storagebits = 32,
+				.endianness = IIO_BE,
+			},
+			.address = num_channels,
+			.scan_index = num_channels,
+		};
+	};
+
 	return 0;
 }
 
-- 
2.45.2

Re: [PATCH v3 10/10] iio: adc: ad7124: Implement temperature measurement

[David Lechner]

On 11/22/24 5:33 AM, Uwe Kleine-König wrote:
> If the maximal count of channels the driver supports isn't fully
> utilized, add an attribute providing the internal temperature.
> 

...

> @@ -901,6 +945,32 @@ 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 = {
> +				.sign = 'u',

The combination of .bipolar = true and .sign = 'u' looks a bit suspicions.

> +				.realbits = 24,
> +				.storagebits = 32,
> +				.endianness = IIO_BE,
> +			},
> +			.address = num_channels,
> +			.scan_index = num_channels,
> +		};
> +	};
> +
>  	return 0;
>  }
>  

Re: [PATCH v3 10/10] iio: adc: ad7124: Implement temperature measurement

[Andy Shevchenko]

On Fri, Nov 22, 2024 at 1:34 PM Uwe Kleine-König
<u.kleine-koenig@baylibre.com> wrote:
>
> If the maximal count of channels the driver supports isn't fully
> utilized, add an attribute providing the internal temperature.

...

>         case IIO_CHAN_INFO_SCALE:
> -               mutex_lock(&st->cfgs_lock);
> +               switch (chan->type) {
> +               case IIO_VOLTAGE:
> +                       mutex_lock(&st->cfgs_lock);

Side note 1: cleanup.h at some point?

...

>         case IIO_CHAN_INFO_OFFSET:
> -               mutex_lock(&st->cfgs_lock);
> -               if (st->channels[chan->address].cfg.bipolar)
> -                       *val = -(1 << (chan->scan_type.realbits - 1));
> -               else
> -                       *val = 0;
> +               switch (chan->type) {
> +               case IIO_VOLTAGE:
> +                       mutex_lock(&st->cfgs_lock);
> +                       if (st->channels[chan->address].cfg.bipolar)
> +                               *val = -(1 << (chan->scan_type.realbits - 1));

Side note 2: BIT() ?

...

>         case IIO_CHAN_INFO_SAMP_FREQ:
>                 mutex_lock(&st->cfgs_lock);
>                 *val = st->channels[chan->address].cfg.odr;
>                 mutex_unlock(&st->cfgs_lock);
>
>                 return IIO_VAL_INT;
> +
>         case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
>                 mutex_lock(&st->cfgs_lock);
>                 *val = ad7124_get_3db_filter_freq(st, chan->scan_index);
>                 mutex_unlock(&st->cfgs_lock);
>
>                 return IIO_VAL_INT;
> +

Seems like stray / unrelated changes. Do you really want to combine
this with style changing? Usually we either change style first
followed by featuring, or vice versa.

>         default:
>                 return -EINVAL;
>         }
> @@ -645,6 +685,7 @@ static int ad7124_write_raw(struct iio_dev *indio_dev,
>
>                 ad7124_set_channel_odr(st, chan->address, val);
>                 break;
> +
>         case IIO_CHAN_INFO_SCALE:
>                 if (val != 0) {
>                         ret = -EINVAL;
> @@ -666,6 +707,7 @@ static int ad7124_write_raw(struct iio_dev *indio_dev,
>
>                 st->channels[chan->address].cfg.pga_bits = res;
>                 break;
> +
>         case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
>                 if (val2 != 0) {
>                         ret = -EINVAL;

Ditto.

...

> +       /* Add one for temperature */
> +       st->num_channels = min(num_channels + 1, AD7124_MAX_CHANNELS);

Is the type of both arguments the same?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 10/10] iio: adc: ad7124: Implement temperature measurement

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 457 bytes --]

On Fri, Nov 22, 2024 at 10:31:07PM +0200, Andy Shevchenko wrote:
> On Fri, Nov 22, 2024 at 1:34 PM Uwe Kleine-König
> <u.kleine-koenig@baylibre.com> wrote:
> > +       /* Add one for temperature */
> > +       st->num_channels = min(num_channels + 1, AD7124_MAX_CHANNELS);
> 
> Is the type of both arguments the same?

Hmm, my compiler is happy with it at least. I don't understand why
though. I'll do a few more tests ...

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 10/10] iio: adc: ad7124: Implement temperature measurement

[Andy Shevchenko]

On Mon, Nov 25, 2024 at 1:27 PM Uwe Kleine-König
<u.kleine-koenig@baylibre.com> wrote:
> On Fri, Nov 22, 2024 at 10:31:07PM +0200, Andy Shevchenko wrote:
> > On Fri, Nov 22, 2024 at 1:34 PM Uwe Kleine-König
> > <u.kleine-koenig@baylibre.com> wrote:
> > > +       /* Add one for temperature */
> > > +       st->num_channels = min(num_channels + 1, AD7124_MAX_CHANNELS);
> >
> > Is the type of both arguments the same?
>
> Hmm, my compiler is happy with it at least. I don't understand why
> though. I'll do a few more tests ...

If num_channels is signed int or shorter than (independently on the
sign) int, then it's obvious why. + 1 makes it int.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 10/10] iio: adc: ad7124: Implement temperature measurement

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 852 bytes --]

On Mon, Nov 25, 2024 at 03:47:25PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 25, 2024 at 1:27 PM Uwe Kleine-König
> <u.kleine-koenig@baylibre.com> wrote:
> > On Fri, Nov 22, 2024 at 10:31:07PM +0200, Andy Shevchenko wrote:
> > > On Fri, Nov 22, 2024 at 1:34 PM Uwe Kleine-König
> > > <u.kleine-koenig@baylibre.com> wrote:
> > > > +       /* Add one for temperature */
> > > > +       st->num_channels = min(num_channels + 1, AD7124_MAX_CHANNELS);
> > >
> > > Is the type of both arguments the same?
> >
> > Hmm, my compiler is happy with it at least. I don't understand why
> > though. I'll do a few more tests ...
> 
> If num_channels is signed int or shorter than (independently on the
> sign) int, then it's obvious why. + 1 makes it int.

Ah indeed, I should have understood that without that explanation.
Thanks!

Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 10/10] iio: adc: ad7124: Implement temperature measurement

[Andy Shevchenko]

On Mon, Nov 25, 2024 at 4:52 PM Uwe Kleine-König
<u.kleine-koenig@baylibre.com> wrote:
> On Mon, Nov 25, 2024 at 03:47:25PM +0200, Andy Shevchenko wrote:
> > On Mon, Nov 25, 2024 at 1:27 PM Uwe Kleine-König
> > <u.kleine-koenig@baylibre.com> wrote:
> > > On Fri, Nov 22, 2024 at 10:31:07PM +0200, Andy Shevchenko wrote:
> > > > On Fri, Nov 22, 2024 at 1:34 PM Uwe Kleine-König
> > > > <u.kleine-koenig@baylibre.com> wrote:
> > > > > +       /* Add one for temperature */
> > > > > +       st->num_channels = min(num_channels + 1, AD7124_MAX_CHANNELS);
> > > >
> > > > Is the type of both arguments the same?
> > >
> > > Hmm, my compiler is happy with it at least. I don't understand why
> > > though. I'll do a few more tests ...
> >
> > If num_channels is signed int or shorter than (independently on the
> > sign) int, then it's obvious why. + 1 makes it int.
>
> Ah indeed, I should have understood that without that explanation.

Yeah, but a closer look shows to me that num_channels is unsigned int
or did I look in the wrong place? If that's true, that should make a
warning appear since AD7124_MAX_CHANNELS is signed int...


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 10/10] iio: adc: ad7124: Implement temperature measurement

[Nuno Sá]

On Mon, 2024-11-25 at 21:33 +0200, Andy Shevchenko wrote:
> On Mon, Nov 25, 2024 at 4:52 PM Uwe Kleine-König
> <u.kleine-koenig@baylibre.com> wrote:
> > On Mon, Nov 25, 2024 at 03:47:25PM +0200, Andy Shevchenko wrote:
> > > On Mon, Nov 25, 2024 at 1:27 PM Uwe Kleine-König
> > > <u.kleine-koenig@baylibre.com> wrote:
> > > > On Fri, Nov 22, 2024 at 10:31:07PM +0200, Andy Shevchenko wrote:
> > > > > On Fri, Nov 22, 2024 at 1:34 PM Uwe Kleine-König
> > > > > <u.kleine-koenig@baylibre.com> wrote:
> > > > > > +       /* Add one for temperature */
> > > > > > +       st->num_channels = min(num_channels + 1,
> > > > > > AD7124_MAX_CHANNELS);
> > > > > 
> > > > > Is the type of both arguments the same?
> > > > 
> > > > Hmm, my compiler is happy with it at least. I don't understand why
> > > > though. I'll do a few more tests ...
> > > 
> > > If num_channels is signed int or shorter than (independently on the
> > > sign) int, then it's obvious why. + 1 makes it int.
> > 
> > Ah indeed, I should have understood that without that explanation.
> 
> Yeah, but a closer look shows to me that num_channels is unsigned int
> or did I look in the wrong place? If that's true, that should make a
> warning appear since AD7124_MAX_CHANNELS is signed int...
> 
> 

Hmm,

Weren't the min()/max() macros improved for things like this?

https://elixir.bootlin.com/linux/v6.12.1/source/include/linux/minmax.h#L22

- Nuno Sá

Re: [PATCH v3 10/10] iio: adc: ad7124: Implement temperature measurement

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1483 bytes --]

On Mon, Nov 25, 2024 at 09:33:12PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 25, 2024 at 4:52 PM Uwe Kleine-König
> <u.kleine-koenig@baylibre.com> wrote:
> > On Mon, Nov 25, 2024 at 03:47:25PM +0200, Andy Shevchenko wrote:
> > > On Mon, Nov 25, 2024 at 1:27 PM Uwe Kleine-König
> > > <u.kleine-koenig@baylibre.com> wrote:
> > > > On Fri, Nov 22, 2024 at 10:31:07PM +0200, Andy Shevchenko wrote:
> > > > > On Fri, Nov 22, 2024 at 1:34 PM Uwe Kleine-König
> > > > > <u.kleine-koenig@baylibre.com> wrote:
> > > > > > +       /* Add one for temperature */
> > > > > > +       st->num_channels = min(num_channels + 1, AD7124_MAX_CHANNELS);
> > > > >
> > > > > Is the type of both arguments the same?
> > > >
> > > > Hmm, my compiler is happy with it at least. I don't understand why
> > > > though. I'll do a few more tests ...
> > >
> > > If num_channels is signed int or shorter than (independently on the
> > > sign) int, then it's obvious why. + 1 makes it int.
> >
> > Ah indeed, I should have understood that without that explanation.
> 
> Yeah, but a closer look shows to me that num_channels is unsigned int
> or did I look in the wrong place? If that's true, that should make a
> warning appear since AD7124_MAX_CHANNELS is signed int...

The ideas in the definition of min are a bit hard to follow, but IIUC it
doesn't warn because AD7124_MAX_CHANNELS is non-negative and so there is
no danger for misinterpretation.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]