lkml.org 
[lkml]   [2018]   [Feb]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC 4/4] input: misc: Add Gateworks System Controller support
    Hi Tim,

    On Tue, Feb 27, 2018 at 05:21:14PM -0800, Tim Harvey wrote:
    > 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..."

    > 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.

    > +#define DEBUG

    Please no.

    > +
    > +#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?

    > + 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?

    > + 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();

    > + 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?

    > +
    > + 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().

    > +
    > + 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.

    > +
    > + 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 <tharvey@gateworks.com>");
    > +MODULE_DESCRIPTION("GSC input driver");
    > +MODULE_LICENSE("GPL v2");
    > --
    > 2.7.4
    >

    Thanks.

    --
    Dmitry

    \
     
     \ /
      Last update: 2018-02-28 05:55    [W:4.913 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site