Re: [PATCH 1/2] OMAP: Add keypad driver #3



--- Russell King <rmk+lkml@xxxxxxxxxxxxxxxx> wrote:

+
+ /* read the keypad status */
+ if (cpu_is_omap24xx()) {
+ int i;
+ for (i = 0; i < omap_kp->rows; i++)
+ disable_irq(OMAP_GPIO_IRQ(row_gpios[i]));
+ } else
+ /* disable keyboard interrupt and schedule for handling */
+ omap_writew(1, OMAP_MPUIO_BASE + OMAP_MPUIO_KBD_MASKIT);
+
+ if (!cpu_is_omap24xx()) {

This seems obfuscated. It would be trivial to combine these two if()
clauses.

Oops. I will update this in the next patch.


And a general note about the omap24xx vs !omap24xx differences in
this
file - would it make more sense for code readability to have two
completely separate drivers?

Yeah, I had same thought when I did the integration of omap24xx H4 gpio
based keypad driver from TI OMAP tree to omap-git. But, if Tony, Juha
and Richard agrees, then I can roll-out new omap2-gpio-keypad driver
patch along with changes into existing omap-keypad.c(will become
omap1-keypad.c then). It will be also easy for me to maintain omap2
keypad driver, as I don't have access to OMAP1 based boards.

+
+ if (machine_is_omap_osk() || machine_is_omap_h2()
+ || machine_is_omap_h3())
+ udelay(9);
+ else
+ udelay(4);

Wouldn't it be better to pass this via the platform device driver?
It
seems likely that other delays may be required with differing
hardware.

Yes, we can. I will make that change.

+
+ if (machine_is_omap_h2() || machine_is_omap_h3() ||
+ machine_is_omap_perseus2()) {
+ omap_writew(0xff, OMAP_MPUIO_BASE + OMAP_MPUIO_GPIO_DEBOUNCING);
+ }

Maybe this should be a flag or something? Why does h2, h3 and
perseus2
require this and not others?

Yes we can put the flag there through platform data, but OMAP1
(h2/h3/perseus) owners should comment on that. Kevin/Tony?

Thanx for the detailed review.

---Komal Shah
http://komalshah.blogspot.com/

__________________________________________________
Do You Yahoo!?
Tired of spam? Yahoo! Mail has the best spam protection around
http://mail.yahoo.com
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/



Relevant Pages

  • Re: [PATCH] uio: User IRQ Mode
    ... In this mode the user space driver ... is responsible for acknowledging and re-enabling the interrupt. ... This can easily be done without your patch. ...
    (Linux-Kernel)
  • Re: [PATCH] ne.c fix for hibernate and rmmod oops fix
    ... This patch is ready to be merged. ... that was not the case before, because the probe is invasive. ... legacy driver. ... const unsigned char *buf, const int start_page); ...
    (Linux-Kernel)
  • Re: [PATCH] Blackfin: blackfin on-chip SPI controller driver
    ... patch, rather than initial-plus-cleanups. ... Please put this in Kconfig up with the other SPI controller drivers, ... relevant points in the driver. ... place to reverse any DMA mappings ... ...
    (Linux-Kernel)
  • Re: [patch 2.6.20-rc3 1/3] rtc-cmos driver
    ... To build your new patch for ARM I have modified the line "depends on ... configured that driver as a module. ... As for the RTC patch, it does work on the shark, and is needed. ...
    (Linux-Kernel)
  • Re: [PATCH 0/4, v14] PCI, ACPI: Physical PCI slot objects
    ... To emulate the platform that assigns the same name to multiple ... With this patch, I ... varies depending on whether pci_slot driver is loaded or not. ... I got the following error message when I loaded pciehp driver. ...
    (Linux-Kernel)