On Thu, Sep 4, 2008 at 6:38 PM, Eric Miao <eric.y.miao@xxxxxxxxx> wrote:
> On Thu, Sep 4, 2008 at 6:00 PM, Russell King - ARM Linux
> <linux@xxxxxxxxxxxxxxxx> 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.
>
So maybe I can simplify that code a bit more :)
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
