linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/4] Input: adp5588-keys - drop CONFIG_PM guards
@ 2022-05-28  4:56 Dmitry Torokhov
  2022-05-28  4:56 ` [PATCH 2/4] Input: adp5588-keys - switch to using threaded interrupt Dmitry Torokhov
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Dmitry Torokhov @ 2022-05-28  4:56 UTC (permalink / raw)
  To: Michael Hennerich; +Cc: Uwe Kleine-König, linux-input, linux-kernel

To improve compile-time coverage let's drop #ifdef CONFIG_PM guards
and use SIMPLE_DEV_PM_OPS and __maybe_unused attributes and rely on
the linker to drop unused code.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---

Since we are talking about keeping the driver and switching it away from
platform data and towards device properties, here are some cleanups.

Only compiled, not tested.

 drivers/input/keyboard/adp5588-keys.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c
index 1592da4de336..ea67d0834be1 100644
--- a/drivers/input/keyboard/adp5588-keys.c
+++ b/drivers/input/keyboard/adp5588-keys.c
@@ -612,8 +612,7 @@ static int adp5588_remove(struct i2c_client *client)
 	return 0;
 }
 
-#ifdef CONFIG_PM
-static int adp5588_suspend(struct device *dev)
+static int __maybe_unused adp5588_suspend(struct device *dev)
 {
 	struct adp5588_kpad *kpad = dev_get_drvdata(dev);
 	struct i2c_client *client = kpad->client;
@@ -627,7 +626,7 @@ static int adp5588_suspend(struct device *dev)
 	return 0;
 }
 
-static int adp5588_resume(struct device *dev)
+static int __maybe_unused adp5588_resume(struct device *dev)
 {
 	struct adp5588_kpad *kpad = dev_get_drvdata(dev);
 	struct i2c_client *client = kpad->client;
@@ -640,11 +639,7 @@ static int adp5588_resume(struct device *dev)
 	return 0;
 }
 
-static const struct dev_pm_ops adp5588_dev_pm_ops = {
-	.suspend = adp5588_suspend,
-	.resume  = adp5588_resume,
-};
-#endif
+static SIMPLE_DEV_PM_OPS(adp5588_dev_pm_ops, adp5588_suspend, adp5588_resume);
 
 static const struct i2c_device_id adp5588_id[] = {
 	{ "adp5588-keys", 0 },
@@ -656,9 +651,7 @@ MODULE_DEVICE_TABLE(i2c, adp5588_id);
 static struct i2c_driver adp5588_driver = {
 	.driver = {
 		.name = KBUILD_MODNAME,
-#ifdef CONFIG_PM
 		.pm   = &adp5588_dev_pm_ops,
-#endif
 	},
 	.probe    = adp5588_probe,
 	.remove   = adp5588_remove,
-- 
2.36.1.124.g0e6072fb45-goog


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/4] Input: adp5588-keys - switch to using threaded interrupt
  2022-05-28  4:56 [PATCH 1/4] Input: adp5588-keys - drop CONFIG_PM guards Dmitry Torokhov
@ 2022-05-28  4:56 ` Dmitry Torokhov
  2022-05-30 10:10   ` Hennerich, Michael
  2022-05-28  4:56 ` [PATCH 3/4] Input: adp5588-keys - switch to using managed resources Dmitry Torokhov
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Dmitry Torokhov @ 2022-05-28  4:56 UTC (permalink / raw)
  To: Michael Hennerich; +Cc: Uwe Kleine-König, linux-input, linux-kernel

Instead of using hard interrupt handler and manually scheduling work
item to handle I2C communications, let's switch to threaded interrupt
handling.

While at that enforce the readout delay required on pre- revision 4
silicon.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/keyboard/adp5588-keys.c | 81 +++++++++++++++------------
 1 file changed, 45 insertions(+), 36 deletions(-)

diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c
index ea67d0834be1..ac21873ba1d7 100644
--- a/drivers/input/keyboard/adp5588-keys.c
+++ b/drivers/input/keyboard/adp5588-keys.c
@@ -8,17 +8,19 @@
  * Copyright (C) 2008-2010 Analog Devices Inc.
  */
 
-#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/gpio/driver.h>
+#include <linux/i2c.h>
+#include <linux/input.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
-#include <linux/workqueue.h>
-#include <linux/errno.h>
-#include <linux/pm.h>
+#include <linux/ktime.h>
+#include <linux/module.h>
 #include <linux/platform_device.h>
-#include <linux/input.h>
-#include <linux/i2c.h>
-#include <linux/gpio/driver.h>
+#include <linux/pm.h>
 #include <linux/slab.h>
+#include <linux/timekeeping.h>
 
 #include <linux/platform_data/adp5588.h>
 
@@ -36,11 +38,12 @@
  * asserted.
  */
 #define WA_DELAYED_READOUT_REVID(rev)		((rev) < 4)
+#define WA_DELAYED_READOUT_TIME			25
 
 struct adp5588_kpad {
 	struct i2c_client *client;
 	struct input_dev *input;
-	struct delayed_work work;
+	ktime_t irq_time;
 	unsigned long delay;
 	unsigned short keycode[ADP5588_KEYMAPSIZE];
 	const struct adp5588_gpi_map *gpimap;
@@ -289,13 +292,36 @@ static void adp5588_report_events(struct adp5588_kpad *kpad, int ev_cnt)
 	}
 }
 
-static void adp5588_work(struct work_struct *work)
+static irqreturn_t adp5588_hard_irq(int irq, void *handle)
+{
+	struct adp5588_kpad *kpad = handle;
+
+	kpad->irq_time = ktime_get();
+
+	return IRQ_WAKE_THREAD;
+}
+
+static irqreturn_t adp5588_thread_irq(int irq, void *handle)
 {
-	struct adp5588_kpad *kpad = container_of(work,
-						struct adp5588_kpad, work.work);
+	struct adp5588_kpad *kpad = handle;
 	struct i2c_client *client = kpad->client;
+	ktime_t target_time, now;
+	unsigned long delay;
 	int status, ev_cnt;
 
+	/*
+	 * Readout needs to wait for at least 25ms after the notification
+	 * for REVID < 4.
+	 */
+	if (kpad->delay) {
+		target_time = ktime_add_ms(kpad->irq_time, kpad->delay);
+		now = ktime_get();
+		if (ktime_before(now, target_time)) {
+			delay = ktime_to_us(ktime_sub(target_time, now));
+			usleep_range(delay, delay + 1000);
+		}
+	}
+
 	status = adp5588_read(client, INT_STAT);
 
 	if (status & ADP5588_OVR_FLOW_INT)	/* Unlikely and should never happen */
@@ -308,20 +334,8 @@ static void adp5588_work(struct work_struct *work)
 			input_sync(kpad->input);
 		}
 	}
