[PATCH 0/5] iio: accel: mma9551/mma9553 Cleanup and update

[PATCH 0/5] iio: accel: mma9551/mma9553 Cleanup and update

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

This series came about because I was looking to write a dt-binding for these
two (currently missing entirely) and I discovered the mma9551 driver in
particular was doing some unusual things.

I've left the gpio based interrupt stuff in there because it's possible
there are boards out there using it.  Note however, I've only tested the
fwnode_irq_get() patch using a hacked up version of QEMU and stubbing out
some error paths because I'm too lazy to emulate it properly ;)

The ACPI entries seem unlikely, but please shout if anyone knows of
them being used in the wild.

It would be particularly helpful if anyone who has either of these
parts could both give this a spin and let me know so I can ask
for testing in future.

Thanks,

Jonathan

Cc: Andy Shevchenko <andy.shevchenko@gmail.com>

Jonathan Cameron (5):
  iio: accel: mma9551/mma9553: Drop explicit ACPI match support
  iio: accel: mma9551/mma9553: Simplify pm logic
  iio: accel: mma9551: Add support to get irqs directly from fwnode
  iio: accel: mma9551: Use devm managed functions to tidy up probe()
  iio: accel: mma9553: Use devm managed functions to tidy up probe()

 drivers/iio/accel/mma9551.c | 156 ++++++++++++++----------------------
 drivers/iio/accel/mma9553.c | 121 +++++++++-------------------
 2 files changed, 96 insertions(+), 181 deletions(-)

-- 
2.31.1

[PATCH 1/5] iio: accel: mma9551/mma9553: Drop explicit ACPI match support

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

The IDs look highly suspicious as they are just the part numbers in
capitals, so lets drop these if no one screams.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/iio/accel/mma9551.c | 21 ---------------------
 drivers/iio/accel/mma9553.c | 21 ---------------------
 2 files changed, 42 deletions(-)

diff --git a/drivers/iio/accel/mma9551.c b/drivers/iio/accel/mma9551.c
index 4c359fb05480..2b74f67536a3 100644
--- a/drivers/iio/accel/mma9551.c
+++ b/drivers/iio/accel/mma9551.c
@@ -435,17 +435,6 @@ static int mma9551_gpio_probe(struct iio_dev *indio_dev)
 	return 0;
 }
 
-static const char *mma9551_match_acpi_device(struct device *dev)
-{
-	const struct acpi_device_id *id;
-
-	id = acpi_match_device(dev->driver->acpi_match_table, dev);
-	if (!id)
-		return NULL;
-
-	return dev_name(dev);
-}
-
 static int mma9551_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -464,8 +453,6 @@ static int mma9551_probe(struct i2c_client *client,
 
 	if (id)
 		name = id->name;
-	else if (ACPI_HANDLE(&client->dev))
-		name = mma9551_match_acpi_device(&client->dev);
 
 	ret = mma9551_init(data);
 	if (ret < 0)
@@ -591,13 +578,6 @@ static const struct dev_pm_ops mma9551_pm_ops = {
 			   mma9551_runtime_resume, NULL)
 };
 
-static const struct acpi_device_id mma9551_acpi_match[] = {
-	{"MMA9551", 0},
-	{},
-};
-
-MODULE_DEVICE_TABLE(acpi, mma9551_acpi_match);
-
 static const struct i2c_device_id mma9551_id[] = {
 	{"mma9551", 0},
 	{}
@@ -608,7 +588,6 @@ MODULE_DEVICE_TABLE(i2c, mma9551_id);
 static struct i2c_driver mma9551_driver = {
 	.driver = {
 		   .name = MMA9551_DRV_NAME,
-		   .acpi_match_table = ACPI_PTR(mma9551_acpi_match),
 		   .pm = &mma9551_pm_ops,
 		   },
 	.probe = mma9551_probe,
diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
index ba3ecb3b57dc..32c9a79ebfec 100644
--- a/drivers/iio/accel/mma9553.c
+++ b/drivers/iio/accel/mma9553.c
@@ -1062,17 +1062,6 @@ static irqreturn_t mma9553_event_handler(int irq, void *private)
 	return IRQ_HANDLED;
 }
 
-static const char *mma9553_match_acpi_device(struct device *dev)
-{
-	const struct acpi_device_id *id;
-
-	id = acpi_match_device(dev->driver->acpi_match_table, dev);
-	if (!id)
-		return NULL;
-
-	return dev_name(dev);
-}
-
 static int mma9553_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -1091,8 +1080,6 @@ static int mma9553_probe(struct i2c_client *client,
 
 	if (id)
 		name = id->name;
-	else if (ACPI_HANDLE(&client->dev))
-		name = mma9553_match_acpi_device(&client->dev);
 	else
 		return -ENOSYS;
 
@@ -1230,13 +1217,6 @@ static const struct dev_pm_ops mma9553_pm_ops = {
 			   mma9553_runtime_resume, NULL)
 };
 
-static const struct acpi_device_id mma9553_acpi_match[] = {
-	{"MMA9553", 0},
-	{},
-};
-
-MODULE_DEVICE_TABLE(acpi, mma9553_acpi_match);
-
 static const struct i2c_device_id mma9553_id[] = {
 	{"mma9553", 0},
 	{},
@@ -1247,7 +1227,6 @@ MODULE_DEVICE_TABLE(i2c, mma9553_id);
 static struct i2c_driver mma9553_driver = {
 	.driver = {
 		   .name = MMA9553_DRV_NAME,
-		   .acpi_match_table = ACPI_PTR(mma9553_acpi_match),
 		   .pm = &mma9553_pm_ops,
 		   },
 	.probe = mma9553_probe,
-- 
2.31.1

[PATCH 2/5] iio: accel: mma9551/mma9553: Simplify pm logic

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

I can't see why we shouldn't sleep in the system resume path to allow
the device firmware to fully wakeup.  Having done that, the runtime
system functions are identical (down to an error print) so use
pm_runtime_force_suspend() and pm_runtime_force_resume() to reduce
repitition.

General preference in IIO is now to mark these functions __maybe_unused
instead of using ifdefs as it is easy to get them wrong.
Here they appear correct, but provide a less than desirable example
to copy into other drivers.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/accel/mma9551.c | 37 ++++--------------------------------
 drivers/iio/accel/mma9553.c | 38 ++++---------------------------------
 2 files changed, 8 insertions(+), 67 deletions(-)

diff --git a/drivers/iio/accel/mma9551.c b/drivers/iio/accel/mma9551.c
index 2b74f67536a3..1b4a8b27f14a 100644
--- a/drivers/iio/accel/mma9551.c
+++ b/drivers/iio/accel/mma9551.c
@@ -510,8 +510,7 @@ static int mma9551_remove(struct i2c_client *client)
 	return 0;
 }
 
-#ifdef CONFIG_PM
-static int mma9551_runtime_suspend(struct device *dev)
+static __maybe_unused int mma9551_runtime_suspend(struct device *dev)
 {
 	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
 	struct mma9551_data *data = iio_priv(indio_dev);
@@ -522,13 +521,13 @@ static int mma9551_runtime_suspend(struct device *dev)
 	mutex_unlock(&data->mutex);
 	if (ret < 0) {
 		dev_err(&data->client->dev, "powering off device failed\n");
-		return -EAGAIN;
+		return ret;
 	}
 
 	return 0;
 }
 
-static int mma9551_runtime_resume(struct device *dev)
+static __maybe_unused int mma9551_runtime_resume(struct device *dev)
 {
 	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
 	struct mma9551_data *data = iio_priv(indio_dev);
@@ -542,38 +541,10 @@ static int mma9551_runtime_resume(struct device *dev)
 
 	return 0;
 }
-#endif
-
-#ifdef CONFIG_PM_SLEEP
-static int mma9551_suspend(struct device *dev)
-{
-	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
-	struct mma9551_data *data = iio_priv(indio_dev);
-	int ret;
-
-	mutex_lock(&data->mutex);
-	ret = mma9551_set_device_state(data->client, false);
-	mutex_unlock(&data->mutex);
-
-	return ret;
-}
-
-static int mma9551_resume(struct device *dev)
-{
-	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
-	struct mma9551_data *data = iio_priv(indio_dev);
-	int ret;
 
-	mutex_lock(&data->mutex);
-	ret = mma9551_set_device_state(data->client, true);
-	mutex_unlock(&data->mutex);
-
-	return ret;
-}
-#endif
 
 static const struct dev_pm_ops mma9551_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(mma9551_suspend, mma9551_resume)
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
 	SET_RUNTIME_PM_OPS(mma9551_runtime_suspend,
 			   mma9551_runtime_resume, NULL)
 };
diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
index 32c9a79ebfec..dc2a3316c1a3 100644
--- a/drivers/iio/accel/mma9553.c
+++ b/drivers/iio/accel/mma9553.c
@@ -1149,8 +1149,7 @@ static int mma9553_remove(struct i2c_client *client)
 	return 0;
 }
 
-#ifdef CONFIG_PM
-static int mma9553_runtime_suspend(struct device *dev)
+static __maybe_unused int mma9553_runtime_suspend(struct device *dev)
 {
 	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
 	struct mma9553_data *data = iio_priv(indio_dev);
@@ -1161,13 +1160,13 @@ static int mma9553_runtime_suspend(struct device *dev)
 	mutex_unlock(&data->mutex);
 	if (ret < 0) {
 		dev_err(&data->client->dev, "powering off device failed\n");
-		return -EAGAIN;
+		return ret;
 	}
 
 	return 0;
 }
 
-static int mma9553_runtime_resume(struct device *dev)
+static __maybe_unused int mma9553_runtime_resume(struct device *dev)
 {
 	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
 	struct mma9553_data *data = iio_priv(indio_dev);
@@ -1181,38 +1180,9 @@ static int mma9553_runtime_resume(struct device *dev)
 
 	return 0;
 }
-#endif
-
-#ifdef CONFIG_PM_SLEEP
-static int mma9553_suspend(struct device *dev)
-{
-	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
-	struct mma9553_data *data = iio_priv(indio_dev);
-	int ret;
-
-	mutex_lock(&data->mutex);
-	ret = mma9551_set_device_state(data->client, false);
-	mutex_unlock(&data->mutex);
-
-	return ret;
-}
-
-static int mma9553_resume(struct device *dev)
-{
-	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
-	struct mma9553_data *data = iio_priv(indio_dev);
-	int ret;
-
-	mutex_lock(&data->mutex);
-	ret = mma9551_set_device_state(data->client, true);
-	mutex_unlock(&data->mutex);
-
-	return ret;
-}
-#endif
 
 static const struct dev_pm_ops mma9553_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(mma9553_suspend, mma9553_resume)
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
 	SET_RUNTIME_PM_OPS(mma9553_runtime_suspend,
 			   mma9553_runtime_resume, NULL)
 };
-- 
2.31.1

[PATCH 3/5] iio: accel: mma9551: Add support to get irqs directly from fwnode

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

The driver previous supported using GPIO requests to retrieve
multiple interrupt lines.  As existing firmware may be using
this method, we need to continue to support it.  However, that doesn't
stop us also supporting just getting irqs directly.

The handling of irqflags has to take into account the fact that using
a GPIO method to identify the interrupt does not convey direction of
the trigger that fwnode_irq_get() will. So we need to set the
IRQF_TRIGGER_RISING in that path but not otherwise, where it will
cause an issue if we reprobe the driver after removal.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/accel/mma9551.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/accel/mma9551.c b/drivers/iio/accel/mma9551.c
index 1b4a8b27f14a..a0bb4ccdbec7 100644
--- a/drivers/iio/accel/mma9551.c
+++ b/drivers/iio/accel/mma9551.c
@@ -406,30 +406,37 @@ static int mma9551_gpio_probe(struct iio_dev *indio_dev)
 	int i, ret;
 	struct mma9551_data *data = iio_priv(indio_dev);
 	struct device *dev = &data->client->dev;
+	unsigned long irqflags = IRQF_ONESHOT;
 
 	for (i = 0; i < MMA9551_GPIO_COUNT; i++) {
-		gpio = devm_gpiod_get_index(dev, NULL, i, GPIOD_IN);
-		if (IS_ERR(gpio)) {
-			dev_err(dev, "acpi gpio get index failed\n");
-			return PTR_ERR(gpio);
-		}
-
-		ret = gpiod_to_irq(gpio);
-		if (ret < 0)
+		/* GPIO provided for backwards compatibility reasons */
+		ret = fwnode_irq_get(dev_fwnode(dev), i);
+		if (ret == -EPROBE_DEFER)
 			return ret;
 
+		/* fwnode_irq_get() returns 0 for not present on OF, and -EINVAL for ACPI */
+		if (ret == 0 || ret == -EINVAL) {
+			gpio = devm_gpiod_get_index(dev, NULL, i, GPIOD_IN);
+			if (IS_ERR(gpio)) {
+				dev_err(dev, "gpio get index failed\n");
+				return PTR_ERR(gpio);
+			}
+
+			ret = gpiod_to_irq(gpio);
+			if (ret < 0)
+				return ret;
+			/* GPIO interrupt does npt have a specified direction */
+			irqflags |= IRQF_TRIGGER_RISING;
+		}
 		data->irqs[i] = ret;
 		ret = devm_request_threaded_irq(dev, data->irqs[i],
-				NULL, mma9551_event_handler,
-				IRQF_TRIGGER_RISING | IRQF_ONESHOT,
-				MMA9551_IRQ_NAME, indio_dev);
+						NULL, mma9551_event_handler,
+						irqflags,
+						MMA9551_IRQ_NAME, indio_dev);
 		if (ret < 0) {
 			dev_err(dev, "request irq %d failed\n", data->irqs[i]);
 			return ret;
 		}
-
-		dev_dbg(dev, "gpio resource, no:%d irq:%d\n",
-			desc_to_gpio(gpio), data->irqs[i]);
 	}
 
 	return 0;
-- 
2.31.1

[PATCH 4/5] iio: accel: mma9551: Use devm managed functions to tidy up probe()

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

The error handling in here left runtime pm enabled, and didn't do the
same steps as occurred in remove.  Moving over to fully devm_ managed
makes it harder to get this stuff wrong, so let's do that.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/accel/mma9551.c | 71 ++++++++++++++++++++-----------------
 1 file changed, 38 insertions(+), 33 deletions(-)

diff --git a/drivers/iio/accel/mma9551.c b/drivers/iio/accel/mma9551.c
index a0bb4ccdbec7..f539772f8295 100644
--- a/drivers/iio/accel/mma9551.c
+++ b/drivers/iio/accel/mma9551.c
@@ -389,6 +389,15 @@ static irqreturn_t mma9551_event_handler(int irq, void *private)
 	return IRQ_HANDLED;
 }
 
