* Re: [PATCH v2 1/2] input: misc: Add generic input driver to read encoded GPIO lines
@ 2016-08-23 15:27 ` Andrew F. Davis
0 siblings, 0 replies; 22+ messages in thread
From: Andrew F. Davis @ 2016-08-23 15:27 UTC (permalink / raw)
To: Vignesh R, Dmitry Torokhov
Cc: Rob Herring, Mark Rutland, Tony Lindgren, Russell King,
Arnd Bergmann, Daniel Hung-yu Wu, Grant Grundler, S Twiss,
Moritz Fischer, Jorge Ramirez-Ortiz, John Stultz, linux-input,
devicetree, linux-kernel, linux-omap, linux-arm-kernel
On 08/23/2016 01:16 AM, Vignesh R wrote:
> 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
> intefacing 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>
> ---
>
> v2: Use decoder,max-value instead of gpio-decoder,max-value.
>
> .../devicetree/bindings/input/gpio-decoder.txt | 23 ++++
> drivers/input/misc/Kconfig | 12 ++
> drivers/input/misc/Makefile | 1 +
> drivers/input/misc/gpio_decoder.c | 130 +++++++++++++++++++++
> 4 files changed, 166 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..f27f083a9075
> --- /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 (atleast two) to be decoded to a number with
^^ two words
> + 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..c44513cac3b7 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 && OF
> + 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 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..eec0fda6f562
> --- /dev/null
> +++ b/drivers/input/misc/gpio_decoder.c
> @@ -0,0 +1,130 @@
> +/*
> + * gpio-decoder.c
> + *
^ the name of the file often changes without this being updated, it
really doesn't add anything anyway, would recommend dropping it.
> + * A generic driver to reads multiple gpio lines and translate the
> + * encoded numeric value into an input event.
> + *
> + * 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.
> + */
> +
> +#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]);
> + ret = ret << 1 | val;
Just to be safe, could you mask val with 0x1 here.
> + }
> +
> + 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;
> + poll_dev->input->dev.parent = dev;
> + 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;
> +}
> +
> +static const struct of_device_id gpio_decoder_of_match[] = {
> + { .compatible = "gpio-decoder", },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, gpio_decoder_of_match);
> +
> +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),
When the struct 'gpio_decoder_of_match' is always defined (not
conditional on OF support), then 'of_match_ptr' does nothing and can be
dropped here.
> + }
> +};
> +module_platform_driver(gpio_decoder_driver);
> +
> +MODULE_DESCRIPTION("GPIO decoder input driver");
> +MODULE_AUTHOR("Vignesh R <vigneshr@ti.com>");
> +MODULE_LICENSE("GPL v2");
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH v2 1/2] input: misc: Add generic input driver to read encoded GPIO lines
@ 2016-08-23 15:27 ` Andrew F. Davis
0 siblings, 0 replies; 22+ messages in thread
From: Andrew F. Davis @ 2016-08-23 15:27 UTC (permalink / raw)
To: linux-arm-kernel
On 08/23/2016 01:16 AM, Vignesh R wrote:
> 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
> intefacing 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>
> ---
>
> v2: Use decoder,max-value instead of gpio-decoder,max-value.
>
> .../devicetree/bindings/input/gpio-decoder.txt | 23 ++++
> drivers/input/misc/Kconfig | 12 ++
> drivers/input/misc/Makefile | 1 +
> drivers/input/misc/gpio_decoder.c | 130 +++++++++++++++++++++
> 4 files changed, 166 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..f27f083a9075
> --- /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 (atleast two) to be decoded to a number with
^^ two words
> + 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..c44513cac3b7 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 && OF
> + 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 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..eec0fda6f562
> --- /dev/null
> +++ b/drivers/input/misc/gpio_decoder.c
> @@ -0,0 +1,130 @@
> +/*
> + * gpio-decoder.c
> + *
^ the name of the file often changes without this being updated, it
really doesn't add anything anyway, would recommend dropping it.
> + * A generic driver to reads multiple gpio lines and translate the
> + * encoded numeric value into an input event.
> + *
> + * 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.
> + */
> +
> +#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]);
> + ret = ret << 1 | val;
Just to be safe, could you mask val with 0x1 here.
> + }
> +
> + 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;
> + poll_dev->input->dev.parent = dev;
> + 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;
> +}
> +
> +static const struct of_device_id gpio_decoder_of_match[] = {
> + { .compatible = "gpio-decoder", },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, gpio_decoder_of_match);
> +
> +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),
When the struct 'gpio_decoder_of_match' is always defined (not
conditional on OF support), then 'of_match_ptr' does nothing and can be
dropped here.
> + }
> +};
> +module_platform_driver(gpio_decoder_driver);
> +
> +MODULE_DESCRIPTION("GPIO decoder input driver");
> +MODULE_AUTHOR("Vignesh R <vigneshr@ti.com>");
> +MODULE_LICENSE("GPL v2");
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 1/2] input: misc: Add generic input driver to read encoded GPIO lines
2016-08-23 15:27 ` Andrew F. Davis
(?)
@ 2016-08-23 20:01 ` Dmitry Torokhov
-1 siblings, 0 replies; 22+ messages in thread
From: Dmitry Torokhov @ 2016-08-23 20:01 UTC (permalink / raw)
To: Andrew F. Davis
Cc: Mark Rutland, Moritz Fischer, John Stultz, Vignesh R,
Arnd Bergmann, devicetree, Tony Lindgren, Daniel Hung-yu Wu,
Russell King, linux-kernel, Grant Grundler, Rob Herring, S Twiss,
linux-input, Jorge Ramirez-Ortiz, linux-omap, linux-arm-kernel
On Tue, Aug 23, 2016 at 10:27:59AM -0500, Andrew F. Davis wrote:
> On 08/23/2016 01:16 AM, Vignesh R wrote:
> > 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
> > intefacing 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>
> > ---
> >
> > v2: Use decoder,max-value instead of gpio-decoder,max-value.
> >
> > .../devicetree/bindings/input/gpio-decoder.txt | 23 ++++
> > drivers/input/misc/Kconfig | 12 ++
> > drivers/input/misc/Makefile | 1 +
> > drivers/input/misc/gpio_decoder.c | 130 +++++++++++++++++++++
> > 4 files changed, 166 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..f27f083a9075
> > --- /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 (atleast two) to be decoded to a number with
>
> ^^ two words
>
> > + 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..c44513cac3b7 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 && OF
> > + 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 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..eec0fda6f562
> > --- /dev/null
> > +++ b/drivers/input/misc/gpio_decoder.c
> > @@ -0,0 +1,130 @@
> > +/*
> > + * gpio-decoder.c
> > + *
>
> ^ the name of the file often changes without this being updated, it
> really doesn't add anything anyway, would recommend dropping it.
>
> > + * A generic driver to reads multiple gpio lines and translate the
> > + * encoded numeric value into an input event.
> > + *
> > + * 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.
> > + */
> > +
> > +#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]);
> > + ret = ret << 1 | val;
>
> Just to be safe, could you mask val with 0x1 here.
We also want to check for negative. So I guess:
val = gpiod_get_value_cansleep(gpios->desc[i]);
if (val >= 0)
ret = (ret << 1) | !!val;
?
>
> > + }
> > +
> > + 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;
> > + poll_dev->input->dev.parent = dev;
No need to set for devices allocated with
devm_input_allocate_polled_device.
> > + 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;
> > +}
> > +
> > +static const struct of_device_id gpio_decoder_of_match[] = {
> > + { .compatible = "gpio-decoder", },
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(of, gpio_decoder_of_match);
> > +
> > +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),
>
> When the struct 'gpio_decoder_of_match' is always defined (not
> conditional on OF support), then 'of_match_ptr' does nothing and can be
> dropped here.
There is nothing in this driver that makes it OF-specific. I'd leave
of_match_ptr() as is, added #ifdef CONFIG_OF guard around
gpio_decoder_of_match, and changed "depends on GPIOLIB && OF" to
"depends on GPIOLIB || COMPILE_TEST".
Thanks.
>
> > + }
> > +};
> > +module_platform_driver(gpio_decoder_driver);
> > +
> > +MODULE_DESCRIPTION("GPIO decoder input driver");
> > +MODULE_AUTHOR("Vignesh R <vigneshr@ti.com>");
> > +MODULE_LICENSE("GPL v2");
--
Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 1/2] input: misc: Add generic input driver to read encoded GPIO lines
@ 2016-08-23 20:01 ` Dmitry Torokhov
0 siblings, 0 replies; 22+ messages in thread
From: Dmitry Torokhov @ 2016-08-23 20:01 UTC (permalink / raw)
To: Andrew F. Davis
Cc: Vignesh R, Rob Herring, Mark Rutland, Tony Lindgren, Russell King,
Arnd Bergmann, Daniel Hung-yu Wu, Grant Grundler, S Twiss,
Moritz Fischer, Jorge Ramirez-Ortiz, John Stultz, linux-input,
devicetree, linux-kernel, linux-omap, linux-arm-kernel
On Tue, Aug 23, 2016 at 10:27:59AM -0500, Andrew F. Davis wrote:
> On 08/23/2016 01:16 AM, Vignesh R wrote:
> > 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
> > intefacing 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>
> > ---
> >
> > v2: Use decoder,max-value instead of gpio-decoder,max-value.
> >
> > .../devicetree/bindings/input/gpio-decoder.txt | 23 ++++
> > drivers/input/misc/Kconfig | 12 ++
> > drivers/input/misc/Makefile | 1 +
> > drivers/input/misc/gpio_decoder.c | 130 +++++++++++++++++++++
> > 4 files changed, 166 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..f27f083a9075
> > --- /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 (atleast two) to be decoded to a number with
>
> ^^ two words
>
> > + 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..c44513cac3b7 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 && OF
> > + 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 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..eec0fda6f562
> > --- /dev/null
> > +++ b/drivers/input/misc/gpio_decoder.c
> > @@ -0,0 +1,130 @@
> > +/*
> > + * gpio-decoder.c
> > + *
>
> ^ the name of the file often changes without this being updated, it
> really doesn't add anything anyway, would recommend dropping it.
>
> > + * A generic driver to reads multiple gpio lines and translate the
> > + * encoded numeric value into an input event.
> > + *
> > + * 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.
> > + */
> > +
> > +#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]);
> > + ret = ret << 1 | val;
>
> Just to be safe, could you mask val with 0x1 here.
We also want to check for negative. So I guess:
val = gpiod_get_value_cansleep(gpios->desc[i]);
if (val >= 0)
ret = (ret << 1) | !!val;
?
>
> > + }
> > +
> > + 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;
> > + poll_dev->input->dev.parent = dev;
No need to set for devices allocated with
devm_input_allocate_polled_device.
> > + 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;
> > +}
> > +
> > +static const struct of_device_id gpio_decoder_of_match[] = {
> > + { .compatible = "gpio-decoder", },
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(of, gpio_decoder_of_match);
> > +
> > +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),
>
> When the struct 'gpio_decoder_of_match' is always defined (not
> conditional on OF support), then 'of_match_ptr' does nothing and can be
> dropped here.
There is nothing in this driver that makes it OF-specific. I'd leave
of_match_ptr() as is, added #ifdef CONFIG_OF guard around
gpio_decoder_of_match, and changed "depends on GPIOLIB && OF" to
"depends on GPIOLIB || COMPILE_TEST".
Thanks.
>
> > + }
> > +};
> > +module_platform_driver(gpio_decoder_driver);
> > +
> > +MODULE_DESCRIPTION("GPIO decoder input driver");
> > +MODULE_AUTHOR("Vignesh R <vigneshr@ti.com>");
> > +MODULE_LICENSE("GPL v2");
--
Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH v2 1/2] input: misc: Add generic input driver to read encoded GPIO lines
@ 2016-08-23 20:01 ` Dmitry Torokhov
0 siblings, 0 replies; 22+ messages in thread
From: Dmitry Torokhov @ 2016-08-23 20:01 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Aug 23, 2016 at 10:27:59AM -0500, Andrew F. Davis wrote:
> On 08/23/2016 01:16 AM, Vignesh R wrote:
> > 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
> > intefacing 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>
> > ---
> >
> > v2: Use decoder,max-value instead of gpio-decoder,max-value.
> >
> > .../devicetree/bindings/input/gpio-decoder.txt | 23 ++++
> > drivers/input/misc/Kconfig | 12 ++
> > drivers/input/misc/Makefile | 1 +
> > drivers/input/misc/gpio_decoder.c | 130 +++++++++++++++++++++
> > 4 files changed, 166 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..f27f083a9075
> > --- /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 (atleast two) to be decoded to a number with
>
> ^^ two words
>
> > + 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..c44513cac3b7 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 && OF
> > + 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 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..eec0fda6f562
> > --- /dev/null
> > +++ b/drivers/input/misc/gpio_decoder.c
> > @@ -0,0 +1,130 @@
> > +/*
> > + * gpio-decoder.c
> > + *
>
> ^ the name of the file often changes without this being updated, it
> really doesn't add anything anyway, would recommend dropping it.
>
> > + * A generic driver to reads multiple gpio lines and translate the
> > + * encoded numeric value into an input event.
> > + *
> > + * 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.
> > + */
> > +
> > +#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]);
> > + ret = ret << 1 | val;
>
> Just to be safe, could you mask val with 0x1 here.
We also want to check for negative. So I guess:
val = gpiod_get_value_cansleep(gpios->desc[i]);
if (val >= 0)
ret = (ret << 1) | !!val;
?
>
> > + }
> > +
> > + 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;
> > + poll_dev->input->dev.parent = dev;
No need to set for devices allocated with
devm_input_allocate_polled_device.
> > + 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;
> > +}
> > +
> > +static const struct of_device_id gpio_decoder_of_match[] = {
> > + { .compatible = "gpio-decoder", },
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(of, gpio_decoder_of_match);
> > +
> > +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),
>
> When the struct 'gpio_decoder_of_match' is always defined (not
> conditional on OF support), then 'of_match_ptr' does nothing and can be
> dropped here.
There is nothing in this driver that makes it OF-specific. I'd leave
of_match_ptr() as is, added #ifdef CONFIG_OF guard around
gpio_decoder_of_match, and changed "depends on GPIOLIB && OF" to
"depends on GPIOLIB || COMPILE_TEST".
Thanks.
>
> > + }
> > +};
> > +module_platform_driver(gpio_decoder_driver);
> > +
> > +MODULE_DESCRIPTION("GPIO decoder input driver");
> > +MODULE_AUTHOR("Vignesh R <vigneshr@ti.com>");
> > +MODULE_LICENSE("GPL v2");
--
Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 1/2] input: misc: Add generic input driver to read encoded GPIO lines
2016-08-23 20:01 ` Dmitry Torokhov
@ 2016-08-24 6:39 ` Vignesh R
-1 siblings, 0 replies; 22+ messages in thread
From: Vignesh R @ 2016-08-24 6:39 UTC (permalink / raw)
To: Dmitry Torokhov, Davis, Andrew
Cc: Rob Herring, Mark Rutland, Tony Lindgren, Russell King,
Arnd Bergmann, Daniel Hung-yu Wu, Grant Grundler, S Twiss,
Moritz Fischer, Jorge Ramirez-Ortiz, John Stultz,
linux-input@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
Hi Andrew, Dmitry,
On Wednesday 24 August 2016 01:31 AM, Dmitry Torokhov wrote:
> On Tue, Aug 23, 2016 at 10:27:59AM -0500, Andrew F. Davis wrote:
>> On 08/23/2016 01:16 AM, Vignesh R wrote:
>>> 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
>>> intefacing 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>
>>> ---
>>>
>>> v2: Use decoder,max-value instead of gpio-decoder,max-value.
>>>
>>> .../devicetree/bindings/input/gpio-decoder.txt | 23 ++++
>>> drivers/input/misc/Kconfig | 12 ++
>>> drivers/input/misc/Makefile | 1 +
>>> drivers/input/misc/gpio_decoder.c | 130 +++++++++++++++++++++
>>> 4 files changed, 166 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..f27f083a9075
>>> --- /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 (atleast two) to be decoded to a number with
>>
>> ^^ two words
Ok.
>>
>>> + first entry representing the MSB.
>>> +
>>> +Optional Properties:
>>> +- decoder,max-value: Maximum possible value that can be reported by
>>> + the gpios.
I will change this to decoder-max-value as suggested previously.
>>> +- 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..c44513cac3b7 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 && OF
>>> + 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 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..eec0fda6f562
>>> --- /dev/null
>>> +++ b/drivers/input/misc/gpio_decoder.c
>>> @@ -0,0 +1,130 @@
>>> +/*
>>> + * gpio-decoder.c
>>> + *
>>
>> ^ the name of the file often changes without this being updated, it
>> really doesn't add anything anyway, would recommend dropping it.
Ok.
>>
>>> + * A generic driver to reads multiple gpio lines and translate the
>>> + * encoded numeric value into an input event.
>>> + *
>>> + * 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.
>>> + */
>>> +
>>> +#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]);
>>> + ret = ret << 1 | val;
>>
>> Just to be safe, could you mask val with 0x1 here.
>
> We also want to check for negative. So I guess:
>
> val = gpiod_get_value_cansleep(gpios->desc[i]);
> if (val >= 0)
> ret = (ret << 1) | !!val;
>
> ?
Agreed.
>
>>
>>> + }
>>> +
>>> + 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;
>>> + poll_dev->input->dev.parent = dev;
>
> No need to set for devices allocated with
> devm_input_allocate_polled_device.
Will drop this.
>
>>> + 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;
>>> +}
>>> +
>>> +static const struct of_device_id gpio_decoder_of_match[] = {
>>> + { .compatible = "gpio-decoder", },
>>> + { },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, gpio_decoder_of_match);
>>> +
>>> +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),
>>
>> When the struct 'gpio_decoder_of_match' is always defined (not
>> conditional on OF support), then 'of_match_ptr' does nothing and can be
>> dropped here.
>
> There is nothing in this driver that makes it OF-specific. I'd leave
> of_match_ptr() as is, added #ifdef CONFIG_OF guard around
> gpio_decoder_of_match,
>and changed "depends on GPIOLIB && OF" to "depends on GPIOLIB || COMPILE_TEST".
>
Agreed. Thanks for the review!
--
Regards
Vignesh
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH v2 1/2] input: misc: Add generic input driver to read encoded GPIO lines
@ 2016-08-24 6:39 ` Vignesh R
0 siblings, 0 replies; 22+ messages in thread
From: Vignesh R @ 2016-08-24 6:39 UTC (permalink / raw)
To: linux-arm-kernel
Hi Andrew, Dmitry,
On Wednesday 24 August 2016 01:31 AM, Dmitry Torokhov wrote:
> On Tue, Aug 23, 2016 at 10:27:59AM -0500, Andrew F. Davis wrote:
>> On 08/23/2016 01:16 AM, Vignesh R wrote:
>>> 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
>>> intefacing 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>
>>> ---
>>>
>>> v2: Use decoder,max-value instead of gpio-decoder,max-value.
>>>
>>> .../devicetree/bindings/input/gpio-decoder.txt | 23 ++++
>>> drivers/input/misc/Kconfig | 12 ++
>>> drivers/input/misc/Makefile | 1 +
>>> drivers/input/misc/gpio_decoder.c | 130 +++++++++++++++++++++
>>> 4 files changed, 166 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..f27f083a9075
>>> --- /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 (atleast two) to be decoded to a number with
>>
>> ^^ two words
Ok.
>>
>>> + first entry representing the MSB.
>>> +
>>> +Optional Properties:
>>> +- decoder,max-value: Maximum possible value that can be reported by
>>> + the gpios.
I will change this to decoder-max-value as suggested previously.
>>> +- 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..c44513cac3b7 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 && OF
>>> + 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 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..eec0fda6f562
>>> --- /dev/null
>>> +++ b/drivers/input/misc/gpio_decoder.c
>>> @@ -0,0 +1,130 @@
>>> +/*
>>> + * gpio-decoder.c
>>> + *
>>
>> ^ the name of the file often changes without this being updated, it
>> really doesn't add anything anyway, would recommend dropping it.
Ok.
>>
>>> + * A generic driver to reads multiple gpio lines and translate the
>>> + * encoded numeric value into an input event.
>>> + *
>>> + * 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.
>>> + */
>>> +
>>> +#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]);
>>> + ret = ret << 1 | val;
>>
>> Just to be safe, could you mask val with 0x1 here.
>
> We also want to check for negative. So I guess:
>
> val = gpiod_get_value_cansleep(gpios->desc[i]);
> if (val >= 0)
> ret = (ret << 1) | !!val;
>
> ?
Agreed.
>
>>
>>> + }
>>> +
>>> + 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;
>>> + poll_dev->input->dev.parent = dev;
>
> No need to set for devices allocated with
> devm_input_allocate_polled_device.
Will drop this.
>
>>> + 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;
>>> +}
>>> +
>>> +static const struct of_device_id gpio_decoder_of_match[] = {
>>> + { .compatible = "gpio-decoder", },
>>> + { },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, gpio_decoder_of_match);
>>> +
>>> +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),
>>
>> When the struct 'gpio_decoder_of_match' is always defined (not
>> conditional on OF support), then 'of_match_ptr' does nothing and can be
>> dropped here.
>
> There is nothing in this driver that makes it OF-specific. I'd leave
> of_match_ptr() as is, added #ifdef CONFIG_OF guard around
> gpio_decoder_of_match,
>and changed "depends on GPIOLIB && OF" to "depends on GPIOLIB || COMPILE_TEST".
>
Agreed. Thanks for the review!
--
Regards
Vignesh
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/2] input: misc: Add generic input driver to read encoded GPIO lines
2016-08-23 15:27 ` Andrew F. Davis
@ 2016-08-23 20:03 ` Dmitry Torokhov
-1 siblings, 0 replies; 22+ messages in thread
From: Dmitry Torokhov @ 2016-08-23 20:03 UTC (permalink / raw)
To: Andrew F. Davis
Cc: Vignesh R, Rob Herring, Mark Rutland, Tony Lindgren, Russell King,
Arnd Bergmann, Daniel Hung-yu Wu, Grant Grundler, S Twiss,
Moritz Fischer, Jorge Ramirez-Ortiz, John Stultz, linux-input,
devicetree, linux-kernel, linux-omap, linux-arm-kernel
On Tue, Aug 23, 2016 at 10:27:59AM -0500, Andrew F. Davis wrote:
> On 08/23/2016 01:16 AM, Vignesh R wrote:
> > 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
> > intefacing 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>
> > ---
> >
> > v2: Use decoder,max-value instead of gpio-decoder,max-value.
That's not what Rob asked for. "decoder-max-value", not "decoder" vendor
prefix.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 1/2] input: misc: Add generic input driver to read encoded GPIO lines
@ 2016-08-23 20:03 ` Dmitry Torokhov
0 siblings, 0 replies; 22+ messages in thread
From: Dmitry Torokhov @ 2016-08-23 20:03 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Aug 23, 2016 at 10:27:59AM -0500, Andrew F. Davis wrote:
> On 08/23/2016 01:16 AM, Vignesh R wrote:
> > 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
> > intefacing 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>
> > ---
> >
> > v2: Use decoder,max-value instead of gpio-decoder,max-value.
That's not what Rob asked for. "decoder-max-value", not "decoder" vendor
prefix.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/2] input: misc: Add generic input driver to read encoded GPIO lines
2016-08-23 20:03 ` Dmitry Torokhov
(?)
@ 2016-08-24 6:39 ` Vignesh R
-1 siblings, 0 replies; 22+ messages in thread
From: Vignesh R @ 2016-08-24 6:39 UTC (permalink / raw)
To: Dmitry Torokhov, Andrew F. Davis
Cc: Rob Herring, Mark Rutland, Tony Lindgren, Russell King,
Arnd Bergmann, Daniel Hung-yu Wu, Grant Grundler, S Twiss,
Moritz Fischer, Jorge Ramirez-Ortiz, John Stultz,
linux-input-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-omap-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On Wednesday 24 August 2016 01:33 AM, Dmitry Torokhov wrote:
> On Tue, Aug 23, 2016 at 10:27:59AM -0500, Andrew F. Davis wrote:
>> On 08/23/2016 01:16 AM, Vignesh R wrote:
>>> 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
>>> intefacing 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-l0cyMroinI0@public.gmane.org>
>>> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>> ---
>>>
>>> v2: Use decoder,max-value instead of gpio-decoder,max-value.
>
> That's not what Rob asked for. "decoder-max-value", not "decoder" vendor
> prefix.
>
Sorry.. Will fix this in v3.
--
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/2] input: misc: Add generic input driver to read encoded GPIO lines
@ 2016-08-24 6:39 ` Vignesh R
0 siblings, 0 replies; 22+ messages in thread
From: Vignesh R @ 2016-08-24 6:39 UTC (permalink / raw)
To: Dmitry Torokhov, Andrew F. Davis
Cc: Rob Herring, Mark Rutland, Tony Lindgren, Russell King,
Arnd Bergmann, Daniel Hung-yu Wu, Grant Grundler, S Twiss,
Moritz Fischer, Jorge Ramirez-Ortiz, John Stultz, linux-input,
devicetree, linux-kernel, linux-omap, linux-arm-kernel
On Wednesday 24 August 2016 01:33 AM, Dmitry Torokhov wrote:
> On Tue, Aug 23, 2016 at 10:27:59AM -0500, Andrew F. Davis wrote:
>> On 08/23/2016 01:16 AM, Vignesh R wrote:
>>> 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
>>> intefacing 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>
>>> ---
>>>
>>> v2: Use decoder,max-value instead of gpio-decoder,max-value.
>
> That's not what Rob asked for. "decoder-max-value", not "decoder" vendor
> prefix.
>
Sorry.. Will fix this in v3.
--
Regards
Vignesh
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 1/2] input: misc: Add generic input driver to read encoded GPIO lines
@ 2016-08-24 6:39 ` Vignesh R
0 siblings, 0 replies; 22+ messages in thread
From: Vignesh R @ 2016-08-24 6:39 UTC (permalink / raw)
To: linux-arm-kernel
On Wednesday 24 August 2016 01:33 AM, Dmitry Torokhov wrote:
> On Tue, Aug 23, 2016 at 10:27:59AM -0500, Andrew F. Davis wrote:
>> On 08/23/2016 01:16 AM, Vignesh R wrote:
>>> 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
>>> intefacing 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>
>>> ---
>>>
>>> v2: Use decoder,max-value instead of gpio-decoder,max-value.
>
> That's not what Rob asked for. "decoder-max-value", not "decoder" vendor
> prefix.
>
Sorry.. Will fix this in v3.
--
Regards
Vignesh
^ permalink raw reply [flat|nested] 22+ messages in thread