Re: [PATCH v3 1/2] Input: Add DaVinci DM365 Keypad support

[Kevin Hilman <khilman@deeprootsystems.com>]

Miguel Aguilar <miguel.aguilar@ridgerun.com> writes:

> Sekhar,
>
> If you and Sneha agree I will write driver in terms of davinci instead of dm365.
>

I add a vote the davinci* naming instead of dm365*.

Kevin

> Thanks,
>
> Miguel Aguilar
>
> Nori, Sekhar wrote:
>> On Fri, Sep 18, 2009 at 20:02:32, Miguel Aguilar wrote:
>>> Sekhar,
>>>
>>> See the comments below.
>>>
>>> Nori, Sekhar wrote:
>>>> On Fri, Sep 18, 2009 at 01:36:36, miguel.aguilar@ridgerun.com wrote:
>>
>> [...]
>>
>>>>> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
>>>>> index a6b989a..08ed7d4 100644
>>>>> --- a/drivers/input/keyboard/Kconfig
>>>>> +++ b/drivers/input/keyboard/Kconfig
>>>>> @@ -361,4 +361,10 @@ config KEYBOARD_XTKBD
>>>>>         To compile this driver as a module, choose M here: the
>>>>>         module will be called xtkbd.
>>>>>
>>>>> +config KEYBOARD_DAVINCI_DM365
>>>> How about calling it just KEYBOARD_DAVINCI in honor of this
>>>> being the first DaVinci keypad driver in drivers/input/keyboard?
>>>> Also, in the hope that this will get reused on a future DaVinci.
>>>>
>>>> Unless, there is something that makes it really DM365 specific..
>>> [MA] It is better keep it specific since that symbols refers to a driver which
>>> is DM365 specific at the moment.
>>
>> Okay. There is no device which reuses the keyscan module at
>> present, but there has been a lot of IP reuse in the DaVinci
>> family before (emac, mmc/sd, i2c, spi etc.).
>>
>>>>> +     tristate "TI DaVinci DM365 Keypad"
>>>>> +     depends on ARCH_DAVINCI_DM365
>>>>> +     help
>>>>> +      Supports the keypad module on the DM365
>>>>> +
>>>>>  endif
>>>> [...]
>>>>
>>>>> diff --git a/drivers/input/keyboard/davinci_dm365_keypad.c b/drivers/input/keyboard/davinci_dm365_keypad.c
>>>>> new file mode 100644
>>>>> index 0000000..5db8eed
>>>>> --- /dev/null
>>>>> +++ b/drivers/input/keyboard/davinci_dm365_keypad.c
>>>>> @@ -0,0 +1,331 @@
>>>>> +/*
>>>>> + * DaVinci DM365 Keypad Driver
>>>>> + *
>>>>> + * 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 DM365_KEYPAD_KEYCTRL         0x0000
>>>> There are many places in the driver (functions, macros) which
>>>> probably can just be called DAVINCI_ instead of DM365_
>>> [MA]Again, these are DM365 specific definitions, If your proposal is to make
>>> this driver generic to Davinci instead of DM365 specific, I should get rid of
>>> all dm365 labels, messages, function names, etc in this driver, even the
>>> driver's name should be davinci_keypad instead of dm365_keypad, so in that sense
>>> not only the macros and the config symbol should be changed; but for doing that
>>> we are assuming that future chips of the Davinci family will include the same
>>> keypad module, so they would be compatible among them to use the same driver.
>>
>> Yes, typically attempt is made to reuse the IP as-is.
>> Slight changes in the IPs re-used have previously been
>> taken care use a "version" field coming from the platform
>> data.
>>
>> The product number population below seems to be a bigger
>> sore than the variable/macro/Kconfig naming.
>>
>>>>> +static irqreturn_t dm365_kp_interrupt(int irq, void *dev_id)
>>>>> +{
>>>>> +     int i;
>>>>> +     u32 prev_status, new_status, changed;
>>>>> +     int keycode = KEY_UNKNOWN;
>>>>> +     struct davinci_kp *dm365_kp = dev_id;
>>>>> +     int *keymap = dm365_kp->pdata->keymap;
>>>>> +     u32 keymapsize = dm365_kp->pdata->keymapsize;
>>>>> +     struct device *dev = &dm365_kp->input->dev;
>>>>> +
>>>>> +     /* Disable interrupt */
>>>>> +     dm365_kp_write(dm365_kp, 0x0, DM365_KEYPAD_INTENA);
>>>>> +
>>>>> +     /* Reading previous and new status of the keypad */
>>>>> +     prev_status = dm365_kp_read(dm365_kp, DM365_KEYPAD_PREVSTATE);
>>>>> +     new_status = dm365_kp_read(dm365_kp, DM365_KEYPAD_CURRENTST);
>>>>> +
>>>>> +     changed = prev_status ^ new_status;
>>>>> +
>>>>> +     for (i = 0; i < keymapsize; i++) {
>>>>> +             if ((changed >> i) & 0x1) {
>>>> Using ffs(changed) will probably lead to a faster ISR, especially
>>>> because only one key would have changed in most cases.
>>> [MA] OK, I will try ffs.
>>>>> +                     keycode = keymap[i];
>>>>> +                     if((new_status >> i) & 0x1) {
>>>>> +                             /* Report release */
>>>>> +                             input_report_key(dm365_kp->input, keycode, 0);
>>>>> +                             input_sync(dm365_kp->input);
>>>>> +                             dev_dbg(dev, "dm365_keypad: key %d released\n",
>>>>> +                                             keycode);
>>>>> +                     } else {
>>>>> +                             /* Report press */
>>>>> +                             input_report_key(dm365_kp->input, keycode, 1);
>>>>> +                             input_sync(dm365_kp->input);
>>>>> +                             dev_dbg(dev, "dm365_keypad: key %d pressed\n",
>>>>> +                                             keycode);
>>>>> +                     }
>>>>> +             }
>>>>> +     }
>>>>> +
>>>>> +     /* Clearing interrupt */
>>>>> +     dm365_kp_write(dm365_kp, DM365_KEYPAD_INT_ALL, DM365_KEYPAD_INTCLR);
>>>>> +
>>>>> +     /* Enable interrupts */
>>>>> +     dm365_kp_write(dm365_kp, 0x1, DM365_KEYPAD_INTENA);
>>>>> +
>>>>> +     return IRQ_HANDLED;
>>
>> I missed this before, if the loop above turns up nothing,
>> then the IRQ was not handled. IRQ_NONE will be a better return
>> in that case.
>
>>
>> [...]
>>
>>>>> +     dm365_kp->irq = platform_get_irq(pdev, 0);
>>>>> +     if (dm365_kp->irq <= 0) {
>>>>> +             dev_err(dev, "%s: No DM365 Keypad irq\n", pdev->name);
>>>>> +             ret = dm365_kp->irq;
>>>> Probe will return success when dm365_kp->irq is 0, but you actually
>>>> want a failure.
>>> [MA] What is the proper error code for this case?.
>>
>> platform_get_irq() returns an error which should be propagated. The
>> question is: Why is 0 not a valid IRQ number for keyscan module?
>>
>> Thanks,
>> Sekhar
>>
>
>
>
> _______________________________________________
> Davinci-linux-open-source mailing list
> Davinci-linux-open-source@linux.davincidsp.com
> http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source