gpio-keys-polled improvements

gpio-keys-polled improvements

[Enrico Weigelt, metux IT consult]

Hello folks,


here're some improvements for the gpio-keys-polled driver.

The first patch isn't in the driver itself, but adds a little module helper
for conditionally declaring oftree module table if oftree is enabled
(only needed for the last patch)


have fun.

--mtx

[PATCH 1/4] mod_devicetable: helper macro for declaring oftree module device table

[Enrico Weigelt, metux IT consult]

Little helper macro that declares an oftree module device table,
if CONFIG_OF is enabled. Otherwise it's just noop.

This is also helpful if some drivers can be built w/ or w/o
oftree support.

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
---
 include/linux/mod_devicetable.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 448621c..853e301 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -245,6 +245,15 @@ struct of_device_id {
 	const void *data;
 };
 
+/*
+ * macro for adding the of module device table only if CONFIG_OF enabled
+ */
+#ifdef CONFIG_OF
+#define MODULE_DEVICE_TABLE_OF(name)	MODULE_DEVICE_TABLE(of,name)
+#else
+#define MODULE_DEVICE_TABLE_OF(name)
+#endif /* CONFIG_OF */
+
 /* VIO */
 struct vio_device_id {
 	char type[32];
-- 
1.9.1

Re: [PATCH 1/4] mod_devicetable: helper macro for declaring oftree module device table

[Dmitry Torokhov]

Hi Enrico,

On Tue, Apr 16, 2019 at 09:57:22PM +0200, Enrico Weigelt, metux IT consult wrote:
> Little helper macro that declares an oftree module device table,
> if CONFIG_OF is enabled. Otherwise it's just noop.
> 
> This is also helpful if some drivers can be built w/ or w/o
> oftree support.

This should go to OF folks, please.

> 
> Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
> ---
>  include/linux/mod_devicetable.h | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 448621c..853e301 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -245,6 +245,15 @@ struct of_device_id {
>  	const void *data;
>  };
>  
> +/*
> + * macro for adding the of module device table only if CONFIG_OF enabled
> + */
> +#ifdef CONFIG_OF
> +#define MODULE_DEVICE_TABLE_OF(name)	MODULE_DEVICE_TABLE(of,name)
> +#else
> +#define MODULE_DEVICE_TABLE_OF(name)
> +#endif /* CONFIG_OF */
> +
>  /* VIO */
>  struct vio_device_id {
>  	char type[32];
> -- 
> 1.9.1
> 

-- 
Dmitry

Re: [PATCH 1/4] mod_devicetable: helper macro for declaring oftree module device table

[Enrico Weigelt, metux IT consult]

On 19.04.19 09:40, Dmitry Torokhov wrote:
> Hi Enrico,
> 
> On Tue, Apr 16, 2019 at 09:57:22PM +0200, Enrico Weigelt, metux IT consult wrote:
>> Little helper macro that declares an oftree module device table,
>> if CONFIG_OF is enabled. Otherwise it's just noop.
>>
>> This is also helpful if some drivers can be built w/ or w/o
>> oftree support.
> 
> This should go to OF folks, please.

hmm, they should be CCed, if my script works right.
This one is only needed for the 4th patch (skip oftree...).


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [PATCH 1/4] mod_devicetable: helper macro for declaring oftree module device table

[Frank Rowand]

On 4/24/19 3:48 AM, Enrico Weigelt, metux IT consult wrote:
> On 19.04.19 09:40, Dmitry Torokhov wrote:
>> Hi Enrico,
>>
>> On Tue, Apr 16, 2019 at 09:57:22PM +0200, Enrico Weigelt, metux IT consult wrote:
>>> Little helper macro that declares an oftree module device table,
>>> if CONFIG_OF is enabled. Otherwise it's just noop.
>>>
>>> This is also helpful if some drivers can be built w/ or w/o
>>> oftree support.
>>
>> This should go to OF folks, please.
> 
> hmm, they should be CCed, if my script works right.
> This one is only needed for the 4th patch (skip oftree...).

Your script did not work (BTDT).  I just happened to notice this on lkml.

-Frank

> 
> 
> --mtx
> 

[PATCH 2/4] input: keyboard: gpio-keys-polled: use input name from pdata if available

[Enrico Weigelt, metux IT consult]

Instead of hardcoding the input name to the driver name
('gpio-keys-polled'), allow the passing a name via platform data
('name' field was already present), but default to old behaviour
in case of NULL.

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: linux-input@vger.kernel.org
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
---
 drivers/input/keyboard/gpio_keys_polled.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/keyboard/gpio_keys_polled.c b/drivers/input/keyboard/gpio_keys_polled.c
index edc7262..3312186 100644
--- a/drivers/input/keyboard/gpio_keys_polled.c
+++ b/drivers/input/keyboard/gpio_keys_polled.c
@@ -272,7 +272,7 @@ static int gpio_keys_polled_probe(struct platform_device *pdev)
 
 	input = poll_dev->input;
 
-	input->name = pdev->name;
+	input->name = (pdata->name ? pdata->name : pdev->name);
 	input->phys = DRV_NAME"/input0";
 
 	input->id.bustype = BUS_HOST;
-- 
1.9.1

[PATCH 3/4] input: keyboard: gpio_keys_polled: use gpio lookup table

[Enrico Weigelt, metux IT consult]

Support the recently introduced gpio lookup tables for
attaching to gpio lines. So, harcoded gpio numbers aren't
needed anymore.

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: linux-input@vger.kernel.org
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
---
 drivers/input/keyboard/gpio_keys_polled.c | 167 +++++++++++++++++++++---------
 1 file changed, 119 insertions(+), 48 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys_polled.c b/drivers/input/keyboard/gpio_keys_polled.c
index 3312186..3f773b2 100644
--- a/drivers/input/keyboard/gpio_keys_polled.c
+++ b/drivers/input/keyboard/gpio_keys_polled.c
@@ -24,6 +24,7 @@
 #include <linux/platform_device.h>
 #include <linux/gpio.h>
 #include <linux/gpio/consumer.h>
+#include <linux/gpio/machine.h>
 #include <linux/gpio_keys.h>
 #include <linux/property.h>
 
@@ -227,6 +228,119 @@ static void gpio_keys_polled_set_abs_params(struct input_dev *input,
 };
 MODULE_DEVICE_TABLE(of, gpio_keys_polled_of_match);
 
+static struct gpio_desc *gpio_keys_polled_get_gpiod_fwnode(
+	struct device *dev,
+	int idx,
+	const char *desc)
+{
+	struct gpio_desc *gpiod;
+	struct fwnode_handle *child;
+	int x;
+
+	/* get the idx'th child node */
+	child = device_get_next_child_node(dev, NULL);
+	while (child && x) {
+		child = device_get_next_child_node(dev, child);
+		x--;
+	}
+
+	if (!child) {
+		dev_err(dev, "missing oftree child node #%d\n", idx);
+		return ERR_PTR(-EINVAL);
+	}
+
+	gpiod = devm_fwnode_get_gpiod_from_child(dev,
+						 NULL,
+						 child,
+						 GPIOD_IN,
+						 desc);
+	if (IS_ERR(gpiod)) {
+		if (PTR_ERR(gpiod) != -EPROBE_DEFER)
+			dev_err(dev,
+				"failed to get gpio: %ld\n",
+				PTR_ERR(gpiod));
+		fwnode_handle_put(child);
+		return gpiod;
+	}
+
+	return gpiod;
+}
+
+static struct gpio_desc *gpio_keys_polled_get_gpiod_legacy(
+	struct device *dev,
+	int idx,
+	const struct gpio_keys_button *button)
+{
+	/*
+	 * Legacy GPIO number so request the GPIO here and
+	 * convert it to descriptor.
+	 */
+	unsigned int flags = GPIOF_IN;
+	struct gpio_desc *gpiod;
+	int error;
+
+	dev_info(dev, "hardcoded gpio IDs are deprecated.\n");
+
+	if (button->active_low)
+		flags |= GPIOF_ACTIVE_LOW;
+
+	error = devm_gpio_request_one(dev, button->gpio,
+			flags, button->desc ? : DRV_NAME);
+	if (error) {
+		dev_err(dev,
+			"unable to claim gpio %u, err=%d\n",
+			button->gpio, error);
+		return ERR_PTR(error);
+	}
+
+	gpiod = gpio_to_desc(button->gpio);
+	if (!gpiod) {
+		dev_err(dev,
+			"unable to convert gpio %u to descriptor\n",
+			button->gpio);
+		return ERR_PTR(-EINVAL);
+	}
+
+	return gpiod;
+}
+
+static struct gpio_desc *gpio_keys_polled_get_gpiod(
+	struct device *dev,
+	int idx,
+	const struct gpio_keys_button *button)
+{
+	struct gpio_desc *gpiod = NULL;
+	int error;
+
+	/* No legacy static platform data - use oftree */
+	if (!dev_get_platdata(dev)) {
+		return gpio_keys_polled_get_gpiod_fwnode(
+			dev, idx, button->desc);
+	}
+
+	gpiod = devm_gpiod_get_index(dev, NULL, idx, GPIOF_IN);
+
+	if (!IS_ERR(gpiod)) {
+		dev_info(dev, "picked gpiod idx %d from gpio table\n", idx);
+		gpiod_set_consumer_name(gpiod, button->desc ? : DRV_NAME);
+		return gpiod;
+	}
+
+	if (PTR_ERR(gpiod) != -ENOENT) {
+		dev_err(dev, "failed fetching gpiod #%d: %d\n",
+			idx, PTR_ERR(gpiod));
+		return gpiod;
+	}
+
+	/* Use legacy gpio id, if defined */
+	if (gpio_is_valid(button->gpio)) {
+		return gpio_keys_polled_get_gpiod_legacy(
+			dev, idx, button);
+	}
+
+	return ERR_PTR(-ENOENT);
+}
+
 static int gpio_keys_polled_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -291,57 +405,14 @@ static int gpio_keys_polled_probe(struct platform_device *pdev)
 
 		if (button->wakeup) {
 			dev_err(dev, DRV_NAME " does not support wakeup\n");
-			fwnode_handle_put(child);
 			return -EINVAL;
 		}
 
-		if (!dev_get_platdata(dev)) {
-			/* No legacy static platform data */
-			child = device_get_next_child_node(dev, child);
-			if (!child) {
-				dev_err(dev, "missing child device node\n");
-				return -EINVAL;
-			}
-
-			bdata->gpiod = devm_fwnode_get_gpiod_from_child(dev,
-								NULL, child,
-								GPIOD_IN,
-								button->desc);
-			if (IS_ERR(bdata->gpiod)) {
-				error = PTR_ERR(bdata->gpiod);
-				if (error != -EPROBE_DEFER)
-					dev_err(dev,
-						"failed to get gpio: %d\n",
-						error);
-				fwnode_handle_put(child);
-				return error;
-			}
-		} else if (gpio_is_valid(button->gpio)) {
-			/*
-			 * Legacy GPIO number so request the GPIO here and
-			 * convert it to descriptor.
-			 */
-			unsigned flags = GPIOF_IN;
-
-			if (button->active_low)
-				flags |= GPIOF_ACTIVE_LOW;
-
-			error = devm_gpio_request_one(dev, button->gpio,
-					flags, button->desc ? : DRV_NAME);
-			if (error) {
-				dev_err(dev,
-					"unable to claim gpio %u, err=%d\n",
-					button->gpio, error);
-				return error;
-			}
-
-			bdata->gpiod = gpio_to_desc(button->gpio);
-			if (!bdata->gpiod) {
-				dev_err(dev,
-					"unable to convert gpio %u to descriptor\n",
-					button->gpio);
-				return -EINVAL;
-			}
+		bdata->gpiod = gpio_keys_polled_get_gpiod(dev, i, button);
+
+		if (IS_ERR(bdata->gpiod)) {
+			dev_err(dev, "failed to fetch gpiod #%d\n", i);
+			return PTR_ERR(bdata->gpiod);
 		}
 
 		bdata->last_state = -1;
-- 
1.9.1

Re: [PATCH 3/4] input: keyboard: gpio_keys_polled: use gpio lookup table

[Dmitry Torokhov]

Hi Enrico,

On Tue, Apr 16, 2019 at 09:57:24PM +0200, Enrico Weigelt, metux IT consult wrote:
> Support the recently introduced gpio lookup tables for
> attaching to gpio lines. So, harcoded gpio numbers aren't
> needed anymore.

I would prefer if gpiod API could parse static board data/gpio lookup
tables for child nodes, instead of adding this to gpio-keys. Now that
Heikki Krogerus work on software nodes has landed I need to resurrect
my patch to gpiolib.

Thanks.

-- 
Dmitry

Re: [PATCH 3/4] input: keyboard: gpio_keys_polled: use gpio lookup table

[Enrico Weigelt, metux IT consult]

On 19.04.19 09:48, Dmitry Torokhov wrote:

> I would prefer if gpiod API could parse static board data/gpio lookup
> tables for child nodes, instead of adding this to gpio-keys. Now that
> Heikki Krogerus work on software nodes has landed I need to resurrect
> my patch to gpiolib.

Of course, a more generic approach would be better.

I did it that way I wasn't that aware that a more generic approach is
in the pipeline, and I needed something usable now.

The patch is already used in the field and seems to work quite well,
but if I'm alway open for better solutions.

I figure that the generic solution will still take some time, so can we
take this one for the next merge window and rework it in the next one ?


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

[PATCH 4/4] input: keyboard: gpio-keys-polled: skip oftree code when CONFIG_OF disabled

[Enrico Weigelt, metux IT consult]

we don't need to build in oftree probing stuff when oftree isn't
enabled at all.

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
---
 drivers/input/keyboard/gpio_keys_polled.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/input/keyboard/gpio_keys_polled.c b/drivers/input/keyboard/gpio_keys_polled.c
index 3f773b2..fbccb89 100644
--- a/drivers/input/keyboard/gpio_keys_polled.c
+++ b/drivers/input/keyboard/gpio_keys_polled.c
@@ -147,6 +147,7 @@ static void gpio_keys_polled_close(struct input_polled_dev *dev)
 static struct gpio_keys_platform_data *
 gpio_keys_polled_get_devtree_pdata(struct device *dev)
 {
+#ifdef CONFIG_OF
 	struct gpio_keys_platform_data *pdata;
 	struct gpio_keys_button *button;
 	struct fwnode_handle *child;
@@ -200,6 +201,9 @@ static void gpio_keys_polled_close(struct input_polled_dev *dev)
 	}
 
 	return pdata;
+#else /* CONFIG_OF */
+	return ERR_PTR(-ENOENT);
+#endif /* CONFIG_OF */
 }
 
 static void gpio_keys_polled_set_abs_params(struct input_dev *input,
@@ -226,7 +230,7 @@ static void gpio_keys_polled_set_abs_params(struct input_dev *input,
 	{ .compatible = "gpio-keys-polled", },
 	{ },
 };
-MODULE_DEVICE_TABLE(of, gpio_keys_polled_of_match);
+MODULE_DEVICE_TABLE_OF(gpio_keys_polled_of_match);
 
 static struct gpio_desc *gpio_keys_polled_get_gpiod_fwnode(
 	struct device *dev,
@@ -452,7 +456,9 @@ static int gpio_keys_polled_probe(struct platform_device *pdev)
 	.probe	= gpio_keys_polled_probe,
 	.driver	= {
 		.name	= DRV_NAME,
+#ifdef CONFIG_OF
 		.of_match_table = gpio_keys_polled_of_match,
+#endif /* CONFIG_OF */
 	},
 };
 module_platform_driver(gpio_keys_polled_driver);
-- 
1.9.1

Re: [PATCH 4/4] input: keyboard: gpio-keys-polled: skip oftree code when CONFIG_OF disabled

[Frank Rowand]

On 4/16/19 12:57 PM, Enrico Weigelt, metux IT consult wrote:
> we don't need to build in oftree probing stuff when oftree isn't
> enabled at all.
> 
> Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
> ---
>  drivers/input/keyboard/gpio_keys_polled.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/keyboard/gpio_keys_polled.c b/drivers/input/keyboard/gpio_keys_polled.c
> index 3f773b2..fbccb89 100644
> --- a/drivers/input/keyboard/gpio_keys_polled.c
> +++ b/drivers/input/keyboard/gpio_keys_polled.c
> @@ -147,6 +147,7 @@ static void gpio_keys_polled_close(struct input_polled_dev *dev)
>  static struct gpio_keys_platform_data *
>  gpio_keys_polled_get_devtree_pdata(struct device *dev)
>  {
> +#ifdef CONFIG_OF
>  	struct gpio_keys_platform_data *pdata;
>  	struct gpio_keys_button *button;
>  	struct fwnode_handle *child;
> @@ -200,6 +201,9 @@ static void gpio_keys_polled_close(struct input_polled_dev *dev)
>  	}
>  
>  	return pdata;
> +#else /* CONFIG_OF */
> +	return ERR_PTR(-ENOENT);
> +#endif /* CONFIG_OF */
>  }
>  
>  static void gpio_keys_polled_set_abs_params(struct input_dev *input,
> @@ -226,7 +230,7 @@ static void gpio_keys_polled_set_abs_params(struct input_dev *input,
>  	{ .compatible = "gpio-keys-polled", },
>  	{ },
>  };


> -MODULE_DEVICE_TABLE(of, gpio_keys_polled_of_match);
> +MODULE_DEVICE_TABLE_OF(gpio_keys_polled_of_match);

Not needed, when you use of_match_ptr() -- see below.

>  
>  static struct gpio_desc *gpio_keys_polled_get_gpiod_fwnode(
>  	struct device *dev,
> @@ -452,7 +456,9 @@ static int gpio_keys_polled_probe(struct platform_device *pdev)
>  	.probe	= gpio_keys_polled_probe,
>  	.driver	= {
>  		.name	= DRV_NAME,

> +#ifdef CONFIG_OF
>  		.of_match_table = gpio_keys_polled_of_match,
> +#endif /* CONFIG_OF */

No need for the #ifdef, use of_match_ptr():

  		.of_match_table = of_match_ptr(gpio_keys_polled_of_match),


>  	},
>  };
>  module_platform_driver(gpio_keys_polled_driver);
> 

Re: [PATCH 4/4] input: keyboard: gpio-keys-polled: skip oftree code when CONFIG_OF disabled

[Enrico Weigelt, metux IT consult]

On 29.04.19 21:44, Frank Rowand wrote:
> On 4/16/19 12:57 PM, Enrico Weigelt, metux IT consult wrote:
>> we don't need to build in oftree probing stuff when oftree isn't
>> enabled at all.
>>
>> Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
>> ---
>>  drivers/input/keyboard/gpio_keys_polled.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/input/keyboard/gpio_keys_polled.c b/drivers/input/keyboard/gpio_keys_polled.c
>> index 3f773b2..fbccb89 100644
>> --- a/drivers/input/keyboard/gpio_keys_polled.c
>> +++ b/drivers/input/keyboard/gpio_keys_polled.c
>> @@ -147,6 +147,7 @@ static void gpio_keys_polled_close(struct input_polled_dev *dev)
>>  static struct gpio_keys_platform_data *
>>  gpio_keys_polled_get_devtree_pdata(struct device *dev)
>>  {
>> +#ifdef CONFIG_OF
>>  	struct gpio_keys_platform_data *pdata;
>>  	struct gpio_keys_button *button;
>>  	struct fwnode_handle *child;
>> @@ -200,6 +201,9 @@ static void gpio_keys_polled_close(struct input_polled_dev *dev)
>>  	}
>>  
>>  	return pdata;
>> +#else /* CONFIG_OF */
>> +	return ERR_PTR(-ENOENT);
>> +#endif /* CONFIG_OF */
>>  }
>>  
>>  static void gpio_keys_polled_set_abs_params(struct input_dev *input,
>> @@ -226,7 +230,7 @@ static void gpio_keys_polled_set_abs_params(struct input_dev *input,
>>  	{ .compatible = "gpio-keys-polled", },
>>  	{ },
>>  };
> 
> 
>> -MODULE_DEVICE_TABLE(of, gpio_keys_polled_of_match);
>> +MODULE_DEVICE_TABLE_OF(gpio_keys_polled_of_match);
> 
> Not needed, when you use of_match_ptr() -- see below.

Shall I remove the MODULE_DEVICE_TABLE... line completely ?

I'd like to have nothing of-related compiled in, when oftree isn't
enabled.

>>  static struct gpio_desc *gpio_keys_polled_get_gpiod_fwnode(
>>  	struct device *dev,
>> @@ -452,7 +456,9 @@ static int gpio_keys_polled_probe(struct platform_device *pdev)
>>  	.probe	= gpio_keys_polled_probe,
>>  	.driver	= {
>>  		.name	= DRV_NAME,
> 
>> +#ifdef CONFIG_OF
>>  		.of_match_table = gpio_keys_polled_of_match,
>> +#endif /* CONFIG_OF */
> 
> No need for the #ifdef, use of_match_ptr():
> 
>   		.of_match_table = of_match_ptr(gpio_keys_polled_of_match),

Ok, thanks.


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287