Re: [PATCH v3 2/4] gpiolib: Add ability to get GPIO pin direction

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

On Thu, Feb 17, 2011 at 05:03:17PM -0600, Peter Tyser wrote:
> Add a new get_direction() function to the gpio_chip structure.  This is
> useful so that the direction of a pin can be determined when its
> initially exported.  Previously, the direction defaulted to "unknown"
> regardless of the actual configuration of the GPIO pin.
> 
> If a GPIO driver implements get_direction(), it is called in
> gpio_request() to set the initial direction of the pin accurately.
IMHO the commit log is conceptually wrong, because it talks about a
"pin".  Better use gpio here.

> Cc: Alek Du <alek.du@intel.com>
> Cc: Samuel Ortiz <sameo@linux.intel.com>
> Cc: David Brownell <dbrownell@users.sourceforge.net>
> Cc: Eric Miao <eric.y.miao@gmail.com>
> Cc: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Cc: Joe Perches <joe@perches.com>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Signed-off-by: Peter Tyser <ptyser@xes-inc.com>
> ---
> Changes since v1:
> - Add support for "unknown" direction
> 
> Changes since v2:
>   Based on Wolfram's feedback:
> - Use GPIOF_DIR_* flags as returns from get_direction()
> - Call spin_lock_irqsave() to before setting flags
> 
>  drivers/gpio/gpiolib.c     |   23 +++++++++++++++++++++++
>  include/asm-generic/gpio.h |    4 ++++
>  2 files changed, 27 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index eb74311..a656a2c 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1174,6 +1174,7 @@ int gpio_request(unsigned gpio, const char *label)
>  	struct gpio_desc	*desc;
>  	struct gpio_chip	*chip;
>  	int			status = -EINVAL;
> +	int			dir;
>  	unsigned long		flags;
>  
>  	spin_lock_irqsave(&gpio_lock, flags);
> @@ -1214,6 +1215,28 @@ int gpio_request(unsigned gpio, const char *label)
>  		}
>  	}
>  
> +	if (chip->get_direction) {
> +		/* chip->get_direction may sleep */
> +		spin_unlock_irqrestore(&gpio_lock, flags);
might_sleep_if(chip->can_sleep) ?

> +		dir = chip->get_direction(chip, gpio - chip->base);
> +		spin_lock_irqsave(&gpio_lock, flags);
> +		switch (dir) {
> +		case GPIOF_DIR_OUT:
> +			set_bit(FLAG_DIR_OUT, &desc->flags);
> +			clear_bit(FLAG_DIR_IN, &desc->flags);
> +			break;
> +		case GPIOF_DIR_IN:
> +			set_bit(FLAG_DIR_IN, &desc->flags);
> +			clear_bit(FLAG_DIR_OUT, &desc->flags);
> +			break;
> +		default:
> +			/* Direction isn't known */
> +			clear_bit(FLAG_DIR_OUT, &desc->flags);
> +			clear_bit(FLAG_DIR_IN, &desc->flags);
> +			break;
> +		}
Alternatively to my suggestion for patch1:
	} else {
		clear_bit(FLAG_DIR_OUT, &desc->flags);
		clear_bit(FLAG_DIR_IN, &desc->flags);
> +	}
> +
>  done:
>  	if (status)
>  		pr_debug("gpio_request: gpio-%d (%s) status %d\n",
> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> index ff5c660..6af5a3e 100644
> --- a/include/asm-generic/gpio.h
> +++ b/include/asm-generic/gpio.h
> @@ -57,6 +57,8 @@ struct device_node;
>   * @direction_input: configures signal "offset" as input, or returns error
>   * @get: returns value for signal "offset"; for output signals this
>   *	returns either the value actually sensed, or zero
> + * @get_direction: optional hook to determine if a GPIO signal is configured
> + *	as an input, output, or unknown
>   * @direction_output: configures signal "offset" as output, or returns error
>   * @set: assigns output value for signal "offset"
>   * @to_irq: optional hook supporting non-static gpio_to_irq() mappings;
> @@ -101,6 +103,8 @@ struct gpio_chip {
>  						unsigned offset);
>  	int			(*get)(struct gpio_chip *chip,
>  						unsigned offset);
> +	int			(*get_direction)(struct gpio_chip *chip,
> +						unsigned offset);
>  	int			(*direction_output)(struct gpio_chip *chip,
>  						unsigned offset, int value);
>  	int			(*set_debounce)(struct gpio_chip *chip,
> -- 
> 1.7.0.4
> 
> 

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |