linux-iio.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2] iio: dac: adi-axi-dac: fix for wrong bus read
@ 2025-04-09  9:16 Angelo Dureghello
  2025-04-09  9:16 ` [PATCH v2 1/2] iio: dac: adi-axi-dac: fix " Angelo Dureghello
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Angelo Dureghello @ 2025-04-09  9:16 UTC (permalink / raw)
  To: Nuno Sa, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	David Lechner, Andy Shevchenko
  Cc: linux-iio, linux-kernel, Jonathan Cameron, Angelo Dureghello

This patchset is intended to fix a random wrong chip ID read, or a
scratchpad test mismatch, tests done in the ad3552r-hs driver probe. The 
bus "read" operation must always check for busy flag before reading.

First patch reorganizes a bit the busy-wait polling code, second patch
fixes the wrong bus read occurence. 

NOTE: due to ongoing changes in adi-axi-dac.c, this patch is intended to be
applied after the linked "ramp generator" patch.

Link: https://lore.kernel.org/linux-iio/20250408-wip-bl-ad3552r-fixes-v4-0-b33c0264bd78@baylibre.com
Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
---
Changes in v2:
- invert patch order, fix first.
- Link to v1: https://lore.kernel.org/r/20250408-ad3552r-fix-bus-read-v1-0-37add66aeb08@baylibre.com

---
Angelo Dureghello (2):
      iio: dac: adi-axi-dac: fix bus read
      iio: dac: adi-axi-dac: use unique bus free check

 drivers/iio/dac/adi-axi-dac.c | 40 +++++++++++++++++++++++++---------------
 1 file changed, 25 insertions(+), 15 deletions(-)
---
base-commit: 6fb85f14853ddde06d57030c753168402bf69cd9
change-id: 20250408-ad3552r-fix-bus-read-1522622fbd2b

Best regards,
-- 
Angelo Dureghello <adureghello@baylibre.com>


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 1/2] iio: dac: adi-axi-dac: fix bus read
  2025-04-09  9:16 [PATCH v2 0/2] iio: dac: adi-axi-dac: fix for wrong bus read Angelo Dureghello
@ 2025-04-09  9:16 ` Angelo Dureghello
  2025-04-09 16:49   ` Andy Shevchenko
  2025-04-09  9:16 ` [PATCH v2 2/2] iio: dac: adi-axi-dac: use unique bus free check Angelo Dureghello
  2025-04-09 13:57 ` [PATCH v2 0/2] iio: dac: adi-axi-dac: fix for wrong bus read Nuno Sá
  2 siblings, 1 reply; 15+ messages in thread
From: Angelo Dureghello @ 2025-04-09  9:16 UTC (permalink / raw)
  To: Nuno Sa, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	David Lechner, Andy Shevchenko
  Cc: linux-iio, linux-kernel, Jonathan Cameron, Angelo Dureghello

From: Angelo Dureghello <adureghello@baylibre.com>

Fix bus read function.

Testing the driver, on a random basis, wrong reads was detected, mainly
by a wrong DAC chip ID read at first boot.
Before reading the expected value from the AXI regmap, need always to
wait for busy flag to be cleared.

Fixes: e61d7178429a ("iio: dac: adi-axi-dac: extend features")
Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
---
 drivers/iio/dac/adi-axi-dac.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c
index 8ed5ad1fa24cef649056bc5f4ca80abbf28b9323..5ee077c58d7f9730aec8a9c9dff5b84108b3a47e 100644
--- a/drivers/iio/dac/adi-axi-dac.c
+++ b/drivers/iio/dac/adi-axi-dac.c
@@ -760,6 +760,7 @@ static int axi_dac_bus_reg_read(struct iio_backend *back, u32 reg, u32 *val,
 {
 	struct axi_dac_state *st = iio_backend_get_priv(back);
 	int ret;
+	u32 ival;
 
 	guard(mutex)(&st->lock);
 
@@ -772,6 +773,13 @@ static int axi_dac_bus_reg_read(struct iio_backend *back, u32 reg, u32 *val,
 	if (ret)
 		return ret;
 
+	ret = regmap_read_poll_timeout(st->regmap,
+				AXI_DAC_UI_STATUS_REG, ival,
+				FIELD_GET(AXI_DAC_UI_STATUS_IF_BUSY, ival) == 0,
+				10, 100 * KILO);
+	if (ret)
+		return ret;
+
 	return regmap_read(st->regmap, AXI_DAC_CUSTOM_RD_REG, val);
 }
 

-- 
2.49.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 2/2] iio: dac: adi-axi-dac: use unique bus free check
  2025-04-09  9:16 [PATCH v2 0/2] iio: dac: adi-axi-dac: fix for wrong bus read Angelo Dureghello
  2025-04-09  9:16 ` [PATCH v2 1/2] iio: dac: adi-axi-dac: fix " Angelo Dureghello
