On Tue, Feb 27, 2018 at 8:54 PM, Dmitry Torokhov
[off-list ref] wrote:

Hi Tim,

Hi Dmitry - thanks for the review!

On Tue, Feb 27, 2018 at 05:21:14PM -0800, Tim Harvey wrote:

quoted

Add support for dispatching Linux Input events for the various interrupts
the Gateworks System Controller provides.

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
 drivers/input/misc/Kconfig     |   6 ++
 drivers/input/misc/Makefile    |   1 +
 drivers/input/misc/gsc-input.c | 196 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 203 insertions(+)
 create mode 100644 drivers/input/misc/gsc-input.c

diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 9f082a3..3d18a0e 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig

@@ -117,6 +117,12 @@ config INPUT_E3X0_BUTTON
        To compile this driver as a module, choose M here: the
        module will be called e3x0_button.

+config INPUT_GSC
+     tristate "Gateworks System Controller input support"
+     depends on MFD_GSC
+     help
+       Say Y here if you want Gateworks System Controller input support.
+

"To compile this driver as a module..."

ok

quoted

 config INPUT_PCSPKR
      tristate "PC Speaker support"
      depends on PCSPKR_PLATFORM

diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index 4b6118d..969debe 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile

@@ -38,6 +38,7 @@ 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_GSC)                      += gsc-input.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/gsc-input.c b/drivers/input/misc/gsc-input.c
new file mode 100644
index 0000000..7cf217c
--- /dev/null
+++ b/drivers/input/misc/gsc-input.c

@@ -0,0 +1,196 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 Gateworks Corporation
+ */

Let's keep the same // comment block fir the copyright notice as well.
An one-line describing what this driver is would be appreciated too.

ok - will have this in v2:

// SPDX-License-Identifier: GPL-2.0
//
// Copyright (C) 2018 Gateworks Corporation
//
// This driver dispatches Linux input events for GSC interrupt events
//

quoted

+#define DEBUG

Please no.

oops, did not mean to submit that

quoted

