| Message ID | 1253654850-11983-1-git-send-email-miguel.aguilar@ridgerun.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Miguel, On Tue, Sep 22, 2009 at 03:27:30PM -0600, miguel.aguilar@ridgerun.com wrote: > From: Miguel Aguilar <miguel.aguilar@ridgerun.com> > > Adds the driver for enabling keypad support for DaVinci platforms. > > DM365 is the only platform that uses this driver at the moment. > > This driver was tested on DM365 EVM rev C. > > Signed-off-by: Miguel Aguilar <miguel.aguilar@ridgerun.com> > --- > arch/arm/configs/davinci_all_defconfig | 1 + > arch/arm/mach-davinci/include/mach/keypad.h | 35 +++ > drivers/input/keyboard/Kconfig | 7 + > drivers/input/keyboard/Makefile | 1 + > drivers/input/keyboard/davinci_keypad.c | 319 +++++++++++++++++++++++++++ > 5 files changed, 363 insertions(+), 0 deletions(-) > create mode 100644 arch/arm/mach-davinci/include/mach/keypad.h > create mode 100644 drivers/input/keyboard/davinci_keypad.c > > diff --git a/arch/arm/configs/davinci_all_defconfig b/arch/arm/configs/davinci_all_defconfig > index ec63c15..e994c83 100644 > --- a/arch/arm/configs/davinci_all_defconfig > +++ b/arch/arm/configs/davinci_all_defconfig > @@ -763,6 +763,7 @@ CONFIG_KEYBOARD_GPIO=y > # CONFIG_KEYBOARD_STOWAWAY is not set > # CONFIG_KEYBOARD_SUNKBD is not set > CONFIG_KEYBOARD_XTKBD=m > +CONFIG_KEYBOARD_DAVINCI_DM365=m > # CONFIG_INPUT_MOUSE is not set > # CONFIG_INPUT_JOYSTICK is not set > # CONFIG_INPUT_TABLET is not set If you want to merge the driver through input tree the defconfig chunk has to go elsewhere. > diff --git a/arch/arm/mach-davinci/include/mach/keypad.h b/arch/arm/mach-davinci/include/mach/keypad.h > new file mode 100644 > index 0000000..922d20e > --- /dev/null > +++ b/arch/arm/mach-davinci/include/mach/keypad.h > @@ -0,0 +1,35 @@ > +/* > + * Copyright (C) 2009 Texas Instruments, Inc > + * > + * Author: Miguel Aguilar <miguel.aguilar@ridgerun.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; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA > + */ > + > +#ifndef DAVINCI_KEYPAD_H > +#define DAVINCI_KEYPAD_H > + > +#include <linux/io.h> > + > +struct davinci_kp_platform_data { > + int *keymap; > + u32 keymapsize; > + u32 rep:1; > + u32 strobe; > + u32 interval; > +}; > + > +#endif > + > diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig > index a6b989a..b6b9517 100644 > --- a/drivers/input/keyboard/Kconfig > +++ b/drivers/input/keyboard/Kconfig > @@ -361,4 +361,11 @@ config KEYBOARD_XTKBD > To compile this driver as a module, choose M here: the > module will be called xtkbd. > > +config KEYBOARD_DAVINCI > + tristate "TI DaVinci Keypad" > + depends on ARCH_DAVINCI_DM365 > + help > + Say Y to enable keypad module support for the TI DaVinci > + platforms (DM365) > + "To compile this driver as a module..." > endif > diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile > index b5b5eae..0b0274e 100644 > --- a/drivers/input/keyboard/Makefile > +++ b/drivers/input/keyboard/Makefile > @@ -31,3 +31,4 @@ obj-$(CONFIG_KEYBOARD_STOWAWAY) += stowaway.o > obj-$(CONFIG_KEYBOARD_SUNKBD) += sunkbd.o > obj-$(CONFIG_KEYBOARD_TOSA) += tosakbd.o > obj-$(CONFIG_KEYBOARD_XTKBD) += xtkbd.o > +obj-$(CONFIG_KEYBOARD_DAVINCI) += davinci_keypad.o > diff --git a/drivers/input/keyboard/davinci_keypad.c b/drivers/input/keyboard/davinci_keypad.c > new file mode 100644 > index 0000000..6f0e793 > --- /dev/null > +++ b/drivers/input/keyboard/davinci_keypad.c > @@ -0,0 +1,319 @@ > +/* > + * DaVinci Keypad Driver for TI platforms > + * > + * Copyright (C) 2009 Texas Instruments, Inc > + * > + * Author: Miguel Aguilar <miguel.aguilar@ridgerun.com> > + * > + * Intial Code: Sandeep Paulraj <s-paulraj@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; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA > + */ > +#include <linux/module.h> > +#include <linux/init.h> > +#include <linux/interrupt.h> > +#include <linux/types.h> > +#include <linux/input.h> > +#include <linux/kernel.h> > +#include <linux/delay.h> > +#include <linux/platform_device.h> > +#include <linux/errno.h> > + > +#include <asm/irq.h> > + > +#include <mach/hardware.h> > +#include <mach/irqs.h> > +#include <mach/keypad.h> > + > +/* Keypad registers */ > +#define DAVINCI_KEYPAD_KEYCTRL 0x0000 > +#define DAVINCI_KEYPAD_INTENA 0x0004 > +#define DAVINCI_KEYPAD_INTFLAG 0x0008 > +#define DAVINCI_KEYPAD_INTCLR 0x000c > +#define DAVINCI_KEYPAD_STRBWIDTH 0x0010 > +#define DAVINCI_KEYPAD_INTERVAL 0x0014 > +#define DAVINCI_KEYPAD_CONTTIME 0x0018 > +#define DAVINCI_KEYPAD_CURRENTST 0x001c > +#define DAVINCI_KEYPAD_PREVSTATE 0x0020 > +#define DAVINCI_KEYPAD_EMUCTRL 0x0024 > +#define DAVINCI_KEYPAD_IODFTCTRL 0x002c > + > +/* Key Control Register (KEYCTRL) */ > +#define DAVINCI_KEYPAD_KEYEN 0x00000001 > +#define DAVINCI_KEYPAD_PREVMODE 0x00000002 > +#define DAVINCI_KEYPAD_CHATOFF 0x00000004 > +#define DAVINCI_KEYPAD_AUTODET 0x00000008 > +#define DAVINCI_KEYPAD_SCANMODE 0x00000010 > +#define DAVINCI_KEYPAD_OUTTYPE 0x00000020 > +#define DAVINCI_KEYPAD_4X4 0x00000040 > + > +/* Masks for the interrupts */ > +#define DAVINCI_KEYPAD_INT_CONT 0x00000008 > +#define DAVINCI_KEYPAD_INT_OFF 0x00000004 > +#define DAVINCI_KEYPAD_INT_ON 0x00000002 > +#define DAVINCI_KEYPAD_INT_CHANGE 0x00000001 > +#define DAVINCI_KEYPAD_INT_ALL 0x0000000f > + > +struct davinci_kp { > + struct input_dev *input; > + struct davinci_kp_platform_data *pdata; > + int irq; > + void __iomem *base; > + resource_size_t pbase; > + size_t base_size; > +}; > + > +static void davinci_kp_write(struct davinci_kp *davinci_kp, u32 val, u32 addr) > +{ > + u32 base = (u32)davinci_kp->base; > + > + __raw_writel(val,(u32 *)(base + addr)); > +} > + > +static u32 davinci_kp_read(struct davinci_kp *davinci_kp, u32 addr) > +{ > + u32 base = (u32)davinci_kp->base; > + > + return __raw_readl((u32 *)(base + addr)); > +} > + > +/* Initializing the kp Module */ > +static void davinci_kp_initialize(struct davinci_kp *davinci_kp) > +{ > + u32 strobe = davinci_kp->pdata->strobe; > + u32 interval = davinci_kp->pdata->interval; > + > + /* Enable all interrupts */ > + davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, DAVINCI_KEYPAD_INTENA); > + > + /* Clear interrupts if any */ > + davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, DAVINCI_KEYPAD_INTCLR); > + > + /* Setup the scan period = strobe + interval */ > + davinci_kp_write(davinci_kp, strobe, DAVINCI_KEYPAD_STRBWIDTH); > + davinci_kp_write(davinci_kp, interval, DAVINCI_KEYPAD_INTERVAL); > + davinci_kp_write(davinci_kp, 0x01, DAVINCI_KEYPAD_CONTTIME); > + > + /* Enable Keyscan module and enable */ > + davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_AUTODET | DAVINCI_KEYPAD_KEYEN, > + DAVINCI_KEYPAD_KEYCTRL); > +} > + > +static irqreturn_t davinci_kp_interrupt(int irq, void *dev_id) > +{ > + struct davinci_kp *davinci_kp = dev_id; > + struct device *dev = &davinci_kp->input->dev; > + int *keymap = davinci_kp->pdata->keymap; > + u32 prev_status, new_status, changed, position; > + int keycode = KEY_UNKNOWN; > + int ret = IRQ_NONE; > + > + /* Disable interrupt */ > + davinci_kp_write(davinci_kp, 0x0, DAVINCI_KEYPAD_INTENA); > + > + /* Reading previous and new status of the keypad */ > + prev_status = davinci_kp_read(davinci_kp, DAVINCI_KEYPAD_PREVSTATE); > + new_status = davinci_kp_read(davinci_kp, DAVINCI_KEYPAD_CURRENTST); > + > + changed = prev_status ^ new_status; > + position = ffs(changed) - 1; > + > + if (changed) { Can there be several buttons that change status at once? > + keycode = keymap[position]; > + if((new_status >> position) & 0x1) { bool release = (new_status >> position) & 0x1; input_report_key(davinci_kp->input, keycode, !release); dev_dbg(dev, "davinci_keypad: key %d %s\n", keycode, release ? "released" : "pressed"); is shorter. > + /* Report release */ > + dev_dbg(dev, "davinci_keypad: key %d released\n", > + keycode); > + input_report_key(davinci_kp->input, keycode, 0); > + } else { > + /* Report press */ > + dev_dbg(dev, "davinci_keypad: key %d pressed\n", > + keycode); > + input_report_key(davinci_kp->input, keycode, 1); > + } > + input_sync(davinci_kp->input); > + ret = IRQ_HANDLED; > + } > + > + /* Clearing interrupt */ > + davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, DAVINCI_KEYPAD_INTCLR); You return IRQ_HANDLED only if keypad state changed but clear interrupt regardless. This is suspicious. > + > + /* Enable interrupts */ > + davinci_kp_write(davinci_kp, 0x1, DAVINCI_KEYPAD_INTENA); > + > + return ret; > +} > + > +static int __init davinci_kp_probe(struct platform_device *pdev) > +{ > + struct davinci_kp *davinci_kp; > + struct input_dev *key_dev; > + struct resource *res, *mem; > + int ret, i; > + struct device * dev = &pdev->dev; > + struct davinci_kp_platform_data *pdata = pdev->dev.platform_data; > + > + dev_info(dev, "DaVinci Keypad Driver\n"); > + > + if (!pdata->keymap) { > + dev_dbg(dev, "no keymap from pdata\n"); > + return -EINVAL; > + } > + > + davinci_kp = kzalloc(sizeof *davinci_kp, GFP_KERNEL); > + if(!davinci_kp) { > + dev_dbg(dev, "could not allocate memory for private data\n"); > + return -ENOMEM; > + } > + > + key_dev = input_allocate_device(); > + if (!key_dev) { > + dev_dbg(dev, "could not allocate input device\n"); > + ret = -ENOMEM; > + goto fail1; > + } > + > + platform_set_drvdata(pdev, davinci_kp); > + > + davinci_kp->input = key_dev; > + > + davinci_kp->irq = platform_get_irq(pdev, 0); > + if (davinci_kp->irq < 0) { > + dev_err(dev, "no keypad irq\n"); > + ret = davinci_kp->irq; > + goto fail2; > + } > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) { > + dev_err(dev, "no mem resource\n"); > + ret = -ENODEV; -EINVAL I'd say. > + goto fail2; > + } > + > + davinci_kp->pbase = res->start; > + davinci_kp->base_size = resource_size(res); > + > + mem = request_mem_region(davinci_kp->pbase, davinci_kp->base_size, pdev->name); > + if (!mem) { > + dev_err(dev, "KEYSCAN registers at %08x are not free\n", > + davinci_kp->pbase); > + ret = -EBUSY; > + goto fail2; > + } > + > + davinci_kp->base = ioremap(davinci_kp->pbase, davinci_kp->base_size); > + if (!davinci_kp->base) { > + dev_err(dev, "can't ioremap MEM resource.\n"); > + ret = -ENOMEM; > + goto fail3; > + } > + > + /* Enable auto repeat feature of Linux input subsystem */ > + if (pdata->rep) > + __set_bit(EV_REP, key_dev->evbit); > + > + /* Setup input device */ > + __set_bit(EV_KEY, key_dev->evbit); > + > + /* Setup the keymap */ > + davinci_kp->pdata = pdata; > + > + for (i = 0; i < davinci_kp->pdata->keymapsize; i++) > + __set_bit(davinci_kp->pdata->keymap[i], key_dev->keybit); > + > + key_dev->name = "davinci_keypad"; > + key_dev->phys = "davinci_keypad/input0"; > + key_dev->dev.parent = &pdev->dev; > + key_dev->id.bustype = BUS_HOST; > + key_dev->id.vendor = 0x0001; > + key_dev->id.product = 0x0001; > + key_dev->id.version = 0x0001; > + key_dev->keycode = davinci_kp->pdata->keymap; Please kopy keymap into the davinci_kp stucture and use it so that platform data is never changed and can be declared const. > + key_dev->keycodesize = sizeof(unsigned int); sizeof(davinci_kp->keymap[0]) is safer. Plus make it unsigned short. > + key_dev->keycodemax = davinci_kp->pdata->keymapsize; > + > + ret = input_register_device(davinci_kp->input); > + if (ret < 0) { > + dev_err(dev, "unable to register DaVinci keypad device\n"); > + goto fail4; > + } > + > + ret = request_irq(davinci_kp->irq, davinci_kp_interrupt, IRQF_DISABLED, > + "davinci_keypad", davinci_kp); > + if (ret < 0) { > + dev_err(dev, "unable to register DaVinci keypad Interrupt\n"); > + goto fail5; > + } > + > + davinci_kp_initialize(davinci_kp); > + > + return 0; > +fail5: > + input_unregister_device(davinci_kp->input); > + key_dev = NULL; > +fail4: > + iounmap(davinci_kp->base); > +fail3: > + release_mem_region(davinci_kp->pbase, davinci_kp->base_size); > +fail2: > + input_free_device(key_dev); > +fail1: > + kfree(davinci_kp); > + > + return ret; > +} > + > +static int __exit davinci_kp_remove(struct platform_device *pdev) __devexit? > +{ > + struct davinci_kp *davinci_kp = platform_get_drvdata(pdev); > + > + free_irq(davinci_kp->irq, davinci_kp); > + > + input_unregister_device(davinci_kp->input); > + > + iounmap(davinci_kp->base); > + release_mem_region(davinci_kp->pbase, davinci_kp->base_size); > + > + platform_set_drvdata(pdev, NULL); > + > + kfree(davinci_kp); > + > + return 0; > +} > + > +static struct platform_driver davinci_kp_driver = { > + .driver = { > + .name = "davinci_keypad", > + .owner = THIS_MODULE, > + }, > + .remove = __exit_p(davinci_kp_remove), __devexit_p(). I think you can still unbind the device even if you use platform_driver_probe. > +}; > + > +static int __init davinci_kp_init(void) > +{ > + return platform_driver_probe(&davinci_kp_driver, davinci_kp_probe); > +} > +module_init(davinci_kp_init); > + > +static void __exit davinci_kp_exit(void) > +{ > + platform_driver_unregister(&davinci_kp_driver); > +} > +module_exit(davinci_kp_exit); > + > +MODULE_AUTHOR("Miguel Aguilar"); > +MODULE_DESCRIPTION("Texas Instruments DaVinci Keypad Driver"); > +MODULE_LICENSE("GPL"); > -- > 1.6.0.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-input" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Dmitry, Dmitry Torokhov wrote: > Hi Miguel, > > On Tue, Sep 22, 2009 at 03:27:30PM -0600, miguel.aguilar@ridgerun.com wrote: >> From: Miguel Aguilar <miguel.aguilar@ridgerun.com> >> >> Adds the driver for enabling keypad support for DaVinci platforms. >> >> DM365 is the only platform that uses this driver at the moment. >> >> This driver was tested on DM365 EVM rev C. >> >> Signed-off-by: Miguel Aguilar <miguel.aguilar@ridgerun.com> >> --- >> arch/arm/configs/davinci_all_defconfig | 1 + >> arch/arm/mach-davinci/include/mach/keypad.h | 35 +++ >> drivers/input/keyboard/Kconfig | 7 + >> drivers/input/keyboard/Makefile | 1 + >> drivers/input/keyboard/davinci_keypad.c | 319 +++++++++++++++++++++++++++ >> 5 files changed, 363 insertions(+), 0 deletions(-) >> create mode 100644 arch/arm/mach-davinci/include/mach/keypad.h >> create mode 100644 drivers/input/keyboard/davinci_keypad.c >> >> diff --git a/arch/arm/configs/davinci_all_defconfig b/arch/arm/configs/davinci_all_defconfig >> index ec63c15..e994c83 100644 >> --- a/arch/arm/configs/davinci_all_defconfig >> +++ b/arch/arm/configs/davinci_all_defconfig >> @@ -763,6 +763,7 @@ CONFIG_KEYBOARD_GPIO=y >> # CONFIG_KEYBOARD_STOWAWAY is not set >> # CONFIG_KEYBOARD_SUNKBD is not set >> CONFIG_KEYBOARD_XTKBD=m >> +CONFIG_KEYBOARD_DAVINCI_DM365=m >> # CONFIG_INPUT_MOUSE is not set >> # CONFIG_INPUT_JOYSTICK is not set >> # CONFIG_INPUT_TABLET is not set > > > If you want to merge the driver through input tree the defconfig chunk > has to go elsewhere. [MA] So, Where it should go? > >> diff --git a/arch/arm/mach-davinci/include/mach/keypad.h b/arch/arm/mach-davinci/include/mach/keypad.h >> new file mode 100644 >> index 0000000..922d20e >> --- /dev/null >> +++ b/arch/arm/mach-davinci/include/mach/keypad.h >> @@ -0,0 +1,35 @@ >> +/* >> + * Copyright (C) 2009 Texas Instruments, Inc >> + * >> + * Author: Miguel Aguilar <miguel.aguilar@ridgerun.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; either version 2 of the License, or >> + * (at your option) any later version. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + * You should have received a copy of the GNU General Public License >> + * along with this program; if not, write to the Free Software >> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA >> + */ >> + >> +#ifndef DAVINCI_KEYPAD_H >> +#define DAVINCI_KEYPAD_H >> + >> +#include <linux/io.h> >> + >> +struct davinci_kp_platform_data { >> + int *keymap; >> + u32 keymapsize; >> + u32 rep:1; >> + u32 strobe; >> + u32 interval; >> +}; >> + >> +#endif >> + >> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig >> index a6b989a..b6b9517 100644 >> --- a/drivers/input/keyboard/Kconfig >> +++ b/drivers/input/keyboard/Kconfig >> @@ -361,4 +361,11 @@ config KEYBOARD_XTKBD >> To compile this driver as a module, choose M here: the >> module will be called xtkbd. >> >> +config KEYBOARD_DAVINCI >> + tristate "TI DaVinci Keypad" >> + depends on ARCH_DAVINCI_DM365 >> + help >> + Say Y to enable keypad module support for the TI DaVinci >> + platforms (DM365) >> + > > "To compile this driver as a module..." [MA] Ok. > >> endif >> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile >> index b5b5eae..0b0274e 100644 >> --- a/drivers/input/keyboard/Makefile >> +++ b/drivers/input/keyboard/Makefile >> @@ -31,3 +31,4 @@ obj-$(CONFIG_KEYBOARD_STOWAWAY) += stowaway.o >> obj-$(CONFIG_KEYBOARD_SUNKBD) += sunkbd.o >> obj-$(CONFIG_KEYBOARD_TOSA) += tosakbd.o >> obj-$(CONFIG_KEYBOARD_XTKBD) += xtkbd.o >> +obj-$(CONFIG_KEYBOARD_DAVINCI) += davinci_keypad.o >> diff --git a/drivers/input/keyboard/davinci_keypad.c b/drivers/input/keyboard/davinci_keypad.c >> new file mode 100644 >> index 0000000..6f0e793 >> --- /dev/null >> +++ b/drivers/input/keyboard/davinci_keypad.c >> @@ -0,0 +1,319 @@ >> +/* >> + * DaVinci Keypad Driver for TI platforms >> + * >> + * Copyright (C) 2009 Texas Instruments, Inc >> + * >> + * Author: Miguel Aguilar <miguel.aguilar@ridgerun.com> >> + * >> + * Intial Code: Sandeep Paulraj <s-paulraj@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; either version 2 of the License, or >> + * (at your option) any later version. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + * You should have received a copy of the GNU General Public License >> + * along with this program; if not, write to the Free Software >> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA >> + */ >> +#include <linux/module.h> >> +#include <linux/init.h> >> +#include <linux/interrupt.h> >> +#include <linux/types.h> >> +#include <linux/input.h> >> +#include <linux/kernel.h> >> +#include <linux/delay.h> >> +#include <linux/platform_device.h> >> +#include <linux/errno.h> >> + >> +#include <asm/irq.h> >> + >> +#include <mach/hardware.h> >> +#include <mach/irqs.h> >> +#include <mach/keypad.h> >> + >> +/* Keypad registers */ >> +#define DAVINCI_KEYPAD_KEYCTRL 0x0000 >> +#define DAVINCI_KEYPAD_INTENA 0x0004 >> +#define DAVINCI_KEYPAD_INTFLAG 0x0008 >> +#define DAVINCI_KEYPAD_INTCLR 0x000c >> +#define DAVINCI_KEYPAD_STRBWIDTH 0x0010 >> +#define DAVINCI_KEYPAD_INTERVAL 0x0014 >> +#define DAVINCI_KEYPAD_CONTTIME 0x0018 >> +#define DAVINCI_KEYPAD_CURRENTST 0x001c >> +#define DAVINCI_KEYPAD_PREVSTATE 0x0020 >> +#define DAVINCI_KEYPAD_EMUCTRL 0x0024 >> +#define DAVINCI_KEYPAD_IODFTCTRL 0x002c >> + >> +/* Key Control Register (KEYCTRL) */ >> +#define DAVINCI_KEYPAD_KEYEN 0x00000001 >> +#define DAVINCI_KEYPAD_PREVMODE 0x00000002 >> +#define DAVINCI_KEYPAD_CHATOFF 0x00000004 >> +#define DAVINCI_KEYPAD_AUTODET 0x00000008 >> +#define DAVINCI_KEYPAD_SCANMODE 0x00000010 >> +#define DAVINCI_KEYPAD_OUTTYPE 0x00000020 >> +#define DAVINCI_KEYPAD_4X4 0x00000040 >> + >> +/* Masks for the interrupts */ >> +#define DAVINCI_KEYPAD_INT_CONT 0x00000008 >> +#define DAVINCI_KEYPAD_INT_OFF 0x00000004 >> +#define DAVINCI_KEYPAD_INT_ON 0x00000002 >> +#define DAVINCI_KEYPAD_INT_CHANGE 0x00000001 >> +#define DAVINCI_KEYPAD_INT_ALL 0x0000000f >> + >> +struct davinci_kp { >> + struct input_dev *input; >> + struct davinci_kp_platform_data *pdata; >> + int irq; >> + void __iomem *base; >> + resource_size_t pbase; >> + size_t base_size; >> +}; >> + >> +static void davinci_kp_write(struct davinci_kp *davinci_kp, u32 val, u32 addr) >> +{ >> + u32 base = (u32)davinci_kp->base; >> + >> + __raw_writel(val,(u32 *)(base + addr)); >> +} >> + >> +static u32 davinci_kp_read(struct davinci_kp *davinci_kp, u32 addr) >> +{ >> + u32 base = (u32)davinci_kp->base; >> + >> + return __raw_readl((u32 *)(base + addr)); >> +} >> + >> +/* Initializing the kp Module */ >> +static void davinci_kp_initialize(struct davinci_kp *davinci_kp) >> +{ >> + u32 strobe = davinci_kp->pdata->strobe; >> + u32 interval = davinci_kp->pdata->interval; >> + >> + /* Enable all interrupts */ >> + davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, DAVINCI_KEYPAD_INTENA); >> + >> + /* Clear interrupts if any */ >> + davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, DAVINCI_KEYPAD_INTCLR); >> + >> + /* Setup the scan period = strobe + interval */ >> + davinci_kp_write(davinci_kp, strobe, DAVINCI_KEYPAD_STRBWIDTH); >> + davinci_kp_write(davinci_kp, interval, DAVINCI_KEYPAD_INTERVAL); >> + davinci_kp_write(davinci_kp, 0x01, DAVINCI_KEYPAD_CONTTIME); >> + >> + /* Enable Keyscan module and enable */ >> + davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_AUTODET | DAVINCI_KEYPAD_KEYEN, >> + DAVINCI_KEYPAD_KEYCTRL); >> +} >> + >> +static irqreturn_t davinci_kp_interrupt(int irq, void *dev_id) >> +{ >> + struct davinci_kp *davinci_kp = dev_id; >> + struct device *dev = &davinci_kp->input->dev; >> + int *keymap = davinci_kp->pdata->keymap; >> + u32 prev_status, new_status, changed, position; >> + int keycode = KEY_UNKNOWN; >> + int ret = IRQ_NONE; >> + >> + /* Disable interrupt */ >> + davinci_kp_write(davinci_kp, 0x0, DAVINCI_KEYPAD_INTENA); >> + >> + /* Reading previous and new status of the keypad */ >> + prev_status = davinci_kp_read(davinci_kp, DAVINCI_KEYPAD_PREVSTATE); >> + new_status = davinci_kp_read(davinci_kp, DAVINCI_KEYPAD_CURRENTST); >> + >> + changed = prev_status ^ new_status; >> + position = ffs(changed) - 1; >> + >> + if (changed) { > > Can there be several buttons that change status at once? [MA] It is not suppose to change several buttons at once. > >> + keycode = keymap[position]; >> + if((new_status >> position) & 0x1) { > > bool release = (new_status >> position) & 0x1; > input_report_key(davinci_kp->input, keycode, !release); > dev_dbg(dev, "davinci_keypad: key %d %s\n", > keycode, release ? "released" : "pressed"); > [MA] Ok I'll try that. > is shorter. > >> + /* Report release */ >> + dev_dbg(dev, "davinci_keypad: key %d released\n", >> + keycode); >> + input_report_key(davinci_kp->input, keycode, 0); >> + } else { >> + /* Report press */ >> + dev_dbg(dev, "davinci_keypad: key %d pressed\n", >> + keycode); >> + input_report_key(davinci_kp->input, keycode, 1); >> + } >> + input_sync(davinci_kp->input); >> + ret = IRQ_HANDLED; >> + } >> + >> + /* Clearing interrupt */ >> + davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, DAVINCI_KEYPAD_INTCLR); > > You return IRQ_HANDLED only if keypad state changed but clear interrupt > regardless. This is suspicious. [MA] Ok. I'll clear the irq status only if IRQ_HANDLED. > >> + >> + /* Enable interrupts */ >> + davinci_kp_write(davinci_kp, 0x1, DAVINCI_KEYPAD_INTENA); >> + >> + return ret; >> +} >> + >> +static int __init davinci_kp_probe(struct platform_device *pdev) >> +{ >> + struct davinci_kp *davinci_kp; >> + struct input_dev *key_dev; >> + struct resource *res, *mem; >> + int ret, i; >> + struct device * dev = &pdev->dev; >> + struct davinci_kp_platform_data *pdata = pdev->dev.platform_data; >> + >> + dev_info(dev, "DaVinci Keypad Driver\n"); >> + >> + if (!pdata->keymap) { >> + dev_dbg(dev, "no keymap from pdata\n"); >> + return -EINVAL; >> + } >> + >> + davinci_kp = kzalloc(sizeof *davinci_kp, GFP_KERNEL); >> + if(!davinci_kp) { >> + dev_dbg(dev, "could not allocate memory for private data\n"); >> + return -ENOMEM; >> + } >> + >> + key_dev = input_allocate_device(); >> + if (!key_dev) { >> + dev_dbg(dev, "could not allocate input device\n"); >> + ret = -ENOMEM; >> + goto fail1; >> + } >> + >> + platform_set_drvdata(pdev, davinci_kp); >> + >> + davinci_kp->input = key_dev; >> + >> + davinci_kp->irq = platform_get_irq(pdev, 0); >> + if (davinci_kp->irq < 0) { >> + dev_err(dev, "no keypad irq\n"); >> + ret = davinci_kp->irq; >> + goto fail2; >> + } >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + if (!res) { >> + dev_err(dev, "no mem resource\n"); >> + ret = -ENODEV; > > -EINVAL I'd say. [MA] platform_get_resource should fail with -ENODEV. > >> + goto fail2; >> + } >> + >> + davinci_kp->pbase = res->start; >> + davinci_kp->base_size = resource_size(res); >> + >> + mem = request_mem_region(davinci_kp->pbase, davinci_kp->base_size, pdev->name); >> + if (!mem) { >> + dev_err(dev, "KEYSCAN registers at %08x are not free\n", >> + davinci_kp->pbase); >> + ret = -EBUSY; >> + goto fail2; >> + } >> + >> + davinci_kp->base = ioremap(davinci_kp->pbase, davinci_kp->base_size); >> + if (!davinci_kp->base) { >> + dev_err(dev, "can't ioremap MEM resource.\n"); >> + ret = -ENOMEM; >> + goto fail3; >> + } >> + >> + /* Enable auto repeat feature of Linux input subsystem */ >> + if (pdata->rep) >> + __set_bit(EV_REP, key_dev->evbit); >> + >> + /* Setup input device */ >> + __set_bit(EV_KEY, key_dev->evbit); >> + >> + /* Setup the keymap */ >> + davinci_kp->pdata = pdata; >> + >> + for (i = 0; i < davinci_kp->pdata->keymapsize; i++) >> + __set_bit(davinci_kp->pdata->keymap[i], key_dev->keybit); >> + >> + key_dev->name = "davinci_keypad"; >> + key_dev->phys = "davinci_keypad/input0"; >> + key_dev->dev.parent = &pdev->dev; >> + key_dev->id.bustype = BUS_HOST; >> + key_dev->id.vendor = 0x0001; >> + key_dev->id.product = 0x0001; >> + key_dev->id.version = 0x0001; >> + key_dev->keycode = davinci_kp->pdata->keymap; > > Please kopy keymap into the davinci_kp stucture and use it so that > platform data is never changed and can be declared const. Do you mean something like this? struct davinci_kp { ... const int *keymap; ... }; > >> + key_dev->keycodesize = sizeof(unsigned int); > > sizeof(davinci_kp->keymap[0]) is safer. Plus make it unsigned short. [MA] Ok. > >> + key_dev->keycodemax = davinci_kp->pdata->keymapsize; >> + >> + ret = input_register_device(davinci_kp->input); >> + if (ret < 0) { >> + dev_err(dev, "unable to register DaVinci keypad device\n"); >> + goto fail4; >> + } >> + >> + ret = request_irq(davinci_kp->irq, davinci_kp_interrupt, IRQF_DISABLED, >> + "davinci_keypad", davinci_kp); >> + if (ret < 0) { >> + dev_err(dev, "unable to register DaVinci keypad Interrupt\n"); >> + goto fail5; >> + } >> + >> + davinci_kp_initialize(davinci_kp); >> + >> + return 0; >> +fail5: >> + input_unregister_device(davinci_kp->input); >> + key_dev = NULL; >> +fail4: >> + iounmap(davinci_kp->base); >> +fail3: >> + release_mem_region(davinci_kp->pbase, davinci_kp->base_size); >> +fail2: >> + input_free_device(key_dev); >> +fail1: >> + kfree(davinci_kp); >> + >> + return ret; >> +} >> + >> +static int __exit davinci_kp_remove(struct platform_device *pdev) > > __devexit? [MA] According to comments from David Brownell to the first version of this patch the __exit should be used. " - Use platform_driver_probe() and __exit/__exit_p(); there's no point in keeping that code around in typical configs, it'd just waste memory. " > >> +{ >> + struct davinci_kp *davinci_kp = platform_get_drvdata(pdev); >> + >> + free_irq(davinci_kp->irq, davinci_kp); >> + >> + input_unregister_device(davinci_kp->input); >> + >> + iounmap(davinci_kp->base); >> + release_mem_region(davinci_kp->pbase, davinci_kp->base_size); >> + >> + platform_set_drvdata(pdev, NULL); >> + >> + kfree(davinci_kp); >> + >> + return 0; >> +} >> + >> +static struct platform_driver davinci_kp_driver = { >> + .driver = { >> + .name = "davinci_keypad", >> + .owner = THIS_MODULE, >> + }, >> + .remove = __exit_p(davinci_kp_remove), > > __devexit_p(). I think you can still unbind the device even if you use > platform_driver_probe. [MA] Same. > >> +}; >> + >> +static int __init davinci_kp_init(void) >> +{ >> + return platform_driver_probe(&davinci_kp_driver, davinci_kp_probe); >> +} >> +module_init(davinci_kp_init); >> + >> +static void __exit davinci_kp_exit(void) >> +{ >> + platform_driver_unregister(&davinci_kp_driver); >> +} >> +module_exit(davinci_kp_exit); >> + >> +MODULE_AUTHOR("Miguel Aguilar"); >> +MODULE_DESCRIPTION("Texas Instruments DaVinci Keypad Driver"); >> +MODULE_LICENSE("GPL"); >> -- >> 1.6.0.4 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-input" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Miguel, [Adding David to CC for the __devexit/__exit discussion] On Wed, Sep 23, 2009 at 08:52:40AM -0600, Miguel Aguilar wrote: > Dmitry, > > Dmitry Torokhov wrote: >> Hi Miguel, >> >> On Tue, Sep 22, 2009 at 03:27:30PM -0600, miguel.aguilar@ridgerun.com wrote: >>> From: Miguel Aguilar <miguel.aguilar@ridgerun.com> >>> >>> Adds the driver for enabling keypad support for DaVinci platforms. >>> >>> DM365 is the only platform that uses this driver at the moment. >>> >>> This driver was tested on DM365 EVM rev C. >>> >>> Signed-off-by: Miguel Aguilar <miguel.aguilar@ridgerun.com> >>> --- >>> arch/arm/configs/davinci_all_defconfig | 1 + >>> arch/arm/mach-davinci/include/mach/keypad.h | 35 +++ >>> drivers/input/keyboard/Kconfig | 7 + >>> drivers/input/keyboard/Makefile | 1 + >>> drivers/input/keyboard/davinci_keypad.c | 319 +++++++++++++++++++++++++++ >>> 5 files changed, 363 insertions(+), 0 deletions(-) >>> create mode 100644 arch/arm/mach-davinci/include/mach/keypad.h >>> create mode 100644 drivers/input/keyboard/davinci_keypad.c >>> >>> diff --git a/arch/arm/configs/davinci_all_defconfig b/arch/arm/configs/davinci_all_defconfig >>> index ec63c15..e994c83 100644 >>> --- a/arch/arm/configs/davinci_all_defconfig >>> +++ b/arch/arm/configs/davinci_all_defconfig >>> @@ -763,6 +763,7 @@ CONFIG_KEYBOARD_GPIO=y >>> # CONFIG_KEYBOARD_STOWAWAY is not set >>> # CONFIG_KEYBOARD_SUNKBD is not set >>> CONFIG_KEYBOARD_XTKBD=m >>> +CONFIG_KEYBOARD_DAVINCI_DM365=m >>> # CONFIG_INPUT_MOUSE is not set >>> # CONFIG_INPUT_JOYSTICK is not set >>> # CONFIG_INPUT_TABLET is not set >> >> >> If you want to merge the driver through input tree the defconfig chunk >> has to go elsewhere. > [MA] So, Where it should go? In a separate patch submitted to the person maintaining defconfig for the arch in question once driver is in mainline. >> >>> diff --git a/arch/arm/mach-davinci/include/mach/keypad.h b/arch/arm/mach-davinci/include/mach/keypad.h >>> new file mode 100644 >>> index 0000000..922d20e >>> --- /dev/null >>> +++ b/arch/arm/mach-davinci/include/mach/keypad.h >>> @@ -0,0 +1,35 @@ >>> +/* >>> + * Copyright (C) 2009 Texas Instruments, Inc >>> + * >>> + * Author: Miguel Aguilar <miguel.aguilar@ridgerun.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; either version 2 of the License, or >>> + * (at your option) any later version. >>> + * >>> + * This program is distributed in the hope that it will be useful, >>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>> + * GNU General Public License for more details. >>> + * >>> + * You should have received a copy of the GNU General Public License >>> + * along with this program; if not, write to the Free Software >>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA >>> + */ >>> + >>> +#ifndef DAVINCI_KEYPAD_H >>> +#define DAVINCI_KEYPAD_H >>> + >>> +#include <linux/io.h> >>> + >>> +struct davinci_kp_platform_data { >>> + int *keymap; >>> + u32 keymapsize; >>> + u32 rep:1; >>> + u32 strobe; >>> + u32 interval; >>> +}; >>> + >>> +#endif >>> + >>> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig >>> index a6b989a..b6b9517 100644 >>> --- a/drivers/input/keyboard/Kconfig >>> +++ b/drivers/input/keyboard/Kconfig >>> @@ -361,4 +361,11 @@ config KEYBOARD_XTKBD >>> To compile this driver as a module, choose M here: the >>> module will be called xtkbd. >>> +config KEYBOARD_DAVINCI >>> + tristate "TI DaVinci Keypad" >>> + depends on ARCH_DAVINCI_DM365 >>> + help >>> + Say Y to enable keypad module support for the TI DaVinci >>> + platforms (DM365) >>> + >> >> "To compile this driver as a module..." > [MA] Ok. >> >>> endif >>> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile >>> index b5b5eae..0b0274e 100644 >>> --- a/drivers/input/keyboard/Makefile >>> +++ b/drivers/input/keyboard/Makefile >>> @@ -31,3 +31,4 @@ obj-$(CONFIG_KEYBOARD_STOWAWAY) += stowaway.o >>> obj-$(CONFIG_KEYBOARD_SUNKBD) += sunkbd.o >>> obj-$(CONFIG_KEYBOARD_TOSA) += tosakbd.o >>> obj-$(CONFIG_KEYBOARD_XTKBD) += xtkbd.o >>> +obj-$(CONFIG_KEYBOARD_DAVINCI) += davinci_keypad.o >>> diff --git a/drivers/input/keyboard/davinci_keypad.c b/drivers/input/keyboard/davinci_keypad.c >>> new file mode 100644 >>> index 0000000..6f0e793 >>> --- /dev/null >>> +++ b/drivers/input/keyboard/davinci_keypad.c >>> @@ -0,0 +1,319 @@ >>> +/* >>> + * DaVinci Keypad Driver for TI platforms >>> + * >>> + * Copyright (C) 2009 Texas Instruments, Inc >>> + * >>> + * Author: Miguel Aguilar <miguel.aguilar@ridgerun.com> >>> + * >>> + * Intial Code: Sandeep Paulraj <s-paulraj@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; either version 2 of the License, or >>> + * (at your option) any later version. >>> + * >>> + * This program is distributed in the hope that it will be useful, >>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>> + * GNU General Public License for more details. >>> + * >>> + * You should have received a copy of the GNU General Public License >>> + * along with this program; if not, write to the Free Software >>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA >>> + */ >>> +#include <linux/module.h> >>> +#include <linux/init.h> >>> +#include <linux/interrupt.h> >>> +#include <linux/types.h> >>> +#include <linux/input.h> >>> +#include <linux/kernel.h> >>> +#include <linux/delay.h> >>> +#include <linux/platform_device.h> >>> +#include <linux/errno.h> >>> + >>> +#include <asm/irq.h> >>> + >>> +#include <mach/hardware.h> >>> +#include <mach/irqs.h> >>> +#include <mach/keypad.h> >>> + >>> +/* Keypad registers */ >>> +#define DAVINCI_KEYPAD_KEYCTRL 0x0000 >>> +#define DAVINCI_KEYPAD_INTENA 0x0004 >>> +#define DAVINCI_KEYPAD_INTFLAG 0x0008 >>> +#define DAVINCI_KEYPAD_INTCLR 0x000c >>> +#define DAVINCI_KEYPAD_STRBWIDTH 0x0010 >>> +#define DAVINCI_KEYPAD_INTERVAL 0x0014 >>> +#define DAVINCI_KEYPAD_CONTTIME 0x0018 >>> +#define DAVINCI_KEYPAD_CURRENTST 0x001c >>> +#define DAVINCI_KEYPAD_PREVSTATE 0x0020 >>> +#define DAVINCI_KEYPAD_EMUCTRL 0x0024 >>> +#define DAVINCI_KEYPAD_IODFTCTRL 0x002c >>> + >>> +/* Key Control Register (KEYCTRL) */ >>> +#define DAVINCI_KEYPAD_KEYEN 0x00000001 >>> +#define DAVINCI_KEYPAD_PREVMODE 0x00000002 >>> +#define DAVINCI_KEYPAD_CHATOFF 0x00000004 >>> +#define DAVINCI_KEYPAD_AUTODET 0x00000008 >>> +#define DAVINCI_KEYPAD_SCANMODE 0x00000010 >>> +#define DAVINCI_KEYPAD_OUTTYPE 0x00000020 >>> +#define DAVINCI_KEYPAD_4X4 0x00000040 >>> + >>> +/* Masks for the interrupts */ >>> +#define DAVINCI_KEYPAD_INT_CONT 0x00000008 >>> +#define DAVINCI_KEYPAD_INT_OFF 0x00000004 >>> +#define DAVINCI_KEYPAD_INT_ON 0x00000002 >>> +#define DAVINCI_KEYPAD_INT_CHANGE 0x00000001 >>> +#define DAVINCI_KEYPAD_INT_ALL 0x0000000f >>> + >>> +struct davinci_kp { >>> + struct input_dev *input; >>> + struct davinci_kp_platform_data *pdata; >>> + int irq; >>> + void __iomem *base; >>> + resource_size_t pbase; >>> + size_t base_size; >>> +}; >>> + >>> +static void davinci_kp_write(struct davinci_kp *davinci_kp, u32 val, u32 addr) >>> +{ >>> + u32 base = (u32)davinci_kp->base; >>> + >>> + __raw_writel(val,(u32 *)(base + addr)); >>> +} >>> + >>> +static u32 davinci_kp_read(struct davinci_kp *davinci_kp, u32 addr) >>> +{ >>> + u32 base = (u32)davinci_kp->base; >>> + >>> + return __raw_readl((u32 *)(base + addr)); >>> +} >>> + >>> +/* Initializing the kp Module */ >>> +static void davinci_kp_initialize(struct davinci_kp *davinci_kp) >>> +{ >>> + u32 strobe = davinci_kp->pdata->strobe; >>> + u32 interval = davinci_kp->pdata->interval; >>> + >>> + /* Enable all interrupts */ >>> + davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, DAVINCI_KEYPAD_INTENA); >>> + >>> + /* Clear interrupts if any */ >>> + davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, DAVINCI_KEYPAD_INTCLR); >>> + >>> + /* Setup the scan period = strobe + interval */ >>> + davinci_kp_write(davinci_kp, strobe, DAVINCI_KEYPAD_STRBWIDTH); >>> + davinci_kp_write(davinci_kp, interval, DAVINCI_KEYPAD_INTERVAL); >>> + davinci_kp_write(davinci_kp, 0x01, DAVINCI_KEYPAD_CONTTIME); >>> + >>> + /* Enable Keyscan module and enable */ >>> + davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_AUTODET | DAVINCI_KEYPAD_KEYEN, >>> + DAVINCI_KEYPAD_KEYCTRL); >>> +} >>> + >>> +static irqreturn_t davinci_kp_interrupt(int irq, void *dev_id) >>> +{ >>> + struct davinci_kp *davinci_kp = dev_id; >>> + struct device *dev = &davinci_kp->input->dev; >>> + int *keymap = davinci_kp->pdata->keymap; >>> + u32 prev_status, new_status, changed, position; >>> + int keycode = KEY_UNKNOWN; >>> + int ret = IRQ_NONE; >>> + >>> + /* Disable interrupt */ >>> + davinci_kp_write(davinci_kp, 0x0, DAVINCI_KEYPAD_INTENA); >>> + >>> + /* Reading previous and new status of the keypad */ >>> + prev_status = davinci_kp_read(davinci_kp, DAVINCI_KEYPAD_PREVSTATE); >>> + new_status = davinci_kp_read(davinci_kp, DAVINCI_KEYPAD_CURRENTST); >>> + >>> + changed = prev_status ^ new_status; >>> + position = ffs(changed) - 1; >>> + >>> + if (changed) { >> >> Can there be several buttons that change status at once? > [MA] It is not suppose to change several buttons at once. "Not supposed to happen" vs. "it can not happen, even in theory"? >> >>> + keycode = keymap[position]; >>> + if((new_status >> position) & 0x1) { >> >> bool release = (new_status >> position) & 0x1; >> input_report_key(davinci_kp->input, keycode, !release); >> dev_dbg(dev, "davinci_keypad: key %d %s\n", >> keycode, release ? "released" : "pressed"); >> > [MA] Ok I'll try that. >> is shorter. >> >>> + /* Report release */ >>> + dev_dbg(dev, "davinci_keypad: key %d released\n", >>> + keycode); >>> + input_report_key(davinci_kp->input, keycode, 0); >>> + } else { >>> + /* Report press */ >>> + dev_dbg(dev, "davinci_keypad: key %d pressed\n", >>> + keycode); >>> + input_report_key(davinci_kp->input, keycode, 1); >>> + } >>> + input_sync(davinci_kp->input); >>> + ret = IRQ_HANDLED; >>> + } >>> + >>> + /* Clearing interrupt */ >>> + davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, DAVINCI_KEYPAD_INTCLR); >> >> You return IRQ_HANDLED only if keypad state changed but clear interrupt >> regardless. This is suspicious. > [MA] Ok. I'll clear the irq status only if IRQ_HANDLED. It really depends... What if key was pressed but is released before interrupt is handled? Do you want to see "spurious IRQ" warnings? >> >>> + >>> + /* Enable interrupts */ >>> + davinci_kp_write(davinci_kp, 0x1, DAVINCI_KEYPAD_INTENA); >>> + >>> + return ret; >>> +} >>> + >>> +static int __init davinci_kp_probe(struct platform_device *pdev) >>> +{ >>> + struct davinci_kp *davinci_kp; >>> + struct input_dev *key_dev; >>> + struct resource *res, *mem; >>> + int ret, i; >>> + struct device * dev = &pdev->dev; >>> + struct davinci_kp_platform_data *pdata = pdev->dev.platform_data; >>> + >>> + dev_info(dev, "DaVinci Keypad Driver\n"); >>> + >>> + if (!pdata->keymap) { >>> + dev_dbg(dev, "no keymap from pdata\n"); >>> + return -EINVAL; >>> + } >>> + >>> + davinci_kp = kzalloc(sizeof *davinci_kp, GFP_KERNEL); >>> + if(!davinci_kp) { >>> + dev_dbg(dev, "could not allocate memory for private data\n"); >>> + return -ENOMEM; >>> + } >>> + >>> + key_dev = input_allocate_device(); >>> + if (!key_dev) { >>> + dev_dbg(dev, "could not allocate input device\n"); >>> + ret = -ENOMEM; >>> + goto fail1; >>> + } >>> + >>> + platform_set_drvdata(pdev, davinci_kp); >>> + >>> + davinci_kp->input = key_dev; >>> + >>> + davinci_kp->irq = platform_get_irq(pdev, 0); >>> + if (davinci_kp->irq < 0) { >>> + dev_err(dev, "no keypad irq\n"); >>> + ret = davinci_kp->irq; >>> + goto fail2; >>> + } >>> + >>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>> + if (!res) { >>> + dev_err(dev, "no mem resource\n"); >>> + ret = -ENODEV; >> >> -EINVAL I'd say. > [MA] platform_get_resource should fail with -ENODEV. If you fail to get platform resource then th eplatform code is set up incorrectly, therefore I'd still argue for -EINVAL. It is not as if device may not be there because then the platform device would not be present at all. >> >>> + goto fail2; >>> + } >>> + >>> + davinci_kp->pbase = res->start; >>> + davinci_kp->base_size = resource_size(res); >>> + >>> + mem = request_mem_region(davinci_kp->pbase, davinci_kp->base_size, pdev->name); >>> + if (!mem) { >>> + dev_err(dev, "KEYSCAN registers at %08x are not free\n", >>> + davinci_kp->pbase); >>> + ret = -EBUSY; >>> + goto fail2; >>> + } >>> + >>> + davinci_kp->base = ioremap(davinci_kp->pbase, davinci_kp->base_size); >>> + if (!davinci_kp->base) { >>> + dev_err(dev, "can't ioremap MEM resource.\n"); >>> + ret = -ENOMEM; >>> + goto fail3; >>> + } >>> + >>> + /* Enable auto repeat feature of Linux input subsystem */ >>> + if (pdata->rep) >>> + __set_bit(EV_REP, key_dev->evbit); >>> + >>> + /* Setup input device */ >>> + __set_bit(EV_KEY, key_dev->evbit); >>> + >>> + /* Setup the keymap */ >>> + davinci_kp->pdata = pdata; >>> + >>> + for (i = 0; i < davinci_kp->pdata->keymapsize; i++) >>> + __set_bit(davinci_kp->pdata->keymap[i], key_dev->keybit); >>> + >>> + key_dev->name = "davinci_keypad"; >>> + key_dev->phys = "davinci_keypad/input0"; >>> + key_dev->dev.parent = &pdev->dev; >>> + key_dev->id.bustype = BUS_HOST; >>> + key_dev->id.vendor = 0x0001; >>> + key_dev->id.product = 0x0001; >>> + key_dev->id.version = 0x0001; >>> + key_dev->keycode = davinci_kp->pdata->keymap; >> >> Please kopy keymap into the davinci_kp stucture and use it so that >> platform data is never changed and can be declared const. > Do you mean something like this? > > struct davinci_kp { > ... > const int *keymap; > ... > }; > More like: struct davinci_kp { ... unsgned char keymap[]; ... }; and then you copy keymap from platform data into device-private structure so it can be safely changed via EVIOCSKEYCODE without affecting platform data. Also, if you reload the module, the keymap with be restored to its original state. The platform data may also be marked as const. If max size of keymap is not known then you may need to allocate it dynamically. >> >>> + key_dev->keycodesize = sizeof(unsigned int); >> >> sizeof(davinci_kp->keymap[0]) is safer. Plus make it unsigned short. > [MA] Ok. >> >>> + key_dev->keycodemax = davinci_kp->pdata->keymapsize; >>> + >>> + ret = input_register_device(davinci_kp->input); >>> + if (ret < 0) { >>> + dev_err(dev, "unable to register DaVinci keypad device\n"); >>> + goto fail4; >>> + } >>> + >>> + ret = request_irq(davinci_kp->irq, davinci_kp_interrupt, IRQF_DISABLED, >>> + "davinci_keypad", davinci_kp); >>> + if (ret < 0) { >>> + dev_err(dev, "unable to register DaVinci keypad Interrupt\n"); >>> + goto fail5; >>> + } >>> + >>> + davinci_kp_initialize(davinci_kp); >>> + >>> + return 0; >>> +fail5: >>> + input_unregister_device(davinci_kp->input); >>> + key_dev = NULL; >>> +fail4: >>> + iounmap(davinci_kp->base); >>> +fail3: >>> + release_mem_region(davinci_kp->pbase, davinci_kp->base_size); >>> +fail2: >>> + input_free_device(key_dev); >>> +fail1: >>> + kfree(davinci_kp); >>> + >>> + return ret; >>> +} >>> + >>> +static int __exit davinci_kp_remove(struct platform_device *pdev) >> >> __devexit? > [MA] According to comments from David Brownell to the first version of > this patch the __exit should be used. > > " - Use platform_driver_probe() and __exit/__exit_p(); > there's no point in keeping that code around in > typical configs, it'd just waste memory. " I am afraid David is wrong here. Even when we register driver with platform_driver_probe() we still have "unbind" attribute in sysfs which may be used to unbind the device from driver. If code is __exit then such attempts will cause oops. > >> >>> +{ >>> + struct davinci_kp *davinci_kp = platform_get_drvdata(pdev); >>> + >>> + free_irq(davinci_kp->irq, davinci_kp); >>> + >>> + input_unregister_device(davinci_kp->input); >>> + >>> + iounmap(davinci_kp->base); >>> + release_mem_region(davinci_kp->pbase, davinci_kp->base_size); >>> + >>> + platform_set_drvdata(pdev, NULL); >>> + >>> + kfree(davinci_kp); >>> + >>> + return 0; >>> +} >>> + >>> +static struct platform_driver davinci_kp_driver = { >>> + .driver = { >>> + .name = "davinci_keypad", >>> + .owner = THIS_MODULE, >>> + }, >>> + .remove = __exit_p(davinci_kp_remove), >> >> __devexit_p(). I think you can still unbind the device even if you use >> platform_driver_probe. > [MA] Same. Right, see above. >> >>> +}; >>> + >>> +static int __init davinci_kp_init(void) >>> +{ >>> + return platform_driver_probe(&davinci_kp_driver, davinci_kp_probe); >>> +} >>> +module_init(davinci_kp_init); >>> + >>> +static void __exit davinci_kp_exit(void) >>> +{ >>> + platform_driver_unregister(&davinci_kp_driver); >>> +} >>> +module_exit(davinci_kp_exit); >>> + >>> +MODULE_AUTHOR("Miguel Aguilar"); >>> +MODULE_DESCRIPTION("Texas Instruments DaVinci Keypad Driver"); >>> +MODULE_LICENSE("GPL"); >>> -- >>> 1.6.0.4 >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-input" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > >
Dmitry, Dmitry Torokhov wrote: > Hi Miguel, > > [Adding David to CC for the __devexit/__exit discussion] > > On Wed, Sep 23, 2009 at 08:52:40AM -0600, Miguel Aguilar wrote: >> Dmitry, >> >> Dmitry Torokhov wrote: >>> Hi Miguel, >>> >>> On Tue, Sep 22, 2009 at 03:27:30PM -0600, miguel.aguilar@ridgerun.com wrote: >>>> From: Miguel Aguilar <miguel.aguilar@ridgerun.com> >>>> >>>> Adds the driver for enabling keypad support for DaVinci platforms. >>>> >>>> DM365 is the only platform that uses this driver at the moment. >>>> >>>> This driver was tested on DM365 EVM rev C. >>>> >>>> Signed-off-by: Miguel Aguilar <miguel.aguilar@ridgerun.com> >>>> --- >>>> arch/arm/configs/davinci_all_defconfig | 1 + >>>> arch/arm/mach-davinci/include/mach/keypad.h | 35 +++ >>>> drivers/input/keyboard/Kconfig | 7 + >>>> drivers/input/keyboard/Makefile | 1 + >>>> drivers/input/keyboard/davinci_keypad.c | 319 +++++++++++++++++++++++++++ >>>> 5 files changed, 363 insertions(+), 0 deletions(-) >>>> create mode 100644 arch/arm/mach-davinci/include/mach/keypad.h >>>> create mode 100644 drivers/input/keyboard/davinci_keypad.c >>>> >>>> diff --git a/arch/arm/configs/davinci_all_defconfig b/arch/arm/configs/davinci_all_defconfig >>>> index ec63c15..e994c83 100644 >>>> --- a/arch/arm/configs/davinci_all_defconfig >>>> +++ b/arch/arm/configs/davinci_all_defconfig >>>> @@ -763,6 +763,7 @@ CONFIG_KEYBOARD_GPIO=y >>>> # CONFIG_KEYBOARD_STOWAWAY is not set >>>> # CONFIG_KEYBOARD_SUNKBD is not set >>>> CONFIG_KEYBOARD_XTKBD=m >>>> +CONFIG_KEYBOARD_DAVINCI_DM365=m >>>> # CONFIG_INPUT_MOUSE is not set >>>> # CONFIG_INPUT_JOYSTICK is not set >>>> # CONFIG_INPUT_TABLET is not set >>> >>> If you want to merge the driver through input tree the defconfig chunk >>> has to go elsewhere. >> [MA] So, Where it should go? > > In a separate patch submitted to the person maintaining defconfig for > the arch in question once driver is in mainline. [MA] Ok I see. > >>>> diff --git a/arch/arm/mach-davinci/include/mach/keypad.h b/arch/arm/mach-davinci/include/mach/keypad.h >>>> new file mode 100644 >>>> index 0000000..922d20e >>>> --- /dev/null >>>> +++ b/arch/arm/mach-davinci/include/mach/keypad.h >>>> @@ -0,0 +1,35 @@ >>>> +/* >>>> + * Copyright (C) 2009 Texas Instruments, Inc >>>> + * >>>> + * Author: Miguel Aguilar <miguel.aguilar@ridgerun.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; either version 2 of the License, or >>>> + * (at your option) any later version. >>>> + * >>>> + * This program is distributed in the hope that it will be useful, >>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>>> + * GNU General Public License for more details. >>>> + * >>>> + * You should have received a copy of the GNU General Public License >>>> + * along with this program; if not, write to the Free Software >>>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA >>>> + */ >>>> + >>>> +#ifndef DAVINCI_KEYPAD_H >>>> +#define DAVINCI_KEYPAD_H >>>> + >>>> +#include <linux/io.h> >>>> + >>>> +struct davinci_kp_platform_data { >>>> + int *keymap; >>>> + u32 keymapsize; >>>> + u32 rep:1; >>>> + u32 strobe; >>>> + u32 interval; >>>> +}; >>>> + >>>> +#endif >>>> + >>>> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig >>>> index a6b989a..b6b9517 100644 >>>> --- a/drivers/input/keyboard/Kconfig >>>> +++ b/drivers/input/keyboard/Kconfig >>>> @@ -361,4 +361,11 @@ config KEYBOARD_XTKBD >>>> To compile this driver as a module, choose M here: the >>>> module will be called xtkbd. >>>> +config KEYBOARD_DAVINCI >>>> + tristate "TI DaVinci Keypad" >>>> + depends on ARCH_DAVINCI_DM365 >>>> + help >>>> + Say Y to enable keypad module support for the TI DaVinci >>>> + platforms (DM365) >>>> + >>> "To compile this driver as a module..." >> [MA] Ok. >>>> endif >>>> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile >>>> index b5b5eae..0b0274e 100644 >>>> --- a/drivers/input/keyboard/Makefile >>>> +++ b/drivers/input/keyboard/Makefile >>>> @@ -31,3 +31,4 @@ obj-$(CONFIG_KEYBOARD_STOWAWAY) += stowaway.o >>>> obj-$(CONFIG_KEYBOARD_SUNKBD) += sunkbd.o >>>> obj-$(CONFIG_KEYBOARD_TOSA) += tosakbd.o >>>> obj-$(CONFIG_KEYBOARD_XTKBD) += xtkbd.o >>>> +obj-$(CONFIG_KEYBOARD_DAVINCI) += davinci_keypad.o >>>> diff --git a/drivers/input/keyboard/davinci_keypad.c b/drivers/input/keyboard/davinci_keypad.c >>>> new file mode 100644 >>>> index 0000000..6f0e793 >>>> --- /dev/null >>>> +++ b/drivers/input/keyboard/davinci_keypad.c >>>> @@ -0,0 +1,319 @@ >>>> +/* >>>> + * DaVinci Keypad Driver for TI platforms >>>> + * >>>> + * Copyright (C) 2009 Texas Instruments, Inc >>>> + * >>>> + * Author: Miguel Aguilar <miguel.aguilar@ridgerun.com> >>>> + * >>>> + * Intial Code: Sandeep Paulraj <s-paulraj@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; either version 2 of the License, or >>>> + * (at your option) any later version. >>>> + * >>>> + * This program is distributed in the hope that it will be useful, >>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>>> + * GNU General Public License for more details. >>>> + * >>>> + * You should have received a copy of the GNU General Public License >>>> + * along with this program; if not, write to the Free Software >>>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA >>>> + */ >>>> +#include <linux/module.h> >>>> +#include <linux/init.h> >>>> +#include <linux/interrupt.h> >>>> +#include <linux/types.h> >>>> +#include <linux/input.h> >>>> +#include <linux/kernel.h> >>>> +#include <linux/delay.h> >>>> +#include <linux/platform_device.h> >>>> +#include <linux/errno.h> >>>> + >>>> +#include <asm/irq.h> >>>> + >>>> +#include <mach/hardware.h> >>>> +#include <mach/irqs.h> >>>> +#include <mach/keypad.h> >>>> + >>>> +/* Keypad registers */ >>>> +#define DAVINCI_KEYPAD_KEYCTRL 0x0000 >>>> +#define DAVINCI_KEYPAD_INTENA 0x0004 >>>> +#define DAVINCI_KEYPAD_INTFLAG 0x0008 >>>> +#define DAVINCI_KEYPAD_INTCLR 0x000c >>>> +#define DAVINCI_KEYPAD_STRBWIDTH 0x0010 >>>> +#define DAVINCI_KEYPAD_INTERVAL 0x0014 >>>> +#define DAVINCI_KEYPAD_CONTTIME 0x0018 >>>> +#define DAVINCI_KEYPAD_CURRENTST 0x001c >>>> +#define DAVINCI_KEYPAD_PREVSTATE 0x0020 >>>> +#define DAVINCI_KEYPAD_EMUCTRL 0x0024 >>>> +#define DAVINCI_KEYPAD_IODFTCTRL 0x002c >>>> + >>>> +/* Key Control Register (KEYCTRL) */ >>>> +#define DAVINCI_KEYPAD_KEYEN 0x00000001 >>>> +#define DAVINCI_KEYPAD_PREVMODE 0x00000002 >>>> +#define DAVINCI_KEYPAD_CHATOFF 0x00000004 >>>> +#define DAVINCI_KEYPAD_AUTODET 0x00000008 >>>> +#define DAVINCI_KEYPAD_SCANMODE 0x00000010 >>>> +#define DAVINCI_KEYPAD_OUTTYPE 0x00000020 >>>> +#define DAVINCI_KEYPAD_4X4 0x00000040 >>>> + >>>> +/* Masks for the interrupts */ >>>> +#define DAVINCI_KEYPAD_INT_CONT 0x00000008 >>>> +#define DAVINCI_KEYPAD_INT_OFF 0x00000004 >>>> +#define DAVINCI_KEYPAD_INT_ON 0x00000002 >>>> +#define DAVINCI_KEYPAD_INT_CHANGE 0x00000001 >>>> +#define DAVINCI_KEYPAD_INT_ALL 0x0000000f >>>> + >>>> +struct davinci_kp { >>>> + struct input_dev *input; >>>> + struct davinci_kp_platform_data *pdata; >>>> + int irq; >>>> + void __iomem *base; >>>> + resource_size_t pbase; >>>> + size_t base_size; >>>> +}; >>>> + >>>> +static void davinci_kp_write(struct davinci_kp *davinci_kp, u32 val, u32 addr) >>>> +{ >>>> + u32 base = (u32)davinci_kp->base; >>>> + >>>> + __raw_writel(val,(u32 *)(base + addr)); >>>> +} >>>> + >>>> +static u32 davinci_kp_read(struct davinci_kp *davinci_kp, u32 addr) >>>> +{ >>>> + u32 base = (u32)davinci_kp->base; >>>> + >>>> + return __raw_readl((u32 *)(base + addr)); >>>> +} >>>> + >>>> +/* Initializing the kp Module */ >>>> +static void davinci_kp_initialize(struct davinci_kp *davinci_kp) >>>> +{ >>>> + u32 strobe = davinci_kp->pdata->strobe; >>>> + u32 interval = davinci_kp->pdata->interval; >>>> + >>>> + /* Enable all interrupts */ >>>> + davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, DAVINCI_KEYPAD_INTENA); >>>> + >>>> + /* Clear interrupts if any */ >>>> + davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, DAVINCI_KEYPAD_INTCLR); >>>> + >>>> + /* Setup the scan period = strobe + interval */ >>>> + davinci_kp_write(davinci_kp, strobe, DAVINCI_KEYPAD_STRBWIDTH); >>>> + davinci_kp_write(davinci_kp, interval, DAVINCI_KEYPAD_INTERVAL); >>>> + davinci_kp_write(davinci_kp, 0x01, DAVINCI_KEYPAD_CONTTIME); >>>> + >>>> + /* Enable Keyscan module and enable */ >>>> + davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_AUTODET | DAVINCI_KEYPAD_KEYEN, >>>> + DAVINCI_KEYPAD_KEYCTRL); >>>> +} >>>> + >>>> +static irqreturn_t davinci_kp_interrupt(int irq, void *dev_id) >>>> +{ >>>> + struct davinci_kp *davinci_kp = dev_id; >>>> + struct device *dev = &davinci_kp->input->dev; >>>> + int *keymap = davinci_kp->pdata->keymap; >>>> + u32 prev_status, new_status, changed, position; >>>> + int keycode = KEY_UNKNOWN; >>>> + int ret = IRQ_NONE; >>>> + >>>> + /* Disable interrupt */ >>>> + davinci_kp_write(davinci_kp, 0x0, DAVINCI_KEYPAD_INTENA); >>>> + >>>> + /* Reading previous and new status of the keypad */ >>>> + prev_status = davinci_kp_read(davinci_kp, DAVINCI_KEYPAD_PREVSTATE); >>>> + new_status = davinci_kp_read(davinci_kp, DAVINCI_KEYPAD_CURRENTST); >>>> + >>>> + changed = prev_status ^ new_status; >>>> + position = ffs(changed) - 1; >>>> + >>>> + if (changed) { >>> Can there be several buttons that change status at once? >> [MA] It is not suppose to change several buttons at once. > > "Not supposed to happen" vs. "it can not happen, even in theory"? [MA]According with the TI documentation the scan process and the way of how the IRQ is handle, expects one key at time. > >>>> + keycode = keymap[position]; >>>> + if((new_status >> position) & 0x1) { >>> bool release = (new_status >> position) & 0x1; >>> input_report_key(davinci_kp->input, keycode, !release); >>> dev_dbg(dev, "davinci_keypad: key %d %s\n", >>> keycode, release ? "released" : "pressed"); >>> >> [MA] Ok I'll try that. >>> is shorter. >>> >>>> + /* Report release */ >>>> + dev_dbg(dev, "davinci_keypad: key %d released\n", >>>> + keycode); >>>> + input_report_key(davinci_kp->input, keycode, 0); >>>> + } else { >>>> + /* Report press */ >>>> + dev_dbg(dev, "davinci_keypad: key %d pressed\n", >>>> + keycode); >>>> + input_report_key(davinci_kp->input, keycode, 1); >>>> + } >>>> + input_sync(davinci_kp->input); >>>> + ret = IRQ_HANDLED; >>>> + } >>>> + >>>> + /* Clearing interrupt */ >>>> + davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, DAVINCI_KEYPAD_INTCLR); >>> You return IRQ_HANDLED only if keypad state changed but clear interrupt >>> regardless. This is suspicious. >> [MA] Ok. I'll clear the irq status only if IRQ_HANDLED. > > It really depends... What if key was pressed but is released before > interrupt is handled? Do you want to see "spurious IRQ" warnings? > So, what is the proper way to clear the interrupt in this particular case? >>>> + >>>> + /* Enable interrupts */ >>>> + davinci_kp_write(davinci_kp, 0x1, DAVINCI_KEYPAD_INTENA); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static int __init davinci_kp_probe(struct platform_device *pdev) >>>> +{ >>>> + struct davinci_kp *davinci_kp; >>>> + struct input_dev *key_dev; >>>> + struct resource *res, *mem; >>>> + int ret, i; >>>> + struct device * dev = &pdev->dev; >>>> + struct davinci_kp_platform_data *pdata = pdev->dev.platform_data; >>>> + >>>> + dev_info(dev, "DaVinci Keypad Driver\n"); >>>> + >>>> + if (!pdata->keymap) { >>>> + dev_dbg(dev, "no keymap from pdata\n"); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + davinci_kp = kzalloc(sizeof *davinci_kp, GFP_KERNEL); >>>> + if(!davinci_kp) { >>>> + dev_dbg(dev, "could not allocate memory for private data\n"); >>>> + return -ENOMEM; >>>> + } >>>> + >>>> + key_dev = input_allocate_device(); >>>> + if (!key_dev) { >>>> + dev_dbg(dev, "could not allocate input device\n"); >>>> + ret = -ENOMEM; >>>> + goto fail1; >>>> + } >>>> + >>>> + platform_set_drvdata(pdev, davinci_kp); >>>> + >>>> + davinci_kp->input = key_dev; >>>> + >>>> + davinci_kp->irq = platform_get_irq(pdev, 0); >>>> + if (davinci_kp->irq < 0) { >>>> + dev_err(dev, "no keypad irq\n"); >>>> + ret = davinci_kp->irq; >>>> + goto fail2; >>>> + } >>>> + >>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>> + if (!res) { >>>> + dev_err(dev, "no mem resource\n"); >>>> + ret = -ENODEV; >>> -EINVAL I'd say. >> [MA] platform_get_resource should fail with -ENODEV. > > If you fail to get platform resource then th eplatform code is set up > incorrectly, therefore I'd still argue for -EINVAL. It is not as if > device may not be there because then the platform device would not be > present at all. [MA] It is weird since most of the drivers use -ENODEV. I addressed the Sergei comments in this sence: " > What are the proper error codes when platform_get_resource, -ENODEV. > request_mem_region -EBUSY. > and ioremap functions fail?. -ENOMEM. " > >>>> + goto fail2; >>>> + } >>>> + >>>> + davinci_kp->pbase = res->start; >>>> + davinci_kp->base_size = resource_size(res); >>>> + >>>> + mem = request_mem_region(davinci_kp->pbase, davinci_kp->base_size, pdev->name); >>>> + if (!mem) { >>>> + dev_err(dev, "KEYSCAN registers at %08x are not free\n", >>>> + davinci_kp->pbase); >>>> + ret = -EBUSY; >>>> + goto fail2; >>>> + } >>>> + >>>> + davinci_kp->base = ioremap(davinci_kp->pbase, davinci_kp->base_size); >>>> + if (!davinci_kp->base) { >>>> + dev_err(dev, "can't ioremap MEM resource.\n"); >>>> + ret = -ENOMEM; >>>> + goto fail3; >>>> + } >>>> + >>>> + /* Enable auto repeat feature of Linux input subsystem */ >>>> + if (pdata->rep) >>>> + __set_bit(EV_REP, key_dev->evbit); >>>> + >>>> + /* Setup input device */ >>>> + __set_bit(EV_KEY, key_dev->evbit); >>>> + >>>> + /* Setup the keymap */ >>>> + davinci_kp->pdata = pdata; >>>> + >>>> + for (i = 0; i < davinci_kp->pdata->keymapsize; i++) >>>> + __set_bit(davinci_kp->pdata->keymap[i], key_dev->keybit); >>>> + >>>> + key_dev->name = "davinci_keypad"; >>>> + key_dev->phys = "davinci_keypad/input0"; >>>> + key_dev->dev.parent = &pdev->dev; >>>> + key_dev->id.bustype = BUS_HOST; >>>> + key_dev->id.vendor = 0x0001; >>>> + key_dev->id.product = 0x0001; >>>> + key_dev->id.version = 0x0001; >>>> + key_dev->keycode = davinci_kp->pdata->keymap; >>> Please kopy keymap into the davinci_kp stucture and use it so that >>> platform data is never changed and can be declared const. >> Do you mean something like this? >> >> struct davinci_kp { >> ... >> const int *keymap; >> ... >> }; >> > > More like: > > struct davinci_kp { > ... > unsgned char keymap[]; > ... > }; > [MA] Ok. > and then you copy keymap from platform data into device-private > structure so it can be safely changed via EVIOCSKEYCODE without > affecting platform data. Also, if you reload the module, the keymap with > be restored to its original state. The platform data may also be marked > as const. [MA] you mean add const in the plaform data of the davinci_kp structure. > > If max size of keymap is not known then you may need to allocate it > dynamically. > >>>> + key_dev->keycodesize = sizeof(unsigned int); >>> sizeof(davinci_kp->keymap[0]) is safer. Plus make it unsigned short. >> [MA] Ok. >>>> + key_dev->keycodemax = davinci_kp->pdata->keymapsize; >>>> + >>>> + ret = input_register_device(davinci_kp->input); >>>> + if (ret < 0) { >>>> + dev_err(dev, "unable to register DaVinci keypad device\n"); >>>> + goto fail4; >>>> + } >>>> + >>>> + ret = request_irq(davinci_kp->irq, davinci_kp_interrupt, IRQF_DISABLED, >>>> + "davinci_keypad", davinci_kp); >>>> + if (ret < 0) { >>>> + dev_err(dev, "unable to register DaVinci keypad Interrupt\n"); >>>> + goto fail5; >>>> + } >>>> + >>>> + davinci_kp_initialize(davinci_kp); >>>> + >>>> + return 0; >>>> +fail5: >>>> + input_unregister_device(davinci_kp->input); >>>> + key_dev = NULL; >>>> +fail4: >>>> + iounmap(davinci_kp->base); >>>> +fail3: >>>> + release_mem_region(davinci_kp->pbase, davinci_kp->base_size); >>>> +fail2: >>>> + input_free_device(key_dev); >>>> +fail1: >>>> + kfree(davinci_kp); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static int __exit davinci_kp_remove(struct platform_device *pdev) >>> __devexit? >> [MA] According to comments from David Brownell to the first version of >> this patch the __exit should be used. >> >> " - Use platform_driver_probe() and __exit/__exit_p(); >> there's no point in keeping that code around in >> typical configs, it'd just waste memory. " > > I am afraid David is wrong here. Even when we register driver with > platform_driver_probe() we still have "unbind" attribute in sysfs which > may be used to unbind the device from driver. If code is __exit then > such attempts will cause oops. [MA] Let's wait here for David response. > >>>> +{ >>>> + struct davinci_kp *davinci_kp = platform_get_drvdata(pdev); >>>> + >>>> + free_irq(davinci_kp->irq, davinci_kp); >>>> + >>>> + input_unregister_device(davinci_kp->input); >>>> + >>>> + iounmap(davinci_kp->base); >>>> + release_mem_region(davinci_kp->pbase, davinci_kp->base_size); >>>> + >>>> + platform_set_drvdata(pdev, NULL); >>>> + >>>> + kfree(davinci_kp); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static struct platform_driver davinci_kp_driver = { >>>> + .driver = { >>>> + .name = "davinci_keypad", >>>> + .owner = THIS_MODULE, >>>> + }, >>>> + .remove = __exit_p(davinci_kp_remove), >>> __devexit_p(). I think you can still unbind the device even if you use >>> platform_driver_probe. >> [MA] Same. > > Right, see above. > >>>> +}; >>>> + >>>> +static int __init davinci_kp_init(void) >>>> +{ >>>> + return platform_driver_probe(&davinci_kp_driver, davinci_kp_probe); >>>> +} >>>> +module_init(davinci_kp_init); >>>> + >>>> +static void __exit davinci_kp_exit(void) >>>> +{ >>>> + platform_driver_unregister(&davinci_kp_driver); >>>> +} >>>> +module_exit(davinci_kp_exit); >>>> + >>>> +MODULE_AUTHOR("Miguel Aguilar"); >>>> +MODULE_DESCRIPTION("Texas Instruments DaVinci Keypad Driver"); >>>> +MODULE_LICENSE("GPL"); >>>> -- >>>> 1.6.0.4 >>>> >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe linux-input" in >>>> the body of a message to majordomo@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > Thanks, Miguel Aguilar -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Miguel Aguilar wrote: > Dmitry, > > Dmitry Torokhov wrote: >> Hi Miguel, >> >> [Adding David to CC for the __devexit/__exit discussion] >> >> On Wed, Sep 23, 2009 at 08:52:40AM -0600, Miguel Aguilar wrote: >>> Dmitry, >>> >>> Dmitry Torokhov wrote: >>>> Hi Miguel, >>>> >>>> On Tue, Sep 22, 2009 at 03:27:30PM -0600, >>>> miguel.aguilar@ridgerun.com wrote: >>>>> From: Miguel Aguilar <miguel.aguilar@ridgerun.com> >>>>> >>>>> Adds the driver for enabling keypad support for DaVinci platforms. >>>>> >>>>> DM365 is the only platform that uses this driver at the moment. >>>>> >>>>> This driver was tested on DM365 EVM rev C. >>>>> >>>>> Signed-off-by: Miguel Aguilar <miguel.aguilar@ridgerun.com> >>>>> --- >>>>> arch/arm/configs/davinci_all_defconfig | 1 + >>>>> arch/arm/mach-davinci/include/mach/keypad.h | 35 +++ >>>>> drivers/input/keyboard/Kconfig | 7 + >>>>> drivers/input/keyboard/Makefile | 1 + >>>>> drivers/input/keyboard/davinci_keypad.c | 319 >>>>> +++++++++++++++++++++++++++ >>>>> 5 files changed, 363 insertions(+), 0 deletions(-) >>>>> create mode 100644 arch/arm/mach-davinci/include/mach/keypad.h >>>>> create mode 100644 drivers/input/keyboard/davinci_keypad.c >>>>> >>>>> diff --git a/arch/arm/configs/davinci_all_defconfig >>>>> b/arch/arm/configs/davinci_all_defconfig >>>>> index ec63c15..e994c83 100644 >>>>> --- a/arch/arm/configs/davinci_all_defconfig >>>>> +++ b/arch/arm/configs/davinci_all_defconfig >>>>> @@ -763,6 +763,7 @@ CONFIG_KEYBOARD_GPIO=y >>>>> # CONFIG_KEYBOARD_STOWAWAY is not set >>>>> # CONFIG_KEYBOARD_SUNKBD is not set >>>>> CONFIG_KEYBOARD_XTKBD=m >>>>> +CONFIG_KEYBOARD_DAVINCI_DM365=m >>>>> # CONFIG_INPUT_MOUSE is not set >>>>> # CONFIG_INPUT_JOYSTICK is not set >>>>> # CONFIG_INPUT_TABLET is not set >>>> >>>> If you want to merge the driver through input tree the defconfig chunk >>>> has to go elsewhere. >>> [MA] So, Where it should go? >> >> In a separate patch submitted to the person maintaining defconfig for >> the arch in question once driver is in mainline. > > [MA] Ok I see. >> >>>>> diff --git a/arch/arm/mach-davinci/include/mach/keypad.h >>>>> b/arch/arm/mach-davinci/include/mach/keypad.h >>>>> new file mode 100644 >>>>> index 0000000..922d20e >>>>> --- /dev/null >>>>> +++ b/arch/arm/mach-davinci/include/mach/keypad.h >>>>> @@ -0,0 +1,35 @@ >>>>> +/* >>>>> + * Copyright (C) 2009 Texas Instruments, Inc >>>>> + * >>>>> + * Author: Miguel Aguilar <miguel.aguilar@ridgerun.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; either version 2 of the License, or >>>>> + * (at your option) any later version. >>>>> + * >>>>> + * This program is distributed in the hope that it will be useful, >>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>>>> + * GNU General Public License for more details. >>>>> + * >>>>> + * You should have received a copy of the GNU General Public License >>>>> + * along with this program; if not, write to the Free Software >>>>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA >>>>> 02111-1307 USA >>>>> + */ >>>>> + >>>>> +#ifndef DAVINCI_KEYPAD_H >>>>> +#define DAVINCI_KEYPAD_H >>>>> + >>>>> +#include <linux/io.h> >>>>> + >>>>> +struct davinci_kp_platform_data { >>>>> + int *keymap; >>>>> + u32 keymapsize; >>>>> + u32 rep:1; >>>>> + u32 strobe; >>>>> + u32 interval; >>>>> +}; >>>>> + >>>>> +#endif >>>>> + >>>>> diff --git a/drivers/input/keyboard/Kconfig >>>>> b/drivers/input/keyboard/Kconfig >>>>> index a6b989a..b6b9517 100644 >>>>> --- a/drivers/input/keyboard/Kconfig >>>>> +++ b/drivers/input/keyboard/Kconfig >>>>> @@ -361,4 +361,11 @@ config KEYBOARD_XTKBD >>>>> To compile this driver as a module, choose M here: the >>>>> module will be called xtkbd. >>>>> +config KEYBOARD_DAVINCI >>>>> + tristate "TI DaVinci Keypad" >>>>> + depends on ARCH_DAVINCI_DM365 >>>>> + help >>>>> + Say Y to enable keypad module support for the TI DaVinci >>>>> + platforms (DM365) >>>>> + >>>> "To compile this driver as a module..." >>> [MA] Ok. >>>>> endif >>>>> diff --git a/drivers/input/keyboard/Makefile >>>>> b/drivers/input/keyboard/Makefile >>>>> index b5b5eae..0b0274e 100644 >>>>> --- a/drivers/input/keyboard/Makefile >>>>> +++ b/drivers/input/keyboard/Makefile >>>>> @@ -31,3 +31,4 @@ obj-$(CONFIG_KEYBOARD_STOWAWAY) += stowaway.o >>>>> obj-$(CONFIG_KEYBOARD_SUNKBD) += sunkbd.o >>>>> obj-$(CONFIG_KEYBOARD_TOSA) += tosakbd.o >>>>> obj-$(CONFIG_KEYBOARD_XTKBD) += xtkbd.o >>>>> +obj-$(CONFIG_KEYBOARD_DAVINCI) += davinci_keypad.o >>>>> diff --git a/drivers/input/keyboard/davinci_keypad.c >>>>> b/drivers/input/keyboard/davinci_keypad.c >>>>> new file mode 100644 >>>>> index 0000000..6f0e793 >>>>> --- /dev/null >>>>> +++ b/drivers/input/keyboard/davinci_keypad.c >>>>> @@ -0,0 +1,319 @@ >>>>> +/* >>>>> + * DaVinci Keypad Driver for TI platforms >>>>> + * >>>>> + * Copyright (C) 2009 Texas Instruments, Inc >>>>> + * >>>>> + * Author: Miguel Aguilar <miguel.aguilar@ridgerun.com> >>>>> + * >>>>> + * Intial Code: Sandeep Paulraj <s-paulraj@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; either version 2 of the License, or >>>>> + * (at your option) any later version. >>>>> + * >>>>> + * This program is distributed in the hope that it will be useful, >>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>>>> + * GNU General Public License for more details. >>>>> + * >>>>> + * You should have received a copy of the GNU General Public License >>>>> + * along with this program; if not, write to the Free Software >>>>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA >>>>> 02111-1307 USA >>>>> + */ >>>>> +#include <linux/module.h> >>>>> +#include <linux/init.h> >>>>> +#include <linux/interrupt.h> >>>>> +#include <linux/types.h> >>>>> +#include <linux/input.h> >>>>> +#include <linux/kernel.h> >>>>> +#include <linux/delay.h> >>>>> +#include <linux/platform_device.h> >>>>> +#include <linux/errno.h> >>>>> + >>>>> +#include <asm/irq.h> >>>>> + >>>>> +#include <mach/hardware.h> >>>>> +#include <mach/irqs.h> >>>>> +#include <mach/keypad.h> >>>>> + >>>>> +/* Keypad registers */ >>>>> +#define DAVINCI_KEYPAD_KEYCTRL 0x0000 >>>>> +#define DAVINCI_KEYPAD_INTENA 0x0004 >>>>> +#define DAVINCI_KEYPAD_INTFLAG 0x0008 >>>>> +#define DAVINCI_KEYPAD_INTCLR 0x000c >>>>> +#define DAVINCI_KEYPAD_STRBWIDTH 0x0010 >>>>> +#define DAVINCI_KEYPAD_INTERVAL 0x0014 >>>>> +#define DAVINCI_KEYPAD_CONTTIME 0x0018 >>>>> +#define DAVINCI_KEYPAD_CURRENTST 0x001c >>>>> +#define DAVINCI_KEYPAD_PREVSTATE 0x0020 >>>>> +#define DAVINCI_KEYPAD_EMUCTRL 0x0024 >>>>> +#define DAVINCI_KEYPAD_IODFTCTRL 0x002c >>>>> + >>>>> +/* Key Control Register (KEYCTRL) */ >>>>> +#define DAVINCI_KEYPAD_KEYEN 0x00000001 >>>>> +#define DAVINCI_KEYPAD_PREVMODE 0x00000002 >>>>> +#define DAVINCI_KEYPAD_CHATOFF 0x00000004 >>>>> +#define DAVINCI_KEYPAD_AUTODET 0x00000008 >>>>> +#define DAVINCI_KEYPAD_SCANMODE 0x00000010 >>>>> +#define DAVINCI_KEYPAD_OUTTYPE 0x00000020 >>>>> +#define DAVINCI_KEYPAD_4X4 0x00000040 >>>>> + >>>>> +/* Masks for the interrupts */ >>>>> +#define DAVINCI_KEYPAD_INT_CONT 0x00000008 >>>>> +#define DAVINCI_KEYPAD_INT_OFF 0x00000004 >>>>> +#define DAVINCI_KEYPAD_INT_ON 0x00000002 >>>>> +#define DAVINCI_KEYPAD_INT_CHANGE 0x00000001 >>>>> +#define DAVINCI_KEYPAD_INT_ALL 0x0000000f >>>>> + >>>>> +struct davinci_kp { >>>>> + struct input_dev *input; >>>>> + struct davinci_kp_platform_data *pdata; >>>>> + int irq; >>>>> + void __iomem *base; >>>>> + resource_size_t pbase; >>>>> + size_t base_size; >>>>> +}; >>>>> + >>>>> +static void davinci_kp_write(struct davinci_kp *davinci_kp, u32 >>>>> val, u32 addr) >>>>> +{ >>>>> + u32 base = (u32)davinci_kp->base; >>>>> + >>>>> + __raw_writel(val,(u32 *)(base + addr)); >>>>> +} >>>>> + >>>>> +static u32 davinci_kp_read(struct davinci_kp *davinci_kp, u32 addr) >>>>> +{ >>>>> + u32 base = (u32)davinci_kp->base; >>>>> + >>>>> + return __raw_readl((u32 *)(base + addr)); >>>>> +} >>>>> + >>>>> +/* Initializing the kp Module */ >>>>> +static void davinci_kp_initialize(struct davinci_kp *davinci_kp) >>>>> +{ >>>>> + u32 strobe = davinci_kp->pdata->strobe; >>>>> + u32 interval = davinci_kp->pdata->interval; >>>>> + >>>>> + /* Enable all interrupts */ >>>>> + davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, >>>>> DAVINCI_KEYPAD_INTENA); >>>>> + >>>>> + /* Clear interrupts if any */ >>>>> + davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, >>>>> DAVINCI_KEYPAD_INTCLR); >>>>> + >>>>> + /* Setup the scan period = strobe + interval */ >>>>> + davinci_kp_write(davinci_kp, strobe, DAVINCI_KEYPAD_STRBWIDTH); >>>>> + davinci_kp_write(davinci_kp, interval, DAVINCI_KEYPAD_INTERVAL); >>>>> + davinci_kp_write(davinci_kp, 0x01, DAVINCI_KEYPAD_CONTTIME); >>>>> + >>>>> + /* Enable Keyscan module and enable */ >>>>> + davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_AUTODET | >>>>> DAVINCI_KEYPAD_KEYEN, >>>>> + DAVINCI_KEYPAD_KEYCTRL); >>>>> +} >>>>> + >>>>> +static irqreturn_t davinci_kp_interrupt(int irq, void *dev_id) >>>>> +{ >>>>> + struct davinci_kp *davinci_kp = dev_id; >>>>> + struct device *dev = &davinci_kp->input->dev; >>>>> + int *keymap = davinci_kp->pdata->keymap; >>>>> + u32 prev_status, new_status, changed, position; >>>>> + int keycode = KEY_UNKNOWN; >>>>> + int ret = IRQ_NONE; >>>>> + >>>>> + /* Disable interrupt */ >>>>> + davinci_kp_write(davinci_kp, 0x0, DAVINCI_KEYPAD_INTENA); >>>>> + >>>>> + /* Reading previous and new status of the keypad */ >>>>> + prev_status = davinci_kp_read(davinci_kp, >>>>> DAVINCI_KEYPAD_PREVSTATE); >>>>> + new_status = davinci_kp_read(davinci_kp, >>>>> DAVINCI_KEYPAD_CURRENTST); >>>>> + >>>>> + changed = prev_status ^ new_status; >>>>> + position = ffs(changed) - 1; >>>>> + >>>>> + if (changed) { >>>> Can there be several buttons that change status at once? >>> [MA] It is not suppose to change several buttons at once. >> >> "Not supposed to happen" vs. "it can not happen, even in theory"? > [MA]According with the TI documentation the scan process and the way of > how the IRQ is handle, expects one key at time. >> >>>>> + keycode = keymap[position]; >>>>> + if((new_status >> position) & 0x1) { >>>> bool release = (new_status >> position) & 0x1; >>>> input_report_key(davinci_kp->input, keycode, !release); >>>> dev_dbg(dev, "davinci_keypad: key %d %s\n", >>>> keycode, release ? "released" : "pressed"); >>>> >>> [MA] Ok I'll try that. >>>> is shorter. >>>> >>>>> + /* Report release */ >>>>> + dev_dbg(dev, "davinci_keypad: key %d released\n", >>>>> + keycode); >>>>> + input_report_key(davinci_kp->input, keycode, 0); >>>>> + } else { >>>>> + /* Report press */ >>>>> + dev_dbg(dev, "davinci_keypad: key %d pressed\n", >>>>> + keycode); >>>>> + input_report_key(davinci_kp->input, keycode, 1); >>>>> + } >>>>> + input_sync(davinci_kp->input); >>>>> + ret = IRQ_HANDLED; >>>>> + } >>>>> + >>>>> + /* Clearing interrupt */ >>>>> + davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, >>>>> DAVINCI_KEYPAD_INTCLR); >>>> You return IRQ_HANDLED only if keypad state changed but clear interrupt >>>> regardless. This is suspicious. >>> [MA] Ok. I'll clear the irq status only if IRQ_HANDLED. >> >> It really depends... What if key was pressed but is released before >> interrupt is handled? Do you want to see "spurious IRQ" warnings? >> > So, what is the proper way to clear the interrupt in this particular case? >>>>> + >>>>> + /* Enable interrupts */ >>>>> + davinci_kp_write(davinci_kp, 0x1, DAVINCI_KEYPAD_INTENA); >>>>> + >>>>> + return ret; >>>>> +} >>>>> + >>>>> +static int __init davinci_kp_probe(struct platform_device *pdev) >>>>> +{ >>>>> + struct davinci_kp *davinci_kp; >>>>> + struct input_dev *key_dev; >>>>> + struct resource *res, *mem; >>>>> + int ret, i; >>>>> + struct device * dev = &pdev->dev; >>>>> + struct davinci_kp_platform_data *pdata = pdev->dev.platform_data; >>>>> + >>>>> + dev_info(dev, "DaVinci Keypad Driver\n"); >>>>> + >>>>> + if (!pdata->keymap) { >>>>> + dev_dbg(dev, "no keymap from pdata\n"); >>>>> + return -EINVAL; >>>>> + } >>>>> + >>>>> + davinci_kp = kzalloc(sizeof *davinci_kp, GFP_KERNEL); >>>>> + if(!davinci_kp) { >>>>> + dev_dbg(dev, "could not allocate memory for private data\n"); >>>>> + return -ENOMEM; >>>>> + } >>>>> + >>>>> + key_dev = input_allocate_device(); >>>>> + if (!key_dev) { >>>>> + dev_dbg(dev, "could not allocate input device\n"); >>>>> + ret = -ENOMEM; >>>>> + goto fail1; >>>>> + } >>>>> + >>>>> + platform_set_drvdata(pdev, davinci_kp); >>>>> + >>>>> + davinci_kp->input = key_dev; >>>>> + >>>>> + davinci_kp->irq = platform_get_irq(pdev, 0); >>>>> + if (davinci_kp->irq < 0) { >>>>> + dev_err(dev, "no keypad irq\n"); >>>>> + ret = davinci_kp->irq; >>>>> + goto fail2; >>>>> + } >>>>> + >>>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>>> + if (!res) { >>>>> + dev_err(dev, "no mem resource\n"); >>>>> + ret = -ENODEV; >>>> -EINVAL I'd say. >>> [MA] platform_get_resource should fail with -ENODEV. >> >> If you fail to get platform resource then th eplatform code is set up >> incorrectly, therefore I'd still argue for -EINVAL. It is not as if >> device may not be there because then the platform device would not be >> present at all. > [MA] It is weird since most of the drivers use -ENODEV. I addressed the > Sergei comments in this sence: > > " > > What are the proper error codes when platform_get_resource, > > -ENODEV. > > > request_mem_region > > -EBUSY. > > > and ioremap functions fail?. > > -ENOMEM. > " > >> >>>>> + goto fail2; >>>>> + } >>>>> + >>>>> + davinci_kp->pbase = res->start; >>>>> + davinci_kp->base_size = resource_size(res); >>>>> + >>>>> + mem = request_mem_region(davinci_kp->pbase, >>>>> davinci_kp->base_size, pdev->name); >>>>> + if (!mem) { >>>>> + dev_err(dev, "KEYSCAN registers at %08x are not free\n", >>>>> + davinci_kp->pbase); >>>>> + ret = -EBUSY; >>>>> + goto fail2; >>>>> + } >>>>> + >>>>> + davinci_kp->base = ioremap(davinci_kp->pbase, >>>>> davinci_kp->base_size); >>>>> + if (!davinci_kp->base) { >>>>> + dev_err(dev, "can't ioremap MEM resource.\n"); >>>>> + ret = -ENOMEM; >>>>> + goto fail3; >>>>> + } >>>>> + >>>>> + /* Enable auto repeat feature of Linux input subsystem */ >>>>> + if (pdata->rep) >>>>> + __set_bit(EV_REP, key_dev->evbit); >>>>> + >>>>> + /* Setup input device */ >>>>> + __set_bit(EV_KEY, key_dev->evbit); >>>>> + >>>>> + /* Setup the keymap */ >>>>> + davinci_kp->pdata = pdata; >>>>> + >>>>> + for (i = 0; i < davinci_kp->pdata->keymapsize; i++) >>>>> + __set_bit(davinci_kp->pdata->keymap[i], key_dev->keybit); >>>>> + >>>>> + key_dev->name = "davinci_keypad"; >>>>> + key_dev->phys = "davinci_keypad/input0"; >>>>> + key_dev->dev.parent = &pdev->dev; >>>>> + key_dev->id.bustype = BUS_HOST; >>>>> + key_dev->id.vendor = 0x0001; >>>>> + key_dev->id.product = 0x0001; >>>>> + key_dev->id.version = 0x0001; >>>>> + key_dev->keycode = davinci_kp->pdata->keymap; >>>> Please kopy keymap into the davinci_kp stucture and use it so that >>>> platform data is never changed and can be declared const. >>> Do you mean something like this? >>> >>> struct davinci_kp { >>> ... >>> const int *keymap; >>> ... >>> }; >>> >> >> More like: >> >> struct davinci_kp { >> ... >> unsgned char keymap[]; >> ... >> }; >> > [MA] Ok. [MA] Why usigned char with no pointer and not u32 as most of the keypad driver as defined? >> and then you copy keymap from platform data into device-private >> structure so it can be safely changed via EVIOCSKEYCODE without >> affecting platform data. Also, if you reload the module, the keymap with >> be restored to its original state. The platform data may also be marked >> as const. > [MA] you mean add const in the plaform data of the davinci_kp structure. >> >> If max size of keymap is not known then you may need to allocate it >> dynamically. >> >>>>> + key_dev->keycodesize = sizeof(unsigned int); >>>> sizeof(davinci_kp->keymap[0]) is safer. Plus make it unsigned short. >>> [MA] Ok. >>>>> + key_dev->keycodemax = davinci_kp->pdata->keymapsize; >>>>> + >>>>> + ret = input_register_device(davinci_kp->input); >>>>> + if (ret < 0) { >>>>> + dev_err(dev, "unable to register DaVinci keypad device\n"); >>>>> + goto fail4; >>>>> + } >>>>> + >>>>> + ret = request_irq(davinci_kp->irq, davinci_kp_interrupt, >>>>> IRQF_DISABLED, >>>>> + "davinci_keypad", davinci_kp); >>>>> + if (ret < 0) { >>>>> + dev_err(dev, "unable to register DaVinci keypad >>>>> Interrupt\n"); >>>>> + goto fail5; >>>>> + } >>>>> + >>>>> + davinci_kp_initialize(davinci_kp); >>>>> + >>>>> + return 0; >>>>> +fail5: >>>>> + input_unregister_device(davinci_kp->input); >>>>> + key_dev = NULL; >>>>> +fail4: >>>>> + iounmap(davinci_kp->base); >>>>> +fail3: >>>>> + release_mem_region(davinci_kp->pbase, davinci_kp->base_size); >>>>> +fail2: >>>>> + input_free_device(key_dev); >>>>> +fail1: >>>>> + kfree(davinci_kp); >>>>> + >>>>> + return ret; >>>>> +} >>>>> + >>>>> +static int __exit davinci_kp_remove(struct platform_device *pdev) >>>> __devexit? >>> [MA] According to comments from David Brownell to the first version >>> of this patch the __exit should be used. >>> >>> " - Use platform_driver_probe() and __exit/__exit_p(); >>> there's no point in keeping that code around in >>> typical configs, it'd just waste memory. " >> >> I am afraid David is wrong here. Even when we register driver with >> platform_driver_probe() we still have "unbind" attribute in sysfs which >> may be used to unbind the device from driver. If code is __exit then >> such attempts will cause oops. > [MA] Let's wait here for David response. >> >>>>> +{ >>>>> + struct davinci_kp *davinci_kp = platform_get_drvdata(pdev); >>>>> + >>>>> + free_irq(davinci_kp->irq, davinci_kp); >>>>> + >>>>> + input_unregister_device(davinci_kp->input); >>>>> + >>>>> + iounmap(davinci_kp->base); >>>>> + release_mem_region(davinci_kp->pbase, davinci_kp->base_size); >>>>> + >>>>> + platform_set_drvdata(pdev, NULL); >>>>> + >>>>> + kfree(davinci_kp); >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static struct platform_driver davinci_kp_driver = { >>>>> + .driver = { >>>>> + .name = "davinci_keypad", >>>>> + .owner = THIS_MODULE, >>>>> + }, >>>>> + .remove = __exit_p(davinci_kp_remove), >>>> __devexit_p(). I think you can still unbind the device even if you use >>>> platform_driver_probe. >>> [MA] Same. >> >> Right, see above. >> >>>>> +}; >>>>> + >>>>> +static int __init davinci_kp_init(void) >>>>> +{ >>>>> + return platform_driver_probe(&davinci_kp_driver, >>>>> davinci_kp_probe); >>>>> +} >>>>> +module_init(davinci_kp_init); >>>>> + >>>>> +static void __exit davinci_kp_exit(void) >>>>> +{ >>>>> + platform_driver_unregister(&davinci_kp_driver); >>>>> +} >>>>> +module_exit(davinci_kp_exit); >>>>> + >>>>> +MODULE_AUTHOR("Miguel Aguilar"); >>>>> +MODULE_DESCRIPTION("Texas Instruments DaVinci Keypad Driver"); >>>>> +MODULE_LICENSE("GPL"); >>>>> -- >>>>> 1.6.0.4 >>>>> >>>>> -- >>>>> To unsubscribe from this list: send the line "unsubscribe >>>>> linux-input" in >>>>> the body of a message to majordomo@vger.kernel.org >>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >> > > Thanks, > > Miguel Aguilar > -- > To unsubscribe from this list: send the line "unsubscribe linux-input" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 23, 2009 at 11:25:59AM -0600, Miguel Aguilar wrote: >>>>> Please kopy keymap into the davinci_kp stucture and use it so that >>>>> platform data is never changed and can be declared const. >>>> Do you mean something like this? >>>> >>>> struct davinci_kp { >>>> ... >>>> const int *keymap; >>>> ... >>>> }; >>>> >>> >>> More like: >>> >>> struct davinci_kp { >>> ... >>> unsgned char keymap[]; >>> ... >>> }; >>> >> [MA] Ok. > [MA] Why usigned char with no pointer and not u32 as most of the keypad > driver as defined? Sorry, meant to say "unsigned short keymap[...]", we not going to have more than 64K keycodes. You need to fill the array dimension, if the size is not known before hand (and if upper bound is too jigh to always allocate max) then you'll have to allocate it separately. Hm, well, if you make it the last element of the davinci_kp structure you can alwqays allocate the needed amount of memory, like this: struct davinci_kp { ... ... unsigned short keymap[]; }; ... kp = kzalloc(sizeof(struct davinci_kp) + sizeof(unsigned short) * pdata->keymap_size, GFP_KERNEL);
On Wednesday 23 September 2009, Dmitry Torokhov wrote: > >> __devexit? > > [MA] According to comments from David Brownell to the first version of > > this patch the __exit should be used. > > > > " - Use platform_driver_probe() and __exit/__exit_p(); > > Â Â there's no point in keeping that code around in > > Â Â typical configs, it'd just waste memory. " > > I am afraid David is wrong here. No, you're just pointing out a bug introduced in some unrelated code. Such bugs happen when folk ignore the code bloat issues. (And didn't Linus recently point out how such code bloat is becoming an issue?) > Even when we register driver with > platform_driver_probe() we still have "unbind" attribute in sysfs which > may be used to unbind the device from driver. If code is __exit then > such attempts will cause oops. That would be a bug in the unbind() code, which doesn't currently recognize that not every driver or bus supports hotplugging. It should probably check for a null release() pointer in the driver, and politely fail in that case. That's just about the only place that a third party (neither the driver nor its hotplug-aware bus framework) will try to decouple device and driver ... and it's doing it wrong. So it's unlikely such bugs will be elsewhere. It's *ALWAYS* been legit to have a NULL pointer in the remove() methods. That's why the __exit_p() -- or for hotpluggable drivers, __devexit_p() -- macros exist: for that particular case. - Dave -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 23, 2009 at 10:51:05AM -0700, David Brownell wrote: > On Wednesday 23 September 2009, Dmitry Torokhov wrote: > > >> __devexit? > > > [MA] According to comments from David Brownell to the first version of > > > this patch the __exit should be used. > > > > > > " - Use platform_driver_probe() and __exit/__exit_p(); > > > Â Â there's no point in keeping that code around in > > > Â Â typical configs, it'd just waste memory. " > > > > I am afraid David is wrong here. > > No, you're just pointing out a bug introduced in some > unrelated code. From the very version of the patch platform_driver_probe() only ensured that probe() would not be called after module inialization is done, it never made any guarantees about removing devices. Hmm, let's add Greg as well here. > Such bugs happen when folk ignore the > code bloat issues. (And didn't Linus recently point > out how such code bloat is becoming an issue?) > > > > Even when we register driver with > > platform_driver_probe() we still have "unbind" attribute in sysfs which > > may be used to unbind the device from driver. If code is __exit then > > such attempts will cause oops. > > That would be a bug in the unbind() code, which doesn't > currently recognize that not every driver or bus supports > hotplugging. It should probably check for a null release() > pointer in the driver, and politely fail in that case. > NULL release (as well as NULL probe) don't mean what you think they do. > That's just about the only place that a third party > (neither the driver nor its hotplug-aware bus framework) > will try to decouple device and driver ... and it's doing > it wrong. So it's unlikely such bugs will be elsewhere. > No, it does exactly what it is supposed to do. You may argue that such facility is not needed but this is different. I'd argue that sometimes you do need to shut off a device even if it is not normally hot-pluggrable. Plus most of the blat is coming from probe() functions, remove()s are fairly small. > It's *ALWAYS* been legit to have a NULL pointer in the > remove() methods. Which means "device does not need any special actions for unbinding", not that device can not be unbound. The same with probe(): NULL does not mean that device can not be bound but rather that it does not need any specal processing during binding. > That's why the __exit_p() -- or for > hotpluggable drivers, __devexit_p() -- macros exist: > for that particular case. Huh? They are here so that the linker/module loader can discard them if they only referenced via driver pointers and we know they not going to be called.
Dmitry Torokhov wrote: > On Wed, Sep 23, 2009 at 11:25:59AM -0600, Miguel Aguilar wrote: >>>>>> Please kopy keymap into the davinci_kp stucture and use it so that >>>>>> platform data is never changed and can be declared const. >>>>> Do you mean something like this? >>>>> >>>>> struct davinci_kp { >>>>> ... >>>>> const int *keymap; >>>>> ... >>>>> }; >>>>> >>>> More like: >>>> >>>> struct davinci_kp { >>>> ... >>>> unsgned char keymap[]; >>>> ... >>>> }; >>>> >>> [MA] Ok. >> [MA] Why usigned char with no pointer and not u32 as most of the keypad >> driver as defined? > > Sorry, meant to say "unsigned short keymap[...]", we not going to have > more than 64K keycodes. You need to fill the array dimension, if the > size is not known before hand (and if upper bound is too jigh to always > allocate max) then you'll have to allocate it separately. Hm, well, if > you make it the last element of the davinci_kp structure you can alwqays > allocate the needed amount of memory, like this: > > struct davinci_kp { > ... > ... > unsigned short keymap[]; > }; > > ... > > kp = kzalloc(sizeof(struct davinci_kp) + > sizeof(unsigned short) * pdata->keymap_size, > GFP_KERNEL); > Why do I need to use keymap[]; instead of *keymap? If I use keypad[] how is the proper way to assign the platform keymap to the private keymap of the driver? -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 23, 2009 at 12:15:24PM -0600, Miguel Aguilar wrote: > Dmitry Torokhov wrote: >> On Wed, Sep 23, 2009 at 11:25:59AM -0600, Miguel Aguilar wrote: >>>>>>> Please kopy keymap into the davinci_kp stucture and use it so that >>>>>>> platform data is never changed and can be declared const. >>>>>> Do you mean something like this? >>>>>> >>>>>> struct davinci_kp { >>>>>> ... >>>>>> const int *keymap; >>>>>> ... >>>>>> }; >>>>>> >>>>> More like: >>>>> >>>>> struct davinci_kp { >>>>> ... >>>>> unsgned char keymap[]; >>>>> ... >>>>> }; >>>>> >>>> [MA] Ok. >>> [MA] Why usigned char with no pointer and not u32 as most of the >>> keypad driver as defined? >> >> Sorry, meant to say "unsigned short keymap[...]", we not going to have >> more than 64K keycodes. You need to fill the array dimension, if the >> size is not known before hand (and if upper bound is too jigh to always >> allocate max) then you'll have to allocate it separately. Hm, well, if >> you make it the last element of the davinci_kp structure you can alwqays >> allocate the needed amount of memory, like this: >> >> struct davinci_kp { >> ... >> ... >> unsigned short keymap[]; >> }; >> >> ... >> >> kp = kzalloc(sizeof(struct davinci_kp) + >> sizeof(unsigned short) * pdata->keymap_size, >> GFP_KERNEL); >> > > Why do I need to use keymap[]; instead of *keymap? If I use keypad[] how > is the proper way to assign the platform keymap to the private keymap of > the driver? memcpy()
On Wednesday 23 September 2009, Dmitry Torokhov wrote: > On Wed, Sep 23, 2009 at 10:51:05AM -0700, David Brownell wrote: > > On Wednesday 23 September 2009, Dmitry Torokhov wrote: > > > >> __devexit? > > > > [MA] According to comments from David Brownell to the first version of > > > > this patch the __exit should be used. > > > > > > > > " - Use platform_driver_probe() and __exit/__exit_p(); > > > > Â Â there's no point in keeping that code around in > > > > Â Â typical configs, it'd just waste memory. " > > > > > > I am afraid David is wrong here. > > > > No, you're just pointing out a bug introduced in some > > unrelated code. > > From the very version of the patch platform_driver_probe() only ensured > that probe() would not be called after module inialization is done, it > never made any guarantees about removing devices. Hmm, let's add Greg as > well here. I don't see your point. If the driver doesn't support any unbinding mechanism -- a remove() method, converse of probe() -- then it can't safely be unbound. And thus, if any code is presuming that *every* driver can be unbound, it's wrong. As I said: bug in other code. Looking at this a bit more, it seems like there will need to be some "can this bus remove this driver" check, since the struct device.remove method is now managed at the bus level. Easy enough to do instead of the null check that I mentioned below. Provide it for platform bus, and the main potential trouble spots will be resolved. > > Such bugs happen when folk ignore the > > code bloat issues. (And didn't Linus recently point > > out how such code bloat is becoming an issue?) Though to be fair, it's *always* been an issue for anyone working with embedded Linux systems. And it's long been a frustration that too few "mainline" developers didn't accept such issues. > > > Even when we register driver with > > > platform_driver_probe() we still have "unbind" attribute in sysfs which > > > may be used to unbind the device from driver. If code is __exit then > > > such attempts will cause oops. > > > > That would be a bug in the unbind() code, which doesn't > > currently recognize that not every driver or bus supports > > hotplugging. It should probably check for a null release() > > pointer in the driver, and politely fail in that case. > > > > NULL release (as well as NULL probe) don't mean what you think they do. I could say the same things about how you think, of course, and probably with just as little accuracy. Let's not be needlessly confrontational. > > That's just about the only place that a third party > > (neither the driver nor its hotplug-aware bus framework) > > will try to decouple device and driver ... and it's doing > > it wrong. So it's unlikely such bugs will be elsewhere. > > > > No, it does exactly what it is supposed to do. You may argue that such > facility is not needed but this is different. I'd argue that sometimes > you do need to shut off a device even if it is not normally > hot-pluggrable. Erm, once a driver is freely poking around in the hardware, and is managing IRQs and other asynch events ... exactly how do you expect to decouple it from the hardware without a remove()? Drivers that *don't* respond asynchronously to hardware events are an extreme minority; it's not worth even considering them as the design center. As a policy choice, there's only one sane one: unbinding the driver always requires an active ack from the driver. Even if that's a trivial/NOP remove() function ... instead of one which cleanly tears down the hardware event stream; the driver's handling thereof; and whatever resources the driver claimed, including links to upper level code in the operating system (like char/block device nodes and many other things). > Plus most of the blat is coming from probe() functions, > remove()s are fairly small. Not always. Of course, there's still that nasty issue with code that probe() and remove() need to share ... for now there's no clean way to have such code be removable. (Like an "__init_or_exit" section annotation.) Drivers have been stuck putting such code in the .text segement, and thus causing additional driver bloat. > > It's *ALWAYS* been legit to have a NULL pointer in the > > remove() methods. > > Which means "device does not need any special actions for unbinding", > not that device can not be unbound. The same with probe(): NULL does not > mean that device can not be bound but rather that it does not need any > specal processing during binding. I don't know where such a model comes from. I'll just say that in quite a few years of driver development I have *never* come across hardware where "doesn't need any attention during driver unbind" makes sense. Recall that any driver has both lower (to hardware) and upper (to rest of OS) links, and maybe more (to other drivers) ... and it's a design goal in Linux to have such links be explicit, so the drivers are more reusable. So requiring some hook -- remove() -- before attempting to break the links internal to the driver model seems like a fairly obvious requirement. > > That's why the __exit_p() -- or for > > hotpluggable drivers, __devexit_p() -- macros exist: > > for that particular case. > > Huh? They are here so that the linker/module loader can discard them if > they only referenced via driver pointers and we know they not going to > be called. Finish that thought. What "exit" section code fits those categories? Driver remove() methods ... and not much else. And for that matter, what else goes into exit sections, other than such code and module hooks to call it? That discarding of exit sections has been around in Linux for an extremely long time, at least on architectures which lean more towards embedded systems than bloat-is-OK servers. It's applied to driver remove methods, and to the code which invokes them via rmmod. Note that your comments all seem to presume that such discarding is either useless or should be phased out for some reason ... but you're not quite making *that* argument. - Dave -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 23, 2009 at 12:29:07PM -0700, David Brownell wrote: > On Wednesday 23 September 2009, Dmitry Torokhov wrote: > > On Wed, Sep 23, 2009 at 10:51:05AM -0700, David Brownell wrote: > > > On Wednesday 23 September 2009, Dmitry Torokhov wrote: > > > > >> __devexit? > > > > > [MA] According to comments from David Brownell to the first version of > > > > > this patch the __exit should be used. > > > > > > > > > > " - Use platform_driver_probe() and __exit/__exit_p(); > > > > > Â Â there's no point in keeping that code around in > > > > > Â Â typical configs, it'd just waste memory. " > > > > > > > > I am afraid David is wrong here. > > > > > > No, you're just pointing out a bug introduced in some > > > unrelated code. > > > > From the very version of the patch platform_driver_probe() only ensured > > that probe() would not be called after module inialization is done, it > > never made any guarantees about removing devices. Hmm, let's add Greg as > > well here. > > I don't see your point. If the driver doesn't support > any unbinding mechanism -- a remove() method, converse > of probe() -- then it can't safely be unbound. > This is one possible design... however you are not talking about the current Linux kernel but some other OS. > And thus, if any code is presuming that *every* driver > can be unbound, it's wrong. > > As I said: bug in other code. Not an implementation bug, the system behaves as designed. If you don't agree with the current design - get the patches in that alter it and _then_ we can change annotation from __devexit to __exit. > > Looking at this a bit more, it seems like there will need > to be some "can this bus remove this driver" check, since > the struct device.remove method is now managed at the bus > level. Easy enough to do instead of the null check that > I mentioned below. Provide it for platform bus, and the > main potential trouble spots will be resolved. > > > > > Such bugs happen when folk ignore the > > > code bloat issues. (And didn't Linus recently point > > > out how such code bloat is becoming an issue?) > > Though to be fair, it's *always* been an issue for anyone > working with embedded Linux systems. And it's long been > a frustration that too few "mainline" developers didn't > accept such issues. > You are rambling ;( > > > > > Even when we register driver with > > > > platform_driver_probe() we still have "unbind" attribute in sysfs which > > > > may be used to unbind the device from driver. If code is __exit then > > > > such attempts will cause oops. > > > > > > That would be a bug in the unbind() code, which doesn't > > > currently recognize that not every driver or bus supports > > > hotplugging. It should probably check for a null release() > > > pointer in the driver, and politely fail in that case. > > > > > > > NULL release (as well as NULL probe) don't mean what you think they do. > > I could say the same things about how you think, of course, > and probably with just as little accuracy. Let's not be > needlessly confrontational. > I am talking about current design of the Linux driver code, as it is present in mainline and in this particular instance probe() and remove() do not do what you think they do. You are of course free to submit patches that would alter the way the system behaves and then you can make the statement above and I will agree with you ;) > > > > That's just about the only place that a third party > > > (neither the driver nor its hotplug-aware bus framework) > > > will try to decouple device and driver ... and it's doing > > > it wrong. So it's unlikely such bugs will be elsewhere. > > > > > > > No, it does exactly what it is supposed to do. You may argue that such > > facility is not needed but this is different. I'd argue that sometimes > > you do need to shut off a device even if it is not normally > > hot-pluggrable. > > Erm, once a driver is freely poking around in the hardware, > and is managing IRQs and other asynch events ... exactly > how do you expect to decouple it from the hardware without > a remove()? Drivers that *don't* respond asynchronously to > hardware events are an extreme minority; it's not worth > even considering them as the design center. > > As a policy choice, there's only one sane one: unbinding > the driver always requires an active ack from the driver. > > Even if that's a trivial/NOP remove() function ... instead > of one which cleanly tears down the hardware event stream; > the driver's handling thereof; and whatever resources the > driver claimed, including links to upper level code in the > operating system (like char/block device nodes and many > other things). Look, this is all great, you are coming with wonderful new design that will surely fix all issues embedded developers have here. But until it is in mainline here is my perspective: I don't want to merge code that can trivially oops the box. I also don't want to encourage people to submit such code in the hopes that once design of the driver core changes their code stops oopsing. > > > > Plus most of the blat is coming from probe() functions, > > remove()s are fairly small. > > Not always. Of course, there's still that nasty issue > with code that probe() and remove() need to share ... for > now there's no clean way to have such code be removable. > (Like an "__init_or_exit" section annotation.) Drivers > have been stuck putting such code in the .text segement, > and thus causing additional driver bloat. > > > > > It's *ALWAYS* been legit to have a NULL pointer in the > > > remove() methods. > > > > Which means "device does not need any special actions for unbinding", > > not that device can not be unbound. The same with probe(): NULL does not > > mean that device can not be bound but rather that it does not need any > > specal processing during binding. > > I don't know where such a model comes from. I'll just > say that in quite a few years of driver development I > have *never* come across hardware where "doesn't need > any attention during driver unbind" makes sense. > > Recall that any driver has both lower (to hardware) and > upper (to rest of OS) links, and maybe more (to other > drivers) ... and it's a design goal in Linux to have such > links be explicit, so the drivers are more reusable. So > requiring some hook -- remove() -- before attempting > to break the links internal to the driver model seems > like a fairly obvious requirement. > > > > > That's why the __exit_p() -- or for > > > hotpluggable drivers, __devexit_p() -- macros exist: > > > for that particular case. > > > > Huh? They are here so that the linker/module loader can discard them if > > they only referenced via driver pointers and we know they not going to > > be called. > > Finish that thought. What "exit" section code fits those > categories? Driver remove() methods ... and not much else. > And for that matter, what else goes into exit sections, > other than such code and module hooks to call it? > > That discarding of exit sections has been around in Linux > for an extremely long time, at least on architectures which > lean more towards embedded systems than bloat-is-OK servers. > It's applied to driver remove methods, and to the code which > invokes them via rmmod. > > Note that your comments all seem to presume that such > discarding is either useless or should be phased out for > some reason ... but you're not quite making *that* > argument. > Umm, no, I did not say that. You implied that __exit_p and __devexit_p have something to do with setting remove to NULL but that is not their main purpose. They are needed so that references are gone, but we could just as easily be using (void *)1 instead of NULL as substitute value.
On Wednesday 23 September 2009, Dmitry Torokhov wrote: > This is one possible design... however you are not talking about the > current Linux kernel but some other OS. What other OS might it be then, which has carried around that exit section scrubbing mechanism for quite a few years now, and is distributed through www.kernel.org with labels such as "Linux 2.6.31" ??? > > And thus, if any code is presuming that *every* driver > > can be unbound, it's wrong. > > > > As I said: bug in other code. > > Not an implementation bug, the system behaves as designed. Yes, absolutely an implementation bug. At best you can say that there are two "designs" that are in conflict with each other. And argue, for some reason, that the relatively-recently-introduced oops (OK, mid-2005, so it's been lurking for quite a while) is "more intended" than the previous safe-no-oops one (predating mid-2005 by many years). > > Looking at this a bit more, it seems like there will need > > to be some "can this bus remove this driver" check, since > > the struct device.remove method is now managed at the bus > > level. Easy enough to do instead of the null check that > > I mentioned below. Provide it for platform bus, and the > > main potential trouble spots will be resolved. > > ... deletia ... > > I am talking about current design of the Linux driver code, as it is > present in mainline and in this particular instance probe() and remove() > do not do what you think they do. You're arguing about what it "should" do, and ignoring all the evidence I've provided. So I guess "talking" is right, not "listening" or better yet "discussing". > Look, this is all great, you are coming with wonderful new design that > will surely fix all issues embedded developers have here. But until it > is in mainline here is my perspective: No, I'm just pointing out how the system has been intended to behave *FOR MANY YEARS NOW* and you are objecting because of a bug in some unrelated and newer code. > > > > That's why the __exit_p() -- or for > > > > hotpluggable drivers, __devexit_p() -- macros exist: > > > > for that particular case. > > > > > > Huh? They are here so that the linker/module loader can discard them if > > > they only referenced via driver pointers and we know they not going to > > > be called. > > > > Finish that thought. What "exit" section code fits those > > categories? Driver remove() methods ... and not much else. > > And for that matter, what else goes into exit sections, > > other than such code and module hooks to call it? > > > > That discarding of exit sections has been around in Linux > > for an extremely long time, at least on architectures which > > lean more towards embedded systems than bloat-is-OK servers. > > It's applied to driver remove methods, and to the code which > > invokes them via rmmod. > > > > Note that your comments all seem to presume that such > > discarding is either useless or should be phased out for > > some reason ... but you're not quite making *that* > > argument. > > > > Umm, no, I did not say that. As I said, your comments *presumed* that. As in, they don't seem to make a lot of sense otherwise. If that "discard exit sections" is a longstanding part of Linux (as it is), then using it in normal ways wouldn't be the problem ... instead of saying that it *is* the problem, you'd be digging deeper. > You implied that __exit_p and __devexit_p > have something to do with setting remove to NULL but that is not their > main purpose. It's what they have *done* since quite a few years before the driver model existed. Saying that what they *do* is somehow not their main purpose ... isn't very useful, as arguments go. > They are needed so that references are gone, but we could > just as easily be using (void *)1 instead of NULL as substitute value. Except that NULL function pointers are valid for purposes other than calling through them, and at least one universal idiom relies on them: test pointer against NULL before using. Where in Linux is there code checking against indirection through "(void *)1)" instead of NULL?? - Dave -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 23, 2009 at 04:05:43PM -0700, David Brownell wrote: > On Wednesday 23 September 2009, Dmitry Torokhov wrote: > > This is one possible design... however you are not talking about the > > current Linux kernel but some other OS. > > What other OS might it be then, which has carried around > that exit section scrubbing mechanism for quite a few years > now, and is distributed through www.kernel.org with labels > such as "Linux 2.6.31" ??? > It can't be it since it does not exibit the behavior you are describing. > > > > And thus, if any code is presuming that *every* driver > > > can be unbound, it's wrong. > > > > > > As I said: bug in other code. > > > > Not an implementation bug, the system behaves as designed. > > Yes, absolutely an implementation bug. > > At best you can say that there are two "designs" that > are in conflict with each other. And argue, for some > reason, that the relatively-recently-introduced oops > (OK, mid-2005, so it's been lurking for quite a while) > is "more intended" than the previous safe-no-oops one > (predating mid-2005 by many years). Driver model was introduced what, 7 years ago? And since then at no point remove methods() could be __devexit. > > > > > Looking at this a bit more, it seems like there will need > > > to be some "can this bus remove this driver" check, since > > > the struct device.remove method is now managed at the bus > > > level. Easy enough to do instead of the null check that > > > I mentioned below. Provide it for platform bus, and the > > > main potential trouble spots will be resolved. > > > > ... deletia ... > > > > > I am talking about current design of the Linux driver code, as it is > > present in mainline and in this particular instance probe() and remove() > > do not do what you think they do. > > You're arguing about what it "should" do, and ignoring > all the evidence I've provided. So I guess "talking" > is right, not "listening" or better yet "discussing". > Ok, then let me tell this once again since you snipped it off: Until driver model is fixed so that using unbind sysfs attribute does not cause trouble if devices discard their remove methods I will not accept or ack drivers that mark their remove() methods as __exit.
Dmitry, I addressed your comments but I still have a couple of questions. >> +CONFIG_KEYBOARD_DAVINCI_DM365=m >> # CONFIG_INPUT_MOUSE is not set >> # CONFIG_INPUT_JOYSTICK is not set >> # CONFIG_INPUT_TABLET is not set > > > If you want to merge the driver through input tree the defconfig chunk > has to go elsewhere. [MA] Ok, independent patch. >> >> +config KEYBOARD_DAVINCI >> + tristate "TI DaVinci Keypad" >> + depends on ARCH_DAVINCI_DM365 >> + help >> + Say Y to enable keypad module support for the TI DaVinci >> + platforms (DM365) >> + > > "To compile this driver as a module..." [MA] Comment added. >> + keycode = keymap[position]; >> + if((new_status >> position) & 0x1) { > > bool release = (new_status >> position) & 0x1; > input_report_key(davinci_kp->input, keycode, !release); > dev_dbg(dev, "davinci_keypad: key %d %s\n", > keycode, release ? "released" : "pressed"); > > is shorter. > >> + /* Report release */ >> + dev_dbg(dev, "davinci_keypad: key %d released\n", >> + keycode); >> + input_report_key(davinci_kp->input, keycode, 0); >> + } else { >> + /* Report press */ >> + dev_dbg(dev, "davinci_keypad: key %d pressed\n", >> + keycode); >> + input_report_key(davinci_kp->input, keycode, 1); >> + } >> + input_sync(davinci_kp->input); >> + ret = IRQ_HANDLED; >> + } >> + >> + /* Clearing interrupt */ >> + davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, DAVINCI_KEYPAD_INTCLR); > > You return IRQ_HANDLED only if keypad state changed but clear interrupt > regardless. This is suspicious. > >> + >> + /* Enable interrupts */ >> + davinci_kp_write(davinci_kp, 0x1, DAVINCI_KEYPAD_INTENA); >> + >> + return ret; >> +} [MA] This is the current irq function: static irqreturn_t davinci_ks_interrupt(int irq, void *dev_id) { struct davinci_ks *davinci_ks = dev_id; struct device *dev = &davinci_ks->input->dev; unsigned short *keymap = davinci_ks->keymap; u32 prev_status, new_status, changed, position; bool release; int keycode = KEY_UNKNOWN; int ret = IRQ_NONE; /* Disable interrupt */ davinci_ks_write(davinci_ks, 0x0, DAVINCI_KEYSCAN_INTENA); /* Reading previous and new status of the key scan */ prev_status = davinci_ks_read(davinci_ks, DAVINCI_KEYSCAN_PREVSTATE); new_status = davinci_ks_read(davinci_ks, DAVINCI_KEYSCAN_CURRENTST); changed = prev_status ^ new_status; position = ffs(changed) - 1; if (changed) { keycode = keymap[position]; release = (new_status >> position) & 0x1; dev_dbg(dev, "davinci_keyscan: key %d %s\n", keycode, release ? "released" : "pressed"); input_report_key(davinci_ks->input, keycode, !release); input_sync(davinci_ks->input); /* Clearing interrupt */ davinci_ks_write(davinci_ks, DAVINCI_KEYSCAN_INT_ALL, DAVINCI_KEYSCAN_INTCLR); ret = IRQ_HANDLED; } /* Enable interrupts */ davinci_ks_write(davinci_ks, 0x1, DAVINCI_KEYSCAN_INTENA); return ret; } >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + if (!res) { >> + dev_err(dev, "no mem resource\n"); >> + ret = -ENODEV; > > -EINVAL I'd say. [MA] Ok. -EINVAL >> + key_dev->id.vendor = 0x0001; >> + key_dev->id.product = 0x0001; >> + key_dev->id.version = 0x0001; >> + key_dev->keycode = davinci_kp->pdata->keymap; > > Please kopy keymap into the davinci_kp stucture and use it so that > platform data is never changed and can be declared const. > [MA] keymap copied to private data. >> + key_dev->keycodesize = sizeof(unsigned int); > > sizeof(davinci_kp->keymap[0]) is safer. Plus make it unsigned short. > [MA] Now it uses sizeof(davinci_kp->keymap[0]) >> +} >> + >> +static int __exit davinci_kp_remove(struct platform_device *pdev) > > __devexit? > [MA] So, this will be __devexit >> +static struct platform_driver davinci_kp_driver = { >> + .driver = { >> + .name = "davinci_keypad", >> + .owner = THIS_MODULE, >> + }, >> + .remove = __exit_p(davinci_kp_remove), > > __devexit_p(). I think you can still unbind the device even if you use > platform_driver_probe. > [MA] ... and __devexit. >> +static int __init davinci_kp_init(void) >> +{ >> + return platform_driver_probe(&davinci_kp_driver, davinci_kp_probe); >> +} >> +module_init(davinci_kp_init); [MA] Should I use platform_driver_probe? >> +static void __exit davinci_kp_exit(void) >> +{ >> + platform_driver_unregister(&davinci_kp_driver); >> +} >> +module_exit(davinci_kp_exit); [MA] Is the module exit function __exit or __devexit Thanks, Miguel Aguilar -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 24, 2009 at 08:59:56AM -0600, Miguel Aguilar wrote: > Dmitry, > > I addressed your comments but I still have a couple of questions. Thank you for making the adjustments. > > [MA] This is the current irq function: > static irqreturn_t davinci_ks_interrupt(int irq, void *dev_id) > { > struct davinci_ks *davinci_ks = dev_id; > struct device *dev = &davinci_ks->input->dev; > unsigned short *keymap = davinci_ks->keymap; > u32 prev_status, new_status, changed, position; > bool release; > int keycode = KEY_UNKNOWN; > int ret = IRQ_NONE; > > /* Disable interrupt */ > davinci_ks_write(davinci_ks, 0x0, DAVINCI_KEYSCAN_INTENA); > > /* Reading previous and new status of the key scan */ > prev_status = davinci_ks_read(davinci_ks, DAVINCI_KEYSCAN_PREVSTATE); > new_status = davinci_ks_read(davinci_ks, DAVINCI_KEYSCAN_CURRENTST); > > changed = prev_status ^ new_status; > position = ffs(changed) - 1; > > if (changed) { > keycode = keymap[position]; > release = (new_status >> position) & 0x1; > dev_dbg(dev, "davinci_keyscan: key %d %s\n", > keycode, release ? "released" : "pressed"); > > input_report_key(davinci_ks->input, keycode, !release); > input_sync(davinci_ks->input); > > /* Clearing interrupt */ > davinci_ks_write(davinci_ks, DAVINCI_KEYSCAN_INT_ALL, > DAVINCI_KEYSCAN_INTCLR); > > ret = IRQ_HANDLED; > } > > /* Enable interrupts */ > davinci_ks_write(davinci_ks, 0x1, DAVINCI_KEYSCAN_INTENA); > > return ret; I'd just return IRQ_HANDLED unconditionally and I think you should go through all bits in changed to ensure that you don't lose key presses (unless controller never ever reports more than 1 key pressed). > } > > > >>> +static int __init davinci_kp_init(void) >>> +{ >>> + return platform_driver_probe(&davinci_kp_driver, davinci_kp_probe); >>> +} >>> +module_init(davinci_kp_init); > [MA] Should I use platform_driver_probe? > I don't see why not - you are still saving memory due to discarding davinci_kp_probe. And if driver core is changed so that unbind is a NOP for drivers registered with platform_driver_probe I will gladly accept a follow-up patch to change __devexit to __exit for even more savings. >>> +static void __exit davinci_kp_exit(void) >>> +{ >>> + platform_driver_unregister(&davinci_kp_driver); >>> +} >>> +module_exit(davinci_kp_exit); > [MA] Is the module exit function __exit or __devexit > Module ext routines are always __exit (and inits are __init). Thanks.
diff --git a/arch/arm/configs/davinci_all_defconfig b/arch/arm/configs/davinci_all_defconfig index ec63c15..e994c83 100644 --- a/arch/arm/configs/davinci_all_defconfig +++ b/arch/arm/configs/davinci_all_defconfig @@ -763,6 +763,7 @@ CONFIG_KEYBOARD_GPIO=y # CONFIG_KEYBOARD_STOWAWAY is not set # CONFIG_KEYBOARD_SUNKBD is not set CONFIG_KEYBOARD_XTKBD=m +CONFIG_KEYBOARD_DAVINCI_DM365=m # CONFIG_INPUT_MOUSE is not set # CONFIG_INPUT_JOYSTICK is not set # CONFIG_INPUT_TABLET is not set diff --git a/arch/arm/mach-davinci/include/mach/keypad.h b/arch/arm/mach-davinci/include/mach/keypad.h new file mode 100644 index 0000000..922d20e --- /dev/null +++ b/arch/arm/mach-davinci/include/mach/keypad.h @@ -0,0 +1,35 @@ +/* + * Copyright (C) 2009 Texas Instruments, Inc + * + * Author: Miguel Aguilar <miguel.aguilar@ridgerun.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; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + */ + +#ifndef DAVINCI_KEYPAD_H +#define DAVINCI_KEYPAD_H + +#include <linux/io.h> + +struct davinci_kp_platform_data { + int *keymap; + u32 keymapsize; + u32 rep:1; + u32 strobe; + u32 interval; +}; + +#endif + diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig index a6b989a..b6b9517 100644 --- a/drivers/input/keyboard/Kconfig +++ b/drivers/input/keyboard/Kconfig @@ -361,4 +361,11 @@ config KEYBOARD_XTKBD To compile this driver as a module, choose M here: the module will be called xtkbd. +config KEYBOARD_DAVINCI + tristate "TI DaVinci Keypad" + depends on ARCH_DAVINCI_DM365 + help + Say Y to enable keypad module support for the TI DaVinci + platforms (DM365) + endif diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile index b5b5eae..0b0274e 100644 --- a/drivers/input/keyboard/Makefile +++ b/drivers/input/keyboard/Makefile @@ -31,3 +31,4 @@ obj-$(CONFIG_KEYBOARD_STOWAWAY) += stowaway.o obj-$(CONFIG_KEYBOARD_SUNKBD) += sunkbd.o obj-$(CONFIG_KEYBOARD_TOSA) += tosakbd.o obj-$(CONFIG_KEYBOARD_XTKBD) += xtkbd.o +obj-$(CONFIG_KEYBOARD_DAVINCI) += davinci_keypad.o diff --git a/drivers/input/keyboard/davinci_keypad.c b/drivers/input/keyboard/davinci_keypad.c new file mode 100644 index 0000000..6f0e793 --- /dev/null +++ b/drivers/input/keyboard/davinci_keypad.c @@ -0,0 +1,319 @@ +/* + * DaVinci Keypad Driver for TI platforms + * + * Copyright (C) 2009 Texas Instruments, Inc + * + * Author: Miguel Aguilar <miguel.aguilar@ridgerun.com> + * + * Intial Code: Sandeep Paulraj <s-paulraj@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; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + */ +#include <linux/module.h> +#include <linux/init.h> +#include <linux/interrupt.h> +#include <linux/types.h> +#include <linux/input.h> +#include <linux/kernel.h> +#include <linux/delay.h> +#include <linux/platform_device.h> +#include <linux/errno.h> + +#include <asm/irq.h> + +#include <mach/hardware.h> +#include <mach/irqs.h> +#include <mach/keypad.h> + +/* Keypad registers */ +#define DAVINCI_KEYPAD_KEYCTRL 0x0000 +#define DAVINCI_KEYPAD_INTENA 0x0004 +#define DAVINCI_KEYPAD_INTFLAG 0x0008 +#define DAVINCI_KEYPAD_INTCLR 0x000c +#define DAVINCI_KEYPAD_STRBWIDTH 0x0010 +#define DAVINCI_KEYPAD_INTERVAL 0x0014 +#define DAVINCI_KEYPAD_CONTTIME 0x0018 +#define DAVINCI_KEYPAD_CURRENTST 0x001c +#define DAVINCI_KEYPAD_PREVSTATE 0x0020 +#define DAVINCI_KEYPAD_EMUCTRL 0x0024 +#define DAVINCI_KEYPAD_IODFTCTRL 0x002c + +/* Key Control Register (KEYCTRL) */ +#define DAVINCI_KEYPAD_KEYEN 0x00000001 +#define DAVINCI_KEYPAD_PREVMODE 0x00000002 +#define DAVINCI_KEYPAD_CHATOFF 0x00000004 +#define DAVINCI_KEYPAD_AUTODET 0x00000008 +#define DAVINCI_KEYPAD_SCANMODE 0x00000010 +#define DAVINCI_KEYPAD_OUTTYPE 0x00000020 +#define DAVINCI_KEYPAD_4X4 0x00000040 + +/* Masks for the interrupts */ +#define DAVINCI_KEYPAD_INT_CONT 0x00000008 +#define DAVINCI_KEYPAD_INT_OFF 0x00000004 +#define DAVINCI_KEYPAD_INT_ON 0x00000002 +#define DAVINCI_KEYPAD_INT_CHANGE 0x00000001 +#define DAVINCI_KEYPAD_INT_ALL 0x0000000f + +struct davinci_kp { + struct input_dev *input; + struct davinci_kp_platform_data *pdata; + int irq; + void __iomem *base; + resource_size_t pbase; + size_t base_size; +}; + +static void davinci_kp_write(struct davinci_kp *davinci_kp, u32 val, u32 addr) +{ + u32 base = (u32)davinci_kp->base; + + __raw_writel(val,(u32 *)(base + addr)); +} + +static u32 davinci_kp_read(struct davinci_kp *davinci_kp, u32 addr) +{ + u32 base = (u32)davinci_kp->base; + + return __raw_readl((u32 *)(base + addr)); +} + +/* Initializing the kp Module */ +static void davinci_kp_initialize(struct davinci_kp *davinci_kp) +{ + u32 strobe = davinci_kp->pdata->strobe; + u32 interval = davinci_kp->pdata->interval; + + /* Enable all interrupts */ + davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, DAVINCI_KEYPAD_INTENA); + + /* Clear interrupts if any */ + davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, DAVINCI_KEYPAD_INTCLR); + + /* Setup the scan period = strobe + interval */ + davinci_kp_write(davinci_kp, strobe, DAVINCI_KEYPAD_STRBWIDTH); + davinci_kp_write(davinci_kp, interval, DAVINCI_KEYPAD_INTERVAL); + davinci_kp_write(davinci_kp, 0x01, DAVINCI_KEYPAD_CONTTIME); + + /* Enable Keyscan module and enable */ + davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_AUTODET | DAVINCI_KEYPAD_KEYEN, + DAVINCI_KEYPAD_KEYCTRL); +} + +static irqreturn_t davinci_kp_interrupt(int irq, void *dev_id) +{ + struct davinci_kp *davinci_kp = dev_id; + struct device *dev = &davinci_kp->input->dev; + int *keymap = davinci_kp->pdata->keymap; + u32 prev_status, new_status, changed, position; + int keycode = KEY_UNKNOWN; + int ret = IRQ_NONE; + + /* Disable interrupt */ + davinci_kp_write(davinci_kp, 0x0, DAVINCI_KEYPAD_INTENA); + + /* Reading previous and new status of the keypad */ + prev_status = davinci_kp_read(davinci_kp, DAVINCI_KEYPAD_PREVSTATE); + new_status = davinci_kp_read(davinci_kp, DAVINCI_KEYPAD_CURRENTST); + + changed = prev_status ^ new_status; + position = ffs(changed) - 1; + + if (changed) { + keycode = keymap[position]; + if((new_status >> position) & 0x1) { + /* Report release */ + dev_dbg(dev, "davinci_keypad: key %d released\n", + keycode); + input_report_key(davinci_kp->input, keycode, 0); + } else { + /* Report press */ + dev_dbg(dev, "davinci_keypad: key %d pressed\n", + keycode); + input_report_key(davinci_kp->input, keycode, 1); + } + input_sync(davinci_kp->input); + ret = IRQ_HANDLED; + } + + /* Clearing interrupt */ + davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, DAVINCI_KEYPAD_INTCLR); + + /* Enable interrupts */ + davinci_kp_write(davinci_kp, 0x1, DAVINCI_KEYPAD_INTENA); + + return ret; +} + +static int __init davinci_kp_probe(struct platform_device *pdev) +{ + struct davinci_kp *davinci_kp; + struct input_dev *key_dev; + struct resource *res, *mem; + int ret, i; + struct device * dev = &pdev->dev; + struct davinci_kp_platform_data *pdata = pdev->dev.platform_data; + + dev_info(dev, "DaVinci Keypad Driver\n"); + + if (!pdata->keymap) { + dev_dbg(dev, "no keymap from pdata\n"); + return -EINVAL; + } + + davinci_kp = kzalloc(sizeof *davinci_kp, GFP_KERNEL); + if(!davinci_kp) { + dev_dbg(dev, "could not allocate memory for private data\n"); + return -ENOMEM; + } + + key_dev = input_allocate_device(); + if (!key_dev) { + dev_dbg(dev, "could not allocate input device\n"); + ret = -ENOMEM; + goto fail1; + } + + platform_set_drvdata(pdev, davinci_kp); + + davinci_kp->input = key_dev; + + davinci_kp->irq = platform_get_irq(pdev, 0); + if (davinci_kp->irq < 0) { + dev_err(dev, "no keypad irq\n"); + ret = davinci_kp->irq; + goto fail2; + } + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!res) { + dev_err(dev, "no mem resource\n"); + ret = -ENODEV; + goto fail2; + } + + davinci_kp->pbase = res->start; + davinci_kp->base_size = resource_size(res); + + mem = request_mem_region(davinci_kp->pbase, davinci_kp->base_size, pdev->name); + if (!mem) { + dev_err(dev, "KEYSCAN registers at %08x are not free\n", + davinci_kp->pbase); + ret = -EBUSY; + goto fail2; + } + + davinci_kp->base = ioremap(davinci_kp->pbase, davinci_kp->base_size); + if (!davinci_kp->base) { + dev_err(dev, "can't ioremap MEM resource.\n"); + ret = -ENOMEM; + goto fail3; + } + + /* Enable auto repeat feature of Linux input subsystem */ + if (pdata->rep) + __set_bit(EV_REP, key_dev->evbit); + + /* Setup input device */ + __set_bit(EV_KEY, key_dev->evbit); + + /* Setup the keymap */ + davinci_kp->pdata = pdata; + + for (i = 0; i < davinci_kp->pdata->keymapsize; i++) + __set_bit(davinci_kp->pdata->keymap[i], key_dev->keybit); + + key_dev->name = "davinci_keypad"; + key_dev->phys = "davinci_keypad/input0"; + key_dev->dev.parent = &pdev->dev; + key_dev->id.bustype = BUS_HOST; + key_dev->id.vendor = 0x0001; + key_dev->id.product = 0x0001; + key_dev->id.version = 0x0001; + key_dev->keycode = davinci_kp->pdata->keymap; + key_dev->keycodesize = sizeof(unsigned int); + key_dev->keycodemax = davinci_kp->pdata->keymapsize; + + ret = input_register_device(davinci_kp->input); + if (ret < 0) { + dev_err(dev, "unable to register DaVinci keypad device\n"); + goto fail4; + } + + ret = request_irq(davinci_kp->irq, davinci_kp_interrupt, IRQF_DISABLED, + "davinci_keypad", davinci_kp); + if (ret < 0) { + dev_err(dev, "unable to register DaVinci keypad Interrupt\n"); + goto fail5; + } + + davinci_kp_initialize(davinci_kp); + + return 0; +fail5: + input_unregister_device(davinci_kp->input); + key_dev = NULL; +fail4: + iounmap(davinci_kp->base); +fail3: + release_mem_region(davinci_kp->pbase, davinci_kp->base_size); +fail2: + input_free_device(key_dev); +fail1: + kfree(davinci_kp); + + return ret; +} + +static int __exit davinci_kp_remove(struct platform_device *pdev) +{ + struct davinci_kp *davinci_kp = platform_get_drvdata(pdev); + + free_irq(davinci_kp->irq, davinci_kp); + + input_unregister_device(davinci_kp->input); + + iounmap(davinci_kp->base); + release_mem_region(davinci_kp->pbase, davinci_kp->base_size); + + platform_set_drvdata(pdev, NULL); + + kfree(davinci_kp); + + return 0; +} + +static struct platform_driver davinci_kp_driver = { + .driver = { + .name = "davinci_keypad", + .owner = THIS_MODULE, + }, + .remove = __exit_p(davinci_kp_remove), +}; + +static int __init davinci_kp_init(void) +{ + return platform_driver_probe(&davinci_kp_driver, davinci_kp_probe); +} +module_init(davinci_kp_init); + +static void __exit davinci_kp_exit(void) +{ + platform_driver_unregister(&davinci_kp_driver); +} +module_exit(davinci_kp_exit); + +MODULE_AUTHOR("Miguel Aguilar"); +MODULE_DESCRIPTION("Texas Instruments DaVinci Keypad Driver"); +MODULE_LICENSE("GPL");