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

["Eric Miao" <eric.y.miao@gmail.com>]

On Thu, Sep 4, 2008 at 6:00 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> 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?
>

Indeed, I'll fix the other SPI GPIO CS patch as well.

> 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?
>

There is a warning If none is provided, and probe() will fail.
So actually get_pendown_state() will never reach the last
"return 0" statement.

-- 
Cheers
- eric