+
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#include <linux/mfd/gsc.h>
+
+struct gsc_irq {
+     unsigned int irq;
+     const char *name;
+     unsigned int virq;
+};
+
+static struct gsc_irq gsc_irqs[] = {
+     { GSC_IRQ_PB,           "button" },
+     { GSC_IRQ_KEY_ERASED,   "key-erased" },
+     { GSC_IRQ_EEPROM_WP,    "eeprom-wp" },
+     { GSC_IRQ_TAMPER,       "tamper" },
+     { GSC_IRQ_SWITCH_HOLD,  "button-held" },
+};
+
+struct gsc_input_info {
+     struct device *dev;
+     struct gsc_dev *gsc;
+     struct input_dev *input;
+
+     int irq;
+     struct work_struct irq_work;
+     struct mutex mutex;
+};
+
+static void gsc_input_irq_work(struct work_struct *work)
+{
+     struct gsc_input_info *info = container_of(work, struct gsc_input_info,
+                                                irq_work);
+     struct gsc_dev *gsc = info->gsc;
+     int i, ret = 0;
+     int key, sts;
+     struct gsc_irq *gsc_irq = NULL;
+
+     dev_dbg(gsc->dev, "%s irq%d\n", __func__, info->irq);
+     mutex_lock(&info->mutex);
+
+     for (i = 0; i < ARRAY_SIZE(gsc_irqs);i++)
+             if (info->irq == gsc_irqs[i].virq)
+                     gsc_irq = &gsc_irqs[i];
+     if (!gsc_irq) {
+             dev_err(info->dev, "interrupt: irq%d occurred\n", info->irq);
+             mutex_unlock(&info->mutex);
+             return;
+     }
+
+     ret = regmap_read(info->gsc->regmap, GSC_IRQ_STATUS, &sts);

Why is this needed? To clear irq? What happens if several events happen
at the same time? Do we lose one of them?

it was for original debugging and not needed - will remove

quoted

+     if (ret) {
+             dev_err(info->dev, "failed to read status register\n");
+             mutex_unlock(&info->mutex);
+             return;
+     }
+
+     key = -1;
+     switch (gsc_irq->virq) {
+     case GSC_IRQ_PB:
+             key = BTN_0;
+             break;
+     case GSC_IRQ_KEY_ERASED:
+             key = BTN_1;
+             break;
+     case GSC_IRQ_EEPROM_WP:
+             key = BTN_2;
+             break;
+     case GSC_IRQ_GPIO:
+             key = BTN_3;
+             break;
+     case GSC_IRQ_TAMPER:
+             key = BTN_4;
+             break;
+     case GSC_IRQ_SWITCH_HOLD:
+             key = BTN_5;

Could we provide the mapping in DTS instead of hard-coding them?

Yes, that makes sense. I'll propose something like the following in v2:

gsc_input {
   compatible = "gw,gsc-input";

   button {
      label = "user pushbutton";
      linux,code = <256>;
      interrupts = <0>
   };

   key-erased {
      label = "key erased";
      linux,code = <257>;
      interrupts = <1>
   };

   ...
};

quoted

+             break;
+     }
+
+     if (key != -1) {
+             dev_dbg(&info->input->dev, "bit%d: key=0x%03x %s\n",
+                     gsc_irq->virq, key, gsc_irq->name);
+             input_report_key(info->input, key, 1);

                input_sync();

right - thanks!

quoted

+             input_report_key(info->input, key, 0);
+             input_sync(info->input);
+     }
+
+     mutex_unlock(&info->mutex);
+}
+
+static irqreturn_t gsc_input_irq(int irq, void *data)
+{
+     struct gsc_input_info *info = data;
+
+     dev_dbg(info->gsc->dev, "%s irq%d\n", __func__, irq);
+     info->irq = irq;
+     schedule_work(&info->irq_work);

Why not use threaded interrupt?

I am using request_threaded_irq with thread_fn with thread_fn (vs handler).

Do you mean why use a work procedure? I guess I don't need that and
can call input_report_key directly from the irq.

quoted

+
+     return IRQ_HANDLED;
+}
+
+static int gsc_input_probe(struct platform_device *pdev)
+{
+     struct gsc_dev *gsc = dev_get_drvdata(pdev->dev.parent);
+     struct gsc_input_info *info;
+     struct input_dev *input;
+     int ret, i;
+
+     dev_dbg(&pdev->dev, "%s\n", __func__);
+     info = devm_kzalloc(&pdev->dev, sizeof(struct gsc_input_info),
+                         GFP_KERNEL);
+     if (!info)
+             return -ENOMEM;
+     info->dev = &pdev->dev;
+     info->gsc = gsc;
+
+     /* Register input device */
+     input = devm_input_allocate_device(&pdev->dev);
+     if (!input) {
+             dev_err(&pdev->dev, "Can't allocate input device\n");
+             return -ENOMEM;
+     }
+     info->input = input;
+
+     input->name = KBUILD_MODNAME;
+     input->dev.parent = &pdev->dev;

Not needed - it is set by devm_input_allocate_device().

ok

quoted

+
+     input_set_capability(input, EV_KEY, BTN_0); /* button */
+     input_set_capability(input, EV_KEY, BTN_1); /* key erased */
+     input_set_capability(input, EV_KEY, BTN_2); /* ee wp */
+     input_set_capability(input, EV_KEY, BTN_3); /* gpio */
+     input_set_capability(input, EV_KEY, BTN_4); /* tamper */
+     input_set_capability(input, EV_KEY, BTN_5); /* button held */
+
+     ret = input_register_device(input);
+     if (ret < 0) {
+             dev_err(&pdev->dev, "Can't register input device: %d\n", ret);
+             return ret;
+     }
+
+     platform_set_drvdata(pdev, gsc);
+     mutex_init(&info->mutex);
+     INIT_WORK(&info->irq_work, gsc_input_irq_work);
+
+     /* Support irq domain */
+     for (i = 0; i < ARRAY_SIZE(gsc_irqs); i++) {
+             struct gsc_irq *gsc_irq = &gsc_irqs[i];
+             int virq;
+
+             virq = regmap_irq_get_virq(gsc->irq_chip_data, gsc_irq->irq);
+             if (virq <= 0)
+                     return -EINVAL;
+             gsc_irq->virq = virq;

I'd say mapping should be done by MFD piece. You can add interrupts as
resources and fetch them here.

can you point me to an example dts/driver?

Tim

quoted

+
+             ret = devm_request_threaded_irq(&pdev->dev, virq, NULL,
+                                             gsc_input_irq, 0,
+                                             gsc_irq->name, info);
+             if (ret) {
+                     dev_err(&pdev->dev,
+                             "failed: irq request (IRQ: %d, error: %d\n)",
+                             gsc_irq->irq, ret);
+                     return ret;
+             }
+     }
+
+     return 0;
+}
+
+static const struct of_device_id gsc_input_dt_ids[] = {
+     { .compatible = "gw,gsc-input", },
+     {}
+};
+
+static struct platform_driver gsc_input_driver = {
+     .driver = {
+             .name = KBUILD_MODNAME,
+             .of_match_table = gsc_input_dt_ids,
+     },
+     .probe = gsc_input_probe,
+};
+
+module_platform_driver(gsc_input_driver);
+
+MODULE_AUTHOR("Tim Harvey [off-list ref]");
+MODULE_DESCRIPTION("GSC input driver");
+MODULE_LICENSE("GPL v2");
--
2.7.4

Thanks.

--
Dmitry