[PATCH] gpio: extend gpiod_get*() with flags parameter
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Jul 24 09:23:39 PDT 2014
Hi Alexandre,
Thank you for the patch.
On Friday 25 July 2014 00:04:58 Alexandre Courbot wrote:
> The huge majority of GPIOs have their direction and initial value set
> right after being obtained by one of the gpiod_get() functions. The
> integer GPIO API had gpio_request_one() that took a convenience flags
> parameter allowing to specify an direction and value applied to the
> returned GPIO. This feature greatly simplifies client code and ensures
> errors are always handled properly.
>
> A similar feature has been requested for the gpiod API. Since GPIOs need
> a direction to be used anyway, we prefer to extend the existing
> functions instead of introducing new functions that would raise the
> number of gpiod getters to 16 (!).
>
> The drawback of this approach is that all gpiod clients need to be
> updated, but there aren't that many and the moment and this results in
> smaller (and hopefully safer) code.
>
> Signed-off-by: Alexandre Courbot <acourbot at nvidia.com>
> ---
> This change will be difficult to apply without breaking things, but
> let's try to do it right. Hopefully the benefit will outweight the
> disturbance.
>
> This is a patch against -next to list and update all current gpiod
> consumers. Updates are trivial at first sight, but it would be nice to
> get as many acks as possible from the respective subsystem maintainers.
>
> I'm not sure how this could be applied harmlessly though - maybe through
> a dedicated branch for -next? Problem is that a lot of new code is not
> yet merged into mainline, and conflicts are very likely to occur. Linus,
> do you have any suggestion as to how this can be done without blood being
> spilled?
>
> Documentation/gpio/consumer.txt | 26 ++++++++---
> drivers/gpio/devres.c | 24 ++++++----
> drivers/gpio/gpiolib.c | 53 +++++++++++++------
> drivers/gpu/drm/panel/panel-ld9040.c | 7 +--
> drivers/gpu/drm/panel/panel-s6e8aa0.c | 7 +--
> drivers/gpu/drm/panel/panel-simple.c | 16 ++-----
> drivers/hsi/clients/nokia-modem.c | 7 +--
> drivers/i2c/muxes/i2c-mux-pca954x.c | 4 +-
> drivers/iio/accel/kxcjk-1013.c | 6 +--
> drivers/input/keyboard/clps711x-keypad.c | 6 +--
> drivers/input/misc/gpio-beeper.c | 6 +--
> drivers/input/misc/soc_button_array.c | 2 +-
> drivers/media/i2c/adv7604.c | 6 +--
> drivers/mfd/intel_soc_pmic_core.c | 2 +-
> drivers/mmc/core/slot-gpio.c | 6 +--
> drivers/net/phy/at803x.c | 4 +-
> drivers/power/reset/gpio-poweroff.c | 21 ++-------
> drivers/tty/serial/serial_mctrl_gpio.c | 2 +-
> drivers/video/backlight/pwm_bl.c | 6 +--
> drivers/video/fbdev/omap2/displays-new/panel-dpi.c | 12 ++---
> .../omap2/displays-new/panel-lgphilips-lb035q02.c | 6 +--
> .../omap2/displays-new/panel-sharp-ls037v7dw01.c | 7 +--
> include/linux/gpio/consumer.h | 38 ++++++++++++----
> net/rfkill/rfkill-gpio.c | 16 ++-----
> sound/soc/codecs/adau1977.c | 11 ++---
> sound/soc/codecs/cs4265.c | 5 +-
> sound/soc/codecs/sta350.c | 9 ++--
> sound/soc/codecs/tas2552.c | 4 +-
> sound/soc/jz4740/qi_lb60.c | 10 +---
> sound/soc/omap/rx51.c | 29 +++---------
> sound/soc/soc-jack.c | 9 ++--
> 31 files changed, 160 insertions(+), 207 deletions(-)
>
> diff --git a/Documentation/gpio/consumer.txt
> b/Documentation/gpio/consumer.txt index 7ff30d2..a3fb1d7 100644
> --- a/Documentation/gpio/consumer.txt
> +++ b/Documentation/gpio/consumer.txt
> @@ -29,13 +29,24 @@ gpiod_get() functions. Like many other kernel
> subsystems, gpiod_get() takes the device that will use the GPIO and the
> function the requested GPIO is supposed to fulfill:
>
> - struct gpio_desc *gpiod_get(struct device *dev, const char *con_id)
> + struct gpio_desc *gpiod_get(struct device *dev, const char *con_id,
> + enum gpio_flags flags)
I assume you mean enum gpiod_flags here and in the rest of the documentation.
> If a function is implemented by using several GPIOs together (e.g. a simple
> LED device that displays digits), an additional index argument can be
> specified:
>
> struct gpio_desc *gpiod_get_index(struct device *dev,
> - const char *con_id, unsigned int idx)
> + const char *con_id, unsigned int idx,
> + enum gpio_flags flags)
> +
> +The flags parameter is used to optionally specify a direction and initial
> value +for the GPIO. Values can be:
> +
> +* AS_IS or 0 to not initialize the GPIO at all. The direction must be set
> later
> + with one of the dedicated functions.
> +* INPUT to initialize the GPIO as input.
> +* OUTPUT_LOW to initialize the GPIO as output with a value of 0.
> +* OUTPUT_HIGH to initialize the GPIO as output with a value of 1.
Pretty please, could you at least prefix that with GPIOD_ ? Those names are
too generic.
How about renaming them to GPIOD_AS_IS, GPIOD_IN, GPIOD_OUT_INIT_LOW and
GPIOD_OUT_INIT_HIGH in order to match the GPIOF_ flags ?
> Both functions return either a valid GPIO descriptor, or an error code
> checkable with IS_ERR() (they will never return a NULL pointer). -ENOENT
> will be returned @@ -49,11 +60,13 @@ GPIO has been assigned to the
> requested function:
>
> Device-managed variants of these functions are also defined:
>
> - struct gpio_desc *devm_gpiod_get(struct device *dev, const char *con_id)
> + struct gpio_desc *devm_gpiod_get(struct device *dev, const char *con_id,
> + enum gpio_flags flags)
>
> struct gpio_desc *devm_gpiod_get_index(struct device *dev,
> const char *con_id,
> - unsigned int idx)
> + unsigned int idx,
> + enum gpio_flags flags)
>
> devm_gpiod_get_optional() and devm_gpiod_get_index_optional() exist as
> well.
>
> @@ -72,8 +85,9 @@ Using GPIOs
>
> Setting Direction
> -----------------
> -The first thing a driver must do with a GPIO is setting its direction. This
> is -done by invoking one of the gpiod_direction_*() functions:
> +The first thing a driver must do with a GPIO is setting its direction. If
> no +direction-setting flags as been given to one of the gpiod_get*()
> functions, this +is done by invoking one of the gpiod_direction_*()
> functions:
>
> int gpiod_direction_input(struct gpio_desc *desc)
> int gpiod_direction_output(struct gpio_desc *desc, int value)
--
Regards,
Laurent Pinchart
More information about the dri-devel
mailing list