+static void mma9551_disable_cb(void *_data)
+{
+	struct mma9551_data *data = _data;
+
+	mutex_lock(&data->mutex);
+	mma9551_set_device_state(data->client, false);
+	mutex_unlock(&data->mutex);
+}
+
 static int mma9551_init(struct mma9551_data *data)
 {
 	int ret;
@@ -397,7 +406,12 @@ static int mma9551_init(struct mma9551_data *data)
 	if (ret)
 		return ret;
 
-	return mma9551_set_device_state(data->client, true);
+	ret = mma9551_set_device_state(data->client, true);
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(&data->client->dev, mma9551_disable_cb,
+					data);
 }
 
 static int mma9551_gpio_probe(struct iio_dev *indio_dev)
@@ -442,6 +456,16 @@ static int mma9551_gpio_probe(struct iio_dev *indio_dev)
 	return 0;
 }
 
+static void mma9551_rpm_susp(void *d)
+{
+	pm_runtime_set_suspended(d);
+}
+
+static void mma9551_rpm_disable(void *d)
+{
+	pm_runtime_disable(d);
+}
+
 static int mma9551_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -475,46 +499,28 @@ static int mma9551_probe(struct i2c_client *client,
 
 	ret = mma9551_gpio_probe(indio_dev);
 	if (ret < 0)
-		goto out_poweroff;
+		return ret;
 
 	ret = pm_runtime_set_active(&client->dev);
 	if (ret < 0)
-		goto out_poweroff;
+		return ret;
+
+	ret = devm_add_action_or_reset(&client->dev, mma9551_rpm_susp,
+				       &client->dev);
+	if (ret)
+		return ret;
 
 	pm_runtime_enable(&client->dev);
+	ret = devm_add_action_or_reset(&client->dev, mma9551_rpm_disable,
+				       &client->dev);
+	if (ret)
+		return ret;
+
 	pm_runtime_set_autosuspend_delay(&client->dev,
 					 MMA9551_AUTO_SUSPEND_DELAY_MS);
 	pm_runtime_use_autosuspend(&client->dev);
 
-	ret = iio_device_register(indio_dev);
-	if (ret < 0) {
-		dev_err(&client->dev, "unable to register iio device\n");
-		goto out_poweroff;
-	}
-
-	return 0;
-
-out_poweroff:
-	mma9551_set_device_state(client, false);
-
-	return ret;
-}
-
-static int mma9551_remove(struct i2c_client *client)
-{
-	struct iio_dev *indio_dev = i2c_get_clientdata(client);
-	struct mma9551_data *data = iio_priv(indio_dev);
-
-	iio_device_unregister(indio_dev);
-
-	pm_runtime_disable(&client->dev);
-	pm_runtime_set_suspended(&client->dev);
-
-	mutex_lock(&data->mutex);
-	mma9551_set_device_state(data->client, false);
-	mutex_unlock(&data->mutex);
-
-	return 0;
+	return devm_iio_device_register(&client->dev, indio_dev);
 }
 
 static __maybe_unused int mma9551_runtime_suspend(struct device *dev)
@@ -569,7 +575,6 @@ static struct i2c_driver mma9551_driver = {
 		   .pm = &mma9551_pm_ops,
 		   },
 	.probe = mma9551_probe,
-	.remove = mma9551_remove,
 	.id_table = mma9551_id,
 };
 
-- 
2.31.1

[PATCH 5/5] iio: accel: mma9553: Use devm managed functions to tidy up probe()

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

The error handling in here left runtime pm enabled, and didn't do the
same steps as occurred in remove.  Moving over to fully devm_ managed
makes it harder to get this stuff wrong, so let's do that.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/accel/mma9553.c | 70 ++++++++++++++++++++-----------------
 1 file changed, 37 insertions(+), 33 deletions(-)

diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
index dc2a3316c1a3..14a9d5fedc06 100644
--- a/drivers/iio/accel/mma9553.c
+++ b/drivers/iio/accel/mma9553.c
@@ -375,6 +375,15 @@ static int mma9553_conf_gpio(struct mma9553_data *data)
 	return 0;
 }
 
+static void mma9553_disable_cb(void *_data)
+{
+	struct mma9553_data *data = _data;
+
+	mutex_lock(&data->mutex);
+	mma9551_set_device_state(data->client, false);
+	mutex_unlock(&data->mutex);
+}
+
 static int mma9553_init(struct mma9553_data *data)
 {
 	int ret;
@@ -430,7 +439,11 @@ static int mma9553_init(struct mma9553_data *data)
 		return ret;
 	}
 
-	return mma9551_set_device_state(data->client, true);
+	ret = mma9551_set_device_state(data->client, true);
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(&data->client->dev, mma9553_disable_cb, data);
 }
 
 static int mma9553_read_status_word(struct mma9553_data *data, u16 reg,
@@ -1062,6 +1075,16 @@ static irqreturn_t mma9553_event_handler(int irq, void *private)
 	return IRQ_HANDLED;
 }
 
+static void mma9553_rpm_set_susp(void *d)
+{
+	pm_runtime_set_suspended(d);
+}
+
+static void mma9553_rpm_disable(void *d)
+{
+	pm_runtime_disable(d);
+}
+
 static int mma9553_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -1105,48 +1128,30 @@ static int mma9553_probe(struct i2c_client *client,
 		if (ret < 0) {
 			dev_err(&client->dev, "request irq %d failed\n",
 				client->irq);
-			goto out_poweroff;
+			return ret;
 		}
 	}
 
 	ret = pm_runtime_set_active(&client->dev);
 	if (ret < 0)
-		goto out_poweroff;
+		return ret;
+
+	ret = devm_add_action_or_reset(&client->dev, mma9553_rpm_set_susp,
+				       &client->dev);
+	if (ret)
+		return ret;
 
 	pm_runtime_enable(&client->dev);
+	ret = devm_add_action_or_reset(&client->dev, mma9553_rpm_disable,
+				       &client->dev);
+	if (ret)
+		return ret;
+
 	pm_runtime_set_autosuspend_delay(&client->dev,
 					 MMA9551_AUTO_SUSPEND_DELAY_MS);
 	pm_runtime_use_autosuspend(&client->dev);
 
-	ret = iio_device_register(indio_dev);
-	if (ret < 0) {
-		dev_err(&client->dev, "unable to register iio device\n");
-		goto out_poweroff;
-	}
-
-	dev_dbg(&indio_dev->dev, "Registered device %s\n", name);
-	return 0;
-
-out_poweroff:
-	mma9551_set_device_state(client, false);
-	return ret;
-}
-
-static int mma9553_remove(struct i2c_client *client)
-{
-	struct iio_dev *indio_dev = i2c_get_clientdata(client);
-	struct mma9553_data *data = iio_priv(indio_dev);
-
-	iio_device_unregister(indio_dev);
-
-	pm_runtime_disable(&client->dev);
-	pm_runtime_set_suspended(&client->dev);
-
-	mutex_lock(&data->mutex);
-	mma9551_set_device_state(data->client, false);
-	mutex_unlock(&data->mutex);
-
-	return 0;
+	return devm_iio_device_register(&client->dev, indio_dev);
 }
 
 static __maybe_unused int mma9553_runtime_suspend(struct device *dev)
@@ -1200,7 +1205,6 @@ static struct i2c_driver mma9553_driver = {
 		   .pm = &mma9553_pm_ops,
 		   },
 	.probe = mma9553_probe,
-	.remove = mma9553_remove,
 	.id_table = mma9553_id,
 };
 
-- 
2.31.1

Re: [PATCH 1/5] iio: accel: mma9551/mma9553: Drop explicit ACPI match support

[Andy Shevchenko]

