[PATCH v3 0/2] AM335x-ICE: Add support for rotary switch

This series adds support for rotary-switch on AM335x-ICE that is
connected to TI PCA9536 I2C GPIO expander.
First patch adds new generic driver to read status of group of GPIO
lines and report the value as an input event. The second patch adds DT
entries for the same.

v2: https://lkml.org/lkml/2016/8/23/111
v1: https://lkml.org/lkml/2016/8/12/7


Vignesh R (2):
  input: misc: Add generic input driver to read encoded GPIO lines
  ARM: dts: am335x-icev2: Add nodes for gpio-decoder

 .../devicetree/bindings/input/gpio-decoder.txt     |  23 ++++
 arch/arm/boot/dts/am335x-icev2.dts                 |   9 ++
 drivers/input/misc/Kconfig                         |  12 ++
 drivers/input/misc/Makefile                        |   1 +
 drivers/input/misc/gpio_decoder.c                  | 134 +++++++++++++++++++++
 5 files changed, 179 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/gpio-decoder.txt
 create mode 100644 drivers/input/misc/gpio_decoder.c

-- 
2.9.2

Add a driver to read group of GPIO lines and provide its status as a
numerical value as input event to the system. This will help in
interfacing devices, that can be connected over GPIOs, that provide
input to the system by driving GPIO lines connected to them like a
rotary dial or a switch.