-	adp5588_write(client, INT_STAT, status); /* Status is W1C */
-}
 
-static irqreturn_t adp5588_irq(int irq, void *handle)
-{
-	struct adp5588_kpad *kpad = handle;
-
-	/*
-	 * use keventd context to read the event fifo registers
-	 * Schedule readout at least 25ms after notification for
-	 * REVID < 4
-	 */
-
-	schedule_delayed_work(&kpad->work, kpad->delay);
+	adp5588_write(client, INT_STAT, status); /* Status is W1C */
 
 	return IRQ_HANDLED;
 }
@@ -505,7 +519,6 @@ static int adp5588_probe(struct i2c_client *client,
 
 	kpad->client = client;
 	kpad->input = input;
-	INIT_DELAYED_WORK(&kpad->work, adp5588_work);
 
 	ret = adp5588_read(client, DEV_ID);
 	if (ret < 0) {
@@ -515,7 +528,7 @@ static int adp5588_probe(struct i2c_client *client,
 
 	revid = (u8) ret & ADP5588_DEVICE_ID_MASK;
 	if (WA_DELAYED_READOUT_REVID(revid))
-		kpad->delay = msecs_to_jiffies(30);
+		kpad->delay = msecs_to_jiffies(WA_DELAYED_READOUT_TIME);
 
 	input->name = client->name;
 	input->phys = "adp5588-keys/input0";
@@ -560,9 +573,10 @@ static int adp5588_probe(struct i2c_client *client,
 		goto err_free_mem;
 	}
 
-	error = request_irq(client->irq, adp5588_irq,
-			    IRQF_TRIGGER_FALLING,
-			    client->dev.driver->name, kpad);
+	error = request_threaded_irq(client->irq,
+				     adp5588_hard_irq, adp5588_thread_irq,
+				     IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+				     client->dev.driver->name, kpad);
 	if (error) {
 		dev_err(&client->dev, "irq %d busy?\n", client->irq);
 		goto err_unreg_dev;
@@ -587,7 +601,6 @@ static int adp5588_probe(struct i2c_client *client,
 
  err_free_irq:
 	free_irq(client->irq, kpad);
-	cancel_delayed_work_sync(&kpad->work);
  err_unreg_dev:
 	input_unregister_device(input);
 	input = NULL;
@@ -604,7 +617,6 @@ static int adp5588_remove(struct i2c_client *client)
 
 	adp5588_write(client, CFG, 0);
 	free_irq(client->irq, kpad);
-	cancel_delayed_work_sync(&kpad->work);
 	input_unregister_device(kpad->input);
 	adp5588_gpio_remove(kpad);
 	kfree(kpad);
@@ -614,11 +626,9 @@ static int adp5588_remove(struct i2c_client *client)
 
 static int __maybe_unused adp5588_suspend(struct device *dev)
 {
-	struct adp5588_kpad *kpad = dev_get_drvdata(dev);
-	struct i2c_client *client = kpad->client;
+	struct i2c_client *client = to_i2c_client(dev);
 
 	disable_irq(client->irq);
-	cancel_delayed_work_sync(&kpad->work);
 
 	if (device_may_wakeup(&client->dev))
 		enable_irq_wake(client->irq);
@@ -628,8 +638,7 @@ static int __maybe_unused adp5588_suspend(struct device *dev)
 
 static int __maybe_unused adp5588_resume(struct device *dev)
 {
-	struct adp5588_kpad *kpad = dev_get_drvdata(dev);
-	struct i2c_client *client = kpad->client;
+	struct i2c_client *client = to_i2c_client(dev);
 
 	if (device_may_wakeup(&client->dev))
 		disable_irq_wake(client->irq);
-- 
2.36.1.124.g0e6072fb45-goog


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 3/4] Input: adp5588-keys - switch to using managed resources
  2022-05-28  4:56 [PATCH 1/4] Input: adp5588-keys - drop CONFIG_PM guards Dmitry Torokhov
  2022-05-28  4:56 ` [PATCH 2/4] Input: adp5588-keys - switch to using threaded interrupt Dmitry Torokhov
@ 2022-05-28  4:56 ` Dmitry Torokhov
  2022-05-28 19:37   ` Uwe Kleine-König
  2022-05-30 10:11   ` Hennerich, Michael
  2022-05-28  4:56 ` [PATCH 4/4] Input: adp5588-keys - do not explicitly set device as wakeup source Dmitry Torokhov
  2022-05-30 10:06 ` [PATCH 1/4] Input: adp5588-keys - drop CONFIG_PM guards Hennerich, Michael
  3 siblings, 2 replies; 11+ messages in thread
From: Dmitry Torokhov @ 2022-05-28  4:56 UTC (permalink / raw)
  To: Michael Hennerich; +Cc: Uwe Kleine-König, linux-input, linux-kernel

This simplifies error handling in probe() and reduces amount of explicit
code in remove().

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/keyboard/adp5588-keys.c | 111 +++++++++++---------------
 1 file changed, 45 insertions(+), 66 deletions(-)

diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c
index ac21873ba1d7..df84a2998ed2 100644
--- a/drivers/input/keyboard/adp5588-keys.c
+++ b/drivers/input/keyboard/adp5588-keys.c
@@ -182,6 +182,21 @@ static int adp5588_build_gpiomap(struct adp5588_kpad *kpad,
 	return n_unused;
 }
 
+static void adp5588_gpio_do_teardown(void *_kpad)
+{
+	struct adp5588_kpad *kpad = _kpad;
+	struct device *dev = &kpad->client->dev;
+	const struct adp5588_kpad_platform_data *pdata = dev_get_platdata(dev);
+	const struct adp5588_gpio_platform_data *gpio_data = pdata->gpio_data;
+	int error;
+
+	error = gpio_data->teardown(kpad->client,
+				    kpad->gc.base, kpad->gc.ngpio,
+				    gpio_data->context);
+	if (error)
+		dev_warn(&kpad->client->dev, "teardown failed %d\n", error);
+}
+
 static int adp5588_gpio_add(struct adp5588_kpad *kpad)
 {
 	struct device *dev = &kpad->client->dev;
@@ -198,8 +213,6 @@ static int adp5588_gpio_add(struct adp5588_kpad *kpad)
 		return 0;
 	}
 
-	kpad->export_gpio = true;
-
 	kpad->gc.direction_input = adp5588_gpio_direction_input;
 	kpad->gc.direction_output = adp5588_gpio_direction_output;
 	kpad->gc.get = adp5588_gpio_get_value;
@@ -213,9 +226,9 @@ static int adp5588_gpio_add(struct adp5588_kpad *kpad)
 
 	mutex_init(&kpad->gpio_lock);
 
-	error = gpiochip_add_data(&kpad->gc, kpad);
+	error = devm_gpiochip_add_data(dev, &kpad->gc, kpad);
 	if (error) {
-		dev_err(dev, "gpiochip_add failed, err: %d\n", error);
+		dev_err(dev, "gpiochip_add failed: %d\n", error);
 		return error;
 	}
 
@@ -230,41 +243,24 @@ static int adp5588_gpio_add(struct adp5588_kpad *kpad)
 					 kpad->gc.base, kpad->gc.ngpio,
 					 gpio_data->context);
 		if (error)
-			dev_warn(dev, "setup failed, %d\n", error);
+			dev_warn(dev, "setup failed: %d\n", error);
 	}
 
-	return 0;
-}
-
-static void adp5588_gpio_remove(struct adp5588_kpad *kpad)
-{
-	struct device *dev = &kpad->client->dev;
-	const struct adp5588_kpad_platform_data *pdata = dev_get_platdata(dev);
-	const struct adp5588_gpio_platform_data *gpio_data = pdata->gpio_data;
-	int error;
-
-	if (!kpad->export_gpio)
-		return;
-
 	if (gpio_data->teardown) {
-		error = gpio_data->teardown(kpad->client,
-					    kpad->gc.base, kpad->gc.ngpio,
-					    gpio_data->context);
+		error = devm_add_action(dev, adp5588_gpio_do_teardown, kpad);
 		if (error)
-			dev_warn(dev, "teardown failed %d\n", error);
+			dev_warn(dev, "failed to schedule teardown: %d\n",
+				 error);
 	}
 
-	gpiochip_remove(&kpad->gc);
+	return 0;
 }
+
 #else
 static inline int adp5588_gpio_add(struct adp5588_kpad *kpad)
 {
 	return 0;
 }
-
-static inline void adp5588_gpio_remove(struct adp5588_kpad *kpad)
-{
-}
 #endif
 
 static void adp5588_report_events(struct adp5588_kpad *kpad, int ev_cnt)
@@ -510,21 +506,20 @@ static int adp5588_probe(struct i2c_client *client,
 		return -EINVAL;
 	}
 
-	kpad = kzalloc(sizeof(*kpad), GFP_KERNEL);
-	input = input_allocate_device();
-	if (!kpad || !input) {
-		error = -ENOMEM;
-		goto err_free_mem;
-	}
+	kpad = devm_kzalloc(&client->dev, sizeof(*kpad), GFP_KERNEL);
+	if (!kpad)
+		return -ENOMEM;
+
+	input = devm_input_allocate_device(&client->dev);
+	if (!input)
+		return -ENOMEM;
 
 	kpad->client = client;
 	kpad->input = input;
 
 	ret = adp5588_read(client, DEV_ID);
-	if (ret < 0) {
-		error = ret;
-		goto err_free_mem;
-	}
+	if (ret < 0)
+		return ret;
 
 	revid = (u8) ret & ADP5588_DEVICE_ID_MASK;
 	if (WA_DELAYED_READOUT_REVID(revid))
@@ -532,7 +527,6 @@ static int adp5588_probe(struct i2c_client *client,
 
 	input->name = client->name;
 	input->phys = "adp5588-keys/input0";
-	input->dev.parent = &client->dev;
 
 	input_set_drvdata(input, kpad);
 
@@ -569,58 +563,43 @@ static int adp5588_probe(struct i2c_client *client,
 
 	error = input_register_device(input);
 	if (error) {
-		dev_err(&client->dev, "unable to register input device\n");
-		goto err_free_mem;
+		dev_err(&client->dev, "unable to register input device: %d\n",
+			error);
+		return error;
 	}
 
-	error = request_threaded_irq(client->irq,
-				     adp5588_hard_irq, adp5588_thread_irq,
-				     IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
-				     client->dev.driver->name, kpad);
+	error = devm_request_threaded_irq(&client->dev, client->irq,
+					  adp5588_hard_irq, adp5588_thread_irq,
+					  IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+					  client->dev.driver->name, kpad);
 	if (error) {
-		dev_err(&client->dev, "irq %d busy?\n", client->irq);
-		goto err_unreg_dev;
+		dev_err(&client->dev, "failed to request irq %d: %d\n",
+			client->irq, error);
+		return error;
 	}
 
 	error = adp5588_setup(client);
 	if (error)
-		goto err_free_irq;
+		return error;
 
 	if (kpad->gpimapsize)
 		adp5588_report_switch_state(kpad);
 
 	error = adp5588_gpio_add(kpad);
 	if (error)
-		goto err_free_irq;
+		return error;
 
 	device_init_wakeup(&client->dev, 1);
-	i2c_set_clientdata(client, kpad);
 
 	dev_info(&client->dev, "Rev.%d keypad, irq %d\n", revid, client->irq);
 	return 0;
-
- err_free_irq:
-	free_irq(client->irq, kpad);
- err_unreg_dev:
-	input_unregister_device(input);
-	input = NULL;
- err_free_mem:
-	input_free_device(input);
-	kfree(kpad);
-
-	return error;
 }
 
 static int adp5588_remove(struct i2c_client *client)
 {
-	struct adp5588_kpad *kpad = i2c_get_clientdata(client);
-
 	adp5588_write(client, CFG, 0);
-	free_irq(client->irq, kpad);
-	input_unregister_device(kpad->input);
-	adp5588_gpio_remove(kpad);
-	kfree(kpad);
 
+	/* all resources will be freed by devm */
 	return 0;
 }
 
-- 
2.36.1.124.g0e6072fb45-goog


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 4/4] Input: adp5588-keys - do not explicitly set device as wakeup source
  2022-05-28  4:56 [PATCH 1/4] Input: adp5588-keys - drop CONFIG_PM guards Dmitry Torokhov
  2022-05-28  4:56 ` [PATCH 2/4] Input: adp5588-keys - switch to using threaded interrupt Dmitry Torokhov
  2022-05-28  4:56 ` [PATCH 3/4] Input: adp5588-keys - switch to using managed resources Dmitry Torokhov
@ 2022-05-28  4:56 ` Dmitry Torokhov
  2022-05-30 10:11   ` Hennerich, Michael
  2022-05-30 10:06 ` [PATCH 1/4] Input: adp5588-keys - drop CONFIG_PM guards Hennerich, Michael
  3 siblings, 1 reply; 11+ messages in thread
From: Dmitry Torokhov @ 2022-05-28  4:56 UTC (permalink / raw)
  To: Michael Hennerich; +Cc: Uwe Kleine-König, linux-input, linux-kernel

I2C core will set up device as a wakeup source and will configure interrupt
as a wakeup interrupt if client is created with I2C_CLIENT_WAKE flag. Let's
rely on this facility and to not unconditionally set up the device as
wakeup device in the driver.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/keyboard/adp5588-keys.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c
index df84a2998ed2..2a274240facb 100644
--- a/drivers/input/keyboard/adp5588-keys.c
+++ b/drivers/input/keyboard/adp5588-keys.c
@@ -589,8 +589,6 @@ static int adp5588_probe(struct i2c_client *client,
 	if (error)
 		return error;
 
-	device_init_wakeup(&client->dev, 1);
-
 	dev_info(&client->dev, "Rev.%d keypad, irq %d\n", revid, client->irq);
 	return 0;
 }
@@ -609,9 +607,6 @@ static int __maybe_unused adp5588_suspend(struct device *dev)
 
 	disable_irq(client->irq);
 
-	if (device_may_wakeup(&client->dev))
-		enable_irq_wake(client->irq);
-
 	return 0;
 }
 
@@ -619,9 +614,6 @@ static int __maybe_unused adp5588_resume(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 
-	if (device_may_wakeup(&client->dev))
-		disable_irq_wake(client->irq);
-
 	enable_irq(client->irq);
 
 	return 0;
-- 
2.36.1.124.g0e6072fb45-goog


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/4] Input: adp5588-keys - switch to using managed resources
  2022-05-28  4:56 ` [PATCH 3/4] Input: adp5588-keys - switch to using managed resources Dmitry Torokhov
@ 2022-05-28 19:37   ` Uwe Kleine-König
  2022-05-29  5:22     ` Dmitry Torokhov
  2022-05-30 10:11   ` Hennerich, Michael
  1 sibling, 1 reply; 11+ messages in thread
From: Uwe Kleine-König @ 2022-05-28 19:37 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: Michael Hennerich, linux-input, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1567 bytes --]

On Fri, May 27, 2022 at 09:56:30PM -0700, Dmitry Torokhov wrote:
> This simplifies error handling in probe() and reduces amount of explicit
> code in remove().
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/keyboard/adp5588-keys.c | 111 +++++++++++---------------
>  1 file changed, 45 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c
> index ac21873ba1d7..df84a2998ed2 100644
> --- a/drivers/input/keyboard/adp5588-keys.c
> +++ b/drivers/input/keyboard/adp5588-keys.c
> @@ -182,6 +182,21 @@ static int adp5588_build_gpiomap(struct adp5588_kpad *kpad,
>  	return n_unused;
>  }
>  
> +static void adp5588_gpio_do_teardown(void *_kpad)
> +{
> +	struct adp5588_kpad *kpad = _kpad;
> +	struct device *dev = &kpad->client->dev;
> +	const struct adp5588_kpad_platform_data *pdata = dev_get_platdata(dev);
> +	const struct adp5588_gpio_platform_data *gpio_data = pdata->gpio_data;
> +	int error;
> +
> +	error = gpio_data->teardown(kpad->client,
> +				    kpad->gc.base, kpad->gc.ngpio,
> +				    gpio_data->context);
> +	if (error)
> +		dev_warn(&kpad->client->dev, "teardown failed %d\n", error);
> +}

I think the more sensible approach is to drop support for setup and
teardown. Maybe even rip all usage of adp5588_gpio_platform_data.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/4] Input: adp5588-keys - switch to using managed resources
  2022-05-28 19:37   ` Uwe Kleine-König
@ 2022-05-29  5:22     ` Dmitry Torokhov
  2022-05-29  6:11       ` Uwe Kleine-König
  0 siblings, 1 reply; 11+ messages in thread
From: Dmitry Torokhov @ 2022-05-29  5:22 UTC (permalink / raw)
  To: Uwe Kleine-König; +Cc: Michael Hennerich, linux-input, linux-kernel

On Sat, May 28, 2022 at 09:37:55PM +0200, Uwe Kleine-König wrote:
> On Fri, May 27, 2022 at 09:56:30PM -0700, Dmitry Torokhov wrote:
> > This simplifies error handling in probe() and reduces amount of explicit
> > code in remove().
> > 
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >  drivers/input/keyboard/adp5588-keys.c | 111 +++++++++++---------------
> >  1 file changed, 45 insertions(+), 66 deletions(-)
> > 
> > diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c
> > index ac21873ba1d7..df84a2998ed2 100644
> > --- a/drivers/input/keyboard/adp5588-keys.c
> > +++ b/drivers/input/keyboard/adp5588-keys.c
> > @@ -182,6 +182,21 @@ static int adp5588_build_gpiomap(struct adp5588_kpad *kpad,
> >  	return n_unused;
> >  }
> >  
> > +static void adp5588_gpio_do_teardown(void *_kpad)
> > +{
> > +	struct adp5588_kpad *kpad = _kpad;
> > +	struct device *dev = &kpad->client->dev;
> > +	const struct adp5588_kpad_platform_data *pdata = dev_get_platdata(dev);
> > +	const struct adp5588_gpio_platform_data *gpio_data = pdata->gpio_data;
> > +	int error;
> > +
> > +	error = gpio_data->teardown(kpad->client,
> > +				    kpad->gc.base, kpad->gc.ngpio,
> > +				    gpio_data->context);
> > +	if (error)
> > +		dev_warn(&kpad->client->dev, "teardown failed %d\n", error);
> > +}
> 
> I think the more sensible approach is to drop support for setup and
> teardown. Maybe even rip all usage of adp5588_gpio_platform_data.

That will come with the transition to device tree/device properties. For
now wanted to keep existing functionality intact.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/4] Input: adp5588-keys - switch to using managed resources
  2022-05-29  5:22     ` Dmitry Torokhov
@ 2022-05-29  6:11       ` Uwe Kleine-König
  0 siblings, 0 replies; 11+ messages in thread
From: Uwe Kleine-König @ 2022-05-29  6:11 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: Michael Hennerich, linux-input, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 2071 bytes --]

On Sat, May 28, 2022 at 10:22:07PM -0700, Dmitry Torokhov wrote:
> On Sat, May 28, 2022 at 09:37:55PM +0200, Uwe Kleine-König wrote:
> > On Fri, May 27, 2022 at 09:56:30PM -0700, Dmitry Torokhov wrote:
> > > This simplifies error handling in probe() and reduces amount of explicit
> > > code in remove().
> > > 
> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > ---
> > >  drivers/input/keyboard/adp5588-keys.c | 111 +++++++++++---------------
> > >  1 file changed, 45 insertions(+), 66 deletions(-)
> > > 
> > > diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c
> > > index ac21873ba1d7..df84a2998ed2 100644
> > > --- a/drivers/input/keyboard/adp5588-keys.c
> > > +++ b/drivers/input/keyboard/adp5588-keys.c
> > > @@ -182,6 +182,21 @@ static int adp5588_build_gpiomap(struct adp5588_kpad *kpad,
> > >  	return n_unused;
> > >  }
> > >  
> > > +static void adp5588_gpio_do_teardown(void *_kpad)
> > > +{
> > > +	struct adp5588_kpad *kpad = _kpad;
> > > +	struct device *dev = &kpad->client->dev;
> > > +	const struct adp5588_kpad_platform_data *pdata = dev_get_platdata(dev);
> > > +	const struct adp5588_gpio_platform_data *gpio_data = pdata->gpio_data;
> > > +	int error;
> > > +
> > > +	error = gpio_data->teardown(kpad->client,
> > > +				    kpad->gc.base, kpad->gc.ngpio,
> > > +				    gpio_data->context);
> > > +	if (error)
> > > +		dev_warn(&kpad->client->dev, "teardown failed %d\n", error);
> > > +}
> > 
> > I think the more sensible approach is to drop support for setup and
> > teardown. Maybe even rip all usage of adp5588_gpio_platform_data.
> 
> That will come with the transition to device tree/device properties. For
> now wanted to keep existing functionality intact.

That's up to you. I wouldn't spend an effort to clean up a feature that
is about to be removed.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH 1/4] Input: adp5588-keys - drop CONFIG_PM guards
  2022-05-28  4:56 [PATCH 1/4] Input: adp5588-keys - drop CONFIG_PM guards Dmitry Torokhov
                   ` (2 preceding siblings ...)
  2022-05-28  4:56 ` [PATCH 4/4] Input: adp5588-keys - do not explicitly set device as wakeup source Dmitry Torokhov