+Cc: Hans, who asked to Cc him on such patches.

On Sun, May 23, 2021 at 7:24 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> The IDs look highly suspicious as they are just the part numbers in
> capitals, so lets drop these if no one screams.

let's

I suggest adding some additional information to such commits (based on
what I just have done):
- MMA is registered PNP ID for Micromedia AG (is it part of Freescale?)
- Googling doesn't show any results for such ACPI _HID to be present in the wild

Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> ---
>  drivers/iio/accel/mma9551.c | 21 ---------------------
>  drivers/iio/accel/mma9553.c | 21 ---------------------
>  2 files changed, 42 deletions(-)
>
> diff --git a/drivers/iio/accel/mma9551.c b/drivers/iio/accel/mma9551.c
> index 4c359fb05480..2b74f67536a3 100644
> --- a/drivers/iio/accel/mma9551.c
> +++ b/drivers/iio/accel/mma9551.c
> @@ -435,17 +435,6 @@ static int mma9551_gpio_probe(struct iio_dev *indio_dev)
>         return 0;
>  }
>
> -static const char *mma9551_match_acpi_device(struct device *dev)
> -{
> -       const struct acpi_device_id *id;
> -
> -       id = acpi_match_device(dev->driver->acpi_match_table, dev);
> -       if (!id)
> -               return NULL;
> -
> -       return dev_name(dev);
> -}
> -
>  static int mma9551_probe(struct i2c_client *client,
>                          const struct i2c_device_id *id)
>  {
> @@ -464,8 +453,6 @@ static int mma9551_probe(struct i2c_client *client,
>
>         if (id)
>                 name = id->name;
> -       else if (ACPI_HANDLE(&client->dev))
> -               name = mma9551_match_acpi_device(&client->dev);
>
>         ret = mma9551_init(data);
>         if (ret < 0)
> @@ -591,13 +578,6 @@ static const struct dev_pm_ops mma9551_pm_ops = {
>                            mma9551_runtime_resume, NULL)
>  };
>
> -static const struct acpi_device_id mma9551_acpi_match[] = {
> -       {"MMA9551", 0},
> -       {},
> -};
> -
> -MODULE_DEVICE_TABLE(acpi, mma9551_acpi_match);
> -
>  static const struct i2c_device_id mma9551_id[] = {
>         {"mma9551", 0},
>         {}
> @@ -608,7 +588,6 @@ MODULE_DEVICE_TABLE(i2c, mma9551_id);
>  static struct i2c_driver mma9551_driver = {
>         .driver = {
>                    .name = MMA9551_DRV_NAME,
> -                  .acpi_match_table = ACPI_PTR(mma9551_acpi_match),
>                    .pm = &mma9551_pm_ops,
>                    },
>         .probe = mma9551_probe,
> diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
> index ba3ecb3b57dc..32c9a79ebfec 100644
> --- a/drivers/iio/accel/mma9553.c
> +++ b/drivers/iio/accel/mma9553.c
> @@ -1062,17 +1062,6 @@ static irqreturn_t mma9553_event_handler(int irq, void *private)
>         return IRQ_HANDLED;
>  }
>
> -static const char *mma9553_match_acpi_device(struct device *dev)
> -{
> -       const struct acpi_device_id *id;
> -
> -       id = acpi_match_device(dev->driver->acpi_match_table, dev);
> -       if (!id)
> -               return NULL;
> -
> -       return dev_name(dev);
> -}
> -
>  static int mma9553_probe(struct i2c_client *client,
>                          const struct i2c_device_id *id)
>  {
> @@ -1091,8 +1080,6 @@ static int mma9553_probe(struct i2c_client *client,
>
>         if (id)
>                 name = id->name;
> -       else if (ACPI_HANDLE(&client->dev))
> -               name = mma9553_match_acpi_device(&client->dev);
>         else
>                 return -ENOSYS;
>
> @@ -1230,13 +1217,6 @@ static const struct dev_pm_ops mma9553_pm_ops = {
>                            mma9553_runtime_resume, NULL)
>  };
>
> -static const struct acpi_device_id mma9553_acpi_match[] = {
> -       {"MMA9553", 0},
> -       {},
> -};
> -
> -MODULE_DEVICE_TABLE(acpi, mma9553_acpi_match);
> -
>  static const struct i2c_device_id mma9553_id[] = {
>         {"mma9553", 0},
>         {},
> @@ -1247,7 +1227,6 @@ MODULE_DEVICE_TABLE(i2c, mma9553_id);
>  static struct i2c_driver mma9553_driver = {
>         .driver = {
>                    .name = MMA9553_DRV_NAME,
> -                  .acpi_match_table = ACPI_PTR(mma9553_acpi_match),
>                    .pm = &mma9553_pm_ops,
>                    },
>         .probe = mma9553_probe,
> --
> 2.31.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/5] iio: accel: mma9551: Add support to get irqs directly from fwnode

[Andy Shevchenko]

On Sun, May 23, 2021 at 7:24 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> The driver previous supported using GPIO requests to retrieve

previously

> multiple interrupt lines.  As existing firmware may be using
> this method, we need to continue to support it.  However, that doesn't
> stop us also supporting just getting irqs directly.
>
> The handling of irqflags has to take into account the fact that using
> a GPIO method to identify the interrupt does not convey direction of
> the trigger that fwnode_irq_get() will. So we need to set the
> IRQF_TRIGGER_RISING in that path but not otherwise, where it will
> cause an issue if we reprobe the driver after removal.

...

> +               /* fwnode_irq_get() returns 0 for not present on OF, and -EINVAL for ACPI */
> +               if (ret == 0 || ret == -EINVAL) {
> +                       gpio = devm_gpiod_get_index(dev, NULL, i, GPIOD_IN);
> +                       if (IS_ERR(gpio)) {

> +                               dev_err(dev, "gpio get index failed\n");
> +                               return PTR_ERR(gpio);

This should be dev_err_probe().
(I guess you need to prepend this patch with one that switches to
dev_err_probe() API)

> +                       }
> +
> +                       ret = gpiod_to_irq(gpio);
> +                       if (ret < 0)
> +                               return ret;

> +                       /* GPIO interrupt does npt have a specified direction */
> +                       irqflags |= IRQF_TRIGGER_RISING;

I'm not sure I understand this part. If we are talking about the ACPI
GpioInt() resource, then it should have this flag. If GpioIo() is in
use (which is already a sign of either using the line in dual
direction mode, but this needs to be described in the data sheet and
thus used in the driver, or misdesigned ACPI tables). DT, I suppose,
should have all necessary information.

> +               }



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/5] iio: accel: mma9551/mma9553: Drop explicit ACPI match support

[Alexandru Ardelean]

On Sun, 23 May 2021 at 19:24, Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> The IDs look highly suspicious as they are just the part numbers in
> capitals, so lets drop these if no one screams.
>

I think some headers (at least acpi.h) could be removed with this patch.
Other than that:

Reviewed-by: Alexandru Ardelean <aardelean@deviqon.com>

> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> ---
>  drivers/iio/accel/mma9551.c | 21 ---------------------
>  drivers/iio/accel/mma9553.c | 21 ---------------------
>  2 files changed, 42 deletions(-)
>
> diff --git a/drivers/iio/accel/mma9551.c b/drivers/iio/accel/mma9551.c
> index 4c359fb05480..2b74f67536a3 100644
> --- a/drivers/iio/accel/mma9551.c
> +++ b/drivers/iio/accel/mma9551.c
> @@ -435,17 +435,6 @@ static int mma9551_gpio_probe(struct iio_dev *indio_dev)
>         return 0;
>  }
>
> -static const char *mma9551_match_acpi_device(struct device *dev)
> -{
> -       const struct acpi_device_id *id;
> -
> -       id = acpi_match_device(dev->driver->acpi_match_table, dev);
> -       if (!id)
> -               return NULL;
> -
> -       return dev_name(dev);
> -}
> -
>  static int mma9551_probe(struct i2c_client *client,
>                          const struct i2c_device_id *id)
>  {
> @@ -464,8 +453,6 @@ static int mma9551_probe(struct i2c_client *client,
>
>         if (id)
>                 name = id->name;
> -       else if (ACPI_HANDLE(&client->dev))
> -               name = mma9551_match_acpi_device(&client->dev);
>
>         ret = mma9551_init(data);
>         if (ret < 0)
> @@ -591,13 +578,6 @@ static const struct dev_pm_ops mma9551_pm_ops = {
>                            mma9551_runtime_resume, NULL)
>  };
>
> -static const struct acpi_device_id mma9551_acpi_match[] = {
> -       {"MMA9551", 0},
> -       {},
> -};
> -
> -MODULE_DEVICE_TABLE(acpi, mma9551_acpi_match);
> -
>  static const struct i2c_device_id mma9551_id[] = {
>         {"mma9551", 0},
>         {}
> @@ -608,7 +588,6 @@ MODULE_DEVICE_TABLE(i2c, mma9551_id);
>  static struct i2c_driver mma9551_driver = {
>         .driver = {
>                    .name = MMA9551_DRV_NAME,
> -                  .acpi_match_table = ACPI_PTR(mma9551_acpi_match),
>                    .pm = &mma9551_pm_ops,
>                    },
>         .probe = mma9551_probe,
> diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
> index ba3ecb3b57dc..32c9a79ebfec 100644
> --- a/drivers/iio/accel/mma9553.c
> +++ b/drivers/iio/accel/mma9553.c
> @@ -1062,17 +1062,6 @@ static irqreturn_t mma9553_event_handler(int irq, void *private)
>         return IRQ_HANDLED;
>  }
>
> -static const char *mma9553_match_acpi_device(struct device *dev)
> -{
> -       const struct acpi_device_id *id;
> -
> -       id = acpi_match_device(dev->driver->acpi_match_table, dev);
> -       if (!id)
> -               return NULL;
> -
> -       return dev_name(dev);
> -}
> -
>  static int mma9553_probe(struct i2c_client *client,
>                          const struct i2c_device_id *id)
>  {
> @@ -1091,8 +1080,6 @@ static int mma9553_probe(struct i2c_client *client,
>
>         if (id)
>                 name = id->name;
> -       else if (ACPI_HANDLE(&client->dev))
> -               name = mma9553_match_acpi_device(&client->dev);
>         else
>                 return -ENOSYS;
>
> @@ -1230,13 +1217,6 @@ static const struct dev_pm_ops mma9553_pm_ops = {
>                            mma9553_runtime_resume, NULL)
>  };
>
> -static const struct acpi_device_id mma9553_acpi_match[] = {
> -       {"MMA9553", 0},
> -       {},
> -};
> -
> -MODULE_DEVICE_TABLE(acpi, mma9553_acpi_match);
> -
>  static const struct i2c_device_id mma9553_id[] = {
>         {"mma9553", 0},
>         {},
> @@ -1247,7 +1227,6 @@ MODULE_DEVICE_TABLE(i2c, mma9553_id);
>  static struct i2c_driver mma9553_driver = {
>         .driver = {
>                    .name = MMA9553_DRV_NAME,
> -                  .acpi_match_table = ACPI_PTR(mma9553_acpi_match),
>                    .pm = &mma9553_pm_ops,
>                    },
>         .probe = mma9553_probe,
> --
> 2.31.1
>

Re: [PATCH 2/5] iio: accel: mma9551/mma9553: Simplify pm logic

[Alexandru Ardelean]

On Sun, 23 May 2021 at 19:24, Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> I can't see why we shouldn't sleep in the system resume path to allow
> the device firmware to fully wakeup.  Having done that, the runtime
> system functions are identical (down to an error print) so use
> pm_runtime_force_suspend() and pm_runtime_force_resume() to reduce
> repitition.
>
> General preference in IIO is now to mark these functions __maybe_unused
> instead of using ifdefs as it is easy to get them wrong.
> Here they appear correct, but provide a less than desirable example
> to copy into other drivers.
>

Reviewed-by: Alexandru Ardelean <aardelean@deviqon.com>

> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/iio/accel/mma9551.c | 37 ++++--------------------------------
>  drivers/iio/accel/mma9553.c | 38 ++++---------------------------------
>  2 files changed, 8 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/iio/accel/mma9551.c b/drivers/iio/accel/mma9551.c
> index 2b74f67536a3..1b4a8b27f14a 100644
> --- a/drivers/iio/accel/mma9551.c
> +++ b/drivers/iio/accel/mma9551.c
> @@ -510,8 +510,7 @@ static int mma9551_remove(struct i2c_client *client)
>         return 0;
>  }
>
> -#ifdef CONFIG_PM
> -static int mma9551_runtime_suspend(struct device *dev)
> +static __maybe_unused int mma9551_runtime_suspend(struct device *dev)
>  {
>         struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>         struct mma9551_data *data = iio_priv(indio_dev);
> @@ -522,13 +521,13 @@ static int mma9551_runtime_suspend(struct device *dev)
>         mutex_unlock(&data->mutex);
>         if (ret < 0) {
>                 dev_err(&data->client->dev, "powering off device failed\n");
> -               return -EAGAIN;
> +               return ret;
>         }
>
>         return 0;
>  }
>
> -static int mma9551_runtime_resume(struct device *dev)
> +static __maybe_unused int mma9551_runtime_resume(struct device *dev)
>  {
>         struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>         struct mma9551_data *data = iio_priv(indio_dev);
> @@ -542,38 +541,10 @@ static int mma9551_runtime_resume(struct device *dev)
>
>         return 0;
>  }
> -#endif
> -
> -#ifdef CONFIG_PM_SLEEP
> -static int mma9551_suspend(struct device *dev)
> -{
> -       struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> -       struct mma9551_data *data = iio_priv(indio_dev);
> -       int ret;
> -
> -       mutex_lock(&data->mutex);
> -       ret = mma9551_set_device_state(data->client, false);
> -       mutex_unlock(&data->mutex);
> -
> -       return ret;
> -}
> -
> -static int mma9551_resume(struct device *dev)
> -{
> -       struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> -       struct mma9551_data *data = iio_priv(indio_dev);
> -       int ret;
>
> -       mutex_lock(&data->mutex);
> -       ret = mma9551_set_device_state(data->client, true);
> -       mutex_unlock(&data->mutex);
> -
> -       return ret;
> -}
> -#endif
>
>  static const struct dev_pm_ops mma9551_pm_ops = {
> -       SET_SYSTEM_SLEEP_PM_OPS(mma9551_suspend, mma9551_resume)
> +       SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
>         SET_RUNTIME_PM_OPS(mma9551_runtime_suspend,
>                            mma9551_runtime_resume, NULL)
>  };
> diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
> index 32c9a79ebfec..dc2a3316c1a3 100644
> --- a/drivers/iio/accel/mma9553.c
> +++ b/drivers/iio/accel/mma9553.c
> @@ -1149,8 +1149,7 @@ static int mma9553_remove(struct i2c_client *client)
>         return 0;
>  }
>
> -#ifdef CONFIG_PM
> -static int mma9553_runtime_suspend(struct device *dev)
> +static __maybe_unused int mma9553_runtime_suspend(struct device *dev)
>  {
>         struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>         struct mma9553_data *data = iio_priv(indio_dev);
> @@ -1161,13 +1160,13 @@ static int mma9553_runtime_suspend(struct device *dev)
>         mutex_unlock(&data->mutex);
>         if (ret < 0) {
>                 dev_err(&data->client->dev, "powering off device failed\n");
> -               return -EAGAIN;
> +               return ret;
>         }
>
>         return 0;
>  }
>
> -static int mma9553_runtime_resume(struct device *dev)
> +static __maybe_unused int mma9553_runtime_resume(struct device *dev)
>  {
>         struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>         struct mma9553_data *data = iio_priv(indio_dev);
> @@ -1181,38 +1180,9 @@ static int mma9553_runtime_resume(struct device *dev)
>
>         return 0;
>  }
> -#endif
> -
> -#ifdef CONFIG_PM_SLEEP
> -static int mma9553_suspend(struct device *dev)
> -{
> -       struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> -       struct mma9553_data *data = iio_priv(indio_dev);
> -       int ret;
> -
> -       mutex_lock(&data->mutex);
> -       ret = mma9551_set_device_state(data->client, false);
> -       mutex_unlock(&data->mutex);
> -
> -       return ret;
> -}
> -
> -static int mma9553_resume(struct device *dev)
> -{
> -       struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> -       struct mma9553_data *data = iio_priv(indio_dev);
> -       int ret;
> -
> -       mutex_lock(&data->mutex);
> -       ret = mma9551_set_device_state(data->client, true);
> -       mutex_unlock(&data->mutex);
> -
> -       return ret;
> -}
> -#endif
>
>  static const struct dev_pm_ops mma9553_pm_ops = {
> -       SET_SYSTEM_SLEEP_PM_OPS(mma9553_suspend, mma9553_resume)
> +       SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
>         SET_RUNTIME_PM_OPS(mma9553_runtime_suspend,
>                            mma9553_runtime_resume, NULL)
>  };
> --
> 2.31.1
>

Re: [PATCH 4/5] iio: accel: mma9551: Use devm managed functions to tidy up probe()

[Alexandru Ardelean]

On Sun, 23 May 2021 at 19:24, Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> The error handling in here left runtime pm enabled, and didn't do the
> same steps as occurred in remove.  Moving over to fully devm_ managed
> makes it harder to get this stuff wrong, so let's do that.
>

Reviewed-by: Alexandru Ardelean <aardelean@deviqon.com>

> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/iio/accel/mma9551.c | 71 ++++++++++++++++++++-----------------
>  1 file changed, 38 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/iio/accel/mma9551.c b/drivers/iio/accel/mma9551.c
> index a0bb4ccdbec7..f539772f8295 100644
> --- a/drivers/iio/accel/mma9551.c
> +++ b/drivers/iio/accel/mma9551.c
> @@ -389,6 +389,15 @@ static irqreturn_t mma9551_event_handler(int irq, void *private)
>         return IRQ_HANDLED;
>  }
>
> +static void mma9551_disable_cb(void *_data)
> +{
> +       struct mma9551_data *data = _data;
> +
> +       mutex_lock(&data->mutex);
> +       mma9551_set_device_state(data->client, false);
> +       mutex_unlock(&data->mutex);
> +}
> +
>  static int mma9551_init(struct mma9551_data *data)
>  {
>         int ret;
> @@ -397,7 +406,12 @@ static int mma9551_init(struct mma9551_data *data)
>         if (ret)
>                 return ret;
>
> -       return mma9551_set_device_state(data->client, true);
> +       ret = mma9551_set_device_state(data->client, true);
> +       if (ret)
> +               return ret;
> +
> +       return devm_add_action_or_reset(&data->client->dev, mma9551_disable_cb,
> +                                       data);
>  }
>
>  static int mma9551_gpio_probe(struct iio_dev *indio_dev)
> @@ -442,6 +456,16 @@ static int mma9551_gpio_probe(struct iio_dev *indio_dev)
>         return 0;
>  }
>
> +static void mma9551_rpm_susp(void *d)
> +{
> +       pm_runtime_set_suspended(d);
> +}
> +
> +static void mma9551_rpm_disable(void *d)
> +{
> +       pm_runtime_disable(d);
> +}
> +
>  static int mma9551_probe(struct i2c_client *client,
>                          const struct i2c_device_id *id)
>  {
> @@ -475,46 +499,28 @@ static int mma9551_probe(struct i2c_client *client,
>
>         ret = mma9551_gpio_probe(indio_dev);
>         if (ret < 0)
> -               goto out_poweroff;
> +               return ret;
>
>         ret = pm_runtime_set_active(&client->dev);
>         if (ret < 0)
> -               goto out_poweroff;
> +               return ret;
> +
> +       ret = devm_add_action_or_reset(&client->dev, mma9551_rpm_susp,
> +                                      &client->dev);
> +       if (ret)
> +               return ret;
>
>         pm_runtime_enable(&client->dev);
> +       ret = devm_add_action_or_reset(&client->dev, mma9551_rpm_disable,
> +                                      &client->dev);
> +       if (ret)
> +               return ret;
> +
>         pm_runtime_set_autosuspend_delay(&client->dev,
>                                          MMA9551_AUTO_SUSPEND_DELAY_MS);
>         pm_runtime_use_autosuspend(&client->dev);
>
> -       ret = iio_device_register(indio_dev);
> -       if (ret < 0) {
> -               dev_err(&client->dev, "unable to register iio device\n");
> -               goto out_poweroff;
> -       }
> -
> -       return 0;
> -
> -out_poweroff:
> -       mma9551_set_device_state(client, false);
> -
> -       return ret;
> -}
> -
> -static int mma9551_remove(struct i2c_client *client)
> -{
> -       struct iio_dev *indio_dev = i2c_get_clientdata(client);
> -       struct mma9551_data *data = iio_priv(indio_dev);
> -
> -       iio_device_unregister(indio_dev);
> -
> -       pm_runtime_disable(&client->dev);
> -       pm_runtime_set_suspended(&client->dev);
> -
> -       mutex_lock(&data->mutex);
> -       mma9551_set_device_state(data->client, false);
> -       mutex_unlock(&data->mutex);
> -
> -       return 0;
> +       return devm_iio_device_register(&client->dev, indio_dev);
>  }
>
>  static __maybe_unused int mma9551_runtime_suspend(struct device *dev)
> @@ -569,7 +575,6 @@ static struct i2c_driver mma9551_driver = {
>                    .pm = &mma9551_pm_ops,
>                    },
>         .probe = mma9551_probe,
> -       .remove = mma9551_remove,
>         .id_table = mma9551_id,
>  };
>
> --
> 2.31.1
>

Re: [PATCH 5/5] iio: accel: mma9553: Use devm managed functions to tidy up probe()

[Alexandru Ardelean]

On Sun, 23 May 2021 at 19:24, Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> The error handling in here left runtime pm enabled, and didn't do the
> same steps as occurred in remove.  Moving over to fully devm_ managed
> makes it harder to get this stuff wrong, so let's do that.
>

Reviewed-by: Alexandru Ardelean <aardelean@deviqon.com>

> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/iio/accel/mma9553.c | 70 ++++++++++++++++++++-----------------
>  1 file changed, 37 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
> index dc2a3316c1a3..14a9d5fedc06 100644
> --- a/drivers/iio/accel/mma9553.c
> +++ b/drivers/iio/accel/mma9553.c
> @@ -375,6 +375,15 @@ static int mma9553_conf_gpio(struct mma9553_data *data)
>         return 0;
>  }
>
> +static void mma9553_disable_cb(void *_data)
> +{
> +       struct mma9553_data *data = _data;
> +
> +       mutex_lock(&data->mutex);
> +       mma9551_set_device_state(data->client, false);
> +       mutex_unlock(&data->mutex);
> +}
> +
>  static int mma9553_init(struct mma9553_data *data)
>  {
>         int ret;
> @@ -430,7 +439,11 @@ static int mma9553_init(struct mma9553_data *data)
>                 return ret;
>         }
>
> -       return mma9551_set_device_state(data->client, true);
> +       ret = mma9551_set_device_state(data->client, true);
> +       if (ret)
> +               return ret;
> +
> +       return devm_add_action_or_reset(&data->client->dev, mma9553_disable_cb, data);
>  }
>
>  static int mma9553_read_status_word(struct mma9553_data *data, u16 reg,
> @@ -1062,6 +1075,16 @@ static irqreturn_t mma9553_event_handler(int irq, void *private)
>         return IRQ_HANDLED;
>  }
>
> +static void mma9553_rpm_set_susp(void *d)
> +{
> +       pm_runtime_set_suspended(d);
> +}
> +
> +static void mma9553_rpm_disable(void *d)
> +{
> +       pm_runtime_disable(d);
> +}
> +
>  static int mma9553_probe(struct i2c_client *client,
>                          const struct i2c_device_id *id)
>  {
> @@ -1105,48 +1128,30 @@ static int mma9553_probe(struct i2c_client *client,
>                 if (ret < 0) {
>                         dev_err(&client->dev, "request irq %d failed\n",
>                                 client->irq);
> -                       goto out_poweroff;
> +                       return ret;
>                 }
>         }
>
>         ret = pm_runtime_set_active(&client->dev);
>         if (ret < 0)
> -               goto out_poweroff;
> +               return ret;
> +
> +       ret = devm_add_action_or_reset(&client->dev, mma9553_rpm_set_susp,
> +                                      &client->dev);
> +       if (ret)
> +               return ret;
>
>         pm_runtime_enable(&client->dev);
> +       ret = devm_add_action_or_reset(&client->dev, mma9553_rpm_disable,
> +                                      &client->dev);
> +       if (ret)
> +               return ret;
> +
>         pm_runtime_set_autosuspend_delay(&client->dev,
>                                          MMA9551_AUTO_SUSPEND_DELAY_MS);
>         pm_runtime_use_autosuspend(&client->dev);
>
> -       ret = iio_device_register(indio_dev);
> -       if (ret < 0) {
> -               dev_err(&client->dev, "unable to register iio device\n");
> -               goto out_poweroff;
> -       }
> -
> -       dev_dbg(&indio_dev->dev, "Registered device %s\n", name);
> -       return 0;
> -
> -out_poweroff:
> -       mma9551_set_device_state(client, false);
> -       return ret;
> -}
> -
> -static int mma9553_remove(struct i2c_client *client)
> -{
> -       struct iio_dev *indio_dev = i2c_get_clientdata(client);
> -       struct mma9553_data *data = iio_priv(indio_dev);
> -
> -       iio_device_unregister(indio_dev);
> -
> -       pm_runtime_disable(&client->dev);
> -       pm_runtime_set_suspended(&client->dev);
> -
> -       mutex_lock(&data->mutex);
> -       mma9551_set_device_state(data->client, false);
> -       mutex_unlock(&data->mutex);
> -
> -       return 0;
> +       return devm_iio_device_register(&client->dev, indio_dev);
>  }
>
>  static __maybe_unused int mma9553_runtime_suspend(struct device *dev)
> @@ -1200,7 +1205,6 @@ static struct i2c_driver mma9553_driver = {
>                    .pm = &mma9553_pm_ops,
>                    },
>         .probe = mma9553_probe,
> -       .remove = mma9553_remove,
>         .id_table = mma9553_id,
>  };
>
> --
> 2.31.1
>