For example, a rotary switch can be connected to four GPIO lines. The
status of the GPIO lines reflect the actual position of the rotary
switch dial. For example, if dial points to 9, then the four GPIO lines
connected to the switch will read HLLH(0b'1001 = 9). This value
can be reported as an ABS_* event to the input subsystem.

Signed-off-by: Vignesh R <vigneshr@ti.com>
Acked-by: Rob Herring <robh@kernel.org>
---

v3: Fix comments by Andrew and Dmitry
Link to v2: https://lkml.org/lkml/2016/8/23/79

 .../devicetree/bindings/input/gpio-decoder.txt     |  23 ++++
 drivers/input/misc/Kconfig                         |  12 ++
 drivers/input/misc/Makefile                        |   1 +
 drivers/input/misc/gpio_decoder.c                  | 134 +++++++++++++++++++++
 4 files changed, 170 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/gpio-decoder.txt
 create mode 100644 drivers/input/misc/gpio_decoder.c

diff --git a/Documentation/devicetree/bindings/input/gpio-decoder.txt b/Documentation/devicetree/bindings/input/gpio-decoder.txt
new file mode 100644
index 000000000000..14a77fb96cf0
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/gpio-decoder.txt

@@ -0,0 +1,23 @@
+* GPIO Decoder DT bindings
+
+Required Properties:
+- compatible: should be "gpio-decoder"
+- gpios: a spec of gpios (at least two) to be decoded to a number with
+  first entry representing the MSB.
+
+Optional Properties:
+- decoder-max-value: Maximum possible value that can be reported by
+  the gpios.
+- linux,axis: the input subsystem axis to map to (ABS_X/ABS_Y).
+  Defaults to 0 (ABS_X).
+
+Example:
+	gpio-decoder0 {
+		compatible = "gpio-decoder";
+		gpios = <&pca9536 3 GPIO_ACTIVE_HIGH>,
+			<&pca9536 2 GPIO_ACTIVE_HIGH>,
+			<&pca9536 1 GPIO_ACTIVE_HIGH>,
+			<&pca9536 0 GPIO_ACTIVE_HIGH>;
+		linux,axis = <0>; /* ABS_X */
+		decoder-max-value = <9>;
+	};

diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index efb0ca871327..7cdb89397d18 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig

@@ -292,6 +292,18 @@ config INPUT_GPIO_TILT_POLLED
 	  To compile this driver as a module, choose M here: the
 	  module will be called gpio_tilt_polled.
 
+config INPUT_GPIO_DECODER
+	tristate "Polled GPIO Decoder Input driver"
+	depends on GPIOLIB || COMPILE_TEST
+	select INPUT_POLLDEV
+	help
+	 Say Y here if you want driver to read status of multiple GPIO
+	 lines and report the encoded value as an absolute integer to
+	 input subsystem.
+
+	 To compile this driver as a module, choose M here: the module
+	 will be called gpio_decoder.
+
 config INPUT_IXP4XX_BEEPER
 	tristate "IXP4XX Beeper support"
 	depends on ARCH_IXP4XX

diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index 6a1e5e20fc1c..0b6d025f0487 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile

@@ -35,6 +35,7 @@ obj-$(CONFIG_INPUT_DRV2667_HAPTICS)	+= drv2667.o
 obj-$(CONFIG_INPUT_GP2A)		+= gp2ap002a00f.o
 obj-$(CONFIG_INPUT_GPIO_BEEPER)		+= gpio-beeper.o
 obj-$(CONFIG_INPUT_GPIO_TILT_POLLED)	+= gpio_tilt_polled.o
+obj-$(CONFIG_INPUT_GPIO_DECODER)	+= gpio_decoder.o
 obj-$(CONFIG_INPUT_HISI_POWERKEY)	+= hisi_powerkey.o
 obj-$(CONFIG_HP_SDC_RTC)		+= hp_sdc_rtc.o
 obj-$(CONFIG_INPUT_IMS_PCU)		+= ims-pcu.o

diff --git a/drivers/input/misc/gpio_decoder.c b/drivers/input/misc/gpio_decoder.c
new file mode 100644
index 000000000000..1c2191d4b143
--- /dev/null
+++ b/drivers/input/misc/gpio_decoder.c

@@ -0,0 +1,134 @@
+/*
+ * Copyright (C) 2016 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * A generic driver to read multiple gpio lines and translate the
+ * encoded numeric value into an input event.
+ */
+
+#include <linux/device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/input.h>
+#include <linux/input-polldev.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+struct gpio_decoder {
+	struct input_polled_dev *poll_dev;
+	struct gpio_descs *input_gpios;
+	struct device *dev;
+	u32 axis;
+	u32 last_stable;
+};
+
+static unsigned int gpio_decoder_get_gpios_state(struct gpio_decoder
+						 *decoder)
+{
+	struct gpio_descs *gpios = decoder->input_gpios;
+	unsigned int ret = 0;
+	int i, val;
+
+	for (i = 0; i < gpios->ndescs; i++) {
+		val = gpiod_get_value_cansleep(gpios->desc[i]);
+		if (val >= 0) {
+			ret = (ret << 1) | !!val;
+		} else {
+			dev_err(decoder->dev, "Error reading gpio\n");
+			break;
+		}
+	}
+
+	return ret;
+}
+
+static void gpio_decoder_poll_gpios(struct input_polled_dev *poll_dev)
+{
+	struct gpio_decoder *decoder = poll_dev->private;
+	unsigned int state = gpio_decoder_get_gpios_state(decoder);
+
+	if (state != decoder->last_stable) {
+		input_report_abs(poll_dev->input, decoder->axis, state);
+		input_sync(poll_dev->input);
+		decoder->last_stable = state;
+	}
+}
+
+static int gpio_decoder_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct gpio_decoder *decoder;
+	struct input_polled_dev *poll_dev;
+	u32  max;
+	int err;
+
+	decoder = devm_kzalloc(dev, sizeof(struct gpio_decoder), GFP_KERNEL);
+	if (!decoder)
+		return -ENOMEM;
+
+	device_property_read_u32(dev, "linux,axis", &decoder->axis);
+	decoder->input_gpios = devm_gpiod_get_array(dev, NULL, GPIOD_IN);
+	if (IS_ERR(decoder->input_gpios)) {
+		dev_err(dev, "unable to acquire input gpios\n");
+		return PTR_ERR(decoder->input_gpios);
+	}
+	if (decoder->input_gpios->ndescs < 2) {
+		dev_err(dev, "not enough gpios found\n");
+		return -EINVAL;
+	}
+
+	if (device_property_read_u32(dev, "decoder-max-value", &max))
+		max = BIT(decoder->input_gpios->ndescs);
+
+	decoder->dev = dev;
+	poll_dev = devm_input_allocate_polled_device(decoder->dev);
+	if (!poll_dev)
+		return -ENOMEM;
+
+	poll_dev->private = decoder;
+	poll_dev->poll = gpio_decoder_poll_gpios;
+	decoder->poll_dev = poll_dev;
+
+	poll_dev->input->name = pdev->name;
+	poll_dev->input->id.bustype = BUS_HOST;
+	input_set_abs_params(poll_dev->input, decoder->axis, 0, max, 0, 1);
+
+	err = input_register_polled_device(poll_dev);
+	if (err) {
+		dev_err(dev, "failed to register polled  device\n");
+		return err;
+	}
+	platform_set_drvdata(pdev, decoder);
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id gpio_decoder_of_match[] = {
+	{ .compatible = "gpio-decoder", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, gpio_decoder_of_match);
+#endif
+
+static struct platform_driver gpio_decoder_driver = {
+	.probe		= gpio_decoder_probe,
+	.driver		= {
+		.name	= "gpio-decoder",
+		.of_match_table = of_match_ptr(gpio_decoder_of_match),
+	}
+};
+module_platform_driver(gpio_decoder_driver);
+
+MODULE_DESCRIPTION("GPIO decoder input driver");
+MODULE_AUTHOR("Vignesh R <vigneshr@ti.com>");
+MODULE_LICENSE("GPL v2");

-- 
2.9.2

AM335x ICE board has a rotary-switch connected to PCA9536 I2C GPIO
expander. The position of the rotary-switch is reflected by status of
GPIO lines. Add gpio-decoder node to read these GPIO line status via
gpio-decoder driver and report it as an input event to the system.

Signed-off-by: Vignesh R <vigneshr@ti.com>
---

v3: s/decoder,max-value/decoder-max-value

 arch/arm/boot/dts/am335x-icev2.dts | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/am335x-icev2.dts b/arch/arm/boot/dts/am335x-icev2.dts
index bc0190e4f10f..426022ee85c7 100644
--- a/arch/arm/boot/dts/am335x-icev2.dts
+++ b/arch/arm/boot/dts/am335x-icev2.dts

@@ -139,6 +139,15 @@
 			default-state = "off";
 		};
 	};