@ 2025-04-09  9:16 ` Angelo Dureghello
  2025-04-09 16:51   ` Andy Shevchenko
  2025-04-09 13:57 ` [PATCH v2 0/2] iio: dac: adi-axi-dac: fix for wrong bus read Nuno Sá
  2 siblings, 1 reply; 15+ messages in thread
From: Angelo Dureghello @ 2025-04-09  9:16 UTC (permalink / raw)
  To: Nuno Sa, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	David Lechner, Andy Shevchenko
  Cc: linux-iio, linux-kernel, Jonathan Cameron, Angelo Dureghello

From: Angelo Dureghello <adureghello@baylibre.com>

Use a unique function for the bus free check by polling, to reduce
duplicated code. An error is always thrown in case of timeout.

Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
---
 drivers/iio/dac/adi-axi-dac.c | 42 ++++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c
index 5ee077c58d7f9730aec8a9c9dff5b84108b3a47e..c90068693e9989a49e4c035eecb69606bdcbb196 100644
--- a/drivers/iio/dac/adi-axi-dac.c
+++ b/drivers/iio/dac/adi-axi-dac.c
@@ -635,15 +635,26 @@ static int axi_dac_ddr_disable(struct iio_backend *back)
 			       AXI_DAC_CNTRL_2_SDR_DDR_N);
 }
 
+static int axi_dac_wait_bus_free(struct axi_dac_state *st)
+{
+	u32 val;
+	int ret;
+
+	ret = regmap_read_poll_timeout(st->regmap, AXI_DAC_UI_STATUS_REG, val,
+		FIELD_GET(AXI_DAC_UI_STATUS_IF_BUSY, val) == -1, 10,
+		100 * KILO);
+	if (ret == -ETIMEDOUT)
+		dev_err(st->dev, "AXI bus timeout\n");
+
+	return ret;
+}
+
 static int axi_dac_data_stream_enable(struct iio_backend *back)
 {
 	struct axi_dac_state *st = iio_backend_get_priv(back);
-	int ret, val;
+	int ret;
 
-	ret = regmap_read_poll_timeout(st->regmap,
-				AXI_DAC_UI_STATUS_REG, val,
-				FIELD_GET(AXI_DAC_UI_STATUS_IF_BUSY, val) == 0,
-				10, 100 * KILO);
+	ret = axi_dac_wait_bus_free(st);
 	if (ret)
 		return ret;
 
@@ -734,12 +745,9 @@ static int __axi_dac_bus_reg_write(struct iio_backend *back, u32 reg,
 	if (ret)
 		return ret;
 
-	ret = regmap_read_poll_timeout(st->regmap,
-				AXI_DAC_UI_STATUS_REG, ival,
-				FIELD_GET(AXI_DAC_UI_STATUS_IF_BUSY, ival) == 0,
-				10, 100 * KILO);
-	if (ret == -ETIMEDOUT)
-		dev_err(st->dev, "AXI read timeout\n");
+	ret = axi_dac_wait_bus_free(st);
+	if (ret)
+		return ret;
 
 	/* Cleaning always AXI_DAC_CUSTOM_CTRL_TRANSFER_DATA */
 	return regmap_clear_bits(st->regmap, AXI_DAC_CUSTOM_CTRL_REG,
@@ -760,7 +768,6 @@ static int axi_dac_bus_reg_read(struct iio_backend *back, u32 reg, u32 *val,
 {
 	struct axi_dac_state *st = iio_backend_get_priv(back);
 	int ret;
-	u32 ival;
 
 	guard(mutex)(&st->lock);
 
@@ -773,10 +780,7 @@ static int axi_dac_bus_reg_read(struct iio_backend *back, u32 reg, u32 *val,
 	if (ret)
 		return ret;
 
-	ret = regmap_read_poll_timeout(st->regmap,
-				AXI_DAC_UI_STATUS_REG, ival,
-				FIELD_GET(AXI_DAC_UI_STATUS_IF_BUSY, ival) == 0,
-				10, 100 * KILO);
+	ret = axi_dac_wait_bus_free(st);
 	if (ret)
 		return ret;
 
@@ -787,7 +791,7 @@ static int axi_dac_bus_set_io_mode(struct iio_backend *back,
 				   enum ad3552r_io_mode mode)
 {
 	struct axi_dac_state *st = iio_backend_get_priv(back);
-	int ival, ret;
+	int ret;
 
 	if (mode > AD3552R_IO_MODE_QSPI)
 		return -EINVAL;
@@ -800,9 +804,7 @@ static int axi_dac_bus_set_io_mode(struct iio_backend *back,
 	if (ret)
 		return ret;
 
-	return regmap_read_poll_timeout(st->regmap, AXI_DAC_UI_STATUS_REG, ival,
-			FIELD_GET(AXI_DAC_UI_STATUS_IF_BUSY, ival) == 0, 10,
-			100 * KILO);
+	return axi_dac_wait_bus_free(st);
 }
 
 static void axi_dac_child_remove(void *data)

-- 
2.49.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 0/2] iio: dac: adi-axi-dac: fix for wrong bus read
  2025-04-09  9:16 [PATCH v2 0/2] iio: dac: adi-axi-dac: fix for wrong bus read Angelo Dureghello
  2025-04-09  9:16 ` [PATCH v2 1/2] iio: dac: adi-axi-dac: fix " Angelo Dureghello
  2025-04-09  9:16 ` [PATCH v2 2/2] iio: dac: adi-axi-dac: use unique bus free check Angelo Dureghello
@ 2025-04-09 13:57 ` Nuno Sá
  2025-04-22  8:41   ` Angelo Dureghello
  2025-05-05 11:33   ` Angelo Dureghello
  2 siblings, 2 replies; 15+ messages in thread