Re: [PATCH 3/5] iio: accel: mma9551: Add support to get irqs directly from fwnode

[Alexandru Ardelean]

On Sun, 23 May 2021 at 19:24, Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> The driver previous supported using GPIO requests to retrieve
> multiple interrupt lines.  As existing firmware may be using
> this method, we need to continue to support it.  However, that doesn't
> stop us also supporting just getting irqs directly.
>
> The handling of irqflags has to take into account the fact that using
> a GPIO method to identify the interrupt does not convey direction of
> the trigger that fwnode_irq_get() will. So we need to set the
> IRQF_TRIGGER_RISING in that path but not otherwise, where it will
> cause an issue if we reprobe the driver after removal.
>

I'm not too experienced here [with this GPIO/IRQ API] to be able to
review this confidently.

> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/iio/accel/mma9551.c | 35 +++++++++++++++++++++--------------
>  1 file changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/iio/accel/mma9551.c b/drivers/iio/accel/mma9551.c
> index 1b4a8b27f14a..a0bb4ccdbec7 100644
> --- a/drivers/iio/accel/mma9551.c
> +++ b/drivers/iio/accel/mma9551.c
> @@ -406,30 +406,37 @@ static int mma9551_gpio_probe(struct iio_dev *indio_dev)
>         int i, ret;
>         struct mma9551_data *data = iio_priv(indio_dev);
>         struct device *dev = &data->client->dev;
> +       unsigned long irqflags = IRQF_ONESHOT;
>
>         for (i = 0; i < MMA9551_GPIO_COUNT; i++) {
> -               gpio = devm_gpiod_get_index(dev, NULL, i, GPIOD_IN);
> -               if (IS_ERR(gpio)) {
> -                       dev_err(dev, "acpi gpio get index failed\n");
> -                       return PTR_ERR(gpio);
> -               }
> -
> -               ret = gpiod_to_irq(gpio);
> -               if (ret < 0)
> +               /* GPIO provided for backwards compatibility reasons */
> +               ret = fwnode_irq_get(dev_fwnode(dev), i);
> +               if (ret == -EPROBE_DEFER)
>                         return ret;
>
> +               /* fwnode_irq_get() returns 0 for not present on OF, and -EINVAL for ACPI */
> +               if (ret == 0 || ret == -EINVAL) {
> +                       gpio = devm_gpiod_get_index(dev, NULL, i, GPIOD_IN);
> +                       if (IS_ERR(gpio)) {
> +                               dev_err(dev, "gpio get index failed\n");
> +                               return PTR_ERR(gpio);
> +                       }
> +
> +                       ret = gpiod_to_irq(gpio);
> +                       if (ret < 0)
> +                               return ret;
> +                       /* GPIO interrupt does npt have a specified direction */
> +                       irqflags |= IRQF_TRIGGER_RISING;
> +               }
>                 data->irqs[i] = ret;
>                 ret = devm_request_threaded_irq(dev, data->irqs[i],
> -                               NULL, mma9551_event_handler,
> -                               IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> -                               MMA9551_IRQ_NAME, indio_dev);
> +                                               NULL, mma9551_event_handler,
> +                                               irqflags,
> +                                               MMA9551_IRQ_NAME, indio_dev);
>                 if (ret < 0) {
>                         dev_err(dev, "request irq %d failed\n", data->irqs[i]);
>                         return ret;
>                 }
> -
> -               dev_dbg(dev, "gpio resource, no:%d irq:%d\n",
> -                       desc_to_gpio(gpio), data->irqs[i]);
>         }
>
>         return 0;
> --
> 2.31.1
>