@ 2022-05-30 10:06 ` Hennerich, Michael
  3 siblings, 0 replies; 11+ messages in thread
From: Hennerich, Michael @ 2022-05-30 10:06 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: Uwe Kleine-König, linux-input, linux-kernel



> -----Original Message-----
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Sent: Samstag, 28. Mai 2022 06:56
> To: Hennerich, Michael <Michael.Hennerich@analog.com>
> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>; linux-
> input@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH 1/4] Input: adp5588-keys - drop CONFIG_PM guards
> 
> 
> To improve compile-time coverage let's drop #ifdef CONFIG_PM guards and
> use SIMPLE_DEV_PM_OPS and __maybe_unused attributes and rely on the
> linker to drop unused code.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Acked-by: Michael Hennerich <michael.hennerich@analog.com>

> ---
> 
> Since we are talking about keeping the driver and switching it away from
> platform data and towards device properties, here are some cleanups.
> 
> Only compiled, not tested.
> 
>  drivers/input/keyboard/adp5588-keys.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/input/keyboard/adp5588-keys.c
> b/drivers/input/keyboard/adp5588-keys.c
> index 1592da4de336..ea67d0834be1 100644
> --- a/drivers/input/keyboard/adp5588-keys.c
> +++ b/drivers/input/keyboard/adp5588-keys.c
> @@ -612,8 +612,7 @@ static int adp5588_remove(struct i2c_client *client)
>  	return 0;
>  }
> 
> -#ifdef CONFIG_PM
> -static int adp5588_suspend(struct device *dev)
> +static int __maybe_unused adp5588_suspend(struct device *dev)
>  {
>  	struct adp5588_kpad *kpad = dev_get_drvdata(dev);
>  	struct i2c_client *client = kpad->client; @@ -627,7 +626,7 @@ static
> int adp5588_suspend(struct device *dev)
>  	return 0;
>  }
> 
> -static int adp5588_resume(struct device *dev)
> +static int __maybe_unused adp5588_resume(struct device *dev)
>  {
>  	struct adp5588_kpad *kpad = dev_get_drvdata(dev);
>  	struct i2c_client *client = kpad->client; @@ -640,11 +639,7 @@ static
> int adp5588_resume(struct device *dev)
>  	return 0;
>  }
> 
> -static const struct dev_pm_ops adp5588_dev_pm_ops = {
> -	.suspend = adp5588_suspend,
> -	.resume  = adp5588_resume,
> -};
> -#endif
> +static SIMPLE_DEV_PM_OPS(adp5588_dev_pm_ops, adp5588_suspend,
> +adp5588_resume);
> 
>  static const struct i2c_device_id adp5588_id[] = {
>  	{ "adp5588-keys", 0 },
> @@ -656,9 +651,7 @@ MODULE_DEVICE_TABLE(i2c, adp5588_id);  static
> struct i2c_driver adp5588_driver = {
>  	.driver = {
>  		.name = KBUILD_MODNAME,
> -#ifdef CONFIG_PM
>  		.pm   = &adp5588_dev_pm_ops,
> -#endif
>  	},
>  	.probe    = adp5588_probe,
>  	.remove   = adp5588_remove,
> --
> 2.36.1.124.g0e6072fb45-goog


^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH 2/4] Input: adp5588-keys - switch to using threaded interrupt
  2022-05-28  4:56 ` [PATCH 2/4] Input: adp5588-keys - switch to using threaded interrupt Dmitry Torokhov
