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

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

[Jonathan Cameron]

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

Whilst testing would be ideal, getting some eyes on this should be sufficient
if we can't find anyone with a test part.  Note that patch 3 needs most
attention as it currently has no review tags. 

v3: Updated commit log for patch 3 which didn't reflect the v2 changes (oops).
    Picked up Hans' tag for patch 1 (thanks)
v2: mma9551: Drop the gpio based irq support as not known to be used
    and adds complexity which is nice to get rid of.

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.

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.

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 | 151 ++++++++++++------------------------
 drivers/iio/accel/mma9553.c | 121 +++++++++--------------------
 2 files changed, 85 insertions(+), 187 deletions(-)

-- 
2.34.1

[PATCH v3 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.

Note:
- MMA is registered PNP ID for Micromedia AG (not a part of Freescale)
- Googling doesn't show any results for such ACPI _HID to be present
  in the wild

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Alexandru Ardelean <aardelean@deviqon.com>
Acked-by: Hans de Goede <hdegoede@redhat.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 0570ab1cc064..97704c681098 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.34.1

[PATCH v3 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>
Reviewed-by: Alexandru Ardelean <aardelean@deviqon.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 97704c681098..c24384089714 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.34.1

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

[Jonathan Cameron]

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

Note the interrupt type should be specified by firmware, not the driver
so that is also dropped.

Drop previous gpio based retrieval method. Whilst in theory this
might cause problems with direction if anyone is using ACPI GioIo().
As Andy described in v1, such a situation would typically reflect
a pin that is actually used in both directions (not true here)
or missdesigned ACPI tables.

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

diff --git a/drivers/iio/accel/mma9551.c b/drivers/iio/accel/mma9551.c
index 1b4a8b27f14a..537a7a04654a 100644
--- a/drivers/iio/accel/mma9551.c
+++ b/drivers/iio/accel/mma9551.c
@@ -19,7 +19,7 @@
 
 #define MMA9551_DRV_NAME		"mma9551"
 #define MMA9551_IRQ_NAME		"mma9551_event"
-#define MMA9551_GPIO_COUNT		4
+#define MMA9551_IRQ_COUNT		4
 
 /* Tilt application (inclination in IIO terms). */
 #define MMA9551_TILT_XZ_ANG_REG		0x00
@@ -46,7 +46,7 @@ struct mma9551_data {
 	struct i2c_client *client;
 	struct mutex mutex;
 	int event_enabled[3];
-	int irqs[MMA9551_GPIO_COUNT];
+	int irqs[MMA9551_IRQ_COUNT];
 };
 
 static int mma9551_read_incli_chan(struct i2c_client *client,
@@ -400,36 +400,26 @@ static int mma9551_init(struct mma9551_data *data)
 	return mma9551_set_device_state(data->client, true);
 }
 
-static int mma9551_gpio_probe(struct iio_dev *indio_dev)
+static int mma9551_irq_probe(struct iio_dev *indio_dev)
 {
-	struct gpio_desc *gpio;
 	int i, ret;
 	struct mma9551_data *data = iio_priv(indio_dev);
 	struct device *dev = &data->client->dev;
 
-	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)
+	for (i = 0; i < MMA9551_IRQ_COUNT; i++) {
+		ret = fwnode_irq_get(dev_fwnode(dev), i);
+		if (ret)
 			return ret;
 
 		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,
+						IRQF_ONESHOT,
+						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;
@@ -466,7 +456,7 @@ static int mma9551_probe(struct i2c_client *client,
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &mma9551_info;
 
-	ret = mma9551_gpio_probe(indio_dev);
+	ret = mma9551_irq_probe(indio_dev);
 	if (ret < 0)
 		goto out_poweroff;
 
-- 
2.34.1

[PATCH v3 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>
Reviewed-by: Alexandru Ardelean <aardelean@deviqon.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 537a7a04654a..a00a7e067883 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_irq_probe(struct iio_dev *indio_dev)
@@ -425,6 +439,16 @@ static int mma9551_irq_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)
 {
@@ -458,46 +482,28 @@ static int mma9551_probe(struct i2c_client *client,
 
 	ret = mma9551_irq_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)
@@ -552,7 +558,6 @@ static struct i2c_driver mma9551_driver = {
 		   .pm = &mma9551_pm_ops,
 		   },
 	.probe = mma9551_probe,
-	.remove = mma9551_remove,
 	.id_table = mma9551_id,
 };
 
-- 
2.34.1

[PATCH v3 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>
Reviewed-by: Alexandru Ardelean <aardelean@deviqon.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 c24384089714..72b50495c842 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.34.1

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

[Andy Shevchenko]

On Sun, Dec 5, 2021 at 9:57 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> Note the interrupt type should be specified by firmware, not the driver
> so that is also dropped.
>
> Drop previous gpio based retrieval method. Whilst in theory this
> might cause problems with direction if anyone is using ACPI GioIo().

GpioIo()

> As Andy described in v1, such a situation would typically reflect
> a pin that is actually used in both directions (not true here)
> or missdesigned ACPI tables.

...

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

> +               ret = fwnode_irq_get(dev_fwnode(dev), i);
> +               if (ret)
>                         return ret;

I don't remember why we decided that this gonna work, because
fwnode_irq_get() is not an equivalent to the above, more precisely in
ACPI case it only covers the GSIs (Global System Interrupts) which in
such case may or may not be GPIOs. On x86 it's usually direct IOxAPIC
ones.

So, this conversion would probably make it impossible to use this
device in the ACPI case.

See also this discussion:
https://lore.kernel.org/lkml/20211109200840.135019-1-puranjay12@gmail.com/T/#u

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

On Sun, 5 Dec 2021 22:39:07 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sun, Dec 5, 2021 at 9:57 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > Note the interrupt type should be specified by firmware, not the driver
> > so that is also dropped.
> >
> > Drop previous gpio based retrieval method. Whilst in theory this
> > might cause problems with direction if anyone is using ACPI GioIo().  
> 
> GpioIo()

My proofreading fails me again...

> 
> > As Andy described in v1, such a situation would typically reflect
> > a pin that is actually used in both directions (not true here)
> > or missdesigned ACPI tables.  
> 
> ...
> 
> > -               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)  
> 
> > +               ret = fwnode_irq_get(dev_fwnode(dev), i);
> > +               if (ret)
> >                         return ret;  
> 
> I don't remember why we decided that this gonna work, because
> fwnode_irq_get() is not an equivalent to the above, more precisely in
> ACPI case it only covers the GSIs (Global System Interrupts) which in
> such case may or may not be GPIOs. On x86 it's usually direct IOxAPIC
> ones.

This isn't accessed as a GPIO, but I kind of assume someone was using
it with one given this code.

I think I misread what you'd written in reply to v1.
Thanks for the explanation.

> 
> So, this conversion would probably make it impossible to use this
> device in the ACPI case.

oops :)


> 
> See also this discussion:
> https://lore.kernel.org/lkml/20211109200840.135019-1-puranjay12@gmail.com/T/#u
> 

I'd not made the connection that the non named one was also missing most
of the ways it could be specified.  Ah well, this will take a little while
to get a test framework in place to poke at it properly.  Maybe one for
a bored moment over the festive season.

Jonathan