* [PATCH 1/3] iio: accel: kxsd9: validate input before acquiring runtime PM in write_raw runtime PM in write_raw
[not found] <20260714122520.3201-5-birenpandya@gmail.com>
@ 2026-07-14 12:25 ` Biren Pandya
2026-07-20 2:14 ` Jonathan Cameron
2026-07-14 12:25 ` [PATCH 2/3] iio: accel: kxsd9: modernize runtime PM in read_raw Biren Pandya
2026-07-14 12:25 ` [PATCH 3/3] iio: accel: kxsd9: use dev_err_probe() Biren Pandya
2 siblings, 1 reply; 6+ messages in thread
From: Biren Pandya @ 2026-07-14 12:25 UTC (permalink / raw)
To: jic23, dlechner, nuno.sa, andy, linux-iio, linux-kernel; +Cc: Biren Pandya
Currently, kxsd9_write_raw() acquires a runtime PM reference at the very
top of the function, before checking if the requested mask or value are
actually supported. This causes unnecessary hardware wakeups for invalid
inputs.
Refactor write_raw to validate inputs first. Additionally, modernize the
PM runtime call by replacing pm_runtime_get_sync() with
pm_runtime_resume_and_get() and adding the missing error check to
prevent proceeding if power-on fails.
Suggested-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Biren Pandya <birenpandya@gmail.com>
---
drivers/iio/accel/kxsd9.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/drivers/iio/accel/kxsd9.c b/drivers/iio/accel/kxsd9.c
index 7ac885d94d7f4..b3acb20441d86 100644
--- a/drivers/iio/accel/kxsd9.c
+++ b/drivers/iio/accel/kxsd9.c
@@ -139,18 +139,21 @@ static int kxsd9_write_raw(struct iio_dev *indio_dev,
int val2,
long mask)
{
- int ret = -EINVAL;
struct kxsd9_state *st = iio_priv(indio_dev);
+ int ret;
- pm_runtime_get_sync(st->dev);
+ if (mask != IIO_CHAN_INFO_SCALE)
+ return -EINVAL;
- if (mask == IIO_CHAN_INFO_SCALE) {
- /* Check no integer component */
- if (val)
- ret = -EINVAL;
- else
- ret = kxsd9_write_scale(indio_dev, val2);
- }
+ /* Check no integer component */
+ if (val)
+ return -EINVAL;
+
+ ret = pm_runtime_resume_and_get(st->dev);
+ if (ret < 0)
+ return ret;
+
+ ret = kxsd9_write_scale(indio_dev, val2);
pm_runtime_put_autosuspend(st->dev);
--
2.50.1 (Apple Git-155)
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/3] iio: accel: kxsd9: validate input before acquiring runtime PM in write_raw runtime PM in write_raw
2026-07-14 12:25 ` [PATCH 1/3] iio: accel: kxsd9: validate input before acquiring runtime PM in write_raw runtime PM in write_raw Biren Pandya
@ 2026-07-20 2:14 ` Jonathan Cameron
0 siblings, 0 replies; 6+ messages in thread
From: Jonathan Cameron @ 2026-07-20 2:14 UTC (permalink / raw)
To: Biren Pandya; +Cc: dlechner, nuno.sa, andy, linux-iio, linux-kernel
On Tue, 14 Jul 2026 17:55:21 +0530
Biren Pandya <birenpandya@gmail.com> wrote:
> Currently, kxsd9_write_raw() acquires a runtime PM reference at the very
> top of the function, before checking if the requested mask or value are
> actually supported. This causes unnecessary hardware wakeups for invalid
> inputs.
>
> Refactor write_raw to validate inputs first. Additionally, modernize the
> PM runtime call by replacing pm_runtime_get_sync() with
> pm_runtime_resume_and_get() and adding the missing error check to
> prevent proceeding if power-on fails.
>
> Suggested-by: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Biren Pandya <birenpandya@gmail.com>
Hi Biren,
For a series there should always be a cover letter.
This patch looks fine but you may want to take the approaches
I'll suggest in review of patch 2 and apply them here for consistency.
Thanks,
Jonathan
> ---
> drivers/iio/accel/kxsd9.c | 21 ++++++++++++---------
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/iio/accel/kxsd9.c b/drivers/iio/accel/kxsd9.c
> index 7ac885d94d7f4..b3acb20441d86 100644
> --- a/drivers/iio/accel/kxsd9.c
> +++ b/drivers/iio/accel/kxsd9.c
> @@ -139,18 +139,21 @@ static int kxsd9_write_raw(struct iio_dev *indio_dev,
> int val2,
> long mask)
> {
> - int ret = -EINVAL;
> struct kxsd9_state *st = iio_priv(indio_dev);
> + int ret;
>
> - pm_runtime_get_sync(st->dev);
> + if (mask != IIO_CHAN_INFO_SCALE)
> + return -EINVAL;
>
> - if (mask == IIO_CHAN_INFO_SCALE) {
> - /* Check no integer component */
> - if (val)
> - ret = -EINVAL;
> - else
> - ret = kxsd9_write_scale(indio_dev, val2);
> - }
> + /* Check no integer component */
> + if (val)
> + return -EINVAL;
> +
> + ret = pm_runtime_resume_and_get(st->dev);
> + if (ret < 0)
> + return ret;
> +
> + ret = kxsd9_write_scale(indio_dev, val2);
>
> pm_runtime_put_autosuspend(st->dev);
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/3] iio: accel: kxsd9: modernize runtime PM in read_raw
[not found] <20260714122520.3201-5-birenpandya@gmail.com>
2026-07-14 12:25 ` [PATCH 1/3] iio: accel: kxsd9: validate input before acquiring runtime PM in write_raw runtime PM in write_raw Biren Pandya
@ 2026-07-14 12:25 ` Biren Pandya
2026-07-20 2:15 ` Jonathan Cameron
2026-07-14 12:25 ` [PATCH 3/3] iio: accel: kxsd9: use dev_err_probe() Biren Pandya
2 siblings, 1 reply; 6+ messages in thread
From: Biren Pandya @ 2026-07-14 12:25 UTC (permalink / raw)
To: jic23, dlechner, nuno.sa, andy, linux-iio, linux-kernel; +Cc: Biren Pandya
Replace the legacy pm_runtime_get_sync() call with the modern
pm_runtime_resume_and_get() in kxsd9_read_raw().
If the resume fails, we now correctly abort the read instead of
carrying on and reading garbage data from the sleeping hardware.
Suggested-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Biren Pandya <birenpandya@gmail.com>
---
drivers/iio/accel/kxsd9.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/accel/kxsd9.c b/drivers/iio/accel/kxsd9.c
index b3acb20441d86..680d43135e617 100644
--- a/drivers/iio/accel/kxsd9.c
+++ b/drivers/iio/accel/kxsd9.c
@@ -164,13 +164,15 @@ static int kxsd9_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int *val, int *val2, long mask)
{
- int ret = -EINVAL;
struct kxsd9_state *st = iio_priv(indio_dev);
unsigned int regval;
__be16 raw_val;
u16 nval;
+ int ret;
- pm_runtime_get_sync(st->dev);
+ ret = pm_runtime_resume_and_get(st->dev);
+ if (ret < 0)
+ return ret;
switch (mask) {
case IIO_CHAN_INFO_RAW:
@@ -199,6 +201,9 @@ static int kxsd9_read_raw(struct iio_dev *indio_dev,
*val2 = kxsd9_micro_scales[regval & KXSD9_CTRL_C_FS_MASK];
ret = IIO_VAL_INT_PLUS_MICRO;
break;
+ default:
+ ret = -EINVAL;
+ break;
}
error_ret:
--
2.50.1 (Apple Git-155)
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 2/3] iio: accel: kxsd9: modernize runtime PM in read_raw
2026-07-14 12:25 ` [PATCH 2/3] iio: accel: kxsd9: modernize runtime PM in read_raw Biren Pandya
@ 2026-07-20 2:15 ` Jonathan Cameron
0 siblings, 0 replies; 6+ messages in thread
From: Jonathan Cameron @ 2026-07-20 2:15 UTC (permalink / raw)
To: Biren Pandya; +Cc: dlechner, nuno.sa, andy, linux-iio, linux-kernel
On Tue, 14 Jul 2026 17:55:22 +0530
Biren Pandya <birenpandya@gmail.com> wrote:
> Replace the legacy pm_runtime_get_sync() call with the modern
> pm_runtime_resume_and_get() in kxsd9_read_raw().
>
> If the resume fails, we now correctly abort the read instead of
> carrying on and reading garbage data from the sleeping hardware.
>
> Suggested-by: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Biren Pandya <birenpandya@gmail.com>
> ---
> drivers/iio/accel/kxsd9.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/accel/kxsd9.c b/drivers/iio/accel/kxsd9.c
> index b3acb20441d86..680d43135e617 100644
> --- a/drivers/iio/accel/kxsd9.c
> +++ b/drivers/iio/accel/kxsd9.c
> @@ -164,13 +164,15 @@ static int kxsd9_read_raw(struct iio_dev *indio_dev,
> struct iio_chan_spec const *chan,
> int *val, int *val2, long mask)
> {
> - int ret = -EINVAL;
> struct kxsd9_state *st = iio_priv(indio_dev);
> unsigned int regval;
> __be16 raw_val;
> u16 nval;
> + int ret;
>
> - pm_runtime_get_sync(st->dev);
> + ret = pm_runtime_resume_and_get(st->dev);
Look at PM_RUNTIME_ACQUIRE_AUTOSUSPEND() and consider
how it would improve this code - particularly with respect to
getting rid of the gotos. This function is almost a text book case
of why that stuff is useful!
Thanks,
Jonathan
> + if (ret < 0)
> + return ret;
>
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> @@ -199,6 +201,9 @@ static int kxsd9_read_raw(struct iio_dev *indio_dev,
> *val2 = kxsd9_micro_scales[regval & KXSD9_CTRL_C_FS_MASK];
> ret = IIO_VAL_INT_PLUS_MICRO;
> break;
> + default:
> + ret = -EINVAL;
> + break;
> }
>
> error_ret:
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 3/3] iio: accel: kxsd9: use dev_err_probe()
[not found] <20260714122520.3201-5-birenpandya@gmail.com>
2026-07-14 12:25 ` [PATCH 1/3] iio: accel: kxsd9: validate input before acquiring runtime PM in write_raw runtime PM in write_raw Biren Pandya
2026-07-14 12:25 ` [PATCH 2/3] iio: accel: kxsd9: modernize runtime PM in read_raw Biren Pandya
@ 2026-07-14 12:25 ` Biren Pandya
2026-07-20 2:18 ` Jonathan Cameron
2 siblings, 1 reply; 6+ messages in thread
From: Biren Pandya @ 2026-07-14 12:25 UTC (permalink / raw)
To: jic23, dlechner, nuno.sa, andy, linux-iio, linux-kernel; +Cc: Biren Pandya
Modernize the error paths in kxsd9_common_probe() by using
dev_err_probe() instead of dev_err() when handling failures from
regulators and buffer setup.
This simplifies the error handling and properly suppresses annoying
and unnecessary log spam during -EPROBE_DEFER cases when regulators
are not yet available during boot.
Signed-off-by: Biren Pandya <birenpandya@gmail.com>
---
drivers/iio/accel/kxsd9.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/iio/accel/kxsd9.c b/drivers/iio/accel/kxsd9.c
index 680d43135e617..ffbefd4576dd2 100644
--- a/drivers/iio/accel/kxsd9.c
+++ b/drivers/iio/accel/kxsd9.c
@@ -433,10 +433,9 @@ int kxsd9_common_probe(struct device *dev,
ret = devm_regulator_bulk_get(dev,
ARRAY_SIZE(st->regs),
st->regs);
- if (ret) {
- dev_err(dev, "Cannot get regulators\n");
- return ret;
- }
+ if (ret)
+ return dev_err_probe(dev, ret, "Cannot get regulators\n");
+
/* Default scaling */
st->scale = KXSD9_CTRL_C_FS_2G;
@@ -447,13 +446,15 @@ int kxsd9_common_probe(struct device *dev,
kxsd9_trigger_handler,
&kxsd9_buffer_setup_ops);
if (ret) {
- dev_err(dev, "triggered buffer setup failed\n");
+ dev_err_probe(dev, ret, "triggered buffer setup failed\n");
goto err_power_down;
}
ret = iio_device_register(indio_dev);
- if (ret)
+ if (ret) {
+ dev_err_probe(dev, ret, "device register failed\n");
goto err_cleanup_buffer;
+ }
dev_set_drvdata(dev, indio_dev);
--
2.50.1 (Apple Git-155)
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 3/3] iio: accel: kxsd9: use dev_err_probe()
2026-07-14 12:25 ` [PATCH 3/3] iio: accel: kxsd9: use dev_err_probe() Biren Pandya
@ 2026-07-20 2:18 ` Jonathan Cameron
0 siblings, 0 replies; 6+ messages in thread
From: Jonathan Cameron @ 2026-07-20 2:18 UTC (permalink / raw)
To: Biren Pandya; +Cc: dlechner, nuno.sa, andy, linux-iio, linux-kernel
On Tue, 14 Jul 2026 17:55:23 +0530
Biren Pandya <birenpandya@gmail.com> wrote:
> Modernize the error paths in kxsd9_common_probe() by using
Not sure we need the modernise bit of this.
> dev_err_probe() instead of dev_err() when handling failures from
> regulators and buffer setup.
>
> This simplifies the error handling and properly suppresses annoying
> and unnecessary log spam during -EPROBE_DEFER cases when regulators
> are not yet available during boot.
Not sure we need to state too much about it here, but it does
some more useful stuff than suppressing the log.
Take a look at debug stuff for deferred probing.
Anyhow, this one looks fine to me though the true benefit only
comes when these all become direct returns with a full move to devm
based cleanup.
Jonathan
>
> Signed-off-by: Biren Pandya <birenpandya@gmail.com>
> ---
> drivers/iio/accel/kxsd9.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iio/accel/kxsd9.c b/drivers/iio/accel/kxsd9.c
> index 680d43135e617..ffbefd4576dd2 100644
> --- a/drivers/iio/accel/kxsd9.c
> +++ b/drivers/iio/accel/kxsd9.c
> @@ -433,10 +433,9 @@ int kxsd9_common_probe(struct device *dev,
> ret = devm_regulator_bulk_get(dev,
> ARRAY_SIZE(st->regs),
> st->regs);
> - if (ret) {
> - dev_err(dev, "Cannot get regulators\n");
> - return ret;
> - }
> + if (ret)
> + return dev_err_probe(dev, ret, "Cannot get regulators\n");
> +
> /* Default scaling */
> st->scale = KXSD9_CTRL_C_FS_2G;
>
> @@ -447,13 +446,15 @@ int kxsd9_common_probe(struct device *dev,
> kxsd9_trigger_handler,
> &kxsd9_buffer_setup_ops);
> if (ret) {
> - dev_err(dev, "triggered buffer setup failed\n");
> + dev_err_probe(dev, ret, "triggered buffer setup failed\n");
> goto err_power_down;
> }
>
> ret = iio_device_register(indio_dev);
> - if (ret)
> + if (ret) {
> + dev_err_probe(dev, ret, "device register failed\n");
> goto err_cleanup_buffer;
> + }
>
> dev_set_drvdata(dev, indio_dev);
>
^ permalink raw reply [flat|nested] 6+ messages in thread