Re: [PATCH v2 2/2] pxa/hx4700: Add Synaptics NavPoint (PXA27x SSP/SPI) driver

Re: [PATCH v2 2/2] pxa/hx4700: Add Synaptics NavPoint (PXA27x SSP/SPI) driver

[Paul Parsons]

Hi Igor,

Thanks for reviewing the driver. Specific responses below.

    This patch should be 1/n as the other patches depend on it,
    should not include the mfp stuff and should be sent also to
    linux-input list
    and the input subsystem maintainer (both added to CC).

OK, will do.

+/* Touchpad Controller */
+#define GPIO102_NAVPOINT_PWR    MFP_CFG_OUT(GPIO102, AF0, DRIVE_LOW)
+

    The name is not generic, so IMO can't be placed in the generic file.
    Can't you use the already existing GPIO102_GPIO macro
    and then change the direction in the board or driver file?

MFP_CFG_OUT(GPIO102, ...) had already moved from the patch v1 platform file to patch v2 mfp-pxa27x.h because it was suggested that MFP macros should not be used directly.
Changing the direction in the platform file would surely require using the MFP macros again, so how to keep everyone happy? Maybe I should just define a more generic name such as GPIO102_GPIO_OUT?
    
+config MOUSE_NAVPOINT_PXA27x
+    bool "Synaptics NavPoint (PXA27x SSP/SPI)"
+    depends on PXA27x && PXA_SSP
+    help
+      This option enables support for the Synaptics NavPoint connected to
+      a PXA27x SSP port in SPI slave mode. The driver implements a simple
+      navigation pad. The four raised dots are mapped to UP/DOWN/LEFT/RIGHT
+      buttons and the centre of the touchpad is mapped to the ENTER button.
+      Say Y to enable the Synaptics NavPoint on the HP iPAQ hx4700.

    There is no direct dependency for HP iPAQ hx4700,
    so in theory each board that want to use it can use it.
    I'd remove the "on the HP iPAQ ..." from the above sentence.