Re: [PATCH 3/5] iio: accel: mma9551: Add support to get irqs directly from fwnode

[Jonathan Cameron]

On Mon, 24 May 2021 09:13:30 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sun, May 23, 2021 at 7:24 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > The driver previous supported using GPIO requests to retrieve  
> 
> previously
> 
> > multiple interrupt lines.  As existing firmware may be using
> > this method, we need to continue to support it.  However, that doesn't
> > stop us also supporting just getting irqs directly.
> >
> > The handling of irqflags has to take into account the fact that using
> > a GPIO method to identify the interrupt does not convey direction of
> > the trigger that fwnode_irq_get() will. So we need to set the
> > IRQF_TRIGGER_RISING in that path but not otherwise, where it will
> > cause an issue if we reprobe the driver after removal.  
> 
> ...
> 
> > +               /* fwnode_irq_get() returns 0 for not present on OF, and -EINVAL for ACPI */
> > +               if (ret == 0 || ret == -EINVAL) {
> > +                       gpio = devm_gpiod_get_index(dev, NULL, i, GPIOD_IN);
> > +                       if (IS_ERR(gpio)) {  
> 
> > +                               dev_err(dev, "gpio get index failed\n");
> > +                               return PTR_ERR(gpio);  
> 
> This should be dev_err_probe().
> (I guess you need to prepend this patch with one that switches to
> dev_err_probe() API)
> 
> > +                       }
> > +
> > +                       ret = gpiod_to_irq(gpio);
> > +                       if (ret < 0)
> > +                               return ret;  
> 
> > +                       /* GPIO interrupt does npt have a specified direction */

Gah. What is it with me and spelling in comments...

> > +                       irqflags |= IRQF_TRIGGER_RISING;  
> 
> I'm not sure I understand this part. If we are talking about the ACPI
> GpioInt() resource, then it should have this flag. If GpioIo() is in
> use (which is already a sign of either using the line in dual
> direction mode, but this needs to be described in the data sheet and
> thus used in the driver, or misdesigned ACPI tables). DT, I suppose,
> should have all necessary information.

Honestly I have no idea.  I didn't want to change the exiting flags without
any visibility of what the ACPI tables look like (assuming they exist).
Given I'm proposing killing of the ID, chances are ACPI is broken anyway
now :)  So, more risky is DT out there that just specifies this as a
GPIO.