From: Nuno Sá @ 2025-04-09 13:57 UTC (permalink / raw)
  To: Angelo Dureghello, Nuno Sa, Lars-Peter Clausen, Michael Hennerich,
	Jonathan Cameron, David Lechner, Andy Shevchenko
  Cc: linux-iio, linux-kernel, Jonathan Cameron

On Wed, 2025-04-09 at 11:16 +0200, Angelo Dureghello wrote:
> This patchset is intended to fix a random wrong chip ID read, or a
> scratchpad test mismatch, tests done in the ad3552r-hs driver probe. The 
> bus "read" operation must always check for busy flag before reading.
> 
> First patch reorganizes a bit the busy-wait polling code, second patch
> fixes the wrong bus read occurence. 
> 
> NOTE: due to ongoing changes in adi-axi-dac.c, this patch is intended to be
> applied after the linked "ramp generator" patch.
> 
> Link:
> https://lore.kernel.org/linux-iio/20250408-wip-bl-ad3552r-fixes-v4-0-b33c0264bd78@baylibre.com
> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> ---

LGTM,

Reviewed-by: Nuno Sá <nuno.sa@analog.com>

> Changes in v2:
> - invert patch order, fix first.
> - Link to v1:
> https://lore.kernel.org/r/20250408-ad3552r-fix-bus-read-v1-0-37add66aeb08@baylibre.com
> 
> ---
> Angelo Dureghello (2):
>       iio: dac: adi-axi-dac: fix bus read
>       iio: dac: adi-axi-dac: use unique bus free check
> 
>  drivers/iio/dac/adi-axi-dac.c | 40 +++++++++++++++++++++++++---------------
>  1 file changed, 25 insertions(+), 15 deletions(-)
> ---
> base-commit: 6fb85f14853ddde06d57030c753168402bf69cd9
> change-id: 20250408-ad3552r-fix-bus-read-1522622fbd2b
> 
> Best regards,

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/2] iio: dac: adi-axi-dac: fix bus read
  2025-04-09  9:16 ` [PATCH v2 1/2] iio: dac: adi-axi-dac: fix " Angelo Dureghello
@ 2025-04-09 16:49   ` Andy Shevchenko
  2025-04-12 13:00     ` Jonathan Cameron
  0 siblings, 1 reply; 15+ messages in thread
From: Andy Shevchenko @ 2025-04-09 16:49 UTC (permalink / raw)
  To: Angelo Dureghello
  Cc: Nuno Sa, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	David Lechner, linux-iio, linux-kernel, Jonathan Cameron

On Wed, Apr 09, 2025 at 11:16:54AM +0200, Angelo Dureghello wrote:
> From: Angelo Dureghello <adureghello@baylibre.com>
> 
> Fix bus read function.
> 
> Testing the driver, on a random basis, wrong reads was detected, mainly
> by a wrong DAC chip ID read at first boot.
> Before reading the expected value from the AXI regmap, need always to
> wait for busy flag to be cleared.

...

> +	ret = regmap_read_poll_timeout(st->regmap,
> +				AXI_DAC_UI_STATUS_REG, ival,
> +				FIELD_GET(AXI_DAC_UI_STATUS_IF_BUSY, ival) == 0,
> +				10, 100 * KILO);

It's timeout, we have special constants for that, I believe you wanted to have
USEC_PER_MSEC here.

> +	if (ret)
> +		return ret;

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 2/2] iio: dac: adi-axi-dac: use unique bus free check
  2025-04-09  9:16 ` [PATCH v2 2/2] iio: dac: adi-axi-dac: use unique bus free check Angelo Dureghello
