diff mbox series

[v2,1/2] iio: dac: adi-axi-dac: fix bus read

Message ID 20250409-ad3552r-fix-bus-read-v2-1-34d3b21e8ca0@baylibre.com (mailing list archive)
State Changes Requested
Headers show
Series iio: dac: adi-axi-dac: fix for wrong bus read | expand

Commit Message

Angelo Dureghello April 9, 2025, 9:16 a.m. UTC 
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(+)

Comments

Andy Shevchenko April 9, 2025, 4:49 p.m. UTC | #1 
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;
Jonathan Cameron April 12, 2025, 1 p.m. UTC | #2 
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;  
>
Andy Shevchenko April 12, 2025, 6:03 p.m. UTC | #3 
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.
diff mbox series

Patch

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);
 }