From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Mike Frysinger <vapier@gentoo.org>
Cc: linux-input@vger.kernel.org,
uclinux-dist-devel@blackfin.uclinux.org,
Bryan Wu <cooloney@kernel.org>
Subject: Re: [PATCH v2] input/misc: PCF8574 I2C keypad input device driver
Date: Tue, 9 Mar 2010 13:21:27 -0800 [thread overview]
Message-ID: <20100309212127.GA29333@core.coreip.homeip.net> (raw)
In-Reply-To: <1268168513-7431-1-git-send-email-vapier@gentoo.org>
On Tue, Mar 09, 2010 at 04:01:53PM -0500, Mike Frysinger wrote:
> From: Bryan Wu <cooloney@kernel.org>
>
> Signed-off-by: Bryan Wu <cooloney@kernel.org>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> ---
> v2
> - process all feedback from Dmitry except for threaded irqs; that'll have to be done later
Could I ask why? As it stands the remove() is racy WRT to a work
running. But if you simply add cancel_work_sync() you'll risk leaving
unbalanced enable_irq()/disable_irq().
A few more comments I missed first time around:
>
> drivers/input/misc/Kconfig | 10 ++
> drivers/input/misc/Makefile | 1 +
> drivers/input/misc/pcf8574_keypad.c | 251 +++++++++++++++++++++++++++++++++++
> 3 files changed, 262 insertions(+), 0 deletions(-)
> create mode 100644 drivers/input/misc/pcf8574_keypad.c
>
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index 49a31bd..fa97c58 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -396,4 +396,14 @@ config INPUT_ADXL34X_SPI
> To compile this driver as a module, choose M here: the
> module will be called adxl34x-spi.
>
> +config INPUT_PCF8574
> + tristate "PCF8574 Keypad input device"
> + depends on I2C && EXPERIMENTAL
> + help
> + Say Y here if you want to support a keypad connetced via I2C
> + with a PCF8574.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called pcf8574_keypad.
> +
> endif
> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> index 775db74..8107c57 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_INPUT_AD714X) += ad714x.o
> obj-$(CONFIG_INPUT_AD714X_I2C) += ad714x-i2c.o
> obj-$(CONFIG_INPUT_AD714X_SPI) += ad714x-spi.o
> obj-$(CONFIG_INPUT_APANEL) += apanel.o
> +obj-$(CONFIG_INPUT_PCF8574) += pcf8574_keypad.o
> obj-$(CONFIG_INPUT_ATI_REMOTE) += ati_remote.o
> obj-$(CONFIG_INPUT_ATI_REMOTE2) += ati_remote2.o
> obj-$(CONFIG_INPUT_ATLAS_BTNS) += atlas_btns.o
> diff --git a/drivers/input/misc/pcf8574_keypad.c b/drivers/input/misc/pcf8574_keypad.c
> new file mode 100644
> index 0000000..8b52a7e
> --- /dev/null
> +++ b/drivers/input/misc/pcf8574_keypad.c
> @@ -0,0 +1,251 @@
> +/*
> + * Driver for a keypad w/16 buttons connected to a PCF8574 I2C I/O expander
> + *
> + * Copyright 2005-2008 Analog Devices Inc.
> + *
> + * Licensed under the GPL-2 or later.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/i2c.h>
> +#include <linux/workqueue.h>
> +
> +#define DRV_NAME "pcf8574_keypad"
> +
> +static const unsigned char pcf8574_kp_btncode[] = {
> + [0] = KEY_RESERVED,
> + [1] = KEY_ENTER,
> + [2] = KEY_BACKSLASH,
> + [3] = KEY_0,
> + [4] = KEY_RIGHTBRACE,
> + [5] = KEY_C,
> + [6] = KEY_9,
> + [7] = KEY_8,
> + [8] = KEY_7,
> + [9] = KEY_B,
> + [10] = KEY_6,
> + [11] = KEY_5,
> + [12] = KEY_4,
> + [13] = KEY_A,
> + [14] = KEY_3,
> + [15] = KEY_2,
> + [16] = KEY_1
> +};
> +
> +struct kp_data {
> + unsigned char btncode[17];
Unsigned short here. Also ARRAY_SIZE(pcf8574_kp_btncode).
> + struct input_dev *idev;
> + struct i2c_client *client;
> + char name[64];
> + char phys[32];
> + unsigned char laststate;
> + unsigned char statechanged;
> + unsigned long events_sent;
> + struct work_struct pcf8574_kp_work;
> +};
> +
> +static short read_state(struct kp_data *lp)
> +{
> + unsigned char x, y, a, b;
> +
> + if (lp->client) {
No need for this check - I do not see how we can end up with lp->client
being NULL here.
> + i2c_smbus_write_byte(lp->client, 240);
> + x = 0xF & (~(i2c_smbus_read_byte(lp->client) >> 4));
> +
> + i2c_smbus_write_byte(lp->client, 15);
> + y = 0xF & (~i2c_smbus_read_byte(lp->client));
> +
> + for (a = 0; x > 0; a++)
> + x = x >> 1;
> + for (b = 0; y > 0; b++)
> + y = y >> 1;
> +
> + return ((a - 1) * 4) + b;
> + }
> +
> + return -1;
> +}
> +
> +static void check_and_notify(struct work_struct *work)
> +{
> + struct kp_data *lp =
> + container_of(work, struct kp_data, pcf8574_kp_work);
> + unsigned char nextstate = read_state(lp);
> +
> + lp->statechanged = lp->laststate ^ nextstate;
> +
> + if (lp->statechanged)
Hmm, why is it in lp? Local variable (or even lp->laststate ^ nextstate
expression) would work just fine.
> + input_report_key(lp->idev,
> + nextstate > 17 ? lp->btncode[lp->laststate] :
> + lp->btncode[nextstate],
> + nextstate > 17 ? 0 : 1);
> + lp->laststate = nextstate;
> +
> + input_sync(lp->idev);
> + enable_irq(lp->client->irq);
> +}
> +
> +static irqreturn_t pcf8574_kp_irq_handler(int irq, void *dev_id)
> +{
> + struct kp_data *lp = dev_id;
> +
> + disable_irq_nosync(lp->client->irq);
> + schedule_work(&lp->pcf8574_kp_work);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int __devinit pcf8574_kp_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +{
> + int i, ret;
> + struct input_dev *idev;
> + struct kp_data *lp;
> +
> + if (i2c_smbus_write_byte(client, 240) < 0) {
> + dev_err(&client->dev, "probe: write fail\n");
> + return -ENODEV;
> + }
> +
> + lp = kzalloc(sizeof(*lp), GFP_KERNEL);
> + if (!lp)
> + return -ENOMEM;
> + lp->client = client;
> +
> + idev = input_allocate_device();
> + if (!idev) {
> + dev_err(&client->dev, "Can't allocate input device\n");
> + ret = -ENOMEM;
> + goto fail_allocate;
> + }
> +
> + lp->idev = idev;
> + memcpy(lp->btncode, pcf8574_kp_btncode, sizeof(pcf8574_kp_btncode));
> +
> + idev->evbit[0] = 0;
> +
> + idev->evbit[0] |= BIT_MASK(EV_KEY);
> + idev->keycode = lp->btncode;
> + idev->keycodesize = sizeof(pcf8574_kp_btncode);
Umm 17 bytes is a bit too much ;) sizeof(lp->btncode[0]); is more
appropriate.
> + idev->keycodemax = ARRAY_SIZE(pcf8574_kp_btncode);
> +
> + for (i = 0; i < ARRAY_SIZE(pcf8574_kp_btncode); ++i)
> + __set_bit(lp->btncode[i] & KEY_MAX, idev->keybit);
> +
> + __clear_bit(KEY_RESERVED, idev->keybit);
> +
> + sprintf(lp->name, DRV_NAME);
> + sprintf(lp->phys, "kp_data/input0");
> +
> + idev->name = lp->name;
> + idev->phys = lp->phys;
> + idev->id.bustype = BUS_I2C;
> + idev->id.vendor = 0x0001;
> + idev->id.product = 0x0001;
> + idev->id.version = 0x0100;
> +
> + input_set_drvdata(idev, lp);
> +
> + ret = input_register_device(idev);
> + if (ret) {
> + dev_err(&client->dev, "input_register_device() failed\n");
> + goto fail_register;
> + }
> +
> + lp->statechanged = 0x0;
> +
> + lp->laststate = read_state(lp);
> +
> + /* Set up our workqueue. */
> + INIT_WORK(&lp->pcf8574_kp_work, check_and_notify);
> +
> + i2c_set_clientdata(client, lp);
> +
> + ret = request_irq(client->irq, pcf8574_kp_irq_handler,
> + IRQF_TRIGGER_LOW, DRV_NAME, lp);
> + if (ret) {
> + dev_err(&client->dev, "IRQ %d is not free\n", client->irq);
> + goto fail_irq;
> + }
> +
> + return 0;
> +
> + fail_irq:
> + input_unregister_device(idev);
You need "idev = NULL;" here.
> + fail_register:
> + input_set_drvdata(idev, NULL);
> + input_free_device(idev);
> + fail_allocate:
> + kfree(lp);
> +
> + return ret;
> +}
> +
> +static int __devexit pcf8574_kp_remove(struct i2c_client *client)
> +{
> + struct kp_data *lp = i2c_get_clientdata(client);
> +
> + if (client->irq > 0)
No need for this check.
> + free_irq(client->irq, lp);
> +
> + input_set_drvdata(lp->idev, NULL);
> + input_unregister_device(lp->idev);
> + kfree(lp);
> +
> + i2c_set_clientdata(client, NULL);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int pcf8574_kp_resume(struct i2c_client *client)
> +{
> + enable_irq(client->irq);
> + return 0;
> +}
> +
> +static int pcf8574_kp_suspend(struct i2c_client *client, pm_message_t mesg)
> +{
> + disable_irq(client->irq);
> + return 0;
> +}
> +#else
> +# define pcf8574_kp_resume NULL
> +# define pcf8574_kp_suspend NULL
> +#endif
> +
> +static const struct i2c_device_id pcf8574_kp_id[] = {
> + { DRV_NAME, 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, pcf8574_kp_id);
> +
> +static struct i2c_driver pcf8574_kp_driver = {
> + .driver = {
> + .name = DRV_NAME,
> + .owner = THIS_MODULE
> + },
> + .probe = pcf8574_kp_probe,
> + .remove = __devexit_p(pcf8574_kp_remove),
> + .suspend = pcf8574_kp_suspend,
> + .resume = pcf8574_kp_resume,
> + .id_table = pcf8574_kp_id,
> +};
> +
> +static int __init pcf8574_kp_init(void)
> +{
> + return i2c_add_driver(&pcf8574_kp_driver);
> +}
> +module_init(pcf8574_kp_init);
> +
> +static void __exit pcf8574_kp_exit(void)
> +{
> + i2c_del_driver(&pcf8574_kp_driver);
> +}
> +module_exit(pcf8574_kp_exit);
> +
> +MODULE_AUTHOR("Michael Hennerich");
> +MODULE_DESCRIPTION("Keypad input driver for 16 keys connected to PCF8574");
> +MODULE_LICENSE("GPL");
> --
> 1.7.0.2
>
--
Dmitry
next prev parent reply other threads:[~2010-03-09 21:21 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-03-09 15:01 [PATCH] input/misc: PCF8574 I2C keypad input device driver Mike Frysinger
2010-03-09 18:09 ` Dmitry Torokhov
2010-03-09 21:01 ` [PATCH v2] " Mike Frysinger
2010-03-09 21:21 ` Dmitry Torokhov [this message]
2010-03-09 21:27 ` [Uclinux-dist-devel] " Mike Frysinger
2010-03-09 22:30 ` Dmitry Torokhov
2010-03-10 4:36 ` Dmitry Torokhov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20100309212127.GA29333@core.coreip.homeip.net \
--to=dmitry.torokhov@gmail.com \
--cc=cooloney@kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=uclinux-dist-devel@blackfin.uclinux.org \
--cc=vapier@gentoo.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.