[PATCH] staging: iio: ad9834: fix probe error handling and comment typo

[PATCH] staging: iio: ad9834: fix probe error handling and comment typo

[Angus Gardner]

Use dev_err_probe() for the master clock error path instead of open-coding
dev_err() + PTR_ERR(), consistent with the avdd regulator error path above
it and with the equivalent driver ad9832.c.

Simplify the -ENOMEM return after devm_iio_device_alloc() to a direct
return rather than bouncing through a local variable.

Fix a copy-paste typo in two comments that referred to 'AD9843' instead
of the correct chip name 'AD9834'.

Signed-off-by: Angus Gardner <angusg778@gmail.com>
---
 drivers/staging/iio/frequency/ad9834.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/iio/frequency/ad9834.c b/drivers/staging/iio/frequency/ad9834.c
index bdb2580e29bf..5968aca74ecf 100644
--- a/drivers/staging/iio/frequency/ad9834.c
+++ b/drivers/staging/iio/frequency/ad9834.c
@@ -164,7 +164,7 @@ static ssize_t ad9834_write(struct device *dev,
 		break;
 	case AD9834_OPBITEN:
 		if (st->control & AD9834_MODE) {
-			ret = -EINVAL;  /* AD9843 reserved mode */
+			ret = -EINVAL;  /* AD9834 reserved mode */
 			break;
 		}
 
@@ -239,7 +239,7 @@ static ssize_t ad9834_store_wavetype(struct device *dev,
 				st->control &= ~AD9834_OPBITEN;
 				st->control |= AD9834_MODE;
 			} else if (st->control & AD9834_OPBITEN) {
-				ret = -EINVAL;	/* AD9843 reserved mode */
+				ret = -EINVAL;	/* AD9834 reserved mode */
 			} else {
 				st->control |= AD9834_MODE;
 			}
@@ -389,17 +389,14 @@ static int ad9834_probe(struct spi_device *spi)
 		return dev_err_probe(&spi->dev, ret, "Failed to enable specified AVDD supply\n");
 
 	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
-	if (!indio_dev) {
-		ret = -ENOMEM;
-		return ret;
-	}
+	if (!indio_dev)
+		return -ENOMEM;
 	st = iio_priv(indio_dev);
 	mutex_init(&st->lock);
 	st->mclk = devm_clk_get_enabled(&spi->dev, NULL);
-	if (IS_ERR(st->mclk)) {
-		dev_err(&spi->dev, "Failed to enable master clock\n");
-		return PTR_ERR(st->mclk);
-	}
+	if (IS_ERR(st->mclk))
+		return dev_err_probe(&spi->dev, PTR_ERR(st->mclk),
+				     "Failed to enable master clock\n");
 
 	st->spi = spi;
 	st->devid = spi_get_device_id(spi)->driver_data;
-- 
2.51.0

Re: [PATCH] staging: iio: ad9834: fix probe error handling and comment typo

[Maxwell Doose]

Hi Angus,

On Fri May 1, 2026 at 9:18 PM CDT, Angus Gardner <angusg778@gmail.com> wrote:
> Use dev_err_probe() for the master clock error path instead of open-coding
> dev_err() + PTR_ERR(), consistent with the avdd regulator error path above
> it and with the equivalent driver ad9832.c.
>
> Simplify the -ENOMEM return after devm_iio_device_alloc() to a direct
> return rather than bouncing through a local variable.
>
> Fix a copy-paste typo in two comments that referred to 'AD9843' instead
> of the correct chip name 'AD9834'.
>
> Signed-off-by: Angus Gardner <angusg778@gmail.com>
> ---
>  drivers/staging/iio/frequency/ad9834.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
>

Patch itself looks good but maybe rephrase subject to say "Use probe
error handling and fix comment typo" instead? "Fix" seems a bit generic
for my tastes, and we're not really fixing anything anyways.

I'm not the final authority on naming so its at the discretion of
either Andy or Jonathan, and maybe Greg. So either way,
Reviewed-by: Maxwell Doose <m32285159@gmail.com>

best regards,
maxwell

Re: [PATCH] staging: iio: ad9834: fix probe error handling and comment typo

[Joshua Crofts]

On Sat, 2 May 2026 at 21:54, Maxwell Doose <m32285159@gmail.com> wrote:
>
> Hi Angus,
>
> On Fri May 1, 2026 at 9:18 PM CDT, Angus Gardner <angusg778@gmail.com> wrote:
> > Use dev_err_probe() for the master clock error path instead of open-coding
> > dev_err() + PTR_ERR(), consistent with the avdd regulator error path above
> > it and with the equivalent driver ad9832.c.

I found another dev_err() call in the same function, maybe change this one also?
https://elixir.bootlin.com/linux/v7.0.1/source/drivers/staging/iio/frequency/ad9834.c#L452

> > Simplify the -ENOMEM return after devm_iio_device_alloc() to a direct
> > return rather than bouncing through a local variable.
> >
> > Fix a copy-paste typo in two comments that referred to 'AD9843' instead
> > of the correct chip name 'AD9834'.
> >
> > Signed-off-by: Angus Gardner <angusg778@gmail.com>
> > ---
> >  drivers/staging/iio/frequency/ad9834.c | 17 +++++++----------
> >  1 file changed, 7 insertions(+), 10 deletions(-)
> >
>
> Patch itself looks good but maybe rephrase subject to say "Use probe
> error handling and fix comment typo" instead? "Fix" seems a bit generic
> for my tastes, and we're not really fixing anything anyways.
>
> I'm not the final authority on naming so its at the discretion of
> either Andy or Jonathan, and maybe Greg. So either way,
> Reviewed-by: Maxwell Doose <m32285159@gmail.com>

You're mixing different logical changes into one patch, therefore it's
harder to review and harder to keep track of the changes after merging
(e.g. running git blame). Instead, you should have a series (named
something like "ad9834: driver cleanup") and three patches under it:
1. simplify -ENOMEM errno return
2. use dev_err_probe()
3. fix typos in comments

-- 
Kind regards

CJD

Re: [PATCH] staging: iio: ad9834: fix probe error handling and comment typo

[Maxwell Doose]

On Sun May 3, 2026 at 3:53 AM CDT, Joshua Crofts wrote:
> On Sat, 2 May 2026 at 21:54, Maxwell Doose <m32285159@gmail.com> wrote:
>>
>> Hi Angus,
>>
>> On Fri May 1, 2026 at 9:18 PM CDT, Angus Gardner <angusg778@gmail.com> wrote:
>> > Use dev_err_probe() for the master clock error path instead of open-coding
>> > dev_err() + PTR_ERR(), consistent with the avdd regulator error path above
>> > it and with the equivalent driver ad9832.c.
>
> I found another dev_err() call in the same function, maybe change this one also?
> https://elixir.bootlin.com/linux/v7.0.1/source/drivers/staging/iio/frequency/ad9834.c#L452
>

Shoot, I seem to have missed that in my review. Nice catch.

>
>> > Simplify the -ENOMEM return after devm_iio_device_alloc() to a direct
>> > return rather than bouncing through a local variable.
>> >
>> > Fix a copy-paste typo in two comments that referred to 'AD9843' instead
>> > of the correct chip name 'AD9834'.
>> >
>> > Signed-off-by: Angus Gardner <angusg778@gmail.com>
>> > ---
>> >  drivers/staging/iio/frequency/ad9834.c | 17 +++++++----------
>> >  1 file changed, 7 insertions(+), 10 deletions(-)
>> >
>>
>> Patch itself looks good but maybe rephrase subject to say "Use probe
>> error handling and fix comment typo" instead? "Fix" seems a bit generic
>> for my tastes, and we're not really fixing anything anyways.
>>
>> I'm not the final authority on naming so its at the discretion of
>> either Andy or Jonathan, and maybe Greg. So either way,
>> Reviewed-by: Maxwell Doose <m32285159@gmail.com>
>
> You're mixing different logical changes into one patch, therefore it's
> harder to review and harder to keep track of the changes after merging
> (e.g. running git blame). Instead, you should have a series (named
> something like "ad9834: driver cleanup") and three patches under it:
> 1. simplify -ENOMEM errno return
> 2. use dev_err_probe()
> 3. fix typos in comments
>

Of course this is undeniable but I think the other changes are so
miniscule that it wouldn't matter as much, but that's just my personal
opinion. Angus, you should probably split this into a patch series.

Either way, I still stand by my reviewed-by, at least based on technical
correctness, but make sure you at least add the missing dev_err_probe()
call that Joshua wanted Angus :)

best regards,
maxwell