@ 2022-05-30 10:10   ` Hennerich, Michael
  0 siblings, 0 replies; 11+ messages in thread
From: Hennerich, Michael @ 2022-05-30 10:10 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: Uwe Kleine-König, linux-input, linux-kernel



> -----Original Message-----
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Sent: Samstag, 28. Mai 2022 06:56
> To: Hennerich, Michael <Michael.Hennerich@analog.com>
> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>; linux-
> input@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH 2/4] Input: adp5588-keys - switch to using threaded interrupt
> 
> [External]
> 
> Instead of using hard interrupt handler and manually scheduling work item to
> handle I2C communications, let's switch to threaded interrupt handling.
> 
> While at that enforce the readout delay required on pre- revision 4 silicon.
> 

Looks good to me. I'll test this altogether with the devicetree and matrix_keypad helper updates.

> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Acked-by: Michael Hennerich <michael.hennerich@analog.com>

> ---
>  drivers/input/keyboard/adp5588-keys.c | 81 +++++++++++++++------------
>  1 file changed, 45 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/input/keyboard/adp5588-keys.c
> b/drivers/input/keyboard/adp5588-keys.c
> index ea67d0834be1..ac21873ba1d7 100644
> --- a/drivers/input/keyboard/adp5588-keys.c
> +++ b/drivers/input/keyboard/adp5588-keys.c
> @@ -8,17 +8,19 @@
>   * Copyright (C) 2008-2010 Analog Devices Inc.
>   */
> 
> -#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
> -#include <linux/workqueue.h>
> -#include <linux/errno.h>
> -#include <linux/pm.h>
> +#include <linux/ktime.h>
> +#include <linux/module.h>
>  #include <linux/platform_device.h>
> -#include <linux/input.h>
> -#include <linux/i2c.h>
> -#include <linux/gpio/driver.h>
> +#include <linux/pm.h>
>  #include <linux/slab.h>
> +#include <linux/timekeeping.h>
> 
>  #include <linux/platform_data/adp5588.h>
> 
> @@ -36,11 +38,12 @@
>   * asserted.
>   */
>  #define WA_DELAYED_READOUT_REVID(rev)		((rev) < 4)
> +#define WA_DELAYED_READOUT_TIME			25
> 
>  struct adp5588_kpad {
>  	struct i2c_client *client;
>  	struct input_dev *input;
> -	struct delayed_work work;
> +	ktime_t irq_time;
>  	unsigned long delay;
>  	unsigned short keycode[ADP5588_KEYMAPSIZE];
>  	const struct adp5588_gpi_map *gpimap;
> @@ -289,13 +292,36 @@ static void adp5588_report_events(struct
> adp5588_kpad *kpad, int ev_cnt)
>  	}
>  }
> 
> -static void adp5588_work(struct work_struct *work)
> +static irqreturn_t adp5588_hard_irq(int irq, void *handle) {
> +	struct adp5588_kpad *kpad = handle;
> +
> +	kpad->irq_time = ktime_get();
> +
> +	return IRQ_WAKE_THREAD;
> +}
> +
> +static irqreturn_t adp5588_thread_irq(int irq, void *handle)
>  {
> -	struct adp5588_kpad *kpad = container_of(work,
> -						struct adp5588_kpad,
> work.work);
> +	struct adp5588_kpad *kpad = handle;
>  	struct i2c_client *client = kpad->client;
> +	ktime_t target_time, now;
> +	unsigned long delay;
>  	int status, ev_cnt;
> 
> +	/*
> +	 * Readout needs to wait for at least 25ms after the notification
> +	 * for REVID < 4.
> +	 */
> +	if (kpad->delay) {
> +		target_time = ktime_add_ms(kpad->irq_time, kpad->delay);
> +		now = ktime_get();
> +		if (ktime_before(now, target_time)) {
> +			delay = ktime_to_us(ktime_sub(target_time, now));
> +			usleep_range(delay, delay + 1000);
> +		}
> +	}
> +
>  	status = adp5588_read(client, INT_STAT);
> 
>  	if (status & ADP5588_OVR_FLOW_INT)	/* Unlikely and should never
> happen */
> @@ -308,20 +334,8 @@ static void adp5588_work(struct work_struct *work)
>  			input_sync(kpad->input);
>  		}
>  	}
> -	adp5588_write(client, INT_STAT, status); /* Status is W1C */
> -}
> 
> -static irqreturn_t adp5588_irq(int irq, void *handle) -{
> -	struct adp5588_kpad *kpad = handle;
> -
> -	/*
> -	 * use keventd context to read the event fifo registers
> -	 * Schedule readout at least 25ms after notification for
> -	 * REVID < 4
> -	 */
> -
> -	schedule_delayed_work(&kpad->work, kpad->delay);
> +	adp5588_write(client, INT_STAT, status); /* Status is W1C */
> 
>  	return IRQ_HANDLED;
>  }
> @@ -505,7 +519,6 @@ static int adp5588_probe(struct i2c_client *client,
> 
>  	kpad->client = client;
>  	kpad->input = input;
> -	INIT_DELAYED_WORK(&kpad->work, adp5588_work);
> 
>  	ret = adp5588_read(client, DEV_ID);
>  	if (ret < 0) {
> @@ -515,7 +528,7 @@ static int adp5588_probe(struct i2c_client *client,
> 
>  	revid = (u8) ret & ADP5588_DEVICE_ID_MASK;
>  	if (WA_DELAYED_READOUT_REVID(revid))
> -		kpad->delay = msecs_to_jiffies(30);
> +		kpad->delay =
> msecs_to_jiffies(WA_DELAYED_READOUT_TIME);
> 
>  	input->name = client->name;
>  	input->phys = "adp5588-keys/input0";
> @@ -560,9 +573,10 @@ static int adp5588_probe(struct i2c_client *client,
>  		goto err_free_mem;
>  	}
> 
> -	error = request_irq(client->irq, adp5588_irq,
> -			    IRQF_TRIGGER_FALLING,
> -			    client->dev.driver->name, kpad);
> +	error = request_threaded_irq(client->irq,
> +				     adp5588_hard_irq, adp5588_thread_irq,
> +				     IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +				     client->dev.driver->name, kpad);
>  	if (error) {
>  		dev_err(&client->dev, "irq %d busy?\n", client->irq);
>  		goto err_unreg_dev;
> @@ -587,7 +601,6 @@ static int adp5588_probe(struct i2c_client *client,
> 
>   err_free_irq:
>  	free_irq(client->irq, kpad);
> -	cancel_delayed_work_sync(&kpad->work);
>   err_unreg_dev:
>  	input_unregister_device(input);
>  	input = NULL;
> @@ -604,7 +617,6 @@ static int adp5588_remove(struct i2c_client *client)
> 
>  	adp5588_write(client, CFG, 0);
>  	free_irq(client->irq, kpad);
> -	cancel_delayed_work_sync(&kpad->work);
>  	input_unregister_device(kpad->input);
>  	adp5588_gpio_remove(kpad);
>  	kfree(kpad);
> @@ -614,11 +626,9 @@ static int adp5588_remove(struct i2c_client *client)
> 
>  static int __maybe_unused adp5588_suspend(struct device *dev)  {
> -	struct adp5588_kpad *kpad = dev_get_drvdata(dev);
> -	struct i2c_client *client = kpad->client;
> +	struct i2c_client *client = to_i2c_client(dev);
> 
>  	disable_irq(client->irq);
> -	cancel_delayed_work_sync(&kpad->work);
> 
>  	if (device_may_wakeup(&client->dev))
>  		enable_irq_wake(client->irq);
> @@ -628,8 +638,7 @@ static int __maybe_unused adp5588_suspend(struct
> device *dev)
> 
>  static int __maybe_unused adp5588_resume(struct device *dev)  {
> -	struct adp5588_kpad *kpad = dev_get_drvdata(dev);
> -	struct i2c_client *client = kpad->client;
> +	struct i2c_client *client = to_i2c_client(dev);
> 
>  	if (device_may_wakeup(&client->dev))
>  		disable_irq_wake(client->irq);
> --
> 2.36.1.124.g0e6072fb45-goog


^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH 3/4] Input: adp5588-keys - switch to using managed resources
  2022-05-28  4:56 ` [PATCH 3/4] Input: adp5588-keys - switch to using managed resources Dmitry Torokhov
  2022-05-28 19:37   ` Uwe Kleine-König
@ 2022-05-30 10:11   ` Hennerich, Michael
  1 sibling, 0 replies; 11+ messages in thread
