[PATCH v2 0/6] iio: accel: mma8452: improve coding style, pm and resource cleanup

[PATCH v2 0/6] iio: accel: mma8452: improve coding style, pm and resource cleanup

[Sanjay Chitroda]

From: Sanjay Chitroda <sanjayembeddedse@gmail.com>

Hi all,

This series contains a small cleanup and improvements to use modern kernel
helper API and coding style for mma8452 accelarator driver.

The changes modernize mutex with guard(), dev_err_probe usage,
resolve checkpatch CHECKS and pm_ptr macro usage.

Changes in v2:
- 0005: address review comment from Andy and Geert
  and use DEFINE_RUNTIME_DEV_PM_OPS macro
- Added new cleanup channges in mma8452 driver

No functional behavior changes are intended.

Testing:
  - Compiled with W=1
  - Build-tested on QEMU x86_64

Feedback and reviews are very welcome.

Thanks,
Sanjay Chitroda

Sanjay Chitroda (6):
  iio: accel: mma8452: cleanup codestyle warning
  iio: accel: mma8452: sort headers alphabetically
  iio: accel: mma8452: use local struct device
  iio: accel: mma8452: Use dev_err_probe()
  iio: accel: mma8452: use pm_ptr() for dev_pm_ops
  iio: accel: mma8452: use guard() to release mutexes

 drivers/iio/accel/mma8452.c | 161 +++++++++++++++++-------------------
 1 file changed, 76 insertions(+), 85 deletions(-)


base-commit: eade2b843d9b1f668fc1775f15611bb0a1999cd9
-- 
2.34.1

[PATCH v2 1/6] iio: accel: mma8452: cleanup codestyle warning

[Sanjay Chitroda]

From: Sanjay Chitroda <sanjayembeddedse@gmail.com>

Reported by checkpatch:
FILE: drivers/iio/accel/mma8452.c