Plan B would be to just drop the GPIO support entirely.

Would GpioInt() get picked up by the the fwnode_irq_get() path?

I'm guessing these were on a dev board 6+ years ago, but whilst I can
find references to the mma9553 on some freescale platforms, not finding
much on the mma9551.

Looking a bit deeper they are both listed as obsolete parts now (according to
digikey as I can't find status on nxp.com)
...  So plan C is just remove the drivers on the basis they are significantly
odd and we don't know of a platform anyone cares about with them on.

Mind you, aside from having a lack of documented bindings (which was what was
annoying me, they aren't doing any harm or causing any real maintenance burden.)
More than possible someone out there is using them.  The mm9953 appears on the
warpboard.org reference platform, but seems the sensor was never enabled upstream.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/arch/arm/boot/dts/imx6sl-warp.dts
https://revotics.com/warp?v=a284e24d5f46

Also, only some passing references in there, so I'd guess it got dropped in
later revisions?  Shaun, any ideas?

Jonathan


> 
> > +               }  
> 
> 
> 

Re: [PATCH 3/5] iio: accel: mma9551: Add support to get irqs directly from fwnode

[Jonathan Cameron]

On Mon, 24 May 2021 10:27:36 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Mon, 24 May 2021 09:13:30 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
> > On Sun, May 23, 2021 at 7:24 PM Jonathan Cameron <jic23@kernel.org> wrote:  
> > >
> > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > >
> > > The driver previous supported using GPIO requests to retrieve    
> > 
> > previously
> >   
> > > multiple interrupt lines.  As existing firmware may be using
> > > this method, we need to continue to support it.  However, that doesn't
> > > stop us also supporting just getting irqs directly.
> > >
> > > The handling of irqflags has to take into account the fact that using
> > > a GPIO method to identify the interrupt does not convey direction of
> > > the trigger that fwnode_irq_get() will. So we need to set the
> > > IRQF_TRIGGER_RISING in that path but not otherwise, where it will
> > > cause an issue if we reprobe the driver after removal.    
> > 
> > ...
> >   
> > > +               /* fwnode_irq_get() returns 0 for not present on OF, and -EINVAL for ACPI */
> > > +               if (ret == 0 || ret == -EINVAL) {
> > > +                       gpio = devm_gpiod_get_index(dev, NULL, i, GPIOD_IN);
> > > +                       if (IS_ERR(gpio)) {    
> >   
> > > +                               dev_err(dev, "gpio get index failed\n");
> > > +                               return PTR_ERR(gpio);    
> > 
> > This should be dev_err_probe().
> > (I guess you need to prepend this patch with one that switches to
> > dev_err_probe() API)
> >   
> > > +                       }
> > > +
> > > +                       ret = gpiod_to_irq(gpio);
> > > +                       if (ret < 0)
> > > +                               return ret;    
> >   
> > > +                       /* GPIO interrupt does npt have a specified direction */  
> 
> Gah. What is it with me and spelling in comments...
> 
> > > +                       irqflags |= IRQF_TRIGGER_RISING;    
> > 
> > I'm not sure I understand this part. If we are talking about the ACPI
> > GpioInt() resource, then it should have this flag. If GpioIo() is in
> > use (which is already a sign of either using the line in dual
> > direction mode, but this needs to be described in the data sheet and
> > thus used in the driver, or misdesigned ACPI tables). DT, I suppose,
> > should have all necessary information.  
> 
> Honestly I have no idea.  I didn't want to change the exiting flags without
> any visibility of what the ACPI tables look like (assuming they exist).
> Given I'm proposing killing of the ID, chances are ACPI is broken anyway
> now :)  So, more risky is DT out there that just specifies this as a
> GPIO.
> 
> Plan B would be to just drop the GPIO support entirely.
> 
> Would GpioInt() get picked up by the the fwnode_irq_get() path?
> 
> I'm guessing these were on a dev board 6+ years ago, but whilst I can
> find references to the mma9553 on some freescale platforms, not finding
> much on the mma9551.
> 
> Looking a bit deeper they are both listed as obsolete parts now (according to
> digikey as I can't find status on nxp.com)
> ...  So plan C is just remove the drivers on the basis they are significantly
> odd and we don't know of a platform anyone cares about with them on.
> 
> Mind you, aside from having a lack of documented bindings (which was what was
> annoying me, they aren't doing any harm or causing any real maintenance burden.)
> More than possible someone out there is using them.  The mm9953 appears on the
> warpboard.org reference platform, but seems the sensor was never enabled upstream.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/arch/arm/boot/dts/imx6sl-warp.dts
> https://revotics.com/warp?v=a284e24d5f46
> 
> Also, only some passing references in there, so I'd guess it got dropped in
> later revisions?  Shaun, any ideas?

I'm going to gamble a bit here and just drop the gpio support entirely.
We don't have any known boards out there running this driver so I 'might'
break someones hobby board, but hopefully they'll fix up their DT.

Without a confirmed user I'm not keen to maintain the complexity.

Jonathan

> 
> Jonathan
> 
> 
> >   
> > > +               }    
> > 
> > 
> >   
>