Re: [PATCH] gpiodev-keys, gpiodev-diagonal + button patches for h5000

On Po, 2007-04-16 at 18:21 +0300, Paul Sokolovsky wrote:
> Hello Milan,
>
> Sorry for delay with processing your patches.

  No problem; better to have delayed patches than ugly/buggy code in
kernel:). You are doing great job by reviewing them, thanks:)

>
> Sunday, April 8, 2007, 11:59:36 PM, you wrote:
>
> > Hello,
>
> > first of all, I apologize for sending patch, which adds both generic
> > and port-specific functionality at once. In the future I will try to
> > avoid this, but it's quite late now:) I would also like to thank
> > psokolovsky and bd2 for their patience with me.
>
> I think you did the great job with these drivers, and there's no need to try to have the most general solution from the first time ;-).
>

  :) Yes, but it would be nice if something general enough could be
written.

>
> > Patch and files, which can be found at
> > http://handhelds.org/~mmp/h5000/gpiodev/ , make following changes to
> > kernel tree:
> > 1) "Upgrade" from gpio-keys to gpiodev-keys, which uses gpio_dev . Of
> > course, not removing gpio-keys .
> > 2) Add gpiodev-diagonal driver. This driver implements special case
> > where joypad's GPIOs are connected in diagonal, instead of normal way.
> > 3) Modifications for drivers/soc/samcop_base.c, so it implements
> > gpio_dev.
> > 4) Modifications to arch/arm/mach-pxa/h5400/h5400.c, which replace
> > gpio-keys code with gpiodev-keys and gpiodev-diagonal.
>
> All patches were applied, except for few hunks in this file.
> Specifically, update to use gpiodev-keys/gpiodev-diagonal had
> conflicts, can you cvs update, resolev them, and resubmit? Btw, this:
>
> + { _KEY_RECORD, { NULL, SAMCOP_GPA_RECORD }, 1, "Record button" },
>
> + for (i = 1; i < h5400_pxa_keys_data.nbuttons; i++)
> + h5400_button_table[i].gpio.gpio_dev = &h5400_samcop;
>
> Is now how gpiodev API was intended to use. Even though GPIO id's are
> structured, they are still constant (link-time constant though). So,
> it is intended that it will be written as:
>
> + { _KEY_RECORD, { &h5400_samcop.dev, SAMCOP_GPA_RECORD }, 1, "Record button" },
>

  I will try in the closest future, just when I'll finish some
school-related tasks. Btw, the conflicts were in Kconfig's right? AFAIK,
gpiodev-* were new files... . Maybe it would be worth to convert the
*-diagonal driver to h5000-specific driver, as there are also other
buttons which need strategy similar to the one implemented in *diagonal.
Otherwise we would end with three different key drivers for h5000, which
is a bit large number, considering that it has just ~13 buttons:)

  To the non-static setting of device - I made this workaround because
of some bug gcc was saying; maybe I overlooked something:)

>
> Other issue is this hunk:
>
> static int h5400_dma_needs_bounce (struct device *dev, dma_addr_t addr, size_t size)
> {
> - return (addr >= H5400_SAMCOP_BASE + _SAMCOP_SRAM_Base + SAMCOP_SRAM_SIZE);
> + return (addr + size >= H5400_SAMCOP_BASE + _SAMCOP_SRAM_Base + SAMCOP_SRAM_SIZE);
> };
>
>
> Are you suer this is correct still? What it said before is that
> bouncing was required when block start lies after the SAMCOP SRAM.
> With your change, it is "debouncing is required if next byte after
> block end lies after the SAMCOP SRAM". Doesn't make much sense, IMHO,
> unless it's some kind of arithmetic optimization, in which case it's
> worth some comment. Please reconfirm.

  Err, that was a residue from my experiments from samcop's experiments,
please ignore it. Anton already asked me about this and I promised to
remove it from patches; I forgot about it, though... .

>
>
>
> > 5) Modification go cpu-pxa.c to export pxagpio_device
> > 6) Fix to include/asm/hardware/ipaq-samcop.h, thanks to which IRQ
> > handling works now correctly (much thanks rv6502 for pointing out the
> > problem).
>
> > Patch should be applicable to the most recent kernel tree, header
> > files go to include/linux and .c files to drivers/input/keyboard. I hope
> > I did not forget anything. If so, or there are any objections, let me
> > know:)
>
> > Milan
>
> > _______________________________________________
> > H5400-port mailing list
> > H5400-port_at_handhelds.org
> > https://www.handhelds.org/mailman/listinfo/h5400-port
>
>
Received on Mon Apr 16 2007 - 11:45:30 EDT