@ 2025-04-09 16:51   ` Andy Shevchenko
  2025-04-29 14:32     ` Angelo Dureghello
  0 siblings, 1 reply; 15+ messages in thread
From: Andy Shevchenko @ 2025-04-09 16:51 UTC (permalink / raw)
  To: Angelo Dureghello
  Cc: Nuno Sa, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	David Lechner, linux-iio, linux-kernel, Jonathan Cameron

On Wed, Apr 09, 2025 at 11:16:55AM +0200, Angelo Dureghello wrote:
> From: Angelo Dureghello <adureghello@baylibre.com>
> 
> Use a unique function for the bus free check by polling, to reduce
> duplicated code. An error is always thrown in case of timeout.

...

> +static int axi_dac_wait_bus_free(struct axi_dac_state *st)
> +{
> +	u32 val;
> +	int ret;
> +
> +	ret = regmap_read_poll_timeout(st->regmap, AXI_DAC_UI_STATUS_REG, val,
> +		FIELD_GET(AXI_DAC_UI_STATUS_IF_BUSY, val) == -1, 10,
> +		100 * KILO);

Same comment as in the previous patch. Okay, it seems more than in the single
case. Perhaps to change that as well here?

> +	if (ret == -ETIMEDOUT)
> +		dev_err(st->dev, "AXI bus timeout\n");

Why do you need this? The error code will go to the user space at the end? If
yes, it will be enough to have it printed there, no?

> +	return ret;
> +}


-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/2] iio: dac: adi-axi-dac: fix bus read
  2025-04-09 16:49   ` Andy Shevchenko
@ 2025-04-12 13:00     ` Jonathan Cameron
  2025-04-12 18:03       ` Andy Shevchenko
  0 siblings, 1 reply; 15+ messages in thread
From: Jonathan Cameron @ 2025-04-12 13:00 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Angelo Dureghello, Nuno Sa, Lars-Peter Clausen, Michael Hennerich,
	David Lechner, linux-iio, linux-kernel, Jonathan Cameron

On Wed, 9 Apr 2025 19:49:25 +0300
Andy Shevchenko <andy@kernel.org> wrote:

> On Wed, Apr 09, 2025 at 11:16:54AM +0200, Angelo Dureghello wrote:
> > From: Angelo Dureghello <adureghello@baylibre.com>
> > 
> > Fix bus read function.
> > 
> > Testing the driver, on a random basis, wrong reads was detected, mainly
> > by a wrong DAC chip ID read at first boot.
> > Before reading the expected value from the AXI regmap, need always to
> > wait for busy flag to be cleared.  
> 
> ...
> 
> > +	ret = regmap_read_poll_timeout(st->regmap,
> > +				AXI_DAC_UI_STATUS_REG, ival,
> > +				FIELD_GET(AXI_DAC_UI_STATUS_IF_BUSY, ival) == 0,
> > +				10, 100 * KILO);  
> 
> It's timeout, we have special constants for that, I believe you wanted to have
> USEC_PER_MSEC here.

This is an odd corner case.  If we had a define for that 100 along the lines
of X_TIMEOUT_MSEC then I'd agree that using USEC_PER_MSEC makes complete sense.
All we have is a bare number which has no defined units.  I'd just go with 100000 and
not use the units.h defines at all.  They make sense when lots of zeros are involved
or for standard conversions, but to me not worth it here.

Jonathan

> 
> > +	if (ret)
> > +		return ret;  
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/2] iio: dac: adi-axi-dac: fix bus read
  2025-04-12 13:00     ` Jonathan Cameron
@ 2025-04-12 18:03       ` Andy Shevchenko
  0 siblings, 0 replies; 15+ messages in thread
From: Andy Shevchenko @ 2025-04-12 18:03 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Andy Shevchenko, Angelo Dureghello, Nuno Sa, Lars-Peter Clausen,
	Michael Hennerich, David Lechner, linux-iio, linux-kernel,
	Jonathan Cameron

On Sat, Apr 12, 2025 at 4:00 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Wed, 9 Apr 2025 19:49:25 +0300
> Andy Shevchenko <andy@kernel.org> wrote:
> > On Wed, Apr 09, 2025 at 11:16:54AM +0200, Angelo Dureghello wrote:

...

> > > +   ret = regmap_read_poll_timeout(st->regmap,
> > > +                           AXI_DAC_UI_STATUS_REG, ival,
> > > +                           FIELD_GET(AXI_DAC_UI_STATUS_IF_BUSY, ival) == 0,
> > > +                           10, 100 * KILO);
> >
> > It's timeout, we have special constants for that, I believe you wanted to have
> > USEC_PER_MSEC here.
>
> This is an odd corner case.  If we had a define for that 100 along the lines
> of X_TIMEOUT_MSEC then I'd agree that using USEC_PER_MSEC makes complete sense.
> All we have is a bare number which has no defined units.  I'd just go with 100000 and
> not use the units.h defines at all.  They make sense when lots of zeros are involved
> or for standard conversions, but to me not worth it here.

I still think it's slightly better for at least two reasons:
1) the named constant makes it easier to understand the units (esp.
for the unprepared reader who doesn't know well the kernel internal
APIs);
2) educational and similar cases when somebody will copy'n'paste
(okay, copy, paste, and modify) this with thinking that this is a good
practice, which may lead to bugs when it will be more zeroes that easy
to miscount.

I prefer the constant.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 0/2] iio: dac: adi-axi-dac: fix for wrong bus read
  2025-04-09 13:57 ` [PATCH v2 0/2] iio: dac: adi-axi-dac: fix for wrong bus read Nuno Sá
