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@xxxxxxxxxxxx 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@xxxxxxxxxxxx>
> >> + *
> >> + * Intial Code: Sandeep Paulraj <s-paulraj@xxxxxx>
> >> + *
> >> + * 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
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