CHECK: Alignment should match open parenthesis

Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
---
 drivers/iio/accel/mma8452.c | 47 +++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 23 deletions(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 15172ba2972c..6d9091f696cf 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -704,8 +704,8 @@ static int mma8452_set_hp_filter_frequency(struct mma8452_data *data,
 }
 
 static int __mma8452_write_raw(struct iio_dev *indio_dev,
-			     struct iio_chan_spec const *chan,
-			     int val, int val2, long mask)
+			       struct iio_chan_spec const *chan,
+			       int val, int val2, long mask)
 {
 	struct mma8452_data *data = iio_priv(indio_dev);
 	int i, j, ret;
@@ -786,8 +786,9 @@ static int mma8452_write_raw(struct iio_dev *indio_dev,
 }
 
 static int mma8452_get_event_regs(struct mma8452_data *data,
-		const struct iio_chan_spec *chan, enum iio_event_direction dir,
-		const struct mma8452_event_regs **ev_reg)
+				  const struct iio_chan_spec *chan,
+				  enum iio_event_direction dir,
+				  const struct mma8452_event_regs **ev_reg)
 {
 	if (!chan)
 		return -EINVAL;
@@ -816,11 +817,11 @@ static int mma8452_get_event_regs(struct mma8452_data *data,
 }
 
 static int mma8452_read_event_value(struct iio_dev *indio_dev,
-			       const struct iio_chan_spec *chan,
-			       enum iio_event_type type,
-			       enum iio_event_direction dir,
-			       enum iio_event_info info,
-			       int *val, int *val2)
+				    const struct iio_chan_spec *chan,
+				    enum iio_event_type type,
+				    enum iio_event_direction dir,
+				    enum iio_event_info info,
+				    int *val, int *val2)
 {
 	struct mma8452_data *data = iio_priv(indio_dev);
 	int ret, us, power_mode;
@@ -879,11 +880,11 @@ static int mma8452_read_event_value(struct iio_dev *indio_dev,
 }
 
 static int mma8452_write_event_value(struct iio_dev *indio_dev,
-				const struct iio_chan_spec *chan,
-				enum iio_event_type type,
-				enum iio_event_direction dir,
-				enum iio_event_info info,
-				int val, int val2)
+				     const struct iio_chan_spec *chan,
+				     enum iio_event_type type,
+				     enum iio_event_direction dir,
+				     enum iio_event_info info,
+				     int val, int val2)
 {
 	struct mma8452_data *data = iio_priv(indio_dev);
 	int ret, reg, steps;
@@ -953,8 +954,7 @@ static int mma8452_read_event_config(struct iio_dev *indio_dev,
 	case IIO_EV_DIR_FALLING:
 		return mma8452_freefall_mode_enabled(data);
 	case IIO_EV_DIR_RISING:
-		ret = i2c_smbus_read_byte_data(data->client,
-				ev_regs->ev_cfg);
+		ret = i2c_smbus_read_byte_data(data->client, ev_regs->ev_cfg);
 		if (ret < 0)
 			return ret;
 
@@ -1191,7 +1191,7 @@ static const struct attribute_group mma8452_event_attribute_group = {
 
 static const struct iio_mount_matrix *
 mma8452_get_mount_matrix(const struct iio_dev *indio_dev,
-			   const struct iio_chan_spec *chan)
+			 const struct iio_chan_spec *chan)
 {
 	struct mma8452_data *data = iio_priv(indio_dev);
 
@@ -1514,8 +1514,9 @@ static int mma8452_reset(struct i2c_client *client)
 	 * The following code will read the reset register, and check whether
 	 * this reset works.
 	 */
-	i2c_smbus_write_byte_data(client, MMA8452_CTRL_REG2,
-					MMA8452_CTRL_REG2_RST);
+	i2c_smbus_write_byte_data(client,
+				  MMA8452_CTRL_REG2,
+				  MMA8452_CTRL_REG2_RST);
 
 	for (i = 0; i < 10; i++) {
 		usleep_range(100, 200);
@@ -1645,8 +1646,8 @@ static int mma8452_probe(struct i2c_client *client)
 			dev_dbg(&client->dev, "using interrupt line INT2\n");
 		} else {
 			ret = i2c_smbus_write_byte_data(client,
-						MMA8452_CTRL_REG5,
-						data->chip_info->all_events);
+							MMA8452_CTRL_REG5,
+							data->chip_info->all_events);
 			if (ret < 0)
 				goto disable_regulators;
 
@@ -1654,8 +1655,8 @@ static int mma8452_probe(struct i2c_client *client)
 		}
 
 		ret = i2c_smbus_write_byte_data(client,
-					MMA8452_CTRL_REG4,
-					data->chip_info->enabled_events);
+						MMA8452_CTRL_REG4,
+						data->chip_info->enabled_events);
 		if (ret < 0)
 			goto disable_regulators;
 
-- 
2.34.1

[PATCH v2 2/6] iio: accel: mma8452: sort headers alphabetically

[Sanjay Chitroda]

From: Sanjay Chitroda <sanjayembeddedse@gmail.com>

Sort include headers alphabetically to improve readability.

Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
---
 drivers/iio/accel/mma8452.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 6d9091f696cf..c54c0ea05ac1 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -18,21 +18,23 @@
  * TODO: orientation events
  */
 
-#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
 #include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
 #include <linux/property.h>
-#include <linux/i2c.h>
+#include <linux/types.h>
+
+#include <linux/iio/buffer.h>
+#include <linux/iio/events.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
-#include <linux/iio/buffer.h>
 #include <linux/iio/trigger.h>
 #include <linux/iio/trigger_consumer.h>
 #include <linux/iio/triggered_buffer.h>
-#include <linux/iio/events.h>
-#include <linux/delay.h>
-#include <linux/pm_runtime.h>
+
 #include <linux/regulator/consumer.h>
-#include <linux/types.h>
 
 #define MMA8452_STATUS				0x00
 #define  MMA8452_STATUS_DRDY			(BIT(2) | BIT(1) | BIT(0))
-- 
2.34.1

[PATCH v2 3/6] iio: accel: mma8452: use local struct device

[Sanjay Chitroda]

From: Sanjay Chitroda <sanjayembeddedse@gmail.com>

Introduce a local struct device pointer derived from &client->dev.
This avoids repeated &client->dev usage and improves readability.

Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
---
 drivers/iio/accel/mma8452.c | 51 ++++++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 24 deletions(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index c54c0ea05ac1..99f7763b84ac 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -225,13 +225,14 @@ static int mma8452_set_runtime_pm_state(struct i2c_client *client, bool on)
 {
 #ifdef CONFIG_PM
 	int ret;
+	struct device *dev = &client->dev;
 
 	if (on)
-		ret = pm_runtime_resume_and_get(&client->dev);
+		ret = pm_runtime_resume_and_get(dev);
 	else
-		ret = pm_runtime_put_autosuspend(&client->dev);
+		ret = pm_runtime_put_autosuspend(dev);
 	if (ret < 0) {
-		dev_err(&client->dev,
+		dev_err(dev,
 			"failed to change power state to %d\n", on);
 
 		return ret;
@@ -1548,10 +1549,11 @@ MODULE_DEVICE_TABLE(of, mma8452_dt_ids);
 static int mma8452_probe(struct i2c_client *client)
 {
 	struct mma8452_data *data;
+	struct device *dev = &client->dev;
 	struct iio_dev *indio_dev;
 	int ret;
 
-	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
 	if (!indio_dev)
 		return -ENOMEM;
 
@@ -1561,32 +1563,32 @@ static int mma8452_probe(struct i2c_client *client)
 
 	data->chip_info = i2c_get_match_data(client);
 	if (!data->chip_info)
-		return dev_err_probe(&client->dev, -ENODEV,
+		return dev_err_probe(dev, -ENODEV,
 				     "unknown device model\n");
 
-	ret = iio_read_mount_matrix(&client->dev, &data->orientation);
+	ret = iio_read_mount_matrix(dev, &data->orientation);
 	if (ret)
 		return ret;
 
-	data->vdd_reg = devm_regulator_get(&client->dev, "vdd");
+	data->vdd_reg = devm_regulator_get(dev, "vdd");
 	if (IS_ERR(data->vdd_reg))
-		return dev_err_probe(&client->dev, PTR_ERR(data->vdd_reg),
+		return dev_err_probe(dev, PTR_ERR(data->vdd_reg),
 				     "failed to get VDD regulator!\n");
 
-	data->vddio_reg = devm_regulator_get(&client->dev, "vddio");
+	data->vddio_reg = devm_regulator_get(dev, "vddio");
 	if (IS_ERR(data->vddio_reg))
-		return dev_err_probe(&client->dev, PTR_ERR(data->vddio_reg),
+		return dev_err_probe(dev, PTR_ERR(data->vddio_reg),
 				     "failed to get VDDIO regulator!\n");
 
 	ret = regulator_enable(data->vdd_reg);
 	if (ret) {
-		dev_err(&client->dev, "failed to enable VDD regulator!\n");
+		dev_err(dev, "failed to enable VDD regulator!\n");
 		return ret;
 	}
 
 	ret = regulator_enable(data->vddio_reg);
 	if (ret) {
-		dev_err(&client->dev, "failed to enable VDDIO regulator!\n");
+		dev_err(dev, "failed to enable VDDIO regulator!\n");
 		goto disable_regulator_vdd;
 	}
 
@@ -1609,7 +1611,7 @@ static int mma8452_probe(struct i2c_client *client)
 		goto disable_regulators;
 	}
 
-	dev_info(&client->dev, "registering %s accelerometer; ID 0x%x\n",
+	dev_info(dev, "registering %s accelerometer; ID 0x%x\n",
 		 data->chip_info->name, data->chip_info->chip_id);
 
 	i2c_set_clientdata(client, indio_dev);
@@ -1642,10 +1644,10 @@ static int mma8452_probe(struct i2c_client *client)
 	if (client->irq) {
 		int irq2;
 
-		irq2 = fwnode_irq_get_byname(dev_fwnode(&client->dev), "INT2");
+		irq2 = fwnode_irq_get_byname(dev_fwnode(dev), "INT2");
 
 		if (irq2 == client->irq) {
-			dev_dbg(&client->dev, "using interrupt line INT2\n");
+			dev_dbg(dev, "using interrupt line INT2\n");
 		} else {
 			ret = i2c_smbus_write_byte_data(client,
 							MMA8452_CTRL_REG5,
@@ -1653,7 +1655,7 @@ static int mma8452_probe(struct i2c_client *client)
 			if (ret < 0)
 				goto disable_regulators;
 
-			dev_dbg(&client->dev, "using interrupt line INT1\n");
+			dev_dbg(dev, "using interrupt line INT1\n");
 		}
 
 		ret = i2c_smbus_write_byte_data(client,
@@ -1683,7 +1685,7 @@ static int mma8452_probe(struct i2c_client *client)
 		goto trigger_cleanup;
 
 	if (client->irq) {
-		ret = devm_request_threaded_irq(&client->dev,
+		ret = devm_request_threaded_irq(dev,
 						client->irq,
 						NULL, mma8452_interrupt,
 						IRQF_TRIGGER_LOW | IRQF_ONESHOT,
@@ -1692,14 +1694,14 @@ static int mma8452_probe(struct i2c_client *client)
 			goto buffer_cleanup;
 	}
 
-	ret = pm_runtime_set_active(&client->dev);
+	ret = pm_runtime_set_active(dev);
 	if (ret < 0)
 		goto buffer_cleanup;
 
-	pm_runtime_enable(&client->dev);
-	pm_runtime_set_autosuspend_delay(&client->dev,
+	pm_runtime_enable(dev);
+	pm_runtime_set_autosuspend_delay(dev,
 					 MMA8452_AUTO_SUSPEND_DELAY_MS);
-	pm_runtime_use_autosuspend(&client->dev);
+	pm_runtime_use_autosuspend(dev);
 
 	ret = iio_device_register(indio_dev);
 	if (ret < 0)
@@ -1732,12 +1734,13 @@ static int mma8452_probe(struct i2c_client *client)
 static void mma8452_remove(struct i2c_client *client)
 {
 	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	struct device *dev = &client->dev;
 	struct mma8452_data *data = iio_priv(indio_dev);
 
 	iio_device_unregister(indio_dev);
 
-	pm_runtime_disable(&client->dev);
-	pm_runtime_set_suspended(&client->dev);
+	pm_runtime_disable(dev);
+	pm_runtime_set_suspended(dev);
 
 	iio_triggered_buffer_cleanup(indio_dev);
 	mma8452_trigger_cleanup(indio_dev);
@@ -1758,7 +1761,7 @@ static int mma8452_runtime_suspend(struct device *dev)
 	ret = mma8452_standby(data);
 	mutex_unlock(&data->lock);
 	if (ret < 0) {
-		dev_err(&data->client->dev, "powering off device failed\n");
+		dev_err(dev, "powering off device failed\n");
 		return -EAGAIN;
 	}
 
-- 
2.34.1

[PATCH v2 4/6] iio: accel: mma8452: Use dev_err_probe()

[Sanjay Chitroda]

From: Sanjay Chitroda <sanjayembeddedse@gmail.com>

dev_err_probe() makes error code handling simpler and handle
deferred probe nicely (avoid spamming logs).

Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
---
 drivers/iio/accel/mma8452.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 99f7763b84ac..d1ae2bf37409 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -1563,8 +1563,7 @@ static int mma8452_probe(struct i2c_client *client)
 
 	data->chip_info = i2c_get_match_data(client);
 	if (!data->chip_info)
-		return dev_err_probe(dev, -ENODEV,
-				     "unknown device model\n");
+		return dev_err_probe(dev, -ENODEV, "unknown device model\n");
 
 	ret = iio_read_mount_matrix(dev, &data->orientation);
 	if (ret)
@@ -1581,14 +1580,12 @@ static int mma8452_probe(struct i2c_client *client)
 				     "failed to get VDDIO regulator!\n");
 
 	ret = regulator_enable(data->vdd_reg);
-	if (ret) {
-		dev_err(dev, "failed to enable VDD regulator!\n");
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to enable VDD regulator!\n");
 
 	ret = regulator_enable(data->vddio_reg);
 	if (ret) {
-		dev_err(dev, "failed to enable VDDIO regulator!\n");
+		dev_err_probe(dev, ret, "failed to enable VDDIO regulator!\n");
 		goto disable_regulator_vdd;
 	}
 
-- 
2.34.1

[PATCH v2 5/6] iio: accel: mma8452: use pm_ptr() for dev_pm_ops

[Sanjay Chitroda]

From: Sanjay Chitroda <sanjayembeddedse@gmail.com>

Use pm_ptr() and DEFINE_RUNTIME_DEV_PM_OPS() so the PM ops pointer is
automatically handle when CONFIG_PM is enabled/disabled. This follows
modern kernel power-management conventions.

Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
---
Changes in v2:
- Use DEFINE_RUNTIME_DEV_PM_OPS to address review comment and resolve 0-day bot warning
- Link to v1: https://lore.kernel.org/all/20260414192045.3598010-1-sanjayembedded@gmail.com/
---
 drivers/iio/accel/mma8452.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index d1ae2bf37409..9983f76a8bcd 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -1747,7 +1747,6 @@ static void mma8452_remove(struct i2c_client *client)
 	regulator_disable(data->vdd_reg);
 }
 
-#ifdef CONFIG_PM
 static int mma8452_runtime_suspend(struct device *dev)
 {
 	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
@@ -1815,13 +1814,9 @@ static int mma8452_runtime_resume(struct device *dev)
 
 	return ret;
 }
-#endif
 
-static const struct dev_pm_ops mma8452_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
-	SET_RUNTIME_PM_OPS(mma8452_runtime_suspend,
-			   mma8452_runtime_resume, NULL)
-};
+static DEFINE_RUNTIME_DEV_PM_OPS(mma8452_pm_ops, mma8452_runtime_suspend,
+				 mma8452_runtime_resume, NULL);
 
 static const struct i2c_device_id mma8452_id[] = {
 	{ "fxls8471", (kernel_ulong_t)&mma_chip_info_table[fxls8471] },
@@ -1838,7 +1833,7 @@ static struct i2c_driver mma8452_driver = {
 	.driver = {
 		.name	= "mma8452",
 		.of_match_table = mma8452_dt_ids,
-		.pm	= &mma8452_pm_ops,
+		.pm	= pm_ptr(&mma8452_pm_ops),
 	},
 	.probe = mma8452_probe,
 	.remove = mma8452_remove,
-- 
2.34.1

[PATCH v2 6/6] iio: accel: mma8452: use guard() to release mutexes

[Sanjay Chitroda]

From: Sanjay Chitroda <sanjayembeddedse@gmail.com>

Replace explicit mutex_lock() and mutex_unlock() with the guard() and
scoped_guard() macro for cleaner and safer mutex handling.

Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
---
 drivers/iio/accel/mma8452.c | 31 ++++++++++++-------------------
 1 file changed, 12 insertions(+), 19 deletions(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 9983f76a8bcd..1c284bdcc44a 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -18,6 +18,7 @@
  * TODO: orientation events
  */
 
+#include <linux/cleanup.h>
 #include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/mod_devicetable.h>
@@ -500,9 +501,8 @@ static int mma8452_read_raw(struct iio_dev *indio_dev,
 		if (!iio_device_claim_direct(indio_dev))
 			return -EBUSY;
 
-		mutex_lock(&data->lock);
-		ret = mma8452_read(data, buffer);
-		mutex_unlock(&data->lock);
+		scoped_guard(mutex, &data->lock)
+			ret = mma8452_read(data, buffer);
 		iio_device_release_direct(indio_dev);
 		if (ret < 0)
 			return ret;
@@ -600,36 +600,30 @@ static int mma8452_change_config(struct mma8452_data *data, u8 reg, u8 val)
 	int ret;
 	int is_active;
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
 
 	is_active = mma8452_is_active(data);
-	if (is_active < 0) {
-		ret = is_active;
-		goto fail;
-	}
+	if (is_active < 0)
+		return is_active;
 
 	/* config can only be changed when in standby */
 	if (is_active > 0) {
 		ret = mma8452_standby(data);
 		if (ret < 0)
-			goto fail;
+			return ret;
 	}
 
 	ret = i2c_smbus_write_byte_data(data->client, reg, val);
 	if (ret < 0)
-		goto fail;
+		return ret;
 
 	if (is_active > 0) {
 		ret = mma8452_active(data);
 		if (ret < 0)
-			goto fail;
+			return ret;
 	}
 
-	ret = 0;
-fail:
-	mutex_unlock(&data->lock);
-
-	return ret;
+	return 0;
 }
 
 static int mma8452_set_power_mode(struct mma8452_data *data, u8 mode)
@@ -1753,9 +1747,8 @@ static int mma8452_runtime_suspend(struct device *dev)
 	struct mma8452_data *data = iio_priv(indio_dev);
 	int ret;
 
-	mutex_lock(&data->lock);
-	ret = mma8452_standby(data);
-	mutex_unlock(&data->lock);
+	scoped_guard(mutex, &data->lock)
+		ret = mma8452_standby(data);
 	if (ret < 0) {
 		dev_err(dev, "powering off device failed\n");
 		return -EAGAIN;
-- 
2.34.1

Re: [PATCH v2 2/6] iio: accel: mma8452: sort headers alphabetically

[Andy Shevchenko]

On Wed, Apr 22, 2026 at 10:26:39PM +0530, Sanjay Chitroda wrote:

> Sort include headers alphabetically to improve readability.

...

> -#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
>  #include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/property.h>
> -#include <linux/i2c.h>
> +#include <linux/types.h>
> +
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/events.h>
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> -#include <linux/iio/buffer.h>
>  #include <linux/iio/trigger.h>
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/triggered_buffer.h>
> -#include <linux/iio/events.h>
> -#include <linux/delay.h>
> -#include <linux/pm_runtime.h>

> +
>  #include <linux/regulator/consumer.h>
> -#include <linux/types.h>

For this driver the regulator/consumer is a regular header and can be moved to
be between property.h and types.h.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/6] iio: accel: mma8452: use local struct device

[Andy Shevchenko]

On Wed, Apr 22, 2026 at 10:26:40PM +0530, Sanjay Chitroda wrote:

> Introduce a local struct device pointer derived from &client->dev.
> This avoids repeated &client->dev usage and improves readability.

...

> static int mma8452_set_runtime_pm_state(struct i2c_client *client, bool on)
>  {
>  #ifdef CONFIG_PM

So, this seems to be a missed in the PM patch.

>  	int ret;
> +	struct device *dev = &client->dev;

Keep it in reversed xmas tree order.

>  	if (on)
> -		ret = pm_runtime_resume_and_get(&client->dev);
> +		ret = pm_runtime_resume_and_get(dev);
>  	else
> -		ret = pm_runtime_put_autosuspend(&client->dev);
> +		ret = pm_runtime_put_autosuspend(dev);
>  	if (ret < 0) {
> -		dev_err(&client->dev,
> +		dev_err(dev,
>  			"failed to change power state to %d\n", on);
>  
>  		return ret;

>  }

...

>  static int mma8452_probe(struct i2c_client *client)
>  {
>  	struct mma8452_data *data;
> +	struct device *dev = &client->dev;

Same here, reversed xmas tree order.

>  	struct iio_dev *indio_dev;
>  	int ret;

...

>  	data->chip_info = i2c_get_match_data(client);
>  	if (!data->chip_info)
> -		return dev_err_probe(&client->dev, -ENODEV,
> +		return dev_err_probe(dev, -ENODEV,
>  				     "unknown device model\n");

Now it's a single line.

...

>  	ret = regulator_enable(data->vdd_reg);
>  	if (ret) {
> -		dev_err(&client->dev, "failed to enable VDD regulator!\n");
> +		dev_err(dev, "failed to enable VDD regulator!\n");
>  		return ret;

Convert this to dev_err_probe() before doing this patch. In that patch you
introduce temporary dev variable already. Here will use in the other places.

>  	}
>  
>  	ret = regulator_enable(data->vddio_reg);
>  	if (ret) {
> -		dev_err(&client->dev, "failed to enable VDDIO regulator!\n");
> +		dev_err(dev, "failed to enable VDDIO regulator!\n");
>  		goto disable_regulator_vdd;

Ditto.

>  	}

...

> +	pm_runtime_set_autosuspend_delay(dev,
>  					 MMA8452_AUTO_SUSPEND_DELAY_MS);

Now a single line.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/6] iio: accel: mma8452: Use dev_err_probe()

[Andy Shevchenko]

On Wed, Apr 22, 2026 at 10:26:41PM +0530, Sanjay Chitroda wrote:

> dev_err_probe() makes error code handling simpler and handle
> deferred probe nicely (avoid spamming logs).

This patch should go earlier than dev temporary variable one.
And dev needs to be introduced here.

...

>  	ret = regulator_enable(data->vddio_reg);
>  	if (ret) {
> -		dev_err(dev, "failed to enable VDDIO regulator!\n");
> +		dev_err_probe(dev, ret, "failed to enable VDDIO regulator!\n");
>  		goto disable_regulator_vdd;

Before doing these patches, please fix the mess with the devm/non-devm ordering.
There shouldn't be goto after any devm calls.

>  	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/6] iio: accel: mma8452: use pm_ptr() for dev_pm_ops

[Andy Shevchenko]

On Wed, Apr 22, 2026 at 10:26:42PM +0530, Sanjay Chitroda wrote:

> Use pm_ptr() and DEFINE_RUNTIME_DEV_PM_OPS() so the PM ops pointer is
> automatically handle when CONFIG_PM is enabled/disabled. This follows
> modern kernel power-management conventions.

After this patch there are still CONFIG_PM ifdeffery left, why?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/6] iio: accel: mma8452: use guard() to release mutexes

[Andy Shevchenko]

On Wed, Apr 22, 2026 at 10:26:43PM +0530, Sanjay Chitroda wrote:

> Replace explicit mutex_lock() and mutex_unlock() with the guard() and
> scoped_guard() macro for cleaner and safer mutex handling.

This one seems okay, but since it's not the majority of the changes that can
go, I won't give any tag right now, let's see v3 (but wait at least a couple of
days to give chances to others to review v2 in case I missed something).


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/6] iio: accel: mma8452: sort headers alphabetically

[Sanjay Chitroda]

On 23 April 2026 1:00:10 am IST, Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
>On Wed, Apr 22, 2026 at 10:26:39PM +0530, Sanjay Chitroda wrote:
>
>> Sort include headers alphabetically to improve readability.
>
>...
>
>> -#include <linux/module.h>
>> +#include <linux/delay.h>
>> +#include <linux/i2c.h>
>>  #include <linux/mod_devicetable.h>
>> +#include <linux/module.h>
>> +#include <linux/pm_runtime.h>
>>  #include <linux/property.h>
>> -#include <linux/i2c.h>
>> +#include <linux/types.h>
>> +
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/events.h>
>>  #include <linux/iio/iio.h>
>>  #include <linux/iio/sysfs.h>
>> -#include <linux/iio/buffer.h>
>>  #include <linux/iio/trigger.h>
>>  #include <linux/iio/trigger_consumer.h>
>>  #include <linux/iio/triggered_buffer.h>
>> -#include <linux/iio/events.h>
>> -#include <linux/delay.h>
>> -#include <linux/pm_runtime.h>
>
>> +
>>  #include <linux/regulator/consumer.h>
>> -#include <linux/types.h>
>
>For this driver the regulator/consumer is a regular header and can be moved to
>be between property.h and types.h.
>

Andy, Thank you for your review.
Along with alphabetic sort, I have also grouped different header(s), at top linux direct header, then iio headers and at last regulator headers for readability.

If you suggest, I would include this in commit description.

Thanks,
Sanjay Chitroda

Re: [PATCH v2 3/6] iio: accel: mma8452: use local struct device

[Sanjay Chitroda]

On 23 April 2026 1:05:33 am IST, Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
>On Wed, Apr 22, 2026 at 10:26:40PM +0530, Sanjay Chitroda wrote:
>
>> Introduce a local struct device pointer derived from &client->dev.
>> This avoids repeated &client->dev usage and improves readability.
>
>...
>
>> static int mma8452_set_runtime_pm_state(struct i2c_client *client, bool on)
>>  {
>>  #ifdef CONFIG_PM
>
>So, this seems to be a missed in the PM patch.
>
>>  	int ret;
>> +	struct device *dev = &client->dev;
>
>Keep it in reversed xmas tree order.
>
Understood, along with headers would attempt to keep all local variables also in chronological order.

>>  	if (on)
>> -		ret = pm_runtime_resume_and_get(&client->dev);
>> +		ret = pm_runtime_resume_and_get(dev);
>>  	else
>> -		ret = pm_runtime_put_autosuspend(&client->dev);
>> +		ret = pm_runtime_put_autosuspend(dev);
>>  	if (ret < 0) {
>> -		dev_err(&client->dev,
>> +		dev_err(dev,
>>  			"failed to change power state to %d\n", on);
>>  
>>  		return ret;
>
>>  }
>
>...
>
>>  static int mma8452_probe(struct i2c_client *client)
>>  {
>>  	struct mma8452_data *data;
>> +	struct device *dev = &client->dev;
>
>Same here, reversed xmas tree order.
>
>>  	struct iio_dev *indio_dev;
>>  	int ret;
>
Sure, understood.
>...
>
>>  	data->chip_info = i2c_get_match_data(client);
>>  	if (!data->chip_info)
>> -		return dev_err_probe(&client->dev, -ENODEV,
>> +		return dev_err_probe(dev, -ENODEV,
>>  				     "unknown device model\n");
>
>Now it's a single line.
>
>...
>
>>  	ret = regulator_enable(data->vdd_reg);
>>  	if (ret) {
>> -		dev_err(&client->dev, "failed to enable VDD regulator!\n");
>> +		dev_err(dev, "failed to enable VDD regulator!\n");
>>  		return ret;
>
>Convert this to dev_err_probe() before doing this patch. In that patch you
>introduce temporary dev variable already. Here will use in the other places.
>
Agree with you, will rebase in next series.
>>  	}
>>  
>>  	ret = regulator_enable(data->vddio_reg);
>>  	if (ret) {
>> -		dev_err(&client->dev, "failed to enable VDDIO regulator!\n");
>> +		dev_err(dev, "failed to enable VDDIO regulator!\n");
>>  		goto disable_regulator_vdd;
>
>Ditto.
>
Sure.
>>  	}
>
>...
>
>> +	pm_runtime_set_autosuspend_delay(dev,
>>  					 MMA8452_AUTO_SUSPEND_DELAY_MS);
>
>Now a single line.
>

Re: [PATCH v2 2/6] iio: accel: mma8452: sort headers alphabetically

[Andy Shevchenko]

On Thu, Apr 23, 2026 at 08:00:28AM +0530, Sanjay Chitroda wrote:
> On 23 April 2026 1:00:10 am IST, Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> >On Wed, Apr 22, 2026 at 10:26:39PM +0530, Sanjay Chitroda wrote:

...

> >For this driver the regulator/consumer is a regular header and can be moved
> >to be between property.h and types.h.

> Andy, Thank you for your review.
> Along with alphabetic sort, I have also grouped different header(s), at top
> linux direct header, then iio headers and at last regulator headers for
> readability.

The grouping of some subset of linux/* or other headers is done only for the
purpose of pointing out the strong relationship with the certain subsystem.
For example, driver of the regulator belongs to the regulator subsystem, and
grouping regulator headers (in case more than a single one in use) makes at
least some sense. This driver has nothing to do with the regulator subsystem,
it doesn't belong to it.

In the above I described just an example, the same applicable for any subsystem
and driver. I.o.w. use the common sense.

> If you suggest, I would include this in commit description.

Yes I do suggest to move it back to the generic linux/* group.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/6] iio: accel: mma8452: use local struct device

[Andy Shevchenko]

On Thu, Apr 23, 2026 at 08:06:20AM +0530, Sanjay Chitroda wrote:
> On 23 April 2026 1:05:33 am IST, Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> >On Wed, Apr 22, 2026 at 10:26:40PM +0530, Sanjay Chitroda wrote:

...

> >>  	int ret;
> >> +	struct device *dev = &client->dev;
> >
> >Keep it in reversed xmas tree order.
> >
> Understood, along with headers would attempt to keep all local variables also
> in chronological order.

I'm not sure. What does chronological mean in this case?

Reversed xmas tree order means to have longest lines first. The assignments may
have dependencies, hence must follow that first and then the reversed xmas
tree. Returned variable might go last in some cases even if it's slightly longer
than other variables for giving it a better visibility.


-- 
With Best Regards,
Andy Shevchenko