@ 2025-04-22  8:41   ` Angelo Dureghello
  2025-05-05 11:33   ` Angelo Dureghello
  1 sibling, 0 replies; 15+ messages in thread
From: Angelo Dureghello @ 2025-04-22  8:41 UTC (permalink / raw)
  To: Nuno Sá
  Cc: Nuno Sa, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	David Lechner, Andy Shevchenko, linux-iio, linux-kernel,
	Jonathan Cameron

Hi all,

please let me know if you want me to use defines for this patchset,
i don't see the need honestly, but eventually it can be done in another
separate patch, for the whole adi-axi-dac file.

On 09.04.2025 14:57, Nuno Sá wrote:
> On Wed, 2025-04-09 at 11:16 +0200, Angelo Dureghello wrote:
> > This patchset is intended to fix a random wrong chip ID read, or a
> > scratchpad test mismatch, tests done in the ad3552r-hs driver probe. The 
> > bus "read" operation must always check for busy flag before reading.
> > 
> > First patch reorganizes a bit the busy-wait polling code, second patch
> > fixes the wrong bus read occurence. 
> > 
> > NOTE: due to ongoing changes in adi-axi-dac.c, this patch is intended to be
> > applied after the linked "ramp generator" patch.
> > 
> > Link:
> > https://lore.kernel.org/linux-iio/20250408-wip-bl-ad3552r-fixes-v4-0-b33c0264bd78@baylibre.com
> > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> > ---
> 
> LGTM,
> 
> Reviewed-by: Nuno Sá <nuno.sa@analog.com>
> 
> > Changes in v2:
> > - invert patch order, fix first.
> > - Link to v1:
> > https://lore.kernel.org/r/20250408-ad3552r-fix-bus-read-v1-0-37add66aeb08@baylibre.com
> > 
> > ---
> > Angelo Dureghello (2):
> >       iio: dac: adi-axi-dac: fix bus read
> >       iio: dac: adi-axi-dac: use unique bus free check
> > 
> >  drivers/iio/dac/adi-axi-dac.c | 40 +++++++++++++++++++++++++---------------
> >  1 file changed, 25 insertions(+), 15 deletions(-)
> > ---
> > base-commit: 6fb85f14853ddde06d57030c753168402bf69cd9
> > change-id: 20250408-ad3552r-fix-bus-read-1522622fbd2b
> > 
> > Best regards,

Regards,
angelo

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 2/2] iio: dac: adi-axi-dac: use unique bus free check
  2025-04-09 16:51   ` Andy Shevchenko
@ 2025-04-29 14:32     ` Angelo Dureghello
  2025-04-29 22:05       ` Andy Shevchenko
  0 siblings, 1 reply; 15+ messages in thread
From: Angelo Dureghello @ 2025-04-29 14:32 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Nuno Sa, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	David Lechner, linux-iio, linux-kernel, Jonathan Cameron

Hi Andy,

sorry, seen this message now only, for some reason sometimes your emails
goes to the spam. Now i marked all as "not spam", let's see.

On 09.04.2025 19:51, Andy Shevchenko wrote:
> On Wed, Apr 09, 2025 at 11:16:55AM +0200, Angelo Dureghello wrote:
> > From: Angelo Dureghello <adureghello@baylibre.com>
> > 
> > Use a unique function for the bus free check by polling, to reduce
> > duplicated code. An error is always thrown in case of timeout.
> 
> ...
> 
> > +static int axi_dac_wait_bus_free(struct axi_dac_state *st)
> > +{
> > +	u32 val;
> > +	int ret;
> > +
> > +	ret = regmap_read_poll_timeout(st->regmap, AXI_DAC_UI_STATUS_REG, val,
> > +		FIELD_GET(AXI_DAC_UI_STATUS_IF_BUSY, val) == -1, 10,
> > +		100 * KILO);
> 
> Same comment as in the previous patch. Okay, it seems more than in the single
> case. Perhaps to change that as well here?
> 

for my personal taste would not use more specific named defines here,
would not change this, in case we can send a separate patch to fix
them all. 

> > +	if (ret == -ETIMEDOUT)
> > +		dev_err(st->dev, "AXI bus timeout\n");
> 
> Why do you need this? The error code will go to the user space at the end? If
> yes, it will be enough to have it printed there, no?
> 

This warning means something very bad happen at AXI level. I never seen
this warning issued, but it may help to debug AXI/HDL issues, would not 
remove it. 

> > +	return ret;
> > +}
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 
Regards,
angelo

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 2/2] iio: dac: adi-axi-dac: use unique bus free check
  2025-04-29 14:32     ` Angelo Dureghello