I copied that wording from my earlier ASIC3 LED driver. I suppose my feeling was that it seemed unlikely that any other platform would ever use this driver (otherwise I wouldn't be the first to write it). I'm happy to remove it.
    
+obj-$(CONFIG_MOUSE_NAVPOINT_PXA27x)    += navpoint.o
 
    This does not look like mouse device... Why place it in the mouse
    directory?
    I think the most reasonable place would be drivers/input/keyboard

Because the underlying hardware is a touchpad controller and most of the touchpad drivers live in the mouse directory. I could have added a mouse interface to this driver but chose not to (at least for now) because the hx4700 platform already has a working touchscreen controller; a second mouse device was not needed. If future platforms require a mouse interface then one could be added relatively easily; surely this would be preferable to replicating the whole driver.

+struct driver_data {
+    struct ssp_device *ssp;
+    int        gpio;
+    struct input_dev *input;
+    int        index;
+    uint8_t        data[8];
+    int        pressed;    /* 1 = pressed, 0 = released */
+    unsigned    code;        /* Key code of the last key pressed */

    Why do you need it?

The code field? To match a button release with a button press. See later.
    
+#define WEST    2416        /* 1/3 width */
+#define EAST    3904        /* 2/3 width */
+#define SOUTH    2480        /* 1/3 height */
+#define NORTH    3424        /* 2/3 height */

    May be supply those via the platform_data?

OK, will do.

+    drv_data = dev;

    Can be done in the declaration line.

OK.

+        if (pressed)
+            drv_data->code = code;

    Why do you need to store the code? You don't use it in any place...

I use it in the next line. The store is conditional whereas the read is unconditional. That's why I need to store it.

+    drv_data = dev;

    This can be done in the declaration line.

OK.

+    status = pxa_ssp_read_reg(ssp, SSSR);
+    ret = (status & (sssr | SSSR_RFS)) ? IRQ_HANDLED : IRQ_NONE;
+
+    if (status & sssr) {
+        dev_warn(dev, "spurious interrupt: 0x%02x\n", status);
+        pxa_ssp_write_reg(ssp, SSSR, (status & sssr));
+    }
+
+    while (status & SSSR_RNE) {
+        u32 data;
+
+        data = pxa_ssp_read_reg(ssp, SSDR);
+        drv_data->data[drv_data->index + 0] = (data >> 8);
+        drv_data->data[drv_data->index + 1] = data;
+        drv_data->index += 2;
+        if ((drv_data->data[0] & 0x07) < drv_data->index)
+            navpoint_packet(dev);
+        status = pxa_ssp_read_reg(ssp, SSSR);
+    }

    Should all this really be done in the interrupt context?

Yes. The FIFO controller triggers a CPU interrupt when the number of RX FIFO entries >= SSCR1.RFT (RX FIFO threshold), which I have set to 1. If I don't drain the RX FIFO then the interrupt source will not be cleared and the interrupt handler will be re-entered immediately (I have verified this). In short, draining the RX FIFO clears the interrupt.

+    drv_data = dev_get_drvdata(dev);
+    ssp = drv_data->ssp;

    The above two can be done in the declaration line...

OK.

+    drv_data = dev_get_drvdata(dev);
+    ssp = drv_data->ssp;

    The above two can be done in the declaration line...

OK.

+    drv_data->gpio = pdata->gpio;

    I'd suggest checking if the supplied gpio is valid
    and also may be configure it for output?

The suspend and resume functions check that the gpio is valid (which is taken to be non-zero) before using it. The platform file has already configured the gpio for output; it's the GPIO102 discussed earlier.

+    (void) navpoint_resume(&pdev->dev);

    Will this compile if !CONFIG_SUSPEND?

oops. OK.

+    drv_data = platform_get_drvdata(pdev);
+    ssp = drv_data->ssp;
+    input = drv_data->input;

    You need neither ssp nor input variables - you only use them once.
    This will remove 4 lines...

Actually the ssp variable is used twice. Regardless, by declaring those two variables I ensured that most of navpoint_remove() was identical to the error return of navpoint_probe(). I suppose I value consistency more than saving lines. I'm happy to move those two assignments to the declaration line.

+    (void) navpoint_suspend(&pdev->dev);

    and will this compile if !CONFIG_SUSPEND?

oops again. OK.

Regards,
Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] pxa/hx4700: Add Synaptics NavPoint (PXA27x SSP/SPI) driver

[Eric Miao]

[-- Attachment #1: Type: text/plain, Size: 7047 bytes --]

On Sun, Aug 14, 2011 at 6:31 PM, Paul Parsons <lost.distance@yahoo.com> wrote:
> Hi Igor,
>
> Thanks for reviewing the driver. Specific responses below.
>
>     This patch should be 1/n as the other patches depend on it,
>     should not include the mfp stuff and should be sent also to
>     linux-input list
>     and the input subsystem maintainer (both added to CC).
>
> OK, will do.
>
> +/* Touchpad Controller */
> +#define GPIO102_NAVPOINT_PWR    MFP_CFG_OUT(GPIO102, AF0, DRIVE_LOW)
> +
>
>     The name is not generic, so IMO can't be placed in the generic file.
>     Can't you use the already existing GPIO102_GPIO macro
>     and then change the direction in the board or driver file?
>
> MFP_CFG_OUT(GPIO102, ...) had already moved from the patch v1 platform file to patch v2 mfp-pxa27x.h because it was suggested that MFP macros should not be used directly.
> Changing the direction in the platform file would surely require using the MFP macros again, so how to keep everyone happy? Maybe I should just define a more generic name such as GPIO102_GPIO_OUT?

I guess we need a new definition for this config, looks like SCLK here
is different from a normal configuration because the pxa27x is used
as a SPI slave here?

And checked a bit with the pxa27x developer manual, there is no SCLK
for this GPIO102????

>
> +config MOUSE_NAVPOINT_PXA27x
> +    bool "Synaptics NavPoint (PXA27x SSP/SPI)"
> +    depends on PXA27x && PXA_SSP
> +    help
> +      This option enables support for the Synaptics NavPoint connected to
> +      a PXA27x SSP port in SPI slave mode. The driver implements a simple
> +      navigation pad. The four raised dots are mapped to UP/DOWN/LEFT/RIGHT
> +      buttons and the centre of the touchpad is mapped to the ENTER button.
> +      Say Y to enable the Synaptics NavPoint on the HP iPAQ hx4700.
>
>     There is no direct dependency for HP iPAQ hx4700,
>     so in theory each board that want to use it can use it.
>     I'd remove the "on the HP iPAQ ..." from the above sentence.
>
> I copied that wording from my earlier ASIC3 LED driver. I suppose my feeling was that it seemed unlikely that any other platform would ever use this driver (otherwise I wouldn't be the first to write it). I'm happy to remove it.
>
> +obj-$(CONFIG_MOUSE_NAVPOINT_PXA27x)    += navpoint.o
>
>     This does not look like mouse device... Why place it in the mouse
>     directory?
>     I think the most reasonable place would be drivers/input/keyboard
>
> Because the underlying hardware is a touchpad controller and most of the touchpad drivers live in the mouse directory. I could have added a mouse interface to this driver but chose not to (at least for now) because the hx4700 platform already has a working touchscreen controller; a second mouse device was not needed. If future platforms require a mouse interface then one could be added relatively easily; surely this would be preferable to replicating the whole driver.
>
> +struct driver_data {
> +    struct ssp_device *ssp;
> +    int        gpio;
> +    struct input_dev *input;
> +    int        index;
> +    uint8_t        data[8];
> +    int        pressed;    /* 1 = pressed, 0 = released */
> +    unsigned    code;        /* Key code of the last key pressed */
>
>     Why do you need it?
>
> The code field? To match a button release with a button press. See later.
>
> +#define WEST    2416        /* 1/3 width */
> +#define EAST    3904        /* 2/3 width */
> +#define SOUTH    2480        /* 1/3 height */
> +#define NORTH    3424        /* 2/3 height */
>
>     May be supply those via the platform_data?
>
> OK, will do.
>
> +    drv_data = dev;
>
>     Can be done in the declaration line.
>
> OK.
>
> +        if (pressed)
> +            drv_data->code = code;
>
>     Why do you need to store the code? You don't use it in any place...
>
> I use it in the next line. The store is conditional whereas the read is unconditional. That's why I need to store it.
>
> +    drv_data = dev;
>
>     This can be done in the declaration line.
>
> OK.
>
> +    status = pxa_ssp_read_reg(ssp, SSSR);
> +    ret = (status & (sssr | SSSR_RFS)) ? IRQ_HANDLED : IRQ_NONE;
> +
> +    if (status & sssr) {
> +        dev_warn(dev, "spurious interrupt: 0x%02x\n", status);
> +        pxa_ssp_write_reg(ssp, SSSR, (status & sssr));
> +    }
> +
> +    while (status & SSSR_RNE) {
> +        u32 data;
> +
> +        data = pxa_ssp_read_reg(ssp, SSDR);
> +        drv_data->data[drv_data->index + 0] = (data >> 8);
> +        drv_data->data[drv_data->index + 1] = data;
> +        drv_data->index += 2;
> +        if ((drv_data->data[0] & 0x07) < drv_data->index)
> +            navpoint_packet(dev);
> +        status = pxa_ssp_read_reg(ssp, SSSR);
> +    }
>
>     Should all this really be done in the interrupt context?
>
> Yes. The FIFO controller triggers a CPU interrupt when the number of RX FIFO entries >= SSCR1.RFT (RX FIFO threshold), which I have set to 1. If I don't drain the RX FIFO then the interrupt source will not be cleared and the interrupt handler will be re-entered immediately (I have verified this). In short, draining the RX FIFO clears the interrupt.
>
> +    drv_data = dev_get_drvdata(dev);
> +    ssp = drv_data->ssp;
>
>     The above two can be done in the declaration line...
>
> OK.
>
> +    drv_data = dev_get_drvdata(dev);
> +    ssp = drv_data->ssp;
>
>     The above two can be done in the declaration line...
>
> OK.
>
> +    drv_data->gpio = pdata->gpio;
>
>     I'd suggest checking if the supplied gpio is valid
>     and also may be configure it for output?
>
> The suspend and resume functions check that the gpio is valid (which is taken to be non-zero) before using it. The platform file has already configured the gpio for output; it's the GPIO102 discussed earlier.
>
> +    (void) navpoint_resume(&pdev->dev);
>
>     Will this compile if !CONFIG_SUSPEND?
>
> oops. OK.
>
> +    drv_data = platform_get_drvdata(pdev);
> +    ssp = drv_data->ssp;
> +    input = drv_data->input;
>
>     You need neither ssp nor input variables - you only use them once.
>     This will remove 4 lines...
>
> Actually the ssp variable is used twice. Regardless, by declaring those two variables I ensured that most of navpoint_remove() was identical to the error return of navpoint_probe(). I suppose I value consistency more than saving lines. I'm happy to move those two assignments to the declaration line.
>
> +    (void) navpoint_suspend(&pdev->dev);
>
>     and will this compile if !CONFIG_SUSPEND?
>
> oops again. OK.
>
> Regards,
> Paul
>

[-- Attachment #2: gpio-102.png --]
[-- Type: image/png, Size: 11983 bytes --]

Re: [PATCH v2 2/2] pxa/hx4700: Add Synaptics NavPoint (PXA27x SSP/SPI) driver

[Paul Parsons]

--- On Mon, 15/8/11, Eric Miao <eric.y.miao@gmail.com> wrote:
> > MFP_CFG_OUT(GPIO102, ...) had already moved from the
> patch v1 platform file to patch v2 mfp-pxa27x.h because it
> was suggested that MFP macros should not be used directly.
> > Changing the direction in the platform file would
> surely require using the MFP macros again, so how to keep
> everyone happy? Maybe I should just define a more generic
> name such as GPIO102_GPIO_OUT?
> 
> I guess we need a new definition for this config, looks
> like SCLK here
> is different from a normal configuration because the pxa27x
> is used
> as a SPI slave here?
> 
> And checked a bit with the pxa27x developer manual, there
> is no SCLK
> for this GPIO102????

GPIO102 is apparently a navpoint power enable; the old linux-2.6.21-hh20 kernel from handhelds.org defines it thus:

#define GPIO_NR_HX4700_SYNAPTICS_POWER_ON        102

I verified that it does need to be asserted before the navpoint will work. I don't possess any hx4700 developer documentation; perhaps Philipp can shed some light on GPIO102.

Re: [PATCH v2 2/2] pxa/hx4700: Add Synaptics NavPoint (PXA27x SSP/SPI) driver

[Eric Miao]

On Mon, Aug 15, 2011 at 5:49 PM, Paul Parsons <lost.distance@yahoo.com> wrote:
> --- On Mon, 15/8/11, Eric Miao <eric.y.miao@gmail.com> wrote:
>> > MFP_CFG_OUT(GPIO102, ...) had already moved from the
>> patch v1 platform file to patch v2 mfp-pxa27x.h because it
>> was suggested that MFP macros should not be used directly.
>> > Changing the direction in the platform file would
>> surely require using the MFP macros again, so how to keep
>> everyone happy? Maybe I should just define a more generic
>> name such as GPIO102_GPIO_OUT?
>>
>> I guess we need a new definition for this config, looks
>> like SCLK here
>> is different from a normal configuration because the pxa27x
>> is used
>> as a SPI slave here?
>>
>> And checked a bit with the pxa27x developer manual, there
>> is no SCLK
>> for this GPIO102????
>
> GPIO102 is apparently a navpoint power enable; the old linux-2.6.21-hh20 kernel from handhelds.org defines it thus:
>
> #define GPIO_NR_HX4700_SYNAPTICS_POWER_ON        102
>
> I verified that it does need to be asserted before the navpoint will work. I don't possess any hx4700 developer documentation; perhaps Philipp can shed some light on GPIO102.
>

Ah, that case it's simply a GPIO then. You could simply use GPIO102_GPIO
to configure the pin. And direction is controlled by gpio_direction_*().

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/2] pxa/hx4700: Add Synaptics NavPoint (PXA27x SSP/SPI) driver

[Igor Grinberg]

My mailer started converting stuff to html from some point, so I send this again, sorry..


On 08/14/11 13:31, Paul Parsons wrote:

> Hi Igor,
>
> Thanks for reviewing the driver. Specific responses below.
>
>     This patch should be 1/n as the other patches depend on it,
>     should not include the mfp stuff and should be sent also to
>     linux-input list
>     and the input subsystem maintainer (both added to CC).
>
> OK, will do.
>
> +/* Touchpad Controller */
> +#define GPIO102_NAVPOINT_PWR    MFP_CFG_OUT(GPIO102, AF0, DRIVE_LOW)
> +
>
>     The name is not generic, so IMO can't be placed in the generic file.
>     Can't you use the already existing GPIO102_GPIO macro
>     and then change the direction in the board or driver file?
>
> MFP_CFG_OUT(GPIO102, ...) had already moved from the patch v1 platform file to patch v2 mfp-pxa27x.h because it was suggested that MFP macros should not be used directly.
> Changing the direction in the platform file would surely require using the MFP macros again, so how to keep everyone happy? Maybe I should just define a more generic name such as GPIO102_GPIO_OUT?

No, you will not need to use the MFP macros, just gpio_direction_*() calls.
Eric has already answered this and I agree with him.
Use the MFP to configure the alternate function and other MFP related stuff
and use the gpio_direction_*() calls later to set the direction of the GPIO.

>     
> +config MOUSE_NAVPOINT_PXA27x
> +    bool "Synaptics NavPoint (PXA27x SSP/SPI)"
> +    depends on PXA27x && PXA_SSP
> +    help
> +      This option enables support for the Synaptics NavPoint connected to
> +      a PXA27x SSP port in SPI slave mode. The driver implements a simple
> +      navigation pad. The four raised dots are mapped to UP/DOWN/LEFT/RIGHT
> +      buttons and the centre of the touchpad is mapped to the ENTER button.
> +      Say Y to enable the Synaptics NavPoint on the HP iPAQ hx4700.
>
>     There is no direct dependency for HP iPAQ hx4700,
>     so in theory each board that want to use it can use it.
>     I'd remove the "on the HP iPAQ ..." from the above sentence.
>
> I copied that wording from my earlier ASIC3 LED driver. I suppose my feeling was that it seemed unlikely that any other platform would ever use this driver (otherwise I wouldn't be the first to write it). I'm happy to remove it.
>     
> +obj-$(CONFIG_MOUSE_NAVPOINT_PXA27x)    += navpoint.o
>  
>     This does not look like mouse device... Why place it in the mouse
>     directory?
>     I think the most reasonable place would be drivers/input/keyboard
>
> Because the underlying hardware is a touchpad controller and most of the touchpad drivers live in the mouse directory. I could have added a mouse interface to this driver but chose not to (at least for now) because the hx4700 platform already has a working touchscreen controller; a second mouse device was not needed. If future platforms require a mouse interface then one could be added relatively easily; surely this would be preferable to replicating the whole driver.

So the device is a full touchpad? and can be used for mouse pointer?
Is the same device used as mouse pointer by some other driver?

> +struct driver_data {
> +    struct ssp_device *ssp;
> +    int        gpio;
> +    struct input_dev *input;
> +    int        index;
> +    uint8_t        data[8];
> +    int        pressed;    /* 1 = pressed, 0 = released */
> +    unsigned    code;        /* Key code of the last key pressed */
>
>     Why do you need it?
>
> The code field? To match a button release with a button press. See later.
>     
> +#define WEST    2416        /* 1/3 width */
> +#define EAST    3904        /* 2/3 width */
> +#define SOUTH    2480        /* 1/3 height */
> +#define NORTH    3424        /* 2/3 height */
>
>     May be supply those via the platform_data?
>
> OK, will do.
>
> +    drv_data = dev;
>
>     Can be done in the declaration line.
>
> OK.
>
> +        if (pressed)
> +            drv_data->code = code;
>
>     Why do you need to store the code? You don't use it in any place...
>
> I use it in the next line. The store is conditional whereas the read is unconditional. That's why I need to store it.

Ok. I've missed that fact, sorry..

> +    drv_data = dev;
>
>     This can be done in the declaration line.
>
> OK.
>
> +    status = pxa_ssp_read_reg(ssp, SSSR);
> +    ret = (status & (sssr | SSSR_RFS)) ? IRQ_HANDLED : IRQ_NONE;
> +
> +    if (status & sssr) {
> +        dev_warn(dev, "spurious interrupt: 0x%02x\n", status);
> +        pxa_ssp_write_reg(ssp, SSSR, (status & sssr));
> +    }
> +
> +    while (status & SSSR_RNE) {
> +        u32 data;
> +
> +        data = pxa_ssp_read_reg(ssp, SSDR);
> +        drv_data->data[drv_data->index + 0] = (data >> 8);
> +        drv_data->data[drv_data->index + 1] = data;
> +        drv_data->index += 2;
> +        if ((drv_data->data[0] & 0x07) < drv_data->index)
> +            navpoint_packet(dev);
> +        status = pxa_ssp_read_reg(ssp, SSSR);
> +    }
>
>     Should all this really be done in the interrupt context?
>
> Yes. The FIFO controller triggers a CPU interrupt when the number of RX FIFO entries >= SSCR1.RFT (RX FIFO threshold), which I have set to 1. If I don't drain the RX FIFO then the interrupt source will not be cleared and the interrupt handler will be re-entered immediately (I have verified this). In short, draining the RX FIFO clears the interrupt.
>
> +    drv_data = dev_get_drvdata(dev);
> +    ssp = drv_data->ssp;
>
>     The above two can be done in the declaration line...
>
> OK.
>
> +    drv_data = dev_get_drvdata(dev);
> +    ssp = drv_data->ssp;
>
>     The above two can be done in the declaration line...
>
> OK.
>
> +    drv_data->gpio = pdata->gpio;
>
>     I'd suggest checking if the supplied gpio is valid
>     and also may be configure it for output?
>
> The suspend and resume functions check that the gpio is valid (which is taken to be non-zero) before using it. The platform file has already configured the gpio for output; it's the GPIO102 discussed earlier.

What I'm suggesting here is check that the gpio is valid here and only once
and not in the suspend/resume functions. That gpio is not going to change,
so there is no point is checking it each time in suspend/resume.
Also the check for non-zero is not correct. 0 is a valid gpio number on most platforms.
You need to use the gpio_is_valid() call for this.

> +    (void) navpoint_resume(&pdev->dev);
>
>     Will this compile if !CONFIG_SUSPEND?
>
> oops. OK.
>
> +    drv_data = platform_get_drvdata(pdev);
> +    ssp = drv_data->ssp;
> +    input = drv_data->input;
>
>     You need neither ssp nor input variables - you only use them once.
>     This will remove 4 lines...
>
> Actually the ssp variable is used twice. Regardless, by declaring those two variables I ensured that most of navpoint_remove() was identical to the error return of navpoint_probe(). I suppose I value consistency more than saving lines. I'm happy to move those two assignments to the declaration line.
>
> +    (void) navpoint_suspend(&pdev->dev);
>
>     and will this compile if !CONFIG_SUSPEND?
>
> oops again. OK.
>
> Regards,
> Paul
>

-- 
Regards,
Igor.

Re: [PATCH v2 2/2] pxa/hx4700: Add Synaptics NavPoint (PXA27x SSP/SPI) driver

[Paul Parsons]

> > MFP_CFG_OUT(GPIO102, ...) had already moved from the
> patch v1 platform file to patch v2 mfp-pxa27x.h because it
> was suggested that MFP macros should not be used directly.
> > Changing the direction in the platform file would
> surely require using the MFP macros again, so how to keep
> everyone happy? Maybe I should just define a more generic
> name such as GPIO102_GPIO_OUT?
> 
> No, you will not need to use the MFP macros, just
> gpio_direction_*() calls.
> Eric has already answered this and I agree with him.
> Use the MFP to configure the alternate function and other
> MFP related stuff
> and use the gpio_direction_*() calls later to set the
> direction of the GPIO.

OK, will do.

> > Because the underlying hardware is a touchpad
> controller and most of the touchpad drivers live in the
> mouse directory. I could have added a mouse interface to
> this driver but chose not to (at least for now) because the
> hx4700 platform already has a working touchscreen
> controller; a second mouse device was not needed. If future
> platforms require a mouse interface then one could be added
> relatively easily; surely this would be preferable to
> replicating the whole driver.
> 
> So the device is a full touchpad? and can be used for mouse
> pointer?

Yes, that's correct.

> Is the same device used as mouse pointer by some other
> driver?

Not as far as I can see. The word "NavPoint" appears only once in the current kernel source: within the hx4700 platform file. It's possible that other PDA models of similar 2004 vintage used the device, but I don't know whether any of them are supported in the kernel.

> > The suspend and resume functions check that the gpio
> is valid (which is taken to be non-zero) before using it.
> The platform file has already configured the gpio for
> output; it's the GPIO102 discussed earlier.
> 
> What I'm suggesting here is check that the gpio is valid
> here and only once
> and not in the suspend/resume functions. That gpio is not
> going to change,
> so there is no point is checking it each time in
> suspend/resume.
> Also the check for non-zero is not correct. 0 is a valid
> gpio number on most platforms.
> You need to use the gpio_is_valid() call for this.

I understand. My feeling was that other platforms wanting to use this driver would not necessarily use a gpio for power on/off; indeed the gpio field was originally a callback to provide full flexibility. Hence suspend / resume needed to use the gpio conditionally anyway. Nevertheless I'm happy to make the change and leave it for future platform developers to revisit suspend / resume as necessary.

Regards,
Paul