From: Hennerich, Michael @ 2022-05-30 10:11 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: Uwe Kleine-König, linux-input, linux-kernel



> -----Original Message-----
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Sent: Samstag, 28. Mai 2022 06:57
> To: Hennerich, Michael <Michael.Hennerich@analog.com>
> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>; linux-
> input@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH 3/4] Input: adp5588-keys - switch to using managed resources
> 
> 
> This simplifies error handling in probe() and reduces amount of explicit code in
> remove().
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Acked-by: Michael Hennerich <michael.hennerich@analog.com>

> ---
>  drivers/input/keyboard/adp5588-keys.c | 111 +++++++++++---------------
>  1 file changed, 45 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/input/keyboard/adp5588-keys.c
> b/drivers/input/keyboard/adp5588-keys.c
> index ac21873ba1d7..df84a2998ed2 100644
> --- a/drivers/input/keyboard/adp5588-keys.c
> +++ b/drivers/input/keyboard/adp5588-keys.c
> @@ -182,6 +182,21 @@ static int adp5588_build_gpiomap(struct
> adp5588_kpad *kpad,
>  	return n_unused;
>  }
> 
> +static void adp5588_gpio_do_teardown(void *_kpad) {
> +	struct adp5588_kpad *kpad = _kpad;
> +	struct device *dev = &kpad->client->dev;
> +	const struct adp5588_kpad_platform_data *pdata =
> dev_get_platdata(dev);
> +	const struct adp5588_gpio_platform_data *gpio_data = pdata-
> >gpio_data;
> +	int error;
> +
> +	error = gpio_data->teardown(kpad->client,
> +				    kpad->gc.base, kpad->gc.ngpio,
> +				    gpio_data->context);
> +	if (error)
> +		dev_warn(&kpad->client->dev, "teardown failed %d\n", error);
> }
> +
>  static int adp5588_gpio_add(struct adp5588_kpad *kpad)  {
>  	struct device *dev = &kpad->client->dev; @@ -198,8 +213,6 @@ static
> int adp5588_gpio_add(struct adp5588_kpad *kpad)
>  		return 0;
>  	}
> 
> -	kpad->export_gpio = true;
> -
>  	kpad->gc.direction_input = adp5588_gpio_direction_input;
>  	kpad->gc.direction_output = adp5588_gpio_direction_output;
>  	kpad->gc.get = adp5588_gpio_get_value; @@ -213,9 +226,9 @@ static
> int adp5588_gpio_add(struct adp5588_kpad *kpad)
> 
>  	mutex_init(&kpad->gpio_lock);
> 
> -	error = gpiochip_add_data(&kpad->gc, kpad);
> +	error = devm_gpiochip_add_data(dev, &kpad->gc, kpad);
>  	if (error) {
> -		dev_err(dev, "gpiochip_add failed, err: %d\n", error);
> +		dev_err(dev, "gpiochip_add failed: %d\n", error);
>  		return error;
>  	}
> 
> @@ -230,41 +243,24 @@ static int adp5588_gpio_add(struct adp5588_kpad
> *kpad)
>  					 kpad->gc.base, kpad->gc.ngpio,
>  					 gpio_data->context);
>  		if (error)
> -			dev_warn(dev, "setup failed, %d\n", error);
> +			dev_warn(dev, "setup failed: %d\n", error);
>  	}
> 
> -	return 0;
> -}
> -
> -static void adp5588_gpio_remove(struct adp5588_kpad *kpad) -{
> -	struct device *dev = &kpad->client->dev;
> -	const struct adp5588_kpad_platform_data *pdata =
> dev_get_platdata(dev);
> -	const struct adp5588_gpio_platform_data *gpio_data = pdata-
> >gpio_data;
> -	int error;
> -
> -	if (!kpad->export_gpio)
> -		return;
> -
>  	if (gpio_data->teardown) {
> -		error = gpio_data->teardown(kpad->client,
> -					    kpad->gc.base, kpad->gc.ngpio,
> -					    gpio_data->context);
> +		error = devm_add_action(dev, adp5588_gpio_do_teardown,
> kpad);
>  		if (error)
> -			dev_warn(dev, "teardown failed %d\n", error);
> +			dev_warn(dev, "failed to schedule teardown: %d\n",
> +				 error);
>  	}
> 
> -	gpiochip_remove(&kpad->gc);
> +	return 0;
>  }
> +
>  #else
>  static inline int adp5588_gpio_add(struct adp5588_kpad *kpad)  {
>  	return 0;
>  }
> -
> -static inline void adp5588_gpio_remove(struct adp5588_kpad *kpad) -{ -}
> #endif
> 
>  static void adp5588_report_events(struct adp5588_kpad *kpad, int ev_cnt)
> @@ -510,21 +506,20 @@ static int adp5588_probe(struct i2c_client *client,
>  		return -EINVAL;
>  	}
> 
> -	kpad = kzalloc(sizeof(*kpad), GFP_KERNEL);
> -	input = input_allocate_device();
> -	if (!kpad || !input) {
> -		error = -ENOMEM;
> -		goto err_free_mem;
> -	}
> +	kpad = devm_kzalloc(&client->dev, sizeof(*kpad), GFP_KERNEL);
> +	if (!kpad)
> +		return -ENOMEM;
> +
> +	input = devm_input_allocate_device(&client->dev);
> +	if (!input)
> +		return -ENOMEM;
> 
>  	kpad->client = client;
>  	kpad->input = input;
> 
>  	ret = adp5588_read(client, DEV_ID);
> -	if (ret < 0) {
> -		error = ret;
> -		goto err_free_mem;
> -	}
> +	if (ret < 0)
> +		return ret;
> 
>  	revid = (u8) ret & ADP5588_DEVICE_ID_MASK;
>  	if (WA_DELAYED_READOUT_REVID(revid))
> @@ -532,7 +527,6 @@ static int adp5588_probe(struct i2c_client *client,
> 
>  	input->name = client->name;
>  	input->phys = "adp5588-keys/input0";
> -	input->dev.parent = &client->dev;
> 
>  	input_set_drvdata(input, kpad);
> 
> @@ -569,58 +563,43 @@ static int adp5588_probe(struct i2c_client *client,
> 
>  	error = input_register_device(input);
>  	if (error) {
> -		dev_err(&client->dev, "unable to register input device\n");
> -		goto err_free_mem;
> +		dev_err(&client->dev, "unable to register input device: %d\n",
> +			error);
> +		return error;
>  	}
> 
> -	error = request_threaded_irq(client->irq,
> -				     adp5588_hard_irq, adp5588_thread_irq,
> -				     IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> -				     client->dev.driver->name, kpad);
> +	error = devm_request_threaded_irq(&client->dev, client->irq,
> +					  adp5588_hard_irq,
> adp5588_thread_irq,
> +					  IRQF_TRIGGER_FALLING |
> IRQF_ONESHOT,
> +					  client->dev.driver->name, kpad);
>  	if (error) {
> -		dev_err(&client->dev, "irq %d busy?\n", client->irq);
> -		goto err_unreg_dev;
> +		dev_err(&client->dev, "failed to request irq %d: %d\n",
> +			client->irq, error);
> +		return error;
>  	}
> 
>  	error = adp5588_setup(client);
>  	if (error)
> -		goto err_free_irq;
> +		return error;
> 
>  	if (kpad->gpimapsize)
>  		adp5588_report_switch_state(kpad);
> 
>  	error = adp5588_gpio_add(kpad);
>  	if (error)
> -		goto err_free_irq;
> +		return error;
> 
>  	device_init_wakeup(&client->dev, 1);
> -	i2c_set_clientdata(client, kpad);
> 
>  	dev_info(&client->dev, "Rev.%d keypad, irq %d\n", revid, client->irq);
>  	return 0;
> -
> - err_free_irq:
> -	free_irq(client->irq, kpad);
> - err_unreg_dev:
> -	input_unregister_device(input);
> -	input = NULL;
> - err_free_mem:
> -	input_free_device(input);
> -	kfree(kpad);
> -
> -	return error;
>  }
> 
>  static int adp5588_remove(struct i2c_client *client)  {
> -	struct adp5588_kpad *kpad = i2c_get_clientdata(client);
> -
>  	adp5588_write(client, CFG, 0);
> -	free_irq(client->irq, kpad);
> -	input_unregister_device(kpad->input);
> -	adp5588_gpio_remove(kpad);
> -	kfree(kpad);
> 
> +	/* all resources will be freed by devm */
>  	return 0;
>  }
> 
> --
> 2.36.1.124.g0e6072fb45-goog


^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH 4/4] Input: adp5588-keys - do not explicitly set device as wakeup source
  2022-05-28  4:56 ` [PATCH 4/4] Input: adp5588-keys - do not explicitly set device as wakeup source Dmitry Torokhov
@ 2022-05-30 10:11   ` Hennerich, Michael
  0 siblings, 0 replies; 11+ messages in thread