+	gpio-decoder {
+		compatible = "gpio-decoder";
+		gpios = <&pca9536 3 GPIO_ACTIVE_HIGH>,
+			<&pca9536 2 GPIO_ACTIVE_HIGH>,
+			<&pca9536 1 GPIO_ACTIVE_HIGH>,
+			<&pca9536 0 GPIO_ACTIVE_HIGH>;
+		linux,axis = <0>; /* ABS_X */
+		decoder-max-value = <9>;
+	};
 };
 
 &am33xx_pinmux {

-- 
2.9.2

Hi,

On 08/24/2016 09:58 AM, Vignesh R wrote:

This series adds support for rotary-switch on AM335x-ICE that is
connected to TI PCA9536 I2C GPIO expander.
First patch adds new generic driver to read status of group of GPIO
lines and report the value as an input event. The second patch adds DT
entries for the same.

v2: https://lkml.org/lkml/2016/8/23/111
v1: https://lkml.org/lkml/2016/8/12/7

Is there a reason why the rotary-encoder driver cannot handle this?
Commit 7dde4e74744 ("Input: rotary-encoder - support more than 2 gpios
as input") added support for that mode AFAIU.

I copied the author of that patch to have a look.


Thanks,
Daniel

Hi,

On Wednesday 24 August 2016 02:05 PM, Daniel Mack wrote:

Hi,

On 08/24/2016 09:58 AM, Vignesh R wrote:

quoted

This series adds support for rotary-switch on AM335x-ICE that is
connected to TI PCA9536 I2C GPIO expander.
First patch adds new generic driver to read status of group of GPIO
lines and report the value as an input event. The second patch adds DT
entries for the same.

v2: https://lkml.org/lkml/2016/8/23/111
v1: https://lkml.org/lkml/2016/8/12/7

Is there a reason why the rotary-encoder driver cannot handle this?
Commit 7dde4e74744 ("Input: rotary-encoder - support more than 2 gpios
as input") added support for that mode AFAIU.

Rotary encoder driver handles incremental encoders only and does not
support absolute encoders. The rotary switch on am335x-ice is different
from the incremental encoders in the
sense that GPIO line status directly reflect the position(number)
pointed by the dial of the encoder. So, there is no need to count steps
or know the direction of rotation as it does not matter.

I did try to enhance rotary-encoder driver to support absolute
encoder[1] but the comment there was to write new driver that simply
translates gpio-encoded value into ABS* event. Indeed, the new driver
looks more simple and can handle more such hardwares.

[1] https://lkml.org/lkml/2016/5/19/98


-- 
Regards
Vignesh

On 08/24/2016 11:15 AM, Vignesh R wrote:

On Wednesday 24 August 2016 02:05 PM, Daniel Mack wrote:

quoted

On 08/24/2016 09:58 AM, Vignesh R wrote:

quoted

This series adds support for rotary-switch on AM335x-ICE that is
connected to TI PCA9536 I2C GPIO expander.
First patch adds new generic driver to read status of group of GPIO
lines and report the value as an input event. The second patch adds DT
entries for the same.

v2: https://lkml.org/lkml/2016/8/23/111
v1: https://lkml.org/lkml/2016/8/12/7

Is there a reason why the rotary-encoder driver cannot handle this?
Commit 7dde4e74744 ("Input: rotary-encoder - support more than 2 gpios
as input") added support for that mode AFAIU.

Rotary encoder driver handles incremental encoders only and does not
support absolute encoders. The rotary switch on am335x-ice is different
from the incremental encoders in the
sense that GPIO line status directly reflect the position(number)
pointed by the dial of the encoder. So, there is no need to count steps
or know the direction of rotation as it does not matter.

I did try to enhance rotary-encoder driver to support absolute
encoder[1] but the comment there was to write new driver that simply
translates gpio-encoded value into ABS* event. Indeed, the new driver
looks more simple and can handle more such hardwares.

Okay. Yes, that makes sense. Thanks for the explanation!


Daniel

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, Aug 24, 2016 at 01:28:58PM +0530, Vignesh R wrote:

I think if we fail reading any one of gpio lines we should skip
reporting the event.

+		}
+	}
+
+	return ret;
+}
+
+static void gpio_decoder_poll_gpios(struct input_polled_dev *poll_dev)
+{
+	struct gpio_decoder *decoder = poll_dev->private;
+	unsigned int state = gpio_decoder_get_gpios_state(decoder);
+
+	if (state != decoder->last_stable) {
+		input_report_abs(poll_dev->input, decoder->axis, state);
+		input_sync(poll_dev->input);
+		decoder->last_stable = state;
+	}
+}
+
+static int gpio_decoder_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct gpio_decoder *decoder;
+	struct input_polled_dev *poll_dev;
+	u32  max;
+	int err;
+
+	decoder = devm_kzalloc(dev, sizeof(struct gpio_decoder), GFP_KERNEL);
+	if (!decoder)
+		return -ENOMEM;
+
+	device_property_read_u32(dev, "linux,axis", &decoder->axis);
+	decoder->input_gpios = devm_gpiod_get_array(dev, NULL, GPIOD_IN);
+	if (IS_ERR(decoder->input_gpios)) {
+		dev_err(dev, "unable to acquire input gpios\n");
+		return PTR_ERR(decoder->input_gpios);
+	}
+	if (decoder->input_gpios->ndescs < 2) {
+		dev_err(dev, "not enough gpios found\n");
+		return -EINVAL;
+	}
+
+	if (device_property_read_u32(dev, "decoder-max-value", &max))
+		max = BIT(decoder->input_gpios->ndescs);

Shouldn't this be

		max = (1U << decoder->input_gpios->ndescs) - 1;

?

+
+	decoder->dev = dev;
+	poll_dev = devm_input_allocate_polled_device(decoder->dev);
+	if (!poll_dev)
+		return -ENOMEM;
+
+	poll_dev->private = decoder;
+	poll_dev->poll = gpio_decoder_poll_gpios;
+	decoder->poll_dev = poll_dev;
+
+	poll_dev->input->name = pdev->name;
+	poll_dev->input->id.bustype = BUS_HOST;
+	input_set_abs_params(poll_dev->input, decoder->axis, 0, max, 0, 1);

Why do you need flat == 1? Do you actually use joydev for this device?

+
+	err = input_register_polled_device(poll_dev);
+	if (err) {
+		dev_err(dev, "failed to register polled  device\n");
+		return err;
+	}
+	platform_set_drvdata(pdev, decoder);
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id gpio_decoder_of_match[] = {
+	{ .compatible = "gpio-decoder", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, gpio_decoder_of_match);
+#endif
+
+static struct platform_driver gpio_decoder_driver = {
+	.probe		= gpio_decoder_probe,
+	.driver		= {
+		.name	= "gpio-decoder",
+		.of_match_table = of_match_ptr(gpio_decoder_of_match),
+	}
+};
+module_platform_driver(gpio_decoder_driver);
+
+MODULE_DESCRIPTION("GPIO decoder input driver");
+MODULE_AUTHOR("Vignesh R [off-list ref]");
+MODULE_LICENSE("GPL v2");
-- 
2.9.2

No need to resubmit, I can adjust on my side.

-- 
Dmitry

On Thursday 25 August 2016 10:26 PM, Dmitry Torokhov wrote:

On Wed, Aug 24, 2016 at 01:28:58PM +0530, Vignesh R wrote:

quoted

Add a driver to read group of GPIO lines and provide its status as a
numerical value as input event to the system. This will help in
interfacing devices, that can be connected over GPIOs, that provide
input to the system by driving GPIO lines connected to them like a
rotary dial or a switch.

For example, a rotary switch can be connected to four GPIO lines. The
status of the GPIO lines reflect the actual position of the rotary
switch dial. For example, if dial points to 9, then the four GPIO lines
connected to the switch will read HLLH(0b'1001 = 9). This value
can be reported as an ABS_* event to the input subsystem.

Signed-off-by: Vignesh R <redacted>
Acked-by: Rob Herring <redacted>
---

v3: Fix comments by Andrew and Dmitry
Link to v2: https://lkml.org/lkml/2016/8/23/79

 .../devicetree/bindings/input/gpio-decoder.txt     |  23 ++++
 drivers/input/misc/Kconfig                         |  12 ++
 drivers/input/misc/Makefile                        |   1 +
 drivers/input/misc/gpio_decoder.c                  | 134 +++++++++++++++++++++
 4 files changed, 170 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/gpio-decoder.txt
 create mode 100644 drivers/input/misc/gpio_decoder.c

diff --git a/Documentation/devicetree/bindings/input/gpio-decoder.txt b/Documentation/devicetree/bindings/input/gpio-decoder.txt
new file mode 100644
index 000000000000..14a77fb96cf0
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/gpio-decoder.txt

@@ -0,0 +1,23 @@
+* GPIO Decoder DT bindings
+
+Required Properties:
+- compatible: should be "gpio-decoder"
+- gpios: a spec of gpios (at least two) to be decoded to a number with
+  first entry representing the MSB.
+
+Optional Properties:
+- decoder-max-value: Maximum possible value that can be reported by
+  the gpios.
+- linux,axis: the input subsystem axis to map to (ABS_X/ABS_Y).
+  Defaults to 0 (ABS_X).
+
+Example:
+	gpio-decoder0 {
+		compatible = "gpio-decoder";
+		gpios = <&pca9536 3 GPIO_ACTIVE_HIGH>,
+			<&pca9536 2 GPIO_ACTIVE_HIGH>,
+			<&pca9536 1 GPIO_ACTIVE_HIGH>,
+			<&pca9536 0 GPIO_ACTIVE_HIGH>;
+		linux,axis = <0>; /* ABS_X */
+		decoder-max-value = <9>;
+	};

diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index efb0ca871327..7cdb89397d18 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig

@@ -292,6 +292,18 @@ config INPUT_GPIO_TILT_POLLED
 	  To compile this driver as a module, choose M here: the
 	  module will be called gpio_tilt_polled.
 
+config INPUT_GPIO_DECODER
+	tristate "Polled GPIO Decoder Input driver"
+	depends on GPIOLIB || COMPILE_TEST
+	select INPUT_POLLDEV
+	help
+	 Say Y here if you want driver to read status of multiple GPIO
+	 lines and report the encoded value as an absolute integer to
+	 input subsystem.
+
+	 To compile this driver as a module, choose M here: the module
+	 will be called gpio_decoder.
+
 config INPUT_IXP4XX_BEEPER
 	tristate "IXP4XX Beeper support"
 	depends on ARCH_IXP4XX

diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index 6a1e5e20fc1c..0b6d025f0487 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile

@@ -35,6 +35,7 @@ obj-$(CONFIG_INPUT_DRV2667_HAPTICS)	+= drv2667.o
 obj-$(CONFIG_INPUT_GP2A)		+= gp2ap002a00f.o
 obj-$(CONFIG_INPUT_GPIO_BEEPER)		+= gpio-beeper.o
 obj-$(CONFIG_INPUT_GPIO_TILT_POLLED)	+= gpio_tilt_polled.o
+obj-$(CONFIG_INPUT_GPIO_DECODER)	+= gpio_decoder.o
 obj-$(CONFIG_INPUT_HISI_POWERKEY)	+= hisi_powerkey.o
 obj-$(CONFIG_HP_SDC_RTC)		+= hp_sdc_rtc.o
 obj-$(CONFIG_INPUT_IMS_PCU)		+= ims-pcu.o

diff --git a/drivers/input/misc/gpio_decoder.c b/drivers/input/misc/gpio_decoder.c
new file mode 100644
index 000000000000..1c2191d4b143
--- /dev/null
+++ b/drivers/input/misc/gpio_decoder.c

@@ -0,0 +1,134 @@
+/*
+ * Copyright (C) 2016 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * A generic driver to read multiple gpio lines and translate the
+ * encoded numeric value into an input event.
+ */
+
+#include <linux/device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/input.h>
+#include <linux/input-polldev.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+struct gpio_decoder {
+	struct input_polled_dev *poll_dev;
+	struct gpio_descs *input_gpios;
+	struct device *dev;
+	u32 axis;
+	u32 last_stable;
+};
+
+static unsigned int gpio_decoder_get_gpios_state(struct gpio_decoder
+						 *decoder)
+{
+	struct gpio_descs *gpios = decoder->input_gpios;
+	unsigned int ret = 0;
+	int i, val;
+
+	for (i = 0; i < gpios->ndescs; i++) {
+		val = gpiod_get_value_cansleep(gpios->desc[i]);
+		if (val >= 0) {
+			ret = (ret << 1) | !!val;
+		} else {
+			dev_err(decoder->dev, "Error reading gpio\n");
+			break;

I think if we fail reading any one of gpio lines we should skip
reporting the event.

Ok.

quoted

+		}
+	}
+
+	return ret;
+}
+
+static void gpio_decoder_poll_gpios(struct input_polled_dev *poll_dev)
+{
+	struct gpio_decoder *decoder = poll_dev->private;
+	unsigned int state = gpio_decoder_get_gpios_state(decoder);
+
+	if (state != decoder->last_stable) {
+		input_report_abs(poll_dev->input, decoder->axis, state);
+		input_sync(poll_dev->input);
+		decoder->last_stable = state;
+	}
+}
+
+static int gpio_decoder_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct gpio_decoder *decoder;
+	struct input_polled_dev *poll_dev;
+	u32  max;
+	int err;
+
+	decoder = devm_kzalloc(dev, sizeof(struct gpio_decoder), GFP_KERNEL);
+	if (!decoder)
+		return -ENOMEM;
+
+	device_property_read_u32(dev, "linux,axis", &decoder->axis);
+	decoder->input_gpios = devm_gpiod_get_array(dev, NULL, GPIOD_IN);
+	if (IS_ERR(decoder->input_gpios)) {
+		dev_err(dev, "unable to acquire input gpios\n");
+		return PTR_ERR(decoder->input_gpios);
+	}
+	if (decoder->input_gpios->ndescs < 2) {
+		dev_err(dev, "not enough gpios found\n");
+		return -EINVAL;
+	}
+
+	if (device_property_read_u32(dev, "decoder-max-value", &max))
+		max = BIT(decoder->input_gpios->ndescs);

Shouldn't this be

		max = (1U << decoder->input_gpios->ndescs) - 1;

?

Oops, you are right.

quoted

+
+	decoder->dev = dev;
+	poll_dev = devm_input_allocate_polled_device(decoder->dev);
+	if (!poll_dev)
+		return -ENOMEM;
+
+	poll_dev->private = decoder;
+	poll_dev->poll = gpio_decoder_poll_gpios;
+	decoder->poll_dev = poll_dev;
+
+	poll_dev->input->name = pdev->name;
+	poll_dev->input->id.bustype = BUS_HOST;
+	input_set_abs_params(poll_dev->input, decoder->axis, 0, max, 0, 1);

Why do you need flat == 1? Do you actually use joydev for this device?

Sorry, I overlooked flat param when I copied this line from
rotary-encoder.c.

quoted

+
+	err = input_register_polled_device(poll_dev);
+	if (err) {
+		dev_err(dev, "failed to register polled  device\n");
+		return err;
+	}
+	platform_set_drvdata(pdev, decoder);
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id gpio_decoder_of_match[] = {
+	{ .compatible = "gpio-decoder", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, gpio_decoder_of_match);
+#endif
+
+static struct platform_driver gpio_decoder_driver = {
+	.probe		= gpio_decoder_probe,
+	.driver		= {
+		.name	= "gpio-decoder",
+		.of_match_table = of_match_ptr(gpio_decoder_of_match),
+	}
+};
+module_platform_driver(gpio_decoder_driver);
+
+MODULE_DESCRIPTION("GPIO decoder input driver");
+MODULE_AUTHOR("Vignesh R [off-list ref]");
+MODULE_LICENSE("GPL v2");
-- 
2.9.2

No need to resubmit, I can adjust on my side.

Thanks a lot!



-- 
Regards
Vignesh
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Aug 29, 2016 at 09:50:28AM +0530, Vignesh R wrote:

On Thursday 25 August 2016 10:26 PM, Dmitry Torokhov wrote:

quoted

On Wed, Aug 24, 2016 at 01:28:58PM +0530, Vignesh R wrote:

quoted

Add a driver to read group of GPIO lines and provide its status as a
numerical value as input event to the system. This will help in
interfacing devices, that can be connected over GPIOs, that provide
input to the system by driving GPIO lines connected to them like a
rotary dial or a switch.

For example, a rotary switch can be connected to four GPIO lines. The
status of the GPIO lines reflect the actual position of the rotary
switch dial. For example, if dial points to 9, then the four GPIO lines
connected to the switch will read HLLH(0b'1001 = 9). This value
can be reported as an ABS_* event to the input subsystem.

Signed-off-by: Vignesh R <vigneshr@ti.com>
Acked-by: Rob Herring <robh@kernel.org>
---

v3: Fix comments by Andrew and Dmitry
Link to v2: https://lkml.org/lkml/2016/8/23/79

 .../devicetree/bindings/input/gpio-decoder.txt     |  23 ++++
 drivers/input/misc/Kconfig                         |  12 ++
 drivers/input/misc/Makefile                        |   1 +
 drivers/input/misc/gpio_decoder.c                  | 134 +++++++++++++++++++++
 4 files changed, 170 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/gpio-decoder.txt
 create mode 100644 drivers/input/misc/gpio_decoder.c

diff --git a/Documentation/devicetree/bindings/input/gpio-decoder.txt b/Documentation/devicetree/bindings/input/gpio-decoder.txt
new file mode 100644
index 000000000000..14a77fb96cf0
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/gpio-decoder.txt

@@ -0,0 +1,23 @@
+* GPIO Decoder DT bindings
+
+Required Properties:
+- compatible: should be "gpio-decoder"
+- gpios: a spec of gpios (at least two) to be decoded to a number with
+  first entry representing the MSB.
+
+Optional Properties:
+- decoder-max-value: Maximum possible value that can be reported by
+  the gpios.
+- linux,axis: the input subsystem axis to map to (ABS_X/ABS_Y).
+  Defaults to 0 (ABS_X).
+
+Example:
+	gpio-decoder0 {
+		compatible = "gpio-decoder";
+		gpios = <&pca9536 3 GPIO_ACTIVE_HIGH>,
+			<&pca9536 2 GPIO_ACTIVE_HIGH>,
+			<&pca9536 1 GPIO_ACTIVE_HIGH>,
+			<&pca9536 0 GPIO_ACTIVE_HIGH>;
+		linux,axis = <0>; /* ABS_X */
+		decoder-max-value = <9>;
+	};

diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index efb0ca871327..7cdb89397d18 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig

@@ -292,6 +292,18 @@ config INPUT_GPIO_TILT_POLLED
 	  To compile this driver as a module, choose M here: the
 	  module will be called gpio_tilt_polled.
 
+config INPUT_GPIO_DECODER
+	tristate "Polled GPIO Decoder Input driver"
+	depends on GPIOLIB || COMPILE_TEST
+	select INPUT_POLLDEV
+	help
+	 Say Y here if you want driver to read status of multiple GPIO
+	 lines and report the encoded value as an absolute integer to
+	 input subsystem.
+
+	 To compile this driver as a module, choose M here: the module
+	 will be called gpio_decoder.
+
 config INPUT_IXP4XX_BEEPER
 	tristate "IXP4XX Beeper support"
 	depends on ARCH_IXP4XX

diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index 6a1e5e20fc1c..0b6d025f0487 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile

@@ -35,6 +35,7 @@ obj-$(CONFIG_INPUT_DRV2667_HAPTICS)	+= drv2667.o
 obj-$(CONFIG_INPUT_GP2A)		+= gp2ap002a00f.o
 obj-$(CONFIG_INPUT_GPIO_BEEPER)		+= gpio-beeper.o
 obj-$(CONFIG_INPUT_GPIO_TILT_POLLED)	+= gpio_tilt_polled.o
+obj-$(CONFIG_INPUT_GPIO_DECODER)	+= gpio_decoder.o
 obj-$(CONFIG_INPUT_HISI_POWERKEY)	+= hisi_powerkey.o
 obj-$(CONFIG_HP_SDC_RTC)		+= hp_sdc_rtc.o
 obj-$(CONFIG_INPUT_IMS_PCU)		+= ims-pcu.o

diff --git a/drivers/input/misc/gpio_decoder.c b/drivers/input/misc/gpio_decoder.c
new file mode 100644
index 000000000000..1c2191d4b143
--- /dev/null
+++ b/drivers/input/misc/gpio_decoder.c

@@ -0,0 +1,134 @@
+/*
+ * Copyright (C) 2016 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * A generic driver to read multiple gpio lines and translate the
+ * encoded numeric value into an input event.
+ */
+
+#include <linux/device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/input.h>
+#include <linux/input-polldev.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+struct gpio_decoder {
+	struct input_polled_dev *poll_dev;
+	struct gpio_descs *input_gpios;
+	struct device *dev;
+	u32 axis;
+	u32 last_stable;
+};
+
+static unsigned int gpio_decoder_get_gpios_state(struct gpio_decoder
+						 *decoder)
+{
+	struct gpio_descs *gpios = decoder->input_gpios;
+	unsigned int ret = 0;
+	int i, val;
+
+	for (i = 0; i < gpios->ndescs; i++) {
+		val = gpiod_get_value_cansleep(gpios->desc[i]);
+		if (val >= 0) {
+			ret = (ret << 1) | !!val;
+		} else {
+			dev_err(decoder->dev, "Error reading gpio\n");
+			break;

I think if we fail reading any one of gpio lines we should skip
reporting the event.

Ok.

quoted

quoted

+		}
+	}
+
+	return ret;
+}
+
+static void gpio_decoder_poll_gpios(struct input_polled_dev *poll_dev)
+{
+	struct gpio_decoder *decoder = poll_dev->private;
+	unsigned int state = gpio_decoder_get_gpios_state(decoder);
+
+	if (state != decoder->last_stable) {
+		input_report_abs(poll_dev->input, decoder->axis, state);
+		input_sync(poll_dev->input);
+		decoder->last_stable = state;
+	}
+}
+
+static int gpio_decoder_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct gpio_decoder *decoder;
+	struct input_polled_dev *poll_dev;
+	u32  max;
+	int err;
+
+	decoder = devm_kzalloc(dev, sizeof(struct gpio_decoder), GFP_KERNEL);
+	if (!decoder)
+		return -ENOMEM;
+
+	device_property_read_u32(dev, "linux,axis", &decoder->axis);
+	decoder->input_gpios = devm_gpiod_get_array(dev, NULL, GPIOD_IN);
+	if (IS_ERR(decoder->input_gpios)) {
+		dev_err(dev, "unable to acquire input gpios\n");
+		return PTR_ERR(decoder->input_gpios);
+	}
+	if (decoder->input_gpios->ndescs < 2) {
+		dev_err(dev, "not enough gpios found\n");
+		return -EINVAL;
+	}
+
+	if (device_property_read_u32(dev, "decoder-max-value", &max))
+		max = BIT(decoder->input_gpios->ndescs);

Shouldn't this be

		max = (1U << decoder->input_gpios->ndescs) - 1;

?

Oops, you are right.

quoted

quoted

+
+	decoder->dev = dev;
+	poll_dev = devm_input_allocate_polled_device(decoder->dev);
+	if (!poll_dev)
+		return -ENOMEM;
+
+	poll_dev->private = decoder;
+	poll_dev->poll = gpio_decoder_poll_gpios;
+	decoder->poll_dev = poll_dev;
+
+	poll_dev->input->name = pdev->name;
+	poll_dev->input->id.bustype = BUS_HOST;
+	input_set_abs_params(poll_dev->input, decoder->axis, 0, max, 0, 1);

Why do you need flat == 1? Do you actually use joydev for this device?

Sorry, I overlooked flat param when I copied this line from
rotary-encoder.c.

quoted

quoted

+
+	err = input_register_polled_device(poll_dev);
+	if (err) {
+		dev_err(dev, "failed to register polled  device\n");
+		return err;
+	}
+	platform_set_drvdata(pdev, decoder);
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id gpio_decoder_of_match[] = {
+	{ .compatible = "gpio-decoder", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, gpio_decoder_of_match);
+#endif
+
+static struct platform_driver gpio_decoder_driver = {
+	.probe		= gpio_decoder_probe,
+	.driver		= {
+		.name	= "gpio-decoder",
+		.of_match_table = of_match_ptr(gpio_decoder_of_match),
+	}
+};
+module_platform_driver(gpio_decoder_driver);
+
+MODULE_DESCRIPTION("GPIO decoder input driver");
+MODULE_AUTHOR("Vignesh R [off-list ref]");
+MODULE_LICENSE("GPL v2");
-- 
2.9.2

No need to resubmit, I can adjust on my side.

Thanks a lot!

Applied, thank you.

DTS change should go through some other tree though.

-- 
Dmitry

* Dmitry Torokhov [off-list ref] [160829 20:26]:

Applied, thank you.

DTS change should go through some other tree though.

Thanks I'm picking up the dts patch into omap-for-v4.9/dt.

Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hello,

On Wed, Aug 24, 2016 at 02:45:17PM +0530, Vignesh R wrote:

On Wednesday 24 August 2016 02:05 PM, Daniel Mack wrote:

quoted

On 08/24/2016 09:58 AM, Vignesh R wrote:

quoted

This series adds support for rotary-switch on AM335x-ICE that is
connected to TI PCA9536 I2C GPIO expander.
First patch adds new generic driver to read status of group of GPIO
lines and report the value as an input event. The second patch adds DT
entries for the same.

v2: https://lkml.org/lkml/2016/8/23/111
v1: https://lkml.org/lkml/2016/8/12/7

Is there a reason why the rotary-encoder driver cannot handle this?
Commit 7dde4e74744 ("Input: rotary-encoder - support more than 2 gpios
as input") added support for that mode AFAIU.

Rotary encoder driver handles incremental encoders only and does not
support absolute encoders. The rotary switch on am335x-ice is different
from the incremental encoders in the
sense that GPIO line status directly reflect the position(number)
pointed by the dial of the encoder. So, there is no need to count steps
or know the direction of rotation as it does not matter.

I'd still prefer to expand drivers/input/misc/rotary_encoder.c to handle
this. Yes, there is no reason to count steps or determine the direction
of a rotation in this case, but still there is much code to share and
IMHO it's ok that a driver that handles several types of similar devices
does too much for some special cases.

I did try to enhance rotary-encoder driver to support absolute
encoder[1] but the comment there was to write new driver that simply
translates gpio-encoded value into ABS* event. Indeed, the new driver
looks more simple and can handle more such hardwares.

Which type of hardware can be handled by your driver that the generic
rotary encoder cannot? I guess it's just a matter of a (simple) patch?

Best regards
Uwe

[1] https://lkml.org/lkml/2016/5/19/98

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html