[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.