@ 2025-04-29 22:05       ` Andy Shevchenko
  2025-04-30 14:15         ` Angelo Dureghello
  0 siblings, 1 reply; 15+ messages in thread
From: Andy Shevchenko @ 2025-04-29 22:05 UTC (permalink / raw)
  To: Angelo Dureghello
  Cc: Andy Shevchenko, Nuno Sa, Lars-Peter Clausen, Michael Hennerich,
	Jonathan Cameron, David Lechner, linux-iio, linux-kernel,
	Jonathan Cameron

On Tue, Apr 29, 2025 at 5:34 PM Angelo Dureghello
<adureghello@baylibre.com> wrote:
> On 09.04.2025 19:51, Andy Shevchenko wrote:
> > On Wed, Apr 09, 2025 at 11:16:55AM +0200, Angelo Dureghello wrote:

...

> > > +   if (ret == -ETIMEDOUT)
> > > +           dev_err(st->dev, "AXI bus timeout\n");
> >
> > Why do you need this? The error code will go to the user space at the end? If
> > yes, it will be enough to have it printed there, no?
> >
>
> This warning means something very bad happen at AXI level. I never seen
> this warning issued, but it may help to debug AXI/HDL issues, would not
> remove it.

But wouldn't user space get the error code and translate to a message if needed?

> > > +   return ret;

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 2/2] iio: dac: adi-axi-dac: use unique bus free check
  2025-04-29 22:05       ` Andy Shevchenko
@ 2025-04-30 14:15         ` Angelo Dureghello
  0 siblings, 0 replies; 15+ messages in thread
From: Angelo Dureghello @ 2025-04-30 14:15 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Andy Shevchenko, Nuno Sa, Lars-Peter Clausen, Michael Hennerich,
	Jonathan Cameron, David Lechner, linux-iio, linux-kernel,
	Jonathan Cameron

Hi Andy,

On 30.04.2025 01:05, Andy Shevchenko wrote:
> On Tue, Apr 29, 2025 at 5:34 PM Angelo Dureghello
> <adureghello@baylibre.com> wrote:
> > On 09.04.2025 19:51, Andy Shevchenko wrote:
> > > On Wed, Apr 09, 2025 at 11:16:55AM +0200, Angelo Dureghello wrote:
> 
> ...
> 
> > > > +   if (ret == -ETIMEDOUT)
> > > > +           dev_err(st->dev, "AXI bus timeout\n");
> > >
> > > Why do you need this? The error code will go to the user space at the end? If
> > > yes, it will be enough to have it printed there, no?
> > >
> >
> > This warning means something very bad happen at AXI level. I never seen
> > this warning issued, but it may help to debug AXI/HDL issues, would not
> > remove it.
> 
> But wouldn't user space get the error code and translate to a message if needed?
> 

depends, bus access is done also at probe level, you would get a generic
-ETIMEOUT, then you need to put traces to understand who is causing it.
I think the message may be useful to check issues, like a buggy HDL.
Anyway, would not re-issue another patch just for this.


Regards,
angelo

> > > > +   return ret;
> 
> -- 
> With Best Regards,
> Andy Shevchenko

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 0/2] iio: dac: adi-axi-dac: fix for wrong bus read
  2025-04-09 13:57 ` [PATCH v2 0/2] iio: dac: adi-axi-dac: fix for wrong bus read Nuno Sá
  2025-04-22  8:41   ` Angelo Dureghello
@ 2025-05-05 11:33   ` Angelo Dureghello
  2025-05-05 12:38     ` Jonathan Cameron
  1 sibling, 1 reply; 15+ messages in thread
From: Angelo Dureghello @ 2025-05-05 11:33 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Nuno Sa, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	David Lechner, Andy Shevchenko, linux-iio, linux-kernel

Hi Jonathan,

asking info about this patch.

As explained, would not do anything else here, please let me know if it can
be accepted or how to proceed.

Thanks a lot,
angelo

