From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Eric Miao" Subject: Re: [PATCH 1/3] Input: ads7846 - introduce .gpio_pendown to get pendown state Date: Thu, 4 Sep 2008 18:39:08 +0800 Message-ID: References: <20080904100051.GC10426@flint.arm.linux.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Return-path: Received: from yx-out-2324.google.com ([74.125.44.30]:58739 "EHLO yx-out-2324.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752578AbYIDKjJ (ORCPT ); Thu, 4 Sep 2008 06:39:09 -0400 Received: by yx-out-2324.google.com with SMTP id 8so1859162yxm.1 for ; Thu, 04 Sep 2008 03:39:08 -0700 (PDT) In-Reply-To: Content-Disposition: inline Sender: linux-input-owner@vger.kernel.org List-Id: linux-input@vger.kernel.org To: Russell King - ARM Linux Cc: ARM Linux , linux-input@vger.kernel.org On Thu, Sep 4, 2008 at 6:38 PM, Eric Miao wrote: > On Thu, Sep 4, 2008 at 6:00 PM, Russell King - ARM Linux > 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 :)