From mboxrd@z Thu Jan 1 00:00:00 1970 From: Miguel Aguilar Subject: Re: [PATCH 1/2] Input: DaVinci Keypad Driver Date: Wed, 23 Sep 2009 11:07:06 -0600 Message-ID: <4ABA55BA.7000009@ridgerun.com> References: <1253654850-11983-1-git-send-email-miguel.aguilar@ridgerun.com> <20090923034643.GB1458@core.coreip.homeip.net> <4ABA3638.5070609@ridgerun.com> <20090923163546.GA13435@core.coreip.homeip.net> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from mail.navvo.net ([74.208.67.6]:50115 "EHLO mail.navvo.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752445AbZIWRHL (ORCPT ); Wed, 23 Sep 2009 13:07:11 -0400 In-Reply-To: <20090923163546.GA13435@core.coreip.homeip.net> Sender: linux-input-owner@vger.kernel.org List-Id: linux-input@vger.kernel.org To: Dmitry Torokhov Cc: nsnehaprabha@ti.com, davinci-linux-open-source@linux.davincidsp.com, linux-input@vger.kernel.org, todd.fischer@ridgerun.com, diego.dompe@ridgerun.com, clark.becker@ridgerun.com, santiago.nunez@ridgerun.com, David Brownell 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 >>>> >>>> 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 >>>> --- >>>> 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 >>>> + * >>>> + * 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 >>>> + >>>> +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 >>>> + * >>>> + * Intial Code: Sandeep Paulraj >>>> + * >>>> + * 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 >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> + >>>> +#include >>>> + >>>> +#include >>>> +#include >>>> +#include >>>> + >>>> +/* 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