On 09.04.2025 14:57, Nuno Sá wrote:
> On Wed, 2025-04-09 at 11:16 +0200, Angelo Dureghello wrote:
> > This patchset is intended to fix a random wrong chip ID read, or a
> > scratchpad test mismatch, tests done in the ad3552r-hs driver probe. The 
> > bus "read" operation must always check for busy flag before reading.
> > 
> > First patch reorganizes a bit the busy-wait polling code, second patch
> > fixes the wrong bus read occurence. 
> > 
> > NOTE: due to ongoing changes in adi-axi-dac.c, this patch is intended to be
> > applied after the linked "ramp generator" patch.
> > 
> > Link:
> > https://lore.kernel.org/linux-iio/20250408-wip-bl-ad3552r-fixes-v4-0-b33c0264bd78@baylibre.com
> > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> > ---
> 
> LGTM,
> 
> Reviewed-by: Nuno Sá <nuno.sa@analog.com>
> 
> > Changes in v2:
> > - invert patch order, fix first.
> > - Link to v1:
> > https://lore.kernel.org/r/20250408-ad3552r-fix-bus-read-v1-0-37add66aeb08@baylibre.com
> > 
> > ---
> > Angelo Dureghello (2):
> >       iio: dac: adi-axi-dac: fix bus read
> >       iio: dac: adi-axi-dac: use unique bus free check
> > 
> >  drivers/iio/dac/adi-axi-dac.c | 40 +++++++++++++++++++++++++---------------
> >  1 file changed, 25 insertions(+), 15 deletions(-)
> > ---
> > base-commit: 6fb85f14853ddde06d57030c753168402bf69cd9
> > change-id: 20250408-ad3552r-fix-bus-read-1522622fbd2b
> > 
> > Best regards,

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 0/2] iio: dac: adi-axi-dac: fix for wrong bus read
  2025-05-05 11:33   ` Angelo Dureghello
@ 2025-05-05 12:38     ` Jonathan Cameron
  2025-05-19 19:03       ` Angelo Dureghello
  0 siblings, 1 reply; 15+ messages in thread
From: Jonathan Cameron @ 2025-05-05 12:38 UTC (permalink / raw)
  To: Angelo Dureghello
  Cc: Jonathan Cameron, Nuno Sa, Lars-Peter Clausen, Michael Hennerich,
	David Lechner, Andy Shevchenko, linux-iio, linux-kernel

On Mon, 5 May 2025 13:33:16 +0200
Angelo Dureghello <adureghello@baylibre.com> wrote:

> Hi Jonathan,
> 
> asking info about this patch.
> 
> As explained, would not do anything else here, please let me know if it can
> be accepted or how to proceed.
I've queued up patch 1 as a fix.  Patch 2 will need to wait on that.

> 
> Thanks a lot,
> angelo
> 
> On 09.04.2025 14:57, Nuno Sá wrote:
> > On Wed, 2025-04-09 at 11:16 +0200, Angelo Dureghello wrote:  
> > > This patchset is intended to fix a random wrong chip ID read, or a
> > > scratchpad test mismatch, tests done in the ad3552r-hs driver probe. The 
> > > bus "read" operation must always check for busy flag before reading.
> > > 
> > > First patch reorganizes a bit the busy-wait polling code, second patch
> > > fixes the wrong bus read occurence. 
> > > 
> > > NOTE: due to ongoing changes in adi-axi-dac.c, this patch is intended to be
> > > applied after the linked "ramp generator" patch.
> > > 
> > > Link:
> > > https://lore.kernel.org/linux-iio/20250408-wip-bl-ad3552r-fixes-v4-0-b33c0264bd78@baylibre.com
> > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> > > ---  
> > 
> > LGTM,
> > 
> > Reviewed-by: Nuno Sá <nuno.sa@analog.com>
> >   
> > > Changes in v2:
> > > - invert patch order, fix first.
> > > - Link to v1:
> > > https://lore.kernel.org/r/20250408-ad3552r-fix-bus-read-v1-0-37add66aeb08@baylibre.com
> > > 
> > > ---
> > > Angelo Dureghello (2):
> > >       iio: dac: adi-axi-dac: fix bus read
> > >       iio: dac: adi-axi-dac: use unique bus free check
> > > 
> > >  drivers/iio/dac/adi-axi-dac.c | 40 +++++++++++++++++++++++++---------------
> > >  1 file changed, 25 insertions(+), 15 deletions(-)
> > > ---
> > > base-commit: 6fb85f14853ddde06d57030c753168402bf69cd9
> > > change-id: 20250408-ad3552r-fix-bus-read-1522622fbd2b
> > > 
> > > Best regards,  
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 0/2] iio: dac: adi-axi-dac: fix for wrong bus read
  2025-05-05 12:38     ` Jonathan Cameron
