Re: [PATCH 1/3] Input: ads7846 - introduce .gpio_pendown to get pendown state

[Russell King - ARM Linux <linux@xxxxxxxxxxxxxxxx>]

On Thu, Sep 04, 2008 at 05:21:05PM +0800, Eric Miao wrote:
> The GPIO connected to ADS7846 nPENIRQ signal is usually used to get
> the pendown state as well. Introduce a .gpio_pendown, and use this
> to decide the pendown state if .get_pendown_state is NULL.

> +static int get_pendown_state(struct ads7846 *ts)
> +{
> +	if (ts->get_pendown_state)
> +		return ts->get_pendown_state();
> +
> +	if (ts->gpio_pendown != -1)
> +		return !gpio_get_value(ts->gpio_pendown);
> +
> +	return 0;
> +}
> +


> +	int	gpio_pendown;		/* the GPIO usually connected to PENIRQ
> +					 * and used to decide pendown state
> +					 * (-1) to use (*get_pendown_state) */
> +

The comment doesn't match the code.  get_pendown_state() is used if it's
non-NULL, otherwise, the GPIO if it's not -1.

I'm getting a little worried about these tests for valid gpio numbers in
drivers - some check for non-zero, this one checks for not -1.  I get the
feeling that this is storing up problems for later.

Maybe using gpio_is_valid() would be a good idea?

But... a related question: do we need to do the check here?  Surely either
a pendown function is going to be supplied, or if not we're going to use
a GPIO, in which case the GPIO better be valid.  Maybe something to check
at driver initialisation time only?

-------------------------------------------------------------------
List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel
FAQ:        http://www.arm.linux.org.uk/mailinglists/faq.php
Etiquette:  http://www.arm.linux.org.uk/mailinglists/etiquette.php