PATCH/RFC [0/3]: Fix suspend and other acceleromter issues

Sean McNeil sean at mcneil.com
Sat Nov 15 11:25:58 CET 2008


Andy Green wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Somebody in the thread at some point said:
> | On Sat, 15 Nov 2008 08:59:02 +0000
> | Andy Green <andy at openmoko.com> wrote:
> |
> |> One big patch for stable-tracking update is fine, thanks for looking
> |> at it.  At least they are on a common bitbang basis now...
> |
> | OK, I'll look at this (unless Nelson or someone else beats me to it 
> ;-))
> |
> |> How is the performance about them stopping servicing interrupts at the
> |> moment from your experience?  Sen McNeil had some changes to the GPIO
> |> actions in the bitbang which he reckoned removed the loss of data
> |> completely, I am not sure the various rebasings and branches have kept
> |> what he did...
> |
> | Seans fixes switched the interrupts to level-triggering?
>
> Yes here is the relevant patch
>
> From: Sean McNeil <sean at mcneil.com>
>
> Move to level from edge, fix local_save... to local_irq...
> simplify bitbang sequence
>
> Signed-off-by: Sean McNeil <sean at mcneil.com>
> - ---
>
> ~ arch/arm/mach-s3c2440/mach-gta02.c |   18 ++----------------
> ~ drivers/input/misc/lis302dl.c      |    8 +-------
> ~ 2 files changed, 3 insertions(+), 23 deletions(-)
>
> diff --git a/arch/arm/mach-s3c2440/mach-gta02.c
> b/arch/arm/mach-s3c2440/mach-gta02.c
> index 1658fa0..e4b2b9e 100644
> - --- a/arch/arm/mach-s3c2440/mach-gta02.c
> +++ b/arch/arm/mach-s3c2440/mach-gta02.c
> @@ -1058,13 +1058,12 @@ void gat02_lis302dl_bitbang_read(struct
> lis302dl_info *lis)
> ~     struct lis302dl_platform_data *pdata = lis->pdata;
> ~     u8 shifter = 0xc0 | LIS302DL_REG_OUT_X; /* read, autoincrement */
> ~     int n, n1;
> - -    unsigned long other_cs;
> ~     unsigned long flags;
> ~ #ifdef DEBUG_SPEW_MS
> ~     s8 x, y, z;
> ~ #endif
>
> - -    local_save_flags(flags);
> +    local_irq_save(flags);  <==== we fixed these I think
>
> ~     /*
> ~      * Huh.. "quirk"... CS on this device is not really "CS" like 
> you can
> @@ -1076,33 +1075,21 @@ void gat02_lis302dl_bitbang_read(struct
> lis302dl_info *lis)
> ~      * ensure this is never issued.
> ~      */
>
> - -    if (&lis302_pdata[0] == pdata)
> - -        other_cs = lis302_pdata[1].pin_chip_select;
> - -    else
> - -        other_cs = lis302_pdata[0].pin_chip_select;
> - -
> - -    s3c2410_gpio_setpin(other_cs, 1);
> - -    s3c2410_gpio_setpin(pdata->pin_chip_select, 1);
> ~     s3c2410_gpio_setpin(pdata->pin_clk, 1);
> ~     s3c2410_gpio_setpin(pdata->pin_chip_select, 0);
> ~     for (n = 0; n < 8; n++) { /* write the r/w, inc and address */
> ~         s3c2410_gpio_setpin(pdata->pin_clk, 0);
> - -        s3c2410_gpio_setpin(pdata->pin_mosi, (shifter >> 7) & 1);
> - -        s3c2410_gpio_setpin(pdata->pin_clk, 0);
> +        s3c2410_gpio_setpin(pdata->pin_mosi, (shifter >> (7 - n)) & 1);
> ~         s3c2410_gpio_setpin(pdata->pin_clk, 1);
> - -        s3c2410_gpio_setpin(pdata->pin_clk, 1);
> - -        shifter <<= 1;
> ~     }
>
> ~     for (n = 0; n < 5; n++) { /* 5 consequetive registers */
> ~         for (n1 = 0; n1 < 8; n1++) { /* 8 bits each */
> ~             s3c2410_gpio_setpin(pdata->pin_clk, 0);
> - -            s3c2410_gpio_setpin(pdata->pin_clk, 0);
> ~             shifter <<= 1;
> ~             if (s3c2410_gpio_getpin(pdata->pin_miso))
> ~                 shifter |= 1;
> ~             s3c2410_gpio_setpin(pdata->pin_clk, 1);
> - -            s3c2410_gpio_setpin(pdata->pin_clk, 1);
> ~         }
> ~         switch (n) {
> ~         case 0:
> @@ -1126,7 +1113,6 @@ void gat02_lis302dl_bitbang_read(struct
> lis302dl_info *lis)
> ~         }
> ~     }
> ~     s3c2410_gpio_setpin(pdata->pin_chip_select, 1);
> - -    s3c2410_gpio_setpin(other_cs, 1);
> ~     local_irq_restore(flags);
>
> ~     input_sync(lis->input_dev);
> diff --git a/drivers/input/misc/lis302dl.c 
> b/drivers/input/misc/lis302dl.c
> index 553a731..d01e153 100644
> - --- a/drivers/input/misc/lis302dl.c
> +++ b/drivers/input/misc/lis302dl.c
> @@ -274,10 +274,6 @@ static int lis302dl_input_open(struct input_dev 
> *inp)
> ~     reg_set_bit_mask(lis, LIS302DL_REG_CTRL1, ctrl1, ctrl1);
> ~     local_irq_restore(flags);
>
> - -    /* kick it off -- since we are edge triggered, if we missed the 
> edge
> - -     * permanent low interrupt is death for us */
> - -    (lis->pdata->lis302dl_bitbang_read)(lis);
> - -
> ~     return 0;
> ~ }
>
> @@ -404,8 +400,6 @@ static int __devinit lis302dl_probe(struct
> spi_device *spi)
> ~     mdelay(1);
>
> ~     reg_write(lis, LIS302DL_REG_CTRL2, 0);
> - -    reg_write(lis, LIS302DL_REG_CTRL3, LIS302DL_CTRL3_PP_OD |
> - -                                LIS302DL_CTRL3_IHL);
> ~     reg_write(lis, LIS302DL_REG_FF_WU_THS_1, 0x14);
> ~     reg_write(lis, LIS302DL_REG_FF_WU_DURATION_1, 0x00);
> ~     reg_write(lis, LIS302DL_REG_FF_WU_CFG_1, 0x95);
> @@ -436,7 +430,7 @@ static int __devinit lis302dl_probe(struct
> spi_device *spi)
> ~     lis->pdata = pdata;
>
> ~     rc = request_irq(lis->spi_dev->irq, lis302dl_interrupt,
> - -             IRQF_TRIGGER_FALLING, "lis302dl", lis);
> +             IRQF_TRIGGER_LOW, "lis302dl", lis);
> ~     if (rc < 0) {
> ~         dev_err(&spi->dev, "error requesting IRQ %d\n",
> ~             lis->spi_dev->irq);
>
> If the change to level helps, it feeds this question why is it possible
> for the hardware listening for edge to miss it... do we gratuitously
> clear the source register at some point, or masking the interrupt turns
> off latching of the edge detection for the duration?  Is it this
> business about logical edge being configured but somehow the s3c stuff
> "knows better" and has a level-type handler?  It feels like we are
> hiding the real problem by this like the workqueue solution (although a
> workaround removing the symptoms here shouldn't be sniffed at).


For the accelerometers, the problem boils down to the FIFO not being 
emptied when an interrupt occurs. If you just read 1 sample from the 
fifo on an interrupt and another sample comes in before you finish 
pulling it out, then the interrupt line doesn't toggle and you won't get 
any more edge triggers. You can also put it in a loop reading samples 
until the line toggles.

>
> | Anyway, I've not seen any problems with missed interrupts apart from
> | (maybe!) the resume case since you added the bitbang-all-the-way
> | patches. However, I'm not exactly a heavy user either. I just test the
> | accelerometers with the example python script from the wiki.

You really need to have something reading the input events and then put 
the phone under heavy load. If you don't test this, you could put us 
back to the starting point again where accelerometer data stops.

>
> Great I saw someone mentioned it as an issue on the community list the
> other day but it's hard to know what kernels people are running, they
> tend to mention them by distro name only.  It's quite possible he is
> running one cooked before that change.
>
> - -Andy
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org
>
> iEYEARECAAYFAkkenowACgkQOjLpvpq7dMrCmACfbc2siOlEc+RLkdVPU93jtTMC
> g4wAn2QaBdbiHGVmd/IOuf4yCxV5p1n2
> =UWvJ
> -----END PGP SIGNATURE-----
>




More information about the openmoko-kernel mailing list