From: Hennerich, Michael @ 2022-05-30 10:11 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: Uwe Kleine-König, linux-input, linux-kernel



> -----Original Message-----
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Sent: Samstag, 28. Mai 2022 06:57
> To: Hennerich, Michael <Michael.Hennerich@analog.com>
> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>; linux-
> input@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH 4/4] Input: adp5588-keys - do not explicitly set device as
> wakeup source
> 
> [External]
> 
> I2C core will set up device as a wakeup source and will configure interrupt as a
> wakeup interrupt if client is created with I2C_CLIENT_WAKE flag. Let's rely on
> this facility and to not unconditionally set up the device as wakeup device in the
> driver.
> 


Looks good! Thanks for the cleanup!

> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Acked-by: Michael Hennerich <michael.hennerich@analog.com>


> ---
>  drivers/input/keyboard/adp5588-keys.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/drivers/input/keyboard/adp5588-keys.c
> b/drivers/input/keyboard/adp5588-keys.c
> index df84a2998ed2..2a274240facb 100644
> --- a/drivers/input/keyboard/adp5588-keys.c
> +++ b/drivers/input/keyboard/adp5588-keys.c
> @@ -589,8 +589,6 @@ static int adp5588_probe(struct i2c_client *client,
>  	if (error)
>  		return error;
> 
> -	device_init_wakeup(&client->dev, 1);
> -
>  	dev_info(&client->dev, "Rev.%d keypad, irq %d\n", revid, client->irq);
>  	return 0;
>  }
> @@ -609,9 +607,6 @@ static int __maybe_unused adp5588_suspend(struct
> device *dev)
> 
>  	disable_irq(client->irq);
> 
> -	if (device_may_wakeup(&client->dev))
> -		enable_irq_wake(client->irq);
> -
>  	return 0;
>  }
> 
> @@ -619,9 +614,6 @@ static int __maybe_unused adp5588_resume(struct
> device *dev)  {
>  	struct i2c_client *client = to_i2c_client(dev);
> 
> -	if (device_may_wakeup(&client->dev))
> -		disable_irq_wake(client->irq);
> -
>  	enable_irq(client->irq);
> 
>  	return 0;
> --
> 2.36.1.124.g0e6072fb45-goog


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2022-05-30 10:11 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-28  4:56 [PATCH 1/4] Input: adp5588-keys - drop CONFIG_PM guards Dmitry Torokhov
2022-05-28  4:56 ` [PATCH 2/4] Input: adp5588-keys - switch to using threaded interrupt Dmitry Torokhov
2022-05-30 10:10   ` Hennerich, Michael
2022-05-28  4:56 ` [PATCH 3/4] Input: adp5588-keys - switch to using managed resources Dmitry Torokhov
2022-05-28 19:37   ` Uwe Kleine-König
2022-05-29  5:22     ` Dmitry Torokhov
2022-05-29  6:11       ` Uwe Kleine-König
2022-05-30 10:11   ` Hennerich, Michael
2022-05-28  4:56 ` [PATCH 4/4] Input: adp5588-keys - do not explicitly set device as wakeup source Dmitry Torokhov
2022-05-30 10:11   ` Hennerich, Michael
2022-05-30 10:06 ` [PATCH 1/4] Input: adp5588-keys - drop CONFIG_PM guards Hennerich, Michael

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).