@ 2025-05-19 19:03       ` Angelo Dureghello
  0 siblings, 0 replies; 15+ messages in thread
From: Angelo Dureghello @ 2025-05-19 19:03 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Jonathan Cameron, Nuno Sa, Lars-Peter Clausen, Michael Hennerich,
	David Lechner, Andy Shevchenko, linux-iio, linux-kernel

Hi Jonathan,

i did a mistake on the second patch 2/2.

in axi_dac_wait_bus_free()

	ret = regmap_read_poll_timeout(st->regmap, AXI_DAC_UI_STATUS_REG, val,
		FIELD_GET(AXI_DAC_UI_STATUS_IF_BUSY, val) == -1, 10,

must be

	ret = regmap_read_poll_timeout(st->regmap, AXI_DAC_UI_STATUS_REG, val,
		FIELD_GET(AXI_DAC_UI_STATUS_IF_BUSY, val) == 0, 10,

Not clear what's happen, i missed something while testing.

I apologize, not sure if it is already queued, please let me know
how to fix it.

Regards,
angelo


On 05.05.2025 13:38, Jonathan Cameron wrote:
> On Mon, 5 May 2025 13:33:16 +0200
> Angelo Dureghello <adureghello@baylibre.com> wrote:
> 
> > Hi Jonathan,
> > 
> > asking info about this patch.
> > 
> > As explained, would not do anything else here, please let me know if it can
> > be accepted or how to proceed.
> I've queued up patch 1 as a fix.  Patch 2 will need to wait on that.
> 
> > 
> > Thanks a lot,
> > angelo
> > 
> > On 09.04.2025 14:57, Nuno Sá wrote:
> > > On Wed, 2025-04-09 at 11:16 +0200, Angelo Dureghello wrote:  
> > > > This patchset is intended to fix a random wrong chip ID read, or a
> > > > scratchpad test mismatch, tests done in the ad3552r-hs driver probe. The 
> > > > bus "read" operation must always check for busy flag before reading.
> > > > 
> > > > First patch reorganizes a bit the busy-wait polling code, second patch
> > > > fixes the wrong bus read occurence. 
> > > > 
> > > > NOTE: due to ongoing changes in adi-axi-dac.c, this patch is intended to be
> > > > applied after the linked "ramp generator" patch.
> > > > 
> > > > Link:
> > > > https://lore.kernel.org/linux-iio/20250408-wip-bl-ad3552r-fixes-v4-0-b33c0264bd78@baylibre.com
> > > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> > > > ---  
> > > 
> > > LGTM,
> > > 
> > > Reviewed-by: Nuno Sá <nuno.sa@analog.com>
> > >   
> > > > Changes in v2:
> > > > - invert patch order, fix first.
> > > > - Link to v1:
> > > > https://lore.kernel.org/r/20250408-ad3552r-fix-bus-read-v1-0-37add66aeb08@baylibre.com
> > > > 
> > > > ---
> > > > Angelo Dureghello (2):
> > > >       iio: dac: adi-axi-dac: fix bus read
> > > >       iio: dac: adi-axi-dac: use unique bus free check
> > > > 
> > > >  drivers/iio/dac/adi-axi-dac.c | 40 +++++++++++++++++++++++++---------------
> > > >  1 file changed, 25 insertions(+), 15 deletions(-)
> > > > ---
> > > > base-commit: 6fb85f14853ddde06d57030c753168402bf69cd9
> > > > change-id: 20250408-ad3552r-fix-bus-read-1522622fbd2b
> > > > 
> > > > Best regards,  
> > 
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2025-05-19 19:04 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-09  9:16 [PATCH v2 0/2] iio: dac: adi-axi-dac: fix for wrong bus read Angelo Dureghello
2025-04-09  9:16 ` [PATCH v2 1/2] iio: dac: adi-axi-dac: fix " Angelo Dureghello
2025-04-09 16:49   ` Andy Shevchenko
2025-04-12 13:00     ` Jonathan Cameron
2025-04-12 18:03       ` Andy Shevchenko
2025-04-09  9:16 ` [PATCH v2 2/2] iio: dac: adi-axi-dac: use unique bus free check Angelo Dureghello
2025-04-09 16:51   ` Andy Shevchenko
2025-04-29 14:32     ` Angelo Dureghello
2025-04-29 22:05       ` Andy Shevchenko
2025-04-30 14:15         ` Angelo Dureghello
2025-04-09 13:57 ` [PATCH v2 0/2] iio: dac: adi-axi-dac: fix for wrong bus read Nuno Sá
2025-04-22  8:41   ` Angelo Dureghello
2025-05-05 11:33   ` Angelo Dureghello
2025-05-05 12:38     ` Jonathan Cameron
2025-05-19 19:03       ` Angelo Dureghello

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).