From: Linus Walleij <[email protected]>
This adds two functions for struct gpio_chip chips to provide pin
bias and drive mode settings for individual pins. Implementers does
this a bit differently and usually there are a few possible modes you
can select, I'm providing a few common modes for biasing and driving
pins.
Since we have no previous hacked-up arch-specific drivers for this
we can avoid any __override_functions and we just allow this to be
properly implemented using gpiolib. Further the function is made
non-mandatory, if it is not defined for the chip it will be silently
ignored.
Signed-off-by: Linus Walleij <[email protected]>
---
drivers/gpio/gpiolib.c | 43 ++++++++++++++++++++++++++++++++++++++
include/asm-generic/gpio.h | 12 ++++++++++
include/linux/gpio.h | 49 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 104 insertions(+), 0 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 36a2974..f79f4a3 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1573,6 +1573,49 @@ void __gpio_set_value(unsigned gpio, int value)
EXPORT_SYMBOL_GPL(__gpio_set_value);
/**
+ * gpio_set_bias() - set a gpio's bias
+ * @gpio: gpio whose bias will be set
+ * @bias: bias mode to set
+ * Context: process
+ *
+ * This is used to directly or indirectly to implement gpio_set_bias().
+ * It invokes the associated gpio_chip.set_bias() method. Usually this
+ * applies to input pins.
+ */
+void gpio_set_bias(unsigned gpio, enum gpio_bias bias)
+{
+ struct gpio_chip *chip;
+
+ chip = gpio_to_chip(gpio);
+ /* Implementing this is not mandatory */
+ if (chip->set_bias)
+ chip->set_bias(chip, gpio - chip->base, bias);
+}
+EXPORT_SYMBOL_GPL(gpio_set_bias);
+
+/**
+ * gpio_set_drive() - set a gpio's drive mode
+ * @gpio: gpio whose drive mode will be set
+ * @drive: drive mode to set
+ * Context: process
+ *
+ * This is used to directly or indirectly to implement gpio_set_drive().
+ * It invokes the associated gpio_chip.set_drive() method. Call this
+ * before the __gpio_set_output() function to enable special drive modes.
+ */
+void gpio_set_drive(unsigned gpio, enum gpio_drive drive)
+{
+ struct gpio_chip *chip;
+
+ chip = gpio_to_chip(gpio);
+ /* Implementing this is not mandatory */
+ if (chip->set_drive)
+ chip->set_drive(chip, gpio - chip->base, drive);
+}
+EXPORT_SYMBOL_GPL(gpio_set_drive);
+
+
+/**
* __gpio_cansleep() - report whether gpio value access will sleep
* @gpio: gpio in question
* Context: any
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index ff5c660..b4971b1 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -59,6 +59,8 @@ struct device_node;
* returns either the value actually sensed, or zero
* @direction_output: configures signal "offset" as output, or returns error
* @set: assigns output value for signal "offset"
+ * @set_bias: set a certain bias for the GPIO
+ * @set_drive: set a certain drive mode for the GPIO
* @to_irq: optional hook supporting non-static gpio_to_irq() mappings;
* implementation may not sleep
* @dbg_show: optional routine to show contents in debugfs; default code
@@ -109,6 +111,14 @@ struct gpio_chip {
void (*set)(struct gpio_chip *chip,
unsigned offset, int value);
+ void (*set_bias)(struct gpio_chip *chip,
+ unsigned offset,
+ enum gpio_bias bias);
+
+ void (*set_drive)(struct gpio_chip *chip,
+ unsigned offset,
+ enum gpio_drive drive);
+
int (*to_irq)(struct gpio_chip *chip,
unsigned offset);
@@ -158,6 +168,8 @@ extern int gpio_set_debounce(unsigned gpio, unsigned debounce);
extern int gpio_get_value_cansleep(unsigned gpio);
extern void gpio_set_value_cansleep(unsigned gpio, int value);
+extern void gpio_set_bias(unsigned gpio, enum gpio_bias bias);
+extern void gpio_set_drive(unsigned gpio, enum gpio_drive drive);
/* A platform's <asm/gpio.h> code may want to inline the I/O calls when
* the GPIO is constant and refers to some always-present controller,
diff --git a/include/linux/gpio.h b/include/linux/gpio.h
index 32720ba..6b48c15 100644
--- a/include/linux/gpio.h
+++ b/include/linux/gpio.h
@@ -3,6 +3,43 @@
/* see Documentation/gpio.txt */
+/**
+ * enum gpio_bias - bias modes for GPIOs
+ * @GPIO_BIAS_FLOAT: no specific bias, the GPIO will float or state is no
+ * controlled by software
+ * @GPIO_BIAS_PULL_UP: the GPIO will be pulled up (usually with high impedance
+ * to VDD)
+ * @GPIO_BIAS_PULL_DOWN: the GPIO will be pulled down (usually with high
+ * impedance to GROUND)
+ * @GPIO_BIAS_HIGH: the GPIO will be wired high, connected to VDD
+ * @GPIO_BIAS_GROUND: the GPIO will be grounded, connected to GROUND
+ */
+enum gpio_bias {
+ GPIO_BIAS_FLOAT,
+ GPIO_BIAS_PULL_UP,
+ GPIO_BIAS_PULL_DOWN,
+ GPIO_BIAS_HIGH,
+ GPIO_BIAS_GROUND,
+};
+
+/**
+ * enum gpio_drive - drive modes for GPIOs (output)
+ * @GPIO_DRIVE_PUSH_PULL: the GPIO will be driven actively high and low, this
+ * is the most typical case and is typically achieved with two active
+ * transistors on the output
+ * @GPIO_DRIVE_OPEN_DRAIN: the GPIO will be driven with open drain (open
+ * collector) which means it is usually wired with other output ports
+ * which are then pulled up with an external resistor
+ * @GPIO_DRIVE_OPEN_SOURCE: the GPIO will be driven with open drain
+ * (open emitter) which is the same as open drain mutatis mutandis but
+ * pulled to ground
+ */
+enum gpio_drive {
+ GPIO_DRIVE_PUSH_PULL,
+ GPIO_DRIVE_OPEN_DRAIN,
+ GPIO_DRIVE_OPEN_SOURCE,
+};
+
#ifdef CONFIG_GENERIC_GPIO
#include <asm/gpio.h>
@@ -90,6 +127,18 @@ static inline void gpio_set_value(unsigned gpio, int value)
WARN_ON(1);
}
+static inline void gpio_set_bias(unsigned gpio, enum gpio_bias bias)
+{
+ /* GPIO can never have been requested */
+ WARN_ON(1);
+}
+
+static inline void gpio_set_drive(unsigned gpio, enum gpio_drive drive)
+{
+ /* GPIO can never have been requested */
+ WARN_ON(1);
+}
+
static inline int gpio_cansleep(unsigned gpio)
{
/* GPIO can never have been requested or set as {in,out}put */
--
1.7.3.2
> + * enum gpio_drive - drive modes for GPIOs (output)
> + * @GPIO_DRIVE_PUSH_PULL: the GPIO will be driven actively high and low, this
> + * is the most typical case and is typically achieved with two active
> + * transistors on the output
> + * @GPIO_DRIVE_OPEN_DRAIN: the GPIO will be driven with open drain (open
> + * collector) which means it is usually wired with other output ports
> + * which are then pulled up with an external resistor
> + * @GPIO_DRIVE_OPEN_SOURCE: the GPIO will be driven with open drain
> + * (open emitter) which is the same as open drain mutatis mutandis but
> + * pulled to ground
How about "off". As I've pointed out at various times some devices have
GPIO pins that may or may not be connected depending upon what other chip
logic is doing.
As I've also noted with various other previous suggestions for API
additions you also need an "unknown" state so that a device can implement
the methods but indicate for some pins the value is unknown due to
hardware or firmware limitations.
2011/4/17 Alan Cox <[email protected]>:
>> + * enum gpio_drive - drive modes for GPIOs (output)
>> + * @GPIO_DRIVE_PUSH_PULL: the GPIO will be driven actively high and low, this
>> + * ? is the most typical case and is typically achieved with two active
>> + * ? transistors on the output
>> + * @GPIO_DRIVE_OPEN_DRAIN: the GPIO will be driven with open drain (open
>> + * ? collector) which means it is usually wired with other output ports
>> + * ? which are then pulled up with an external resistor
>> + * @GPIO_DRIVE_OPEN_SOURCE: the GPIO will be driven with open drain
>> + * ? (open emitter) which is the same as open drain mutatis mutandis but
>> + * ? pulled to ground
>
> How about "off". As I've pointed out at various times some devices have
> GPIO pins that may or may not be connected depending upon what other chip
> logic is doing.
Sure, why not.
> As I've also noted with various other previous suggestions for API
> additions you also need an "unknown" state so that a device can implement
> the methods but indicate for some pins the value is unknown due to
> hardware or firmware limitations.
OK do you want an unknown biasing mode too, or is "floating" enough
of a default to cover that?
Yours,
Linus Walleij
On Sun, 17 Apr 2011 23:58:16 +0200
Linus Walleij <[email protected]> wrote:
> 2011/4/17 Alan Cox <[email protected]>:
>
> >> + * enum gpio_drive - drive modes for GPIOs (output)
> >> + * @GPIO_DRIVE_PUSH_PULL: the GPIO will be driven actively high and low, this
> >> + * ? is the most typical case and is typically achieved with two active
> >> + * ? transistors on the output
> >> + * @GPIO_DRIVE_OPEN_DRAIN: the GPIO will be driven with open drain (open
> >> + * ? collector) which means it is usually wired with other output ports
> >> + * ? which are then pulled up with an external resistor
> >> + * @GPIO_DRIVE_OPEN_SOURCE: the GPIO will be driven with open drain
> >> + * ? (open emitter) which is the same as open drain mutatis mutandis but
> >> + * ? pulled to ground
> >
> > How about "off". As I've pointed out at various times some devices have
> > GPIO pins that may or may not be connected depending upon what other chip
> > logic is doing.
>
> Sure, why not.
>
> > As I've also noted with various other previous suggestions for API
> > additions you also need an "unknown" state so that a device can implement
> > the methods but indicate for some pins the value is unknown due to
> > hardware or firmware limitations.
>
> OK do you want an unknown biasing mode too, or is "floating" enough
> of a default to cover that?
I think I'd rather also have unknown for both the drive and the biasing.
It's a way of ensuring we get some kind of complete coverage even if not
perfect.
In the Intel case we also have 'alt status' stuff where the line has been
borrowed by firmware for example so we really genuinly haven't got a clue
what the state is right now.
Hi Linus,
It's really required feature for ARM architectures.
In case of samsung SoCs. it needs more things you described.
In case of setting the drive, we can set the drive strength, 1x, 2x, 3x, and 4x.
So can it add the more like this? or separate enumeration?
> +enum gpio_drive {
> + ? ? ? GPIO_DRIVE_PUSH_PULL,
> + ? ? ? GPIO_DRIVE_OPEN_DRAIN,
> + ? ? ? GPIO_DRIVE_OPEN_SOURCE,
GPIO_DRIVE_STRENGTH_1X,
GPIO_DRIVE_STRENGTH_2X,
GPIO_DRIVE_STRENGTH_3X,
GPIO_DRIVE_STRENGTH_4X,
> +};
or
enum gpio_drive_strength {
GPIO_DRIVE_STRENGTH_1X,
GPIO_DRIVE_STRENGTH_2X,
GPIO_DRIVE_STRENGTH_3X,
GPIO_DRIVE_STRENGTH_4X,
};
Thank you,
Kyungmin Park
On Mon, Apr 18, 2011 at 6:37 AM, Linus Walleij
<[email protected]> wrote:
> From: Linus Walleij <[email protected]>
>
> This adds two functions for struct gpio_chip chips to provide pin
> bias and drive mode settings for individual pins. Implementers does
> this a bit differently and usually there are a few possible modes you
> can select, I'm providing a few common modes for biasing and driving
> pins.
>
> Since we have no previous hacked-up arch-specific drivers for this
> we can avoid any __override_functions and we just allow this to be
> properly implemented using gpiolib. Further the function is made
> non-mandatory, if it is not defined for the chip it will be silently
> ignored.
>
> Signed-off-by: Linus Walleij <[email protected]>
> ---
> ?drivers/gpio/gpiolib.c ? ? | ? 43 ++++++++++++++++++++++++++++++++++++++
> ?include/asm-generic/gpio.h | ? 12 ++++++++++
> ?include/linux/gpio.h ? ? ? | ? 49 ++++++++++++++++++++++++++++++++++++++++++++
> ?3 files changed, 104 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 36a2974..f79f4a3 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1573,6 +1573,49 @@ void __gpio_set_value(unsigned gpio, int value)
> ?EXPORT_SYMBOL_GPL(__gpio_set_value);
>
> ?/**
> + * gpio_set_bias() - set a gpio's bias
> + * @gpio: gpio whose bias will be set
> + * @bias: bias mode to set
> + * Context: process
> + *
> + * This is used to directly or indirectly to implement gpio_set_bias().
> + * It invokes the associated gpio_chip.set_bias() method. Usually this
> + * applies to input pins.
> + */
> +void gpio_set_bias(unsigned gpio, enum gpio_bias bias)
> +{
> + ? ? ? struct gpio_chip ? ? ? ?*chip;
> +
> + ? ? ? chip = gpio_to_chip(gpio);
> + ? ? ? /* Implementing this is not mandatory */
> + ? ? ? if (chip->set_bias)
> + ? ? ? ? ? ? ? chip->set_bias(chip, gpio - chip->base, bias);
> +}
> +EXPORT_SYMBOL_GPL(gpio_set_bias);
> +
> +/**
> + * gpio_set_drive() - set a gpio's drive mode
> + * @gpio: gpio whose drive mode will be set
> + * @drive: drive mode to set
> + * Context: process
> + *
> + * This is used to directly or indirectly to implement gpio_set_drive().
> + * It invokes the associated gpio_chip.set_drive() method. Call this
> + * before the __gpio_set_output() function to enable special drive modes.
> + */
> +void gpio_set_drive(unsigned gpio, enum gpio_drive drive)
> +{
> + ? ? ? struct gpio_chip ? ? ? ?*chip;
> +
> + ? ? ? chip = gpio_to_chip(gpio);
> + ? ? ? /* Implementing this is not mandatory */
> + ? ? ? if (chip->set_drive)
> + ? ? ? ? ? ? ? chip->set_drive(chip, gpio - chip->base, drive);
> +}
> +EXPORT_SYMBOL_GPL(gpio_set_drive);
> +
> +
> +/**
> ?* __gpio_cansleep() - report whether gpio value access will sleep
> ?* @gpio: gpio in question
> ?* Context: any
> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> index ff5c660..b4971b1 100644
> --- a/include/asm-generic/gpio.h
> +++ b/include/asm-generic/gpio.h
> @@ -59,6 +59,8 @@ struct device_node;
> ?* ? ? returns either the value actually sensed, or zero
> ?* @direction_output: configures signal "offset" as output, or returns error
> ?* @set: assigns output value for signal "offset"
> + * @set_bias: set a certain bias for the GPIO
> + * @set_drive: set a certain drive mode for the GPIO
> ?* @to_irq: optional hook supporting non-static gpio_to_irq() mappings;
> ?* ? ? implementation may not sleep
> ?* @dbg_show: optional routine to show contents in debugfs; default code
> @@ -109,6 +111,14 @@ struct gpio_chip {
> ? ? ? ?void ? ? ? ? ? ? ? ? ? ?(*set)(struct gpio_chip *chip,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?unsigned offset, int value);
>
> + ? ? ? void ? ? ? ? ? ? ? ? ? ?(*set_bias)(struct gpio_chip *chip,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? unsigned offset,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? enum gpio_bias bias);
> +
> + ? ? ? void ? ? ? ? ? ? ? ? ? ?(*set_drive)(struct gpio_chip *chip,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? unsigned offset,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? enum gpio_drive drive);
> +
> ? ? ? ?int ? ? ? ? ? ? ? ? ? ? (*to_irq)(struct gpio_chip *chip,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?unsigned offset);
>
> @@ -158,6 +168,8 @@ extern int gpio_set_debounce(unsigned gpio, unsigned debounce);
> ?extern int gpio_get_value_cansleep(unsigned gpio);
> ?extern void gpio_set_value_cansleep(unsigned gpio, int value);
>
> +extern void gpio_set_bias(unsigned gpio, enum gpio_bias bias);
> +extern void gpio_set_drive(unsigned gpio, enum gpio_drive drive);
>
> ?/* A platform's <asm/gpio.h> code may want to inline the I/O calls when
> ?* the GPIO is constant and refers to some always-present controller,
> diff --git a/include/linux/gpio.h b/include/linux/gpio.h
> index 32720ba..6b48c15 100644
> --- a/include/linux/gpio.h
> +++ b/include/linux/gpio.h
> @@ -3,6 +3,43 @@
>
> ?/* see Documentation/gpio.txt */
>
> +/**
> + * enum gpio_bias - bias modes for GPIOs
> + * @GPIO_BIAS_FLOAT: no specific bias, the GPIO will float or state is no
> + * ? ? controlled by software
> + * @GPIO_BIAS_PULL_UP: the GPIO will be pulled up (usually with high impedance
> + * ? ? to VDD)
> + * @GPIO_BIAS_PULL_DOWN: the GPIO will be pulled down (usually with high
> + * ? ? impedance to GROUND)
> + * @GPIO_BIAS_HIGH: the GPIO will be wired high, connected to VDD
> + * @GPIO_BIAS_GROUND: the GPIO will be grounded, connected to GROUND
> + */
> +enum gpio_bias {
> + ? ? ? GPIO_BIAS_FLOAT,
> + ? ? ? GPIO_BIAS_PULL_UP,
> + ? ? ? GPIO_BIAS_PULL_DOWN,
> + ? ? ? GPIO_BIAS_HIGH,
> + ? ? ? GPIO_BIAS_GROUND,
> +};
> +
> +/**
> + * enum gpio_drive - drive modes for GPIOs (output)
> + * @GPIO_DRIVE_PUSH_PULL: the GPIO will be driven actively high and low, this
> + * ? ? is the most typical case and is typically achieved with two active
> + * ? ? transistors on the output
> + * @GPIO_DRIVE_OPEN_DRAIN: the GPIO will be driven with open drain (open
> + * ? ? collector) which means it is usually wired with other output ports
> + * ? ? which are then pulled up with an external resistor
> + * @GPIO_DRIVE_OPEN_SOURCE: the GPIO will be driven with open drain
> + * ? ? (open emitter) which is the same as open drain mutatis mutandis but
> + * ? ? pulled to ground
> + */
> +enum gpio_drive {
> + ? ? ? GPIO_DRIVE_PUSH_PULL,
> + ? ? ? GPIO_DRIVE_OPEN_DRAIN,
> + ? ? ? GPIO_DRIVE_OPEN_SOURCE,
> +};
> +
> ?#ifdef CONFIG_GENERIC_GPIO
> ?#include <asm/gpio.h>
>
> @@ -90,6 +127,18 @@ static inline void gpio_set_value(unsigned gpio, int value)
> ? ? ? ?WARN_ON(1);
> ?}
>
> +static inline void gpio_set_bias(unsigned gpio, enum gpio_bias bias)
> +{
> + ? ? ? /* GPIO can never have been requested */
> + ? ? ? WARN_ON(1);
> +}
> +
> +static inline void gpio_set_drive(unsigned gpio, enum gpio_drive drive)
> +{
> + ? ? ? /* GPIO can never have been requested */
> + ? ? ? WARN_ON(1);
> +}
> +
> ?static inline int gpio_cansleep(unsigned gpio)
> ?{
> ? ? ? ?/* GPIO can never have been requested or set as {in,out}put */
> --
> 1.7.3.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/
>
Hi,
The interface proposed here is a lot better than most ad hoc implementation.
But similar to what Kyungmin Park noticed, I doubt the usefulness
of a limited enumeration for all different types.
On one hand, upper layers benefit from a limited list (like proposed).
On the other hand, most SoC's have more options, which are not comparable.
How will this interface cope with such differences?
a) ignore SoC specifics
b) export it through an _unlimited_ enumeration
c) export it via some GPIO_DRIVE_PROPRIETARY+X where X is very
SoC dependant.
Any suggestions?
Kurt
On Mon, Apr 18, 2011 at 09:09:28AM +0900, Kyungmin Park wrote:
> Hi Linus,
>
> It's really required feature for ARM architectures.
> In case of samsung SoCs. it needs more things you described.
> In case of setting the drive, we can set the drive strength, 1x, 2x, 3x, and 4x.
> So can it add the more like this? or separate enumeration?
>
> > +enum gpio_drive {
> > + GPIO_DRIVE_PUSH_PULL,
> > + GPIO_DRIVE_OPEN_DRAIN,
> > + GPIO_DRIVE_OPEN_SOURCE,
> GPIO_DRIVE_STRENGTH_1X,
> GPIO_DRIVE_STRENGTH_2X,
> GPIO_DRIVE_STRENGTH_3X,
> GPIO_DRIVE_STRENGTH_4X,
> > +};
>
> or
>
> enum gpio_drive_strength {
> GPIO_DRIVE_STRENGTH_1X,
> GPIO_DRIVE_STRENGTH_2X,
> GPIO_DRIVE_STRENGTH_3X,
> GPIO_DRIVE_STRENGTH_4X,
> };
>
> Thank you,
> Kyungmin Park
>
> On Mon, Apr 18, 2011 at 6:37 AM, Linus Walleij
> <[email protected]> wrote:
> > From: Linus Walleij <[email protected]>
> >
> > This adds two functions for struct gpio_chip chips to provide pin
> > bias and drive mode settings for individual pins. Implementers does
> > this a bit differently and usually there are a few possible modes you
> > can select, I'm providing a few common modes for biasing and driving
> > pins.
> >
> > Since we have no previous hacked-up arch-specific drivers for this
> > we can avoid any __override_functions and we just allow this to be
> > properly implemented using gpiolib. Further the function is made
> > non-mandatory, if it is not defined for the chip it will be silently
> > ignored.
> >
> > Signed-off-by: Linus Walleij <[email protected]>
[...]
On 18/04/2011, at 7:37 AM, Linus Walleij wrote:
> From: Linus Walleij <[email protected]>
>
> This adds two functions for struct gpio_chip chips to provide pin
> bias and drive mode settings for individual pins. Implementers does
> this a bit differently and usually there are a few possible modes you
> can select, I'm providing a few common modes for biasing and driving
> pins.
>
> Since we have no previous hacked-up arch-specific drivers for this
> we can avoid any __override_functions and we just allow this to be
> properly implemented using gpiolib. Further the function is made
> non-mandatory, if it is not defined for the chip it will be silently
> ignored.
I don't like this much. Why does the driver ever need to do this? When should a driver even try this this given you can't guarantee any results and as such it's only ever a hint? As far as I can see it would be much much better if you just replaced
platform_device_register(...)
with
set_up_pins(...)
platform_device_register(...)
in board code (or add a device tree, erm, leaf or whatever). Especially things like pin drive strength is meaningless to a driver and would, I imaging, be passed through the plaform data from the board in the common case anyway.
gpiolib handles input and output, high and low because that's all that could reasonably be expected to a) always work and b) be changed at run time in the driver.
So in short - what's the use case? Which driver requires this?
Thanks!
--Ben.
p.s. forgive the (lack of) word wrapping, travelling with a dumb-arse mail client atm :)
>
> Signed-off-by: Linus Walleij <[email protected]>
> ---
> drivers/gpio/gpiolib.c | 43 ++++++++++++++++++++++++++++++++++++++
> include/asm-generic/gpio.h | 12 ++++++++++
> include/linux/gpio.h | 49 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 104 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 36a2974..f79f4a3 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1573,6 +1573,49 @@ void __gpio_set_value(unsigned gpio, int value)
> EXPORT_SYMBOL_GPL(__gpio_set_value);
>
> /**
> + * gpio_set_bias() - set a gpio's bias
> + * @gpio: gpio whose bias will be set
> + * @bias: bias mode to set
> + * Context: process
> + *
> + * This is used to directly or indirectly to implement gpio_set_bias().
> + * It invokes the associated gpio_chip.set_bias() method. Usually this
> + * applies to input pins.
> + */
> +void gpio_set_bias(unsigned gpio, enum gpio_bias bias)
> +{
> + struct gpio_chip *chip;
> +
> + chip = gpio_to_chip(gpio);
> + /* Implementing this is not mandatory */
> + if (chip->set_bias)
> + chip->set_bias(chip, gpio - chip->base, bias);
> +}
> +EXPORT_SYMBOL_GPL(gpio_set_bias);
> +
> +/**
> + * gpio_set_drive() - set a gpio's drive mode
> + * @gpio: gpio whose drive mode will be set
> + * @drive: drive mode to set
> + * Context: process
> + *
> + * This is used to directly or indirectly to implement gpio_set_drive().
> + * It invokes the associated gpio_chip.set_drive() method. Call this
> + * before the __gpio_set_output() function to enable special drive modes.
> + */
> +void gpio_set_drive(unsigned gpio, enum gpio_drive drive)
> +{
> + struct gpio_chip *chip;
> +
> + chip = gpio_to_chip(gpio);
> + /* Implementing this is not mandatory */
> + if (chip->set_drive)
> + chip->set_drive(chip, gpio - chip->base, drive);
> +}
> +EXPORT_SYMBOL_GPL(gpio_set_drive);
> +
> +
> +/**
> * __gpio_cansleep() - report whether gpio value access will sleep
> * @gpio: gpio in question
> * Context: any
> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> index ff5c660..b4971b1 100644
> --- a/include/asm-generic/gpio.h
> +++ b/include/asm-generic/gpio.h
> @@ -59,6 +59,8 @@ struct device_node;
> * returns either the value actually sensed, or zero
> * @direction_output: configures signal "offset" as output, or returns error
> * @set: assigns output value for signal "offset"
> + * @set_bias: set a certain bias for the GPIO
> + * @set_drive: set a certain drive mode for the GPIO
> * @to_irq: optional hook supporting non-static gpio_to_irq() mappings;
> * implementation may not sleep
> * @dbg_show: optional routine to show contents in debugfs; default code
> @@ -109,6 +111,14 @@ struct gpio_chip {
> void (*set)(struct gpio_chip *chip,
> unsigned offset, int value);
>
> + void (*set_bias)(struct gpio_chip *chip,
> + unsigned offset,
> + enum gpio_bias bias);
> +
> + void (*set_drive)(struct gpio_chip *chip,
> + unsigned offset,
> + enum gpio_drive drive);
> +
> int (*to_irq)(struct gpio_chip *chip,
> unsigned offset);
>
> @@ -158,6 +168,8 @@ extern int gpio_set_debounce(unsigned gpio, unsigned debounce);
> extern int gpio_get_value_cansleep(unsigned gpio);
> extern void gpio_set_value_cansleep(unsigned gpio, int value);
>
> +extern void gpio_set_bias(unsigned gpio, enum gpio_bias bias);
> +extern void gpio_set_drive(unsigned gpio, enum gpio_drive drive);
>
> /* A platform's <asm/gpio.h> code may want to inline the I/O calls when
> * the GPIO is constant and refers to some always-present controller,
> diff --git a/include/linux/gpio.h b/include/linux/gpio.h
> index 32720ba..6b48c15 100644
> --- a/include/linux/gpio.h
> +++ b/include/linux/gpio.h
> @@ -3,6 +3,43 @@
>
> /* see Documentation/gpio.txt */
>
> +/**
> + * enum gpio_bias - bias modes for GPIOs
> + * @GPIO_BIAS_FLOAT: no specific bias, the GPIO will float or state is no
> + * controlled by software
> + * @GPIO_BIAS_PULL_UP: the GPIO will be pulled up (usually with high impedance
> + * to VDD)
> + * @GPIO_BIAS_PULL_DOWN: the GPIO will be pulled down (usually with high
> + * impedance to GROUND)
> + * @GPIO_BIAS_HIGH: the GPIO will be wired high, connected to VDD
> + * @GPIO_BIAS_GROUND: the GPIO will be grounded, connected to GROUND
> + */
> +enum gpio_bias {
> + GPIO_BIAS_FLOAT,
> + GPIO_BIAS_PULL_UP,
> + GPIO_BIAS_PULL_DOWN,
> + GPIO_BIAS_HIGH,
> + GPIO_BIAS_GROUND,
> +};
> +
> +/**
> + * enum gpio_drive - drive modes for GPIOs (output)
> + * @GPIO_DRIVE_PUSH_PULL: the GPIO will be driven actively high and low, this
> + * is the most typical case and is typically achieved with two active
> + * transistors on the output
> + * @GPIO_DRIVE_OPEN_DRAIN: the GPIO will be driven with open drain (open
> + * collector) which means it is usually wired with other output ports
> + * which are then pulled up with an external resistor
> + * @GPIO_DRIVE_OPEN_SOURCE: the GPIO will be driven with open drain
> + * (open emitter) which is the same as open drain mutatis mutandis but
> + * pulled to ground
> + */
> +enum gpio_drive {
> + GPIO_DRIVE_PUSH_PULL,
> + GPIO_DRIVE_OPEN_DRAIN,
> + GPIO_DRIVE_OPEN_SOURCE,
> +};
> +
> #ifdef CONFIG_GENERIC_GPIO
> #include <asm/gpio.h>
>
> @@ -90,6 +127,18 @@ static inline void gpio_set_value(unsigned gpio, int value)
> WARN_ON(1);
> }
>
> +static inline void gpio_set_bias(unsigned gpio, enum gpio_bias bias)
> +{
> + /* GPIO can never have been requested */
> + WARN_ON(1);
> +}
> +
> +static inline void gpio_set_drive(unsigned gpio, enum gpio_drive drive)
> +{
> + /* GPIO can never have been requested */
> + WARN_ON(1);
> +}
> +
> static inline int gpio_cansleep(unsigned gpio)
> {
> /* GPIO can never have been requested or set as {in,out}put */
> --
> 1.7.3.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
> gpiolib handles input and output, high and low because that's all that could reasonably be expected to a) always work and b) be changed at run time in the driver.
Imagine if file systems only handled 8.3 filenames because that's all
that could reasonably be expected to a) always work !
> So in short - what's the use case? Which driver requires this?
>
Also your comment re simply in or out on or off is incorrect. Pins may
also be in use by firmware and the like and in a 'neither' state.
Something certain gpio people seem to be in denial over.
Alan
On 18/04/2011, at 6:19 PM, Alan Cox wrote:
>> gpiolib handles input and output, high and low because that's all that could reasonably be expected to a) always work and b) be changed at run time in the driver.
>
> Imagine if file systems only handled 8.3 filenames because that's all
> that could reasonably be expected to a) always work !
Fair call, but bringing this back to the particular case in hand what in that enum is worth 'hinting' in a might-have-an-effect manner rather than just letting the board take care of it?
Still my question stands, where is the driver ever better placed to make these calls than the board code?
>
>> So in short - what's the use case? Which driver requires this?
>>
>
> Also your comment re simply in or out on or off is incorrect. Pins may
> also be in use by firmware and the like and in a 'neither' state.
> Something certain gpio people seem to be in denial over.
Quite right, but should a pin ever be in a neither state and simultaneously controlled by a gpiolib driver? If so, how should it behave and if not, is it anything that stricter enforcement of gpio_request() semantics can't get around?
--Ben.
>
> Alan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On Mon, Apr 18, 2011 at 06:50:12PM +1000, Ben Nizette wrote:
Please fix your MUA to word wrap within paragraphs; I've reflowed your
text for legibility.
> Still my question stands, where is the driver ever better placed to
> make these calls than the board code?
Even if it ends up being the board code using these APIs it seems
sensible to have a standard API that GPIO drivers can use to expose the
functionality. This will help with getting control into code Linux owns
(since people don't have to implement custom APIs) and will mean we can
do things like add control of this for device tree based boards.
> Fair call, but bringing this back to the particular case in hand what in that enum is worth 'hinting' in a might-have-an-effect manner rather than just letting the board take care of it?
>
> Still my question stands, where is the driver ever better placed to make these calls than the board code?
The logical extension to that is to delete the gpio layer because the
board code can do it ?
> > Also your comment re simply in or out on or off is incorrect. Pins may
> > also be in use by firmware and the like and in a 'neither' state.
> > Something certain gpio people seem to be in denial over.
>
> Quite right, but should a pin ever be in a neither state and simultaneously controlled by a gpiolib driver? If so, how should it behave and if not, is it anything that stricter enforcement of gpio_request() semantics can't get around?
That won't fix sysfs.
It also doesn't solve the real problem which is that you've got to
implement platform specific parallel gpio extensions all over the place
when you really want it all using the same 'handle' and request logic.
The gpio layer doesn't seem to know what it is doing. It's a fine resource
allocator and call distributor but it can't make up its mind whether it
wants to just do that job, or to be a proper extensible gpio layer.
Instead it sits there being almost useful but incomplete and unwilling to
either do the job needed or get out of the way.
One possible way to tackle a lot of it would be to actually let the
drivers make the choices instead of imposing arbitarily wrong sematics
in the upper layers.
And for a lot of this stuff that the gpio layer really doesn't want
internal knowledge of other chunks of the kernel have used models like
'get_property/set_property' (eg battery, video4linux etc) so that the mid
layer can plumb in a conversation between the handle owner and the driver
without getting involved in the conversation.
Alan
On 18/04/2011, at 9:59 PM, Mark Brown wrote:
> On Mon, Apr 18, 2011 at 06:50:12PM +1000, Ben Nizette wrote:
>
> Please fix your MUA to word wrap within paragraphs; I've reflowed your
> text for legibility.
Sorry, I apologised in advance in my first email, I've made the mistake
of travelling without a sane MUA :)
>
>> Still my question stands, where is the driver ever better placed to
>> make these calls than the board code?
>
> Even if it ends up being the board code using these APIs it seems
> sensible to have a standard API that GPIO drivers can use to expose the
> functionality. This will help with getting control into code Linux owns
> (since people don't have to implement custom APIs) and will mean we can
> do things like add control of this for device tree based boards.
Oh I'm all for platform-specific libraries abstracting this away from the
board code if that helps, that's certainly the way that, eg, AVR32 does it.
It just doesn't make sense to me to bounce from the board code in to
'generic' gpio code then back to platform-specific implementations when
you could cut out the middle man.
--Ben.
On 18/04/2011, at 10:26 PM, Alan Cox wrote:
>> Fair call, but bringing this back to the particular case in hand what in that enum is worth 'hinting' in a might-have-an-effect manner rather than just letting the board take care of it?
>>
>> Still my question stands, where is the driver ever better placed to make these calls than the board code?
>
> The logical extension to that is to delete the gpio layer because the
> board code can do it ?
Well anything that is done once at startup, yes, I think the board
code probably can and should. The only functionality gpiolib currently
supplies is stuff that only the driver knows when to do, i.e. has to be
done during device operation not once-off at boot.
With that said if anyone can demonstrate why a driver would need to
change drive modes or bias strengths during operation then I'll my
argument is more or less gone and I'm happy enough to see this kind of
functionality end up in gpiolib one way or another.
>
>>> Also your comment re simply in or out on or off is incorrect. Pins may
>>> also be in use by firmware and the like and in a 'neither' state.
>>> Something certain gpio people seem to be in denial over.
>>
>> Quite right, but should a pin ever be in a neither state and simultaneously controlled by a gpiolib driver? If so, how should it behave and if not, is it anything that stricter enforcement of gpio_request() semantics can't get around?
>
> That won't fix sysfs.
>
> It also doesn't solve the real problem which is that you've got to
> implement platform specific parallel gpio extensions all over the place
> when you really want it all using the same 'handle' and request logic.
Fair enough. Forgive me if you've stated this in a previous conversation
then but what do you see as being the best way forward here? Should
gpiolib enforce itself as the One True Allocator and require all firmware
drivers call back in to it to get access to the pins rather than gpiolib
calling back to the platform upon gpio_request()?
>
> The gpio layer doesn't seem to know what it is doing. It's a fine resource
> allocator and call distributor but it can't make up its mind whether it
> wants to just do that job, or to be a proper extensible gpio layer.
> Instead it sits there being almost useful but incomplete and unwilling to
> either do the job needed or get out of the way.
I absolutely agree here and I suppose my responses to this patch are more
or less aimed at keeping gpiolib in the former role which was more or less
the creator's original vision. If we want it to become anything more than
this then I certainly think that has merit but I don't want to go adding it
piece-wise to the existing style of gpiolib interfaces, I'd like people to
step back and have a think about what we want this thing to actually /do/
moving forward.
>
> One possible way to tackle a lot of it would be to actually let the
> drivers make the choices instead of imposing arbitarily wrong sematics
> in the upper layers.
>
> And for a lot of this stuff that the gpio layer really doesn't want
> internal knowledge of other chunks of the kernel have used models like
> 'get_property/set_property' (eg battery, video4linux etc) so that the mid
> layer can plumb in a conversation between the handle owner and the driver
> without getting involved in the conversation.
Yeah that sounds like a more reasonable way to expose this functionality if
the driver does indeed need it.
--Ben.
>
> Alan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On Tue, Apr 19, 2011 at 08:16:12AM +1000, Ben Nizette wrote:
> On 18/04/2011, at 9:59 PM, Mark Brown wrote:
> > Even if it ends up being the board code using these APIs it seems
> > sensible to have a standard API that GPIO drivers can use to expose the
> > functionality. This will help with getting control into code Linux owns
> > (since people don't have to implement custom APIs) and will mean we can
> > do things like add control of this for device tree based boards.
> Oh I'm all for platform-specific libraries abstracting this away from the
> board code if that helps, that's certainly the way that, eg, AVR32 does it.
> It just doesn't make sense to me to bounce from the board code in to
> 'generic' gpio code then back to platform-specific implementations when
> you could cut out the middle man.
We have cross platform abstractions for lots of things - device tree,
which I mentioned, is the obvious example - and many of the GPIO
controllers are themselves cross platform as they're for off-SoC chips.
On 19/04/2011, at 8:31 AM, Mark Brown wrote:
> On Tue, Apr 19, 2011 at 08:16:12AM +1000, Ben Nizette wrote:
>> On 18/04/2011, at 9:59 PM, Mark Brown wrote:
>
>>> Even if it ends up being the board code using these APIs it seems
>>> sensible to have a standard API that GPIO drivers can use to expose the
>>> functionality. This will help with getting control into code Linux owns
>>> (since people don't have to implement custom APIs) and will mean we can
>>> do things like add control of this for device tree based boards.
>
>> Oh I'm all for platform-specific libraries abstracting this away from the
>> board code if that helps, that's certainly the way that, eg, AVR32 does it.
>> It just doesn't make sense to me to bounce from the board code in to
>> 'generic' gpio code then back to platform-specific implementations when
>> you could cut out the middle man.
>
> We have cross platform abstractions for lots of things - device tree,
> which I mentioned, is the obvious example - and many of the GPIO
> controllers are themselves cross platform as they're for off-SoC chips.
OK let's take a step back here: My qualms simply expressed are that the
board code knows how the pin needs to be set up and the gpio provider knows
how the pin can be set up; I don't like the idea of trying to teach gpiolib
and any driver that uses it about those parameters. I don't see it as necessary
or, for that matter, a war we can without exploding enums.
How about we provide a conduit in gpiolib through which the board code can
express its wishes directly to the gpio provider without trying to interpret
it first? The gpio provider can supply the mode enumerations in a header
file (much like they do already with, eg, platform data structures), the
board support writer can make a judgement about which settings are required
and gpiolib just worries about passing essentially an opaque cookie of setup
data through as soon as the latter side is ready to accept it?
Or does the gpio-consuming driver genuinely need to know about these concepts?
--Ben.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
> > It also doesn't solve the real problem which is that you've got to
> > implement platform specific parallel gpio extensions all over the place
> > when you really want it all using the same 'handle' and request logic.
>
> Fair enough. Forgive me if you've stated this in a previous conversation
> then but what do you see as being the best way forward here? Should
> gpiolib enforce itself as the One True Allocator and require all firmware
> drivers call back in to it to get access to the pins rather than gpiolib
> calling back to the platform upon gpio_request()?
We need something to allocate gpios and manage them. gpiolib seems to be
very good at this. We also need gpiolib to route other requests because
gpiolib is the one thing which knows how to turn "gpio 43" a struct and
function calls.
> > And for a lot of this stuff that the gpio layer really doesn't want
> > internal knowledge of other chunks of the kernel have used models like
> > 'get_property/set_property' (eg battery, video4linux etc) so that the mid
> > layer can plumb in a conversation between the handle owner and the driver
> > without getting involved in the conversation.
>
> Yeah that sounds like a more reasonable way to expose this functionality if
> the driver does indeed need it.
Leaving aside the current input/output and on/off bits I would go for
being able to do
gpio_get_property(gpio, GPIO_BIAS, GPIO_BIAS_WHATEVER);
gpio_set_property(gpio, GPIO_BIAS, GPIO_BIAS_WHATEVER_ELSE);
and having gpiolib perform nothing more on these than
is the gpio allocated
does ->get_property() exist
no -EOPNOTSUPP
yes return ->get_proprty(gpio struct, op, val)
For dynamically configurable features that avoids the
gpio_request(35);
magic_platform_hack(&foo[3], blah); /* foo3 will be gpio35 */
type stuff that becomes unmaintainable.
It would be entirely optional for a driver to support any of this stuff,
but it would both allow drivers to do so and also mean that where there
are multiple devices with a common feature *and* a driver wants to use it
that it will be properly abstracted in the driver itself.
Alan
>
> --Ben.
>
> >
> > Alan
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
>
--
--
"Alan, I'm getting a bit worried about you."
-- Linus Torvalds
On Tue, Apr 19, 2011 at 5:38 PM, Alan Cox <[email protected]> wrote:
>> > It also doesn't solve the real problem which is that you've got to
>> > implement platform specific parallel gpio extensions all over the place
>> > when you really want it all using the same 'handle' and request logic.
>>
>> Fair enough. ?Forgive me if you've stated this in a previous conversation
>> then but what do you see as being the best way forward here? ?Should
>> gpiolib enforce itself as the One True Allocator and require all firmware
>> drivers call back in to it to get access to the pins rather than gpiolib
>> calling back to the platform upon gpio_request()?
>
> We need something to allocate gpios and manage them. gpiolib seems to be
> very good at this. We also need gpiolib to route other requests because
> gpiolib is the one thing which knows how to turn "gpio 43" a struct and
> function calls.
>
>> > And for a lot of this stuff that the gpio layer really doesn't want
>> > internal knowledge of other chunks of the kernel have used models like
>> > 'get_property/set_property' (eg battery, video4linux etc) so that the mid
>> > layer can plumb in a conversation between the handle owner and the driver
>> > without getting involved in the conversation.
>>
>> Yeah that sounds like a more reasonable way to expose this functionality if
>> the driver does indeed need it.
>
> Leaving aside the current input/output and on/off bits I would go for
> being able to do
>
> ? ? ? ?gpio_get_property(gpio, GPIO_BIAS, GPIO_BIAS_WHATEVER);
> ? ? ? ?gpio_set_property(gpio, GPIO_BIAS, GPIO_BIAS_WHATEVER_ELSE);
One more consideration, not mentioned previous time, is that pin
configuration for power down mode.
Samsung SoCs has retention GPIO configurations at sleep (suspend)
mode. and restore it at resume time.
it's need to reduce power and proper operation after suspend.
Thank you,
Kyungmin Park
>
> and having gpiolib perform nothing more on these than
>
> ? ? ? ?is the gpio allocated
> ? ? ? ?does ->get_property() exist
> ? ? ? ? ? ? ? ?no -EOPNOTSUPP
> ? ? ? ? ? ? ? ?yes return ->get_proprty(gpio struct, op, val)
>
> For dynamically configurable features that avoids the
>
> ? ? ? ?gpio_request(35);
> ? ? ? ?magic_platform_hack(&foo[3], blah); /* foo3 will be gpio35 */
>
> type stuff that becomes unmaintainable.
>
> It would be entirely optional for a driver to support any of this stuff,
> but it would both allow drivers to do so and also mean that where there
> are multiple devices with a common feature *and* a driver wants to use it
> that it will be properly abstracted in the driver itself.
>
>
> Alan
>
>
>
>>
>> ? ? ? --Ben.
>>
>> >
>> > Alan
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> > the body of a message to [email protected]
>> > More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>> > Please read the FAQ at ?http://www.tux.org/lkml/
>>
>
>
> --
> --
> ? ? ? ?"Alan, I'm getting a bit worried about you."
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?-- Linus Torvalds
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
On 19/04/2011, at 6:38 PM, Alan Cox wrote:
>>> It also doesn't solve the real problem which is that you've got to
>>> implement platform specific parallel gpio extensions all over the place
>>> when you really want it all using the same 'handle' and request logic.
>>
>> Fair enough. Forgive me if you've stated this in a previous conversation
>> then but what do you see as being the best way forward here? Should
>> gpiolib enforce itself as the One True Allocator and require all firmware
>> drivers call back in to it to get access to the pins rather than gpiolib
>> calling back to the platform upon gpio_request()?
>
> We need something to allocate gpios and manage them. gpiolib seems to be
> very good at this. We also need gpiolib to route other requests because
> gpiolib is the one thing which knows how to turn "gpio 43" a struct and
> function calls.
>
>>> And for a lot of this stuff that the gpio layer really doesn't want
>>> internal knowledge of other chunks of the kernel have used models like
>>> 'get_property/set_property' (eg battery, video4linux etc) so that the mid
>>> layer can plumb in a conversation between the handle owner and the driver
>>> without getting involved in the conversation.
>>
>> Yeah that sounds like a more reasonable way to expose this functionality if
>> the driver does indeed need it.
>
> Leaving aside the current input/output and on/off bits I would go for
> being able to do
>
> gpio_get_property(gpio, GPIO_BIAS, GPIO_BIAS_WHATEVER);
> gpio_set_property(gpio, GPIO_BIAS, GPIO_BIAS_WHATEVER_ELSE);
Yeah I'm all for that so long as the capability constants are defined by the gpio provider, eg <linux/gpio/mygpioexpander.h>. There's no way gpiolib should be keeping a big ole list of every possible config option for every gpio provider. Well, maybe gpiolib can know about the options (eg GPIO_BIAS) so long as it doesn't have to enumerate every possible value.
Thanks,
--Ben.
>
> and having gpiolib perform nothing more on these than
>
> is the gpio allocated
> does ->get_property() exist
> no -EOPNOTSUPP
> yes return ->get_proprty(gpio struct, op, val)
>
> For dynamically configurable features that avoids the
>
> gpio_request(35);
> magic_platform_hack(&foo[3], blah); /* foo3 will be gpio35 */
>
> type stuff that becomes unmaintainable.
>
> It would be entirely optional for a driver to support any of this stuff,
> but it would both allow drivers to do so and also mean that where there
> are multiple devices with a common feature *and* a driver wants to use it
> that it will be properly abstracted in the driver itself.
>
>
> Alan
>
>
>
>>
>> --Ben.
>>
>>>
>>> Alan
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>>> the body of a message to [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at http://www.tux.org/lkml/
>>
>
>
> --
> --
> "Alan, I'm getting a bit worried about you."
> -- Linus Torvalds
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
> > gpio_get_property(gpio, GPIO_BIAS, GPIO_BIAS_WHATEVER);
> > gpio_set_property(gpio, GPIO_BIAS, GPIO_BIAS_WHATEVER_ELSE);
>
> Yeah I'm all for that so long as the capability constants are defined by the gpio provider, eg <linux/gpio/mygpioexpander.h>. There's no way gpiolib should be keeping a big ole list of every possible config option for every gpio provider. Well, maybe gpiolib can know about the options (eg GPIO_BIAS) so long as it doesn't have to enumerate every possible value.
It needs to know about any that might become common across multiple
devices so that if multiple devices have the same feature set it works
but I agree entirely about any weird platform or device specific stuff.
Perhaps for that it just needs to define
GPIO_PRIVATE_PROP 0x8000 /* to FFFF */
and be done with it
For multiple properties that can be shared you can't really get away from
needing a common name or number space. The good thing is that gpiolib
itself doesn't really care about that. To the core gpio code it's just a
set of numbers that only drivers and users know about.
Alan
2011/4/18 Ben Nizette <[email protected]>:
>
> On 18/04/2011, at 7:37 AM, Linus Walleij wrote:
>
>> From: Linus Walleij <[email protected]>
>>
>> This adds two functions for struct gpio_chip chips to provide pin
>> bias and drive mode settings for individual pins. Implementers does
>> this a bit differently and usually there are a few possible modes you
>> can select, I'm providing a few common modes for biasing and driving
>> pins.
>>
>> Since we have no previous hacked-up arch-specific drivers for this
>> we can avoid any __override_functions and we just allow this to be
>> properly implemented using gpiolib. Further the function is made
>> non-mandatory, if it is not defined for the chip it will be silently
>> ignored.
>
> I don't like this much. ?Why does the driver ever need to do this? ?When
> should a driver even try this this given you can't guarantee any results
> and as such it's only ever a hint?
I don't get it. Who says the interface should be used by drivers?
It could just as well (and much more likely) be the board code.
> As far as I can see it would be
> much much better if you just replaced
>
> platform_device_register(...)
>
> with
>
> set_up_pins(...)
> platform_device_register(...)
>
> in board code (or add a device tree, erm, leaf or whatever).
Yes that is one of the use cases for this patch set.
We seem to be in 100% agreement :-)
> So in short - what's the use case? ?Which driver requires this?
You just gave the use case yourself.
The intent of the patch set it to generalize the GPIO drivers
so they can be pushed down into drivers/gpio/* where they
belong.
Since only the individual GPIO driver knows for example where
to find the memory-mapped I/O region it has to reside with
the GPIO driver(s).
Even if these pin-specific calls are only called from board
code the mechanism is still needed in order to move,
consolidate and abstract the GPIO code into the gpio
subsystem.
Yours,
Linus Walleij
2011/4/19 Ben Nizette <[email protected]>:
> It just doesn't make sense to me to bounce from the board code in to
> 'generic' gpio code then back to platform-specific implementations when
> you could cut out the middle man.
In the demonstrated case the U300 GPIO controller has a number
of registers banks per 8-bit port, the U300 GPIO driver in
drivers/gpio/u300-cgpio.c knows the physical whereabouts of
these registers. So it maps them into virtual memory and
manipulate them.
Now to configure a pin from the board code without calling out
to the GPIO subsystem you need to map the same registers
into virtual memory a second time from board-u300-gpio.c,
provide custom set-up functions there and manipulate the
same register range from another place.
This is intuitively bad design to me, it's better to compartmentalize
the GPIO pin handling and whatever it takes into *one*
driver file, not two drivers writing into the same register
range, thanks.
Yours,
Linus Walleij
2011/4/18 Alan Cox <[email protected]>:
> And for a lot of this stuff that the gpio layer really doesn't want
> internal knowledge of other chunks of the kernel have used models like
> 'get_property/set_property' (eg battery, video4linux etc) so that the mid
> layer can plumb in a conversation between the handle owner and the driver
> without getting involved in the conversation.
I will implement that when I get to consolidare the
plat-nomadik/gpio.c driver. It has custom sleep modes and
stuff noone will be interested in duplicating.
I was thinking of something along the lines of:
int gpio_set_custom(unsigned gpio, unsigned long data);
int gpio_get_custom(unsigned gpio, unsigned long *data);
So like an ioctl() it can pass in/out arbitrary data by
casting to an unsigned long, handling enums, pointers
to structs and the like.
We used this design pattern in the DMA engine, it
works fine for that.
Yours,
Linus Walleij
2011/4/19 Ben Nizette <[email protected]>:
> On 18/04/2011, at 10:26 PM, Alan Cox wrote:
>
>> The logical extension to that is to delete the gpio layer because the
>> board code can do it ?
>
> Well anything that is done once at startup, yes,
This patch is about biasing and drive modes. We need to alter
these at runtime (from board code, indeed) due to the fact that
when you go to sleep e.g. floating a pin yeilds better power
characteristics.
Linus Walleij
On Wed, 20 Apr 2011 14:19:04 +0200
Linus Walleij <[email protected]> wrote:
> 2011/4/18 Alan Cox <[email protected]>:
>
> > And for a lot of this stuff that the gpio layer really doesn't want
> > internal knowledge of other chunks of the kernel have used models like
> > 'get_property/set_property' (eg battery, video4linux etc) so that the mid
> > layer can plumb in a conversation between the handle owner and the driver
> > without getting involved in the conversation.
>
> I will implement that when I get to consolidare the
> plat-nomadik/gpio.c driver. It has custom sleep modes and
> stuff noone will be interested in duplicating.
>
> I was thinking of something along the lines of:
>
> int gpio_set_custom(unsigned gpio, unsigned long data);
> int gpio_get_custom(unsigned gpio, unsigned long *data);
>
> So like an ioctl() it can pass in/out arbitrary data by
> casting to an unsigned long, handling enums, pointers
> to structs and the like.
Making the data ulong is smarter definitely, although you still need the
property number separate so everyone knows where to look for it with
shared operations (just like with ioctl)
2011/4/19 Kyungmin Park <[email protected]>:
> One more consideration, not mentioned previous time, is that pin
> configuration for power down mode.
> Samsung SoCs has retention GPIO configurations at sleep (suspend)
> mode. and restore it at resume time.
> it's need to reduce power and proper operation after suspend.
Isn't this supposed to be handled by runtime_pm hooks inside
your GPIO driver rather than by someone else talking to
the GPIO driver trying to spool/unspool the state from the
outside in some other place?
Or am I getting things backwards now...?
Yours,
Linus Walleij
2011/4/20 Ben Nizette <[email protected]>:
> On 19/04/2011, at 6:38 PM, Alan Cox wrote:
>> Leaving aside the current input/output and on/off bits I would go for
>> being able to do
>>
>> ? ? ? gpio_get_property(gpio, GPIO_BIAS, GPIO_BIAS_WHATEVER);
>> ? ? ? gpio_set_property(gpio, GPIO_BIAS, GPIO_BIAS_WHATEVER_ELSE);
>
> Yeah I'm all for that so long as the capability constants are defined by the gpio
> provider, eg <linux/gpio/mygpioexpander.h>. ?There's no way gpiolib should be
> keeping a big ole list of every possible config option for every gpio provider.
OK I buy that. I will refactor this solution to some opaque call instead
and start from there.
>?Well, maybe gpiolib can know about the options (eg GPIO_BIAS) so long
> as it doesn't have to enumerate every possible value.
I will drop that even, one parameter is better than two if one of them
is custom nevertheless. What difference does it make..
Yours,
Linus Walleij
On Wed, Apr 20, 2011 at 9:32 PM, Linus Walleij <[email protected]> wrote:
> 2011/4/19 Kyungmin Park <[email protected]>:
>
>> One more consideration, not mentioned previous time, is that pin
>> configuration for power down mode.
>> Samsung SoCs has retention GPIO configurations at sleep (suspend)
>> mode. and restore it at resume time.
>> it's need to reduce power and proper operation after suspend.
>
> Isn't this supposed to be handled by runtime_pm hooks inside
> your GPIO driver rather than by someone else talking to
> the GPIO driver trying to spool/unspool the state from the
> outside in some other place?
It's different from normal gpio input/output pin. It has different
offset and configuration.
As I know, current gpiolib doesn't support it.
Of course these should be handled at board file instead of drivers.
Thank you,
Kyungmin Park
>
> Or am I getting things backwards now...?
>
> Yours,
> Linus Walleij
>
On Tue, Apr 19, 2011 at 4:51 PM, Kyungmin Park <[email protected]> wrote:
>>
>> Leaving aside the current input/output and on/off bits I would go for
>> being able to do
>>
>> ? ? ? ?gpio_get_property(gpio, GPIO_BIAS, GPIO_BIAS_WHATEVER);
>> ? ? ? ?gpio_set_property(gpio, GPIO_BIAS, GPIO_BIAS_WHATEVER_ELSE);
>
> One more consideration, not mentioned previous time, is that pin
> configuration for power down mode.
> Samsung SoCs has retention GPIO configurations at sleep (suspend)
> mode. and restore it at resume time.
> it's need to reduce power and proper operation after suspend.
>
I have a little confusion. In ARM SoC, a lot of pins are used as
multi-functions.
Before suspend, it may be configured as some function that isn't GPIO.
Is it a goal
that avoid declaring gpio_request() for suspend and updating the setting of pin?
Linus,
Are these two patches are post in mailing list? I can't find your
second patch in this
patch series?
On Wed, Apr 20, 2011 at 11:26 PM, Haojian Zhuang
<[email protected]> wrote:
> On Tue, Apr 19, 2011 at 4:51 PM, Kyungmin Park <[email protected]> wrote:
>>>
>>> Leaving aside the current input/output and on/off bits I would go for
>>> being able to do
>>>
>>> ? ? ? ?gpio_get_property(gpio, GPIO_BIAS, GPIO_BIAS_WHATEVER);
>>> ? ? ? ?gpio_set_property(gpio, GPIO_BIAS, GPIO_BIAS_WHATEVER_ELSE);
>>
>> One more consideration, not mentioned previous time, is that pin
>> configuration for power down mode.
>> Samsung SoCs has retention GPIO configurations at sleep (suspend)
>> mode. and restore it at resume time.
>> it's need to reduce power and proper operation after suspend.
>>
> I have a little confusion. In ARM SoC, a lot of pins are used as
> multi-functions.
>
> Before suspend, it may be configured as some function that isn't GPIO.
> Is it a goal
> that avoid declaring gpio_request() for suspend and updating the setting of pin?
E.g., When WiFi is turn on and system goes the sleep, wifi should be
turn on. For this it should be configure the power down gpio
configuration properly. Its' different that call the gpio_set_value
function.
One more, even though some pins are used for other purpose instead of
GPIO. It needs to be configure as input or output at power down mode
registers.
Thank you,
Kyungmin Park
>
> Linus,
> Are these two patches are post in mailing list? I can't find your
> second patch in this
> patch series?
>
On Wed, 20 Apr 2011 14:32:35 +0200
Linus Walleij <[email protected]> wrote:
> 2011/4/19 Kyungmin Park <[email protected]>:
>
> > One more consideration, not mentioned previous time, is that pin
> > configuration for power down mode.
> > Samsung SoCs has retention GPIO configurations at sleep (suspend)
> > mode. and restore it at resume time.
> > it's need to reduce power and proper operation after suspend.
>
> Isn't this supposed to be handled by runtime_pm hooks inside
> your GPIO driver rather than by someone else talking to
> the GPIO driver trying to spool/unspool the state from the
> outside in some other place?
>
> Or am I getting things backwards now...?
IMHO that rather depends upon what knows the right things to do. The
platform may know the right way to manage GPIO pins but it may also be
that only the driver knows because the correct suspended pin states
depend on the driver or on the current runtime configuration - eg if a
specific GPIO must be left enabled for wakeup to work then it depends
whether that device is active not on the platform level properties.
On Wed, 20 Apr 2011 14:38:01 +0200
Linus Walleij <[email protected]> wrote:
> 2011/4/20 Ben Nizette <[email protected]>:
> > On 19/04/2011, at 6:38 PM, Alan Cox wrote:
> >> Leaving aside the current input/output and on/off bits I would go for
> >> being able to do
> >>
> >> ? ? ? gpio_get_property(gpio, GPIO_BIAS, GPIO_BIAS_WHATEVER);
> >> ? ? ? gpio_set_property(gpio, GPIO_BIAS, GPIO_BIAS_WHATEVER_ELSE);
> >
> > Yeah I'm all for that so long as the capability constants are defined by the gpio
> > provider, eg <linux/gpio/mygpioexpander.h>. ?There's no way gpiolib should be
> > keeping a big ole list of every possible config option for every gpio provider.
>
> OK I buy that. I will refactor this solution to some opaque call instead
> and start from there.
>
> >?Well, maybe gpiolib can know about the options (eg GPIO_BIAS) so long
> > as it doesn't have to enumerate every possible value.
>
> I will drop that even, one parameter is better than two if one of them
> is custom nevertheless. What difference does it make..
One parameter means its completely useless and we'll have to go change
the API.
Without the 'operation' being a parameter of its own no driver knows how
to answer the question 'is this shared operation A'
Alan
On Wed, Apr 20, 2011 at 10:40 PM, Kyungmin Park <[email protected]> wrote:
> On Wed, Apr 20, 2011 at 11:26 PM, Haojian Zhuang
> <[email protected]> wrote:
>> On Tue, Apr 19, 2011 at 4:51 PM, Kyungmin Park <[email protected]> wrote:
>>>>
>>>> Leaving aside the current input/output and on/off bits I would go for
>>>> being able to do
>>>>
>>>> ? ? ? ?gpio_get_property(gpio, GPIO_BIAS, GPIO_BIAS_WHATEVER);
>>>> ? ? ? ?gpio_set_property(gpio, GPIO_BIAS, GPIO_BIAS_WHATEVER_ELSE);
>>>
>>> One more consideration, not mentioned previous time, is that pin
>>> configuration for power down mode.
>>> Samsung SoCs has retention GPIO configurations at sleep (suspend)
>>> mode. and restore it at resume time.
>>> it's need to reduce power and proper operation after suspend.
>>>
>> I have a little confusion. In ARM SoC, a lot of pins are used as
>> multi-functions.
>>
>> Before suspend, it may be configured as some function that isn't GPIO.
>> Is it a goal
>> that avoid declaring gpio_request() for suspend and updating the setting of pin?
>
> E.g., When WiFi is turn on and system goes the sleep, wifi should be
> turn on. For this it should be configure the power down gpio
> configuration properly. Its' different that call the gpio_set_value
> function.
>
> One more, even though some pins are used for other purpose instead of
> GPIO. It needs to be configure as input or output at power down mode
> registers.
>
> Thank you,
> Kyungmin Park
>
I have an another question on board initialization. The configuration on pins
are including drive strength, pull and multiple function.
Since this patch can configure drive strength, pull. Will it be preferred to use
in board initialization?
2011/4/20 Haojian Zhuang <[email protected]>:
> Before suspend, it may be configured as some function that isn't GPIO.
> Is it a goal
> that avoid declaring gpio_request() for suspend and updating the setting of pin?
I refer to that as a totally different use case "pinmux or padmux"
which I think we are better off treating as totally orthogonal to GPIO.
I have loose ideas to break pin/pad muxing into a separate driver
directory under drivers/pinpadmux or something.
Some people inevitably think that GPIO and pin/padmux are
intertwined, but as far as I have seen they are not. However there
may be a cross dependency so that a GPIO driver may need to
export an additional pin/padmux interface or so, e.g we have
a separate chip in I2C which can mux pins...
However this is a separate issue altogether, I think we can assume
pin/padmuxing is in the board code until we have a suitable
solution for a framework for that. (Don't know if I'll be able to write
one though.)
> Are these two patches are post in mailing list? I can't find your
> second patch in this
> patch series?
I can see it here ... ?
Yours,
Linus Walleij
2011/4/20 Haojian Zhuang <[email protected]>:
> I have an another question on board initialization. The configuration on pins
> are including drive strength, pull and multiple function.
>
> Since this patch can configure drive strength, pull. Will it be preferred to use
> in board initialization?
I am reusing it (inside the example driver) for drive strength and bias.
Multiple function again is pin/padmuxing.
Even if some people are only muxing pins that can also be used as GPIO,
this is not always the case. We can shunt out the same pins to be used as
I2C or SPI for example, that means this cannot be handled in a GPIO
driver since it has nothing to do with GPIO pins at all.
Thus we need an orthogonal pin/padmux mechanism.
Yours,
Linus Walleij
> Some people inevitably think that GPIO and pin/padmux are
> intertwined, but as far as I have seen they are not. However there
It's not just pin muxing - gpio pins are sometimes shared between
firmware and OS and belong to one or the other depending upon what is
going on. For example the OS may need to own the pin for things like
updating or eeprom writing but the firmware or another device owns it for
day to day processing.
> may be a cross dependency so that a GPIO driver may need to
> export an additional pin/padmux interface or so, e.g we have
> a separate chip in I2C which can mux pins...
Would it not make sense to assume that given a situation where you have a
GPIO that can be routed four ways that you actually implement it like the
rest of the kernel - ie
r = gpio_request(n); /* n, n+1, n+2, n+3 are the four ways
*/
if (r < 0) /* EBUSY - someone else is using one of the
four */
return -EBUSY;
/* Succeeded - will also have set the mux for us */
At that point drivers don't need to know if a GPIO is muxed it'll just be
busy if someone else is using it.
It seems to me that if the goal of the gpio layer is to provide an
abstraction then it can abstract muxes just fine and without needing
drivers to know.
Alan
> Even if some people are only muxing pins that can also be used as GPIO,
> this is not always the case. We can shunt out the same pins to be used as
> I2C or SPI for example, that means this cannot be handled in a GPIO
> driver since it has nothing to do with GPIO pins at all.
It sounds very much to me like it belongs in the GPIO driver. That is the
one piece of code which knows what it is doing for all this configuration.
Whether all of this is exposed by the GPIO midlayer is another question,
certainly the GPIO midlayer/library doesn't want to be involved in the
finer details of such goings on.
The other approach to muxing I posted seems to handle this fine
Alan
2011/4/20 Alan Cox <[email protected]>:
> Would it not make sense to assume that given a situation where you have a
> GPIO that can be routed four ways that you actually implement it like the
> rest of the kernel - ie
>
> ? ? ? ?r = gpio_request(n); ? ?/* n, n+1, n+2, n+3 are the four ways
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?*/
>
> ? ? ? ?if (r < 0) ? ? ?/* EBUSY - someone else is using one of the
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?four */
> ? ? ? ? ? ? ? ?return -EBUSY;
> ? ? ? ?/* Succeeded - will also have set the mux for us */
>
> At that point drivers don't need to know if a GPIO is muxed it'll just be
> busy if someone else is using it.
Yes indeed. I'd achive that by have the GPIO driver call the
padmux subsystem to see if it can mux in that specific pin or whether it's
taken by some other user/mux setting.
> It seems to me that if the goal of the gpio layer is to provide an
> abstraction then it can abstract muxes just fine and without needing
> drivers to know.
It's hard to use for all mux cases because it's pin-oriented rather
than mux-setting oriented I'd say.
For example: pins 0-7 can be used for an SDIO interface, or
you can use pins 0-3 and 4-7 as two SPI interfaces (just making
this up) then what you want is to control different mux alternatives
than mapping specific pins.
So my idea is you abstract muxes separately, we've done so
in the past, see c.f. this crude but working example:
arch/arm/mach-u300/padmux.[c|h]:
enum pmx_settings {
U300_APP_PMX_MMC_SETTING,
U300_APP_PMX_SPI_SETTING
};
struct pmx *pmx_get(struct device *dev, enum pmx_settings setting);
int pmx_put(struct device *dev, struct pmx *pmx);
int pmx_activate(struct device *dev, struct pmx *pmx);
int pmx_deactivate(struct device *dev, struct pmx *pmx);
(then snip mmc.c):
/*
* Setup padmuxing for MMC. Since this must always be
* compiled into the kernel, pmx is never released.
*/
pmx = pmx_get(mmcsd_device, U300_APP_PMX_MMC_SETTING);
if (IS_ERR(pmx))
pr_warning("Could not get padmux handle\n");
else {
ret = pmx_activate(mmcsd_device, pmx);
if (IS_ERR_VALUE(ret))
pr_warning("Could not activate padmuxing\n");
}
So while this enum cannot cut it for the generic case, the idea
is that you enumerate your padmux settings one way or another
and this is orthogonal to GPIO or other such stuff.
Then of course the GPIO driver can in turn call the padmux
subsystem to request its pins or fail/bail out if they are taken.
Yours,
Linus Walleij
> Then of course the GPIO driver can in turn call the padmux
> subsystem to request its pins or fail/bail out if they are taken.
Yes that makes a lot more sense. It preserves the abstraction in the
simple cases but handles the complex stuff right.
Alan
2011/4/20 Alan Cox <[email protected]>:
>> Even if some people are only muxing pins that can also be used as GPIO,
>> this is not always the case. We can shunt out the same pins to be used as
>> I2C or SPI for example, that means this cannot be handled in a GPIO
>> driver since it has nothing to do with GPIO pins at all.
>
> It sounds very much to me like it belongs in the GPIO driver. That is the
> one piece of code which knows what it is doing for all this configuration.
Yeah whether it goes into drivers/pinpadmux and include/linux/pinpadmux.h
or drivers/gpio and include/linux/gpio.h does not matter to me if that is it's
more natural habitat to most people :-)
Now I need to post v2 of this patchset...
Linus Walleij
On 20/04/2011, at 10:04 PM, Linus Walleij wrote:
> 2011/4/18 Ben Nizette <[email protected]>:
>>
>> On 18/04/2011, at 7:37 AM, Linus Walleij wrote:
>>
>>> From: Linus Walleij <[email protected]>
>>>
>>> This adds two functions for struct gpio_chip chips to provide pin
>>> bias and drive mode settings for individual pins. Implementers does
>>> this a bit differently and usually there are a few possible modes you
>>> can select, I'm providing a few common modes for biasing and driving
>>> pins.
>>>
>>> Since we have no previous hacked-up arch-specific drivers for this
>>> we can avoid any __override_functions and we just allow this to be
>>> properly implemented using gpiolib. Further the function is made
>>> non-mandatory, if it is not defined for the chip it will be silently
>>> ignored.
>>
>> I don't like this much. Why does the driver ever need to do this? When
>> should a driver even try this this given you can't guarantee any results
>> and as such it's only ever a hint?
>
> I don't get it. Who says the interface should be used by drivers?
> It could just as well (and much more likely) be the board code.
If the board code and the gpio code are both platform-specific then I didn't
see the point in this patch set when pure platform-only calls would do. I
assumed then that the only people that would benefit from this API would be
those who didn't have access to the platform-specific one, i.e. the gpio
consuming drivers.
Turns out this isn't always the case, eg when the GPIO is off-SoC. In that
case then it's certainly nicest to have an API like yours to deal with it.
In short, don't worry, I'm already more-or-less converted to the need for this
kind of API, it's just the form of it that I think should change a bit (as
discussed elsewhere in thread).
Thanks!
--Ben.
>
>> As far as I can see it would be
>> much much better if you just replaced
>
>>
>> platform_device_register(...)
>>
>> with
>>
>> set_up_pins(...)
>> platform_device_register(...)
>>
>> in board code (or add a device tree, erm, leaf or whatever).
>
> Yes that is one of the use cases for this patch set.
> We seem to be in 100% agreement :-)
>
>> So in short - what's the use case? Which driver requires this?
>
> You just gave the use case yourself.
>
> The intent of the patch set it to generalize the GPIO drivers
> so they can be pushed down into drivers/gpio/* where they
> belong.
>
> Since only the individual GPIO driver knows for example where
> to find the memory-mapped I/O region it has to reside with
> the GPIO driver(s).
>
> Even if these pin-specific calls are only called from board
> code the mechanism is still needed in order to move,
> consolidate and abstract the GPIO code into the gpio
> subsystem.
>
> Yours,
> Linus Walleij
On 20/04/2011, at 10:21 PM, Linus Walleij wrote:
> 2011/4/19 Ben Nizette <[email protected]>:
>> On 18/04/2011, at 10:26 PM, Alan Cox wrote:
>>
>>> The logical extension to that is to delete the gpio layer because the
>>> board code can do it ?
>>
>> Well anything that is done once at startup, yes,
>
> This patch is about biasing and drive modes. We need to alter
> these at runtime (from board code, indeed) due to the fact that
> when you go to sleep e.g. floating a pin yeilds better power
> characteristics.
This is actually an interesting case because floating pins yeild
/worse/ power characteristics (each transistor of the push-pull
is on a little bit and you get a path straight through) [1]. To
get good power performance you want to pull an input pin high or
low but which of those two directions depends on external
constraints, i.e. the board. This is a case where the driver
should /not/ go playing with things it can't fully understand.
Perhaps one of the properties that a board can set in a gpio chip
driver is the suspend state and have suspend/resume hooks in the
gpio chip take care of setting things up on each side.
--Ben.
[1] http://processors.wiki.ti.com/index.php/Optimizing_IO_Power_Consumption
>
> Linus Walleij
RESEND, sorry if you get this twice. Crappy MUA..
On 20/04/2011, at 10:21 PM, Linus Walleij wrote:
> 2011/4/19 Ben Nizette <[email protected]>:
>> On 18/04/2011, at 10:26 PM, Alan Cox wrote:
>>
>>> The logical extension to that is to delete the gpio layer because the
>>> board code can do it ?
>>
>> Well anything that is done once at startup, yes,
>
> This patch is about biasing and drive modes. We need to alter
> these at runtime (from board code, indeed) due to the fact that
> when you go to sleep e.g. floating a pin yeilds better power
> characteristics.
This is actually an interesting case because floating pins yeild
/worse/ power characteristics (each transistor of the push-pull
is on a little bit and you get a path straight through) [1]. To
get good power performance you want to pull an input pin high or
low but which of those two directions depends on external
constraints, i.e. the board. This is a case where the driver
should /not/ go playing with things it can't fully understand.
Perhaps one of the properties that a board can set in a gpio chip
driver is the suspend state and have suspend/resume hooks in the
gpio chip take care of setting things up on each side.
--Ben.
[1] http://processors.wiki.ti.com/index.php/Optimizing_IO_Power_Consumption
>
> Linus Walleij
2011/4/21 Ben Nizette <[email protected]>:
>> This patch is about biasing and drive modes. We need to alter
>> these at runtime (from board code, indeed) due to the fact that
>> when you go to sleep e.g. floating a pin yeilds better power
>> characteristics.
>
> This is actually an interesting case because floating pins yeild
> /worse/ power characteristics (each transistor of the push-pull
> is on a little bit and you get a path straight through) [1]. ?To
> get good power performance you want to pull an input pin high or
> low but which of those two directions depends on external
> constraints, i.e. the board.
Yeah, mea culpa, I twisted it around in my head. I mean the
reverse. So this is why there are GPIO_CONFIG_BIAS_HIGH
and GPIO_CONFIG_BIAS_GROUND in the predefined pin
bias states.
(I do get it, don't worry, I was in electroscience before I
turned computer science actually...)
> ?This is a case where the driver
> should /not/ go playing with things it can't fully understand.
>
> Perhaps one of the properties that a board can set in a gpio chip
> driver is the suspend state and have suspend/resume hooks in the
> gpio chip take care of setting things up on each side.
Yes, the API is not only for drivers, it's for boards alright.
Pushing it to drivers/gpio give us two advantages:
1. Depopulate the overpopulated arch/arm/* etc trees
2. Give an overview so the GPIO maintainer and others can
think about consolidations
3. Clear cut drivers that can have runtime PM hooks etc...
Linus Walleij
Hi Linus,
Just yesterday I wrote pretty much a slimmed version of this on top of
our kernel.
There's a couple of things I added on top of what you already seem to have:
- in include/linux/of_gpio.h the of_gpio_flags were extended to
support open-drain
in device-trees.
- the debugfs support in gpiolib was also updated to export the current state to
the user through sysfs.
I'd also like to add (if not already mentioned by others) that in
general drivers that use
GPIOs are unaware of their physical connection on the board. Having
this information
in device-tree or platform-data equivalent solves this.
Secondly I tend to disagree with the "silently ignored" case when the
underlying driver
does not support setting drive mode. You can easily break the chip if
setting the drive
mode fails (although the HW designers may have selected the faulty
chip then ;) ).
Regards,
Stijn Devriendt
On Sun, Apr 17, 2011 at 11:37 PM, Linus Walleij
<[email protected]> wrote:
> From: Linus Walleij <[email protected]>
>
> This adds two functions for struct gpio_chip chips to provide pin
> bias and drive mode settings for individual pins. Implementers does
> this a bit differently and usually there are a few possible modes you
> can select, I'm providing a few common modes for biasing and driving
> pins.
>
> Since we have no previous hacked-up arch-specific drivers for this
> we can avoid any __override_functions and we just allow this to be
> properly implemented using gpiolib. Further the function is made
> non-mandatory, if it is not defined for the chip it will be silently
> ignored.
>
> Signed-off-by: Linus Walleij <[email protected]>
> ---
> ?drivers/gpio/gpiolib.c ? ? | ? 43 ++++++++++++++++++++++++++++++++++++++
> ?include/asm-generic/gpio.h | ? 12 ++++++++++
> ?include/linux/gpio.h ? ? ? | ? 49 ++++++++++++++++++++++++++++++++++++++++++++
> ?3 files changed, 104 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 36a2974..f79f4a3 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1573,6 +1573,49 @@ void __gpio_set_value(unsigned gpio, int value)
> ?EXPORT_SYMBOL_GPL(__gpio_set_value);
>
> ?/**
> + * gpio_set_bias() - set a gpio's bias
> + * @gpio: gpio whose bias will be set
> + * @bias: bias mode to set
> + * Context: process
> + *
> + * This is used to directly or indirectly to implement gpio_set_bias().
> + * It invokes the associated gpio_chip.set_bias() method. Usually this
> + * applies to input pins.
> + */
> +void gpio_set_bias(unsigned gpio, enum gpio_bias bias)
> +{
> + ? ? ? struct gpio_chip ? ? ? ?*chip;
> +
> + ? ? ? chip = gpio_to_chip(gpio);
> + ? ? ? /* Implementing this is not mandatory */
> + ? ? ? if (chip->set_bias)
> + ? ? ? ? ? ? ? chip->set_bias(chip, gpio - chip->base, bias);
> +}
> +EXPORT_SYMBOL_GPL(gpio_set_bias);
> +
> +/**
> + * gpio_set_drive() - set a gpio's drive mode
> + * @gpio: gpio whose drive mode will be set
> + * @drive: drive mode to set
> + * Context: process
> + *
> + * This is used to directly or indirectly to implement gpio_set_drive().
> + * It invokes the associated gpio_chip.set_drive() method. Call this
> + * before the __gpio_set_output() function to enable special drive modes.
> + */
> +void gpio_set_drive(unsigned gpio, enum gpio_drive drive)
> +{
> + ? ? ? struct gpio_chip ? ? ? ?*chip;
> +
> + ? ? ? chip = gpio_to_chip(gpio);
> + ? ? ? /* Implementing this is not mandatory */
> + ? ? ? if (chip->set_drive)
> + ? ? ? ? ? ? ? chip->set_drive(chip, gpio - chip->base, drive);
> +}
> +EXPORT_SYMBOL_GPL(gpio_set_drive);
> +
> +
> +/**
> ?* __gpio_cansleep() - report whether gpio value access will sleep
> ?* @gpio: gpio in question
> ?* Context: any
> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> index ff5c660..b4971b1 100644
> --- a/include/asm-generic/gpio.h
> +++ b/include/asm-generic/gpio.h
> @@ -59,6 +59,8 @@ struct device_node;
> ?* ? ? returns either the value actually sensed, or zero
> ?* @direction_output: configures signal "offset" as output, or returns error
> ?* @set: assigns output value for signal "offset"
> + * @set_bias: set a certain bias for the GPIO
> + * @set_drive: set a certain drive mode for the GPIO
> ?* @to_irq: optional hook supporting non-static gpio_to_irq() mappings;
> ?* ? ? implementation may not sleep
> ?* @dbg_show: optional routine to show contents in debugfs; default code
> @@ -109,6 +111,14 @@ struct gpio_chip {
> ? ? ? ?void ? ? ? ? ? ? ? ? ? ?(*set)(struct gpio_chip *chip,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?unsigned offset, int value);
>
> + ? ? ? void ? ? ? ? ? ? ? ? ? ?(*set_bias)(struct gpio_chip *chip,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? unsigned offset,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? enum gpio_bias bias);
> +
> + ? ? ? void ? ? ? ? ? ? ? ? ? ?(*set_drive)(struct gpio_chip *chip,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? unsigned offset,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? enum gpio_drive drive);
> +
> ? ? ? ?int ? ? ? ? ? ? ? ? ? ? (*to_irq)(struct gpio_chip *chip,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?unsigned offset);
>
> @@ -158,6 +168,8 @@ extern int gpio_set_debounce(unsigned gpio, unsigned debounce);
> ?extern int gpio_get_value_cansleep(unsigned gpio);
> ?extern void gpio_set_value_cansleep(unsigned gpio, int value);
>
> +extern void gpio_set_bias(unsigned gpio, enum gpio_bias bias);
> +extern void gpio_set_drive(unsigned gpio, enum gpio_drive drive);
>
> ?/* A platform's <asm/gpio.h> code may want to inline the I/O calls when
> ?* the GPIO is constant and refers to some always-present controller,
> diff --git a/include/linux/gpio.h b/include/linux/gpio.h
> index 32720ba..6b48c15 100644
> --- a/include/linux/gpio.h
> +++ b/include/linux/gpio.h
> @@ -3,6 +3,43 @@
>
> ?/* see Documentation/gpio.txt */
>
> +/**
> + * enum gpio_bias - bias modes for GPIOs
> + * @GPIO_BIAS_FLOAT: no specific bias, the GPIO will float or state is no
> + * ? ? controlled by software
> + * @GPIO_BIAS_PULL_UP: the GPIO will be pulled up (usually with high impedance
> + * ? ? to VDD)
> + * @GPIO_BIAS_PULL_DOWN: the GPIO will be pulled down (usually with high
> + * ? ? impedance to GROUND)
> + * @GPIO_BIAS_HIGH: the GPIO will be wired high, connected to VDD
> + * @GPIO_BIAS_GROUND: the GPIO will be grounded, connected to GROUND
> + */
> +enum gpio_bias {
> + ? ? ? GPIO_BIAS_FLOAT,
> + ? ? ? GPIO_BIAS_PULL_UP,
> + ? ? ? GPIO_BIAS_PULL_DOWN,
> + ? ? ? GPIO_BIAS_HIGH,
> + ? ? ? GPIO_BIAS_GROUND,
> +};
> +
> +/**
> + * enum gpio_drive - drive modes for GPIOs (output)
> + * @GPIO_DRIVE_PUSH_PULL: the GPIO will be driven actively high and low, this
> + * ? ? is the most typical case and is typically achieved with two active
> + * ? ? transistors on the output
> + * @GPIO_DRIVE_OPEN_DRAIN: the GPIO will be driven with open drain (open
> + * ? ? collector) which means it is usually wired with other output ports
> + * ? ? which are then pulled up with an external resistor
> + * @GPIO_DRIVE_OPEN_SOURCE: the GPIO will be driven with open drain
> + * ? ? (open emitter) which is the same as open drain mutatis mutandis but
> + * ? ? pulled to ground
> + */
> +enum gpio_drive {
> + ? ? ? GPIO_DRIVE_PUSH_PULL,
> + ? ? ? GPIO_DRIVE_OPEN_DRAIN,
> + ? ? ? GPIO_DRIVE_OPEN_SOURCE,
> +};
> +
> ?#ifdef CONFIG_GENERIC_GPIO
> ?#include <asm/gpio.h>
>
> @@ -90,6 +127,18 @@ static inline void gpio_set_value(unsigned gpio, int value)
> ? ? ? ?WARN_ON(1);
> ?}
>
> +static inline void gpio_set_bias(unsigned gpio, enum gpio_bias bias)
> +{
> + ? ? ? /* GPIO can never have been requested */
> + ? ? ? WARN_ON(1);
> +}
> +
> +static inline void gpio_set_drive(unsigned gpio, enum gpio_drive drive)
> +{
> + ? ? ? /* GPIO can never have been requested */
> + ? ? ? WARN_ON(1);
> +}
> +
> ?static inline int gpio_cansleep(unsigned gpio)
> ?{
> ? ? ? ?/* GPIO can never have been requested or set as {in,out}put */
> --
> 1.7.3.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/
>
2011/4/21 Stijn Devriendt <[email protected]>:
> There's a couple of things I added on top of what you already seem to have:
> - in include/linux/of_gpio.h the of_gpio_flags were extended to
> support open-drain in device-trees.
Device tree support can very well be handled as an add-on I believe,
please feel free to do that on top of my patch set :-)
> - the debugfs support in gpiolib was also updated to export the current state to
> ?the user through sysfs.
Hm, I've rewritten the mechanism (see latest patch set) to just take
anonymous parameter and argument. This means it's hard to do
this in any generic manner, and debugfs printing och config parameters
need to be pushed to each driver.
> I'd also like to add (if not already mentioned by others) that in
> general drivers that use GPIOs are unaware of their physical
> connection on the board. Having this information
> in device-tree or platform-data equivalent solves this.
Yes, this has been mentioned a few times, and a few times I have
replied that there is not one word in the patch set that says anything
about whether drivers or board code or whatever shall make use of
the mechanism, it's just an interface...
I don't understand why this keeps popping up, have I written
*anything* unclear about this?
> Secondly I tend to disagree with the "silently ignored" case when the
> underlying driver
> does not support setting drive mode. You can easily break the chip if
> setting the drive
> mode fails (although the HW designers may have selected the faulty
> chip then ;) ).
In the latest version the config operation returns an error code.
Yours,
Linus Walleij
> > There's a couple of things I added on top of what you already seem to have:
> > - in include/linux/of_gpio.h the of_gpio_flags were extended to
> > support open-drain in device-trees.
>
> Device tree support can very well be handled as an add-on I believe,
> please feel free to do that on top of my patch set :-)
In fact the op code stuff seems to fit very well with device tree as
device tree can now simply attach a list of property/value pairs to an
arbitary GPIO to be fed to it on setup/shutdown/whatever
On 21/04/2011, at 4:48 PM, Linus Walleij wrote:
> 2011/4/21 Ben Nizette <[email protected]>:
>
>>> This patch is about biasing and drive modes. We need to alter
>>> these at runtime (from board code, indeed) due to the fact that
>>> when you go to sleep e.g. floating a pin yeilds better power
>>> characteristics.
>>
>> This is actually an interesting case because floating pins yeild
>> /worse/ power characteristics (each transistor of the push-pull
>> is on a little bit and you get a path straight through) [1]. To
>> get good power performance you want to pull an input pin high or
>> low but which of those two directions depends on external
>> constraints, i.e. the board.
>
> Yeah, mea culpa, I twisted it around in my head. I mean the
> reverse. So this is why there are GPIO_CONFIG_BIAS_HIGH
> and GPIO_CONFIG_BIAS_GROUND in the predefined pin
> bias states.
>
> (I do get it, don't worry, I was in electroscience before I
> turned computer science actually...)
np, did a bit of a double-take myself :)
>
>> This is a case where the driver
>> should /not/ go playing with things it can't fully understand.
>>
>> Perhaps one of the properties that a board can set in a gpio chip
>> driver is the suspend state and have suspend/resume hooks in the
>> gpio chip take care of setting things up on each side.
>
> Yes, the API is not only for drivers, it's for boards alright.
>
> Pushing it to drivers/gpio give us two advantages:
>
> 1. Depopulate the overpopulated arch/arm/* etc trees
> 2. Give an overview so the GPIO maintainer and others can
> think about consolidations
> 3. Clear cut drivers that can have runtime PM hooks etc...
>
Yep I think I like this now, looking forward to seeing a few driver
implementations and seeing what kind of cool custom features i/o IP
blocks have that can now be easily and usefully exposed.
Thanks!
--Ben.
> Linus Walleij
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On 22/04/2011, at 9:36 PM, Linus Walleij wrote:
> 2011/4/21 Stijn Devriendt <[email protected]>:
>
>
>> I'd also like to add (if not already mentioned by others) that in
>> general drivers that use GPIOs are unaware of their physical
>> connection on the board. Having this information
>> in device-tree or platform-data equivalent solves this.
>
> Yes, this has been mentioned a few times, and a few times I have
> replied that there is not one word in the patch set that says anything
> about whether drivers or board code or whatever shall make use of
> the mechanism, it's just an interface...
>
> I don't understand why this keeps popping up, have I written
> *anything* unclear about this?
You haven't, no! Just that the use case for gpiolib was always to expose
gpio functionality to drivers/ in a generic way. The fact that it's
now been picked up extensively throughout arch/ is a (very) happy
side-effect.
--Ben.
On 4/17/2011 2:37 PM, Linus Walleij wrote:
> From: Linus Walleij<[email protected]>
>
> This adds two functions for struct gpio_chip chips to provide pin
> bias and drive mode settings for individual pins. Implementers does
> this a bit differently and usually there are a few possible modes you
> can select, I'm providing a few common modes for biasing and driving
> pins.
>
> Since we have no previous hacked-up arch-specific drivers for this
> we can avoid any __override_functions and we just allow this to be
> properly implemented using gpiolib. Further the function is made
> non-mandatory, if it is not defined for the chip it will be silently
> ignored.
This is a great start for something that the msm tree has been doing
internally through its gpiomux architecture.
Currently, msm has to configure a the pinmux in addition to the drive
and the bias, so we would like to make this more powerful to support
different gpio parameters. This has been mentioned previously - but a
way to define a custom config and set and get its parameters seems like
a more extensible model.
> Signed-off-by: Linus Walleij<[email protected]>
> ---
> drivers/gpio/gpiolib.c | 43 ++++++++++++++++++++++++++++++++++++++
> include/asm-generic/gpio.h | 12 ++++++++++
> include/linux/gpio.h | 49 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 104 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 36a2974..f79f4a3 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1573,6 +1573,49 @@ void __gpio_set_value(unsigned gpio, int value)
> EXPORT_SYMBOL_GPL(__gpio_set_value);
>
> /**
> + * gpio_set_bias() - set a gpio's bias
> + * @gpio: gpio whose bias will be set
> + * @bias: bias mode to set
> + * Context: process
> + *
> + * This is used to directly or indirectly to implement gpio_set_bias().
> + * It invokes the associated gpio_chip.set_bias() method. Usually this
> + * applies to input pins.
> + */
> +void gpio_set_bias(unsigned gpio, enum gpio_bias bias)
> +{
> + struct gpio_chip *chip;
> +
> + chip = gpio_to_chip(gpio);
> + /* Implementing this is not mandatory */
> + if (chip->set_bias)
> + chip->set_bias(chip, gpio - chip->base, bias);
> +}
> +EXPORT_SYMBOL_GPL(gpio_set_bias);
> +
> +/**
> + * gpio_set_drive() - set a gpio's drive mode
> + * @gpio: gpio whose drive mode will be set
> + * @drive: drive mode to set
> + * Context: process
> + *
> + * This is used to directly or indirectly to implement gpio_set_drive().
> + * It invokes the associated gpio_chip.set_drive() method. Call this
> + * before the __gpio_set_output() function to enable special drive modes.
> + */
> +void gpio_set_drive(unsigned gpio, enum gpio_drive drive)
> +{
> + struct gpio_chip *chip;
> +
> + chip = gpio_to_chip(gpio);
> + /* Implementing this is not mandatory */
> + if (chip->set_drive)
> + chip->set_drive(chip, gpio - chip->base, drive);
> +}
> +EXPORT_SYMBOL_GPL(gpio_set_drive);
> +
> +
> +/**
> * __gpio_cansleep() - report whether gpio value access will sleep
> * @gpio: gpio in question
> * Context: any
> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> index ff5c660..b4971b1 100644
> --- a/include/asm-generic/gpio.h
> +++ b/include/asm-generic/gpio.h
> @@ -59,6 +59,8 @@ struct device_node;
> * returns either the value actually sensed, or zero
> * @direction_output: configures signal "offset" as output, or returns error
> * @set: assigns output value for signal "offset"
> + * @set_bias: set a certain bias for the GPIO
> + * @set_drive: set a certain drive mode for the GPIO
> * @to_irq: optional hook supporting non-static gpio_to_irq() mappings;
> * implementation may not sleep
> * @dbg_show: optional routine to show contents in debugfs; default code
> @@ -109,6 +111,14 @@ struct gpio_chip {
> void (*set)(struct gpio_chip *chip,
> unsigned offset, int value);
>
> + void (*set_bias)(struct gpio_chip *chip,
> + unsigned offset,
> + enum gpio_bias bias);
> +
> + void (*set_drive)(struct gpio_chip *chip,
> + unsigned offset,
> + enum gpio_drive drive);
> +
> int (*to_irq)(struct gpio_chip *chip,
> unsigned offset);
>
> @@ -158,6 +168,8 @@ extern int gpio_set_debounce(unsigned gpio, unsigned debounce);
> extern int gpio_get_value_cansleep(unsigned gpio);
> extern void gpio_set_value_cansleep(unsigned gpio, int value);
>
> +extern void gpio_set_bias(unsigned gpio, enum gpio_bias bias);
> +extern void gpio_set_drive(unsigned gpio, enum gpio_drive drive);
With the pinmux architecture a gpio can be multiplexed with different
functional modules.
Another API to select this would make it more powerful. At boot a lot of
gpios are configured by the board-file.
How do you feel about another API that can accepts the gpio config
parameters (void *data) and set them all at once?
This would make it more generic to suit different gpio parameters that
an arch might have.
>
> /* A platform's<asm/gpio.h> code may want to inline the I/O calls when
> * the GPIO is constant and refers to some always-present controller,
> diff --git a/include/linux/gpio.h b/include/linux/gpio.h
> index 32720ba..6b48c15 100644
> --- a/include/linux/gpio.h
> +++ b/include/linux/gpio.h
> @@ -3,6 +3,43 @@
>
> /* see Documentation/gpio.txt */
>
> +/**
> + * enum gpio_bias - bias modes for GPIOs
> + * @GPIO_BIAS_FLOAT: no specific bias, the GPIO will float or state is no
> + * controlled by software
> + * @GPIO_BIAS_PULL_UP: the GPIO will be pulled up (usually with high impedance
> + * to VDD)
> + * @GPIO_BIAS_PULL_DOWN: the GPIO will be pulled down (usually with high
> + * impedance to GROUND)
> + * @GPIO_BIAS_HIGH: the GPIO will be wired high, connected to VDD
> + * @GPIO_BIAS_GROUND: the GPIO will be grounded, connected to GROUND
> + */
> +enum gpio_bias {
> + GPIO_BIAS_FLOAT,
> + GPIO_BIAS_PULL_UP,
> + GPIO_BIAS_PULL_DOWN,
> + GPIO_BIAS_HIGH,
> + GPIO_BIAS_GROUND,
> +};
> +
> +/**
> + * enum gpio_drive - drive modes for GPIOs (output)
> + * @GPIO_DRIVE_PUSH_PULL: the GPIO will be driven actively high and low, this
> + * is the most typical case and is typically achieved with two active
> + * transistors on the output
> + * @GPIO_DRIVE_OPEN_DRAIN: the GPIO will be driven with open drain (open
> + * collector) which means it is usually wired with other output ports
> + * which are then pulled up with an external resistor
> + * @GPIO_DRIVE_OPEN_SOURCE: the GPIO will be driven with open drain
> + * (open emitter) which is the same as open drain mutatis mutandis but
> + * pulled to ground
> + */
> +enum gpio_drive {
> + GPIO_DRIVE_PUSH_PULL,
> + GPIO_DRIVE_OPEN_DRAIN,
> + GPIO_DRIVE_OPEN_SOURCE,
> +};
> +
> #ifdef CONFIG_GENERIC_GPIO
> #include<asm/gpio.h>
>
> @@ -90,6 +127,18 @@ static inline void gpio_set_value(unsigned gpio, int value)
> WARN_ON(1);
> }
>
> +static inline void gpio_set_bias(unsigned gpio, enum gpio_bias bias)
> +{
> + /* GPIO can never have been requested */
> + WARN_ON(1);
> +}
> +
> +static inline void gpio_set_drive(unsigned gpio, enum gpio_drive drive)
> +{
> + /* GPIO can never have been requested */
> + WARN_ON(1);
> +}
> +
> static inline int gpio_cansleep(unsigned gpio)
> {
> /* GPIO can never have been requested or set as {in,out}put */
Thanks,
Rohit Vaswani
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
2011/4/25 Rohit Vaswani <[email protected]>:
> Another API to select this would make it more powerful. At boot a lot of
> gpios are configured by the board-file.
Yeah I have already revised this patchset...
> How do you feel about another API that can accepts the gpio config
> parameters (void *data) and set them all at once?
> This would make it more generic to suit different gpio parameters that an
> arch might have.
It's done that way now, and as Alan noted it fits hand-in-glove with
devicetree initialization that way too.
Yours,
Linus Walleij
On Wed, Apr 20, 2011 at 04:32:38PM +0100, Alan Cox wrote:
> > Even if some people are only muxing pins that can also be used as GPIO,
> > this is not always the case. We can shunt out the same pins to be used as
> > I2C or SPI for example, that means this cannot be handled in a GPIO
> > driver since it has nothing to do with GPIO pins at all.
>
> It sounds very much to me like it belongs in the GPIO driver. That is the
> one piece of code which knows what it is doing for all this configuration.
I don't think so. See PXA devices, where MFP (multifunction pin)
configuration is entirely separate from GPIO - and there's some MFPs
which don't even have GPIOs associated with them.
If I'm reading the PXA930 MFP header file right, it also appears that
GPIOs can be configured on to multiple different pins (eg, GPIO46 can
be on GPIO46 or DF_nWE. So refering to GPIO46 does not give you
sufficient information for exactly what pin is to be configured here.
To complicate matters, MFP configuration on PXA deals with stuff like
low power mode configuration, drive strength, etc which is completely
independent of the GPIO layer.
On Wed, Apr 20, 2011 at 04:43:15PM +0100, Alan Cox wrote:
> > Then of course the GPIO driver can in turn call the padmux
> > subsystem to request its pins or fail/bail out if they are taken.
>
> Yes that makes a lot more sense. It preserves the abstraction in the
> simple cases but handles the complex stuff right.
If one GPIO can be routed to multiple pads, it is not possible for
the GPIO driver to request the padmux code to route the GPIO to the
outside world - the GPIO layer doesn't have the information to tell
the padmux code which pad to route the GPIO to.
On Wednesday, April 27, 2011 2:55 PM, Russell King wrote:
> On Wed, Apr 20, 2011 at 04:32:38PM +0100, Alan Cox wrote:
>>> Even if some people are only muxing pins that can also be used as GPIO,
>>> this is not always the case. We can shunt out the same pins to be used as
>>> I2C or SPI for example, that means this cannot be handled in a GPIO
>>> driver since it has nothing to do with GPIO pins at all.
>>
>> It sounds very much to me like it belongs in the GPIO driver. That is the
>> one piece of code which knows what it is doing for all this configuration.
>
> I don't think so. See PXA devices, where MFP (multifunction pin)
> configuration is entirely separate from GPIO - and there's some MFPs
> which don't even have GPIOs associated with them.
>
> If I'm reading the PXA930 MFP header file right, it also appears that
> GPIOs can be configured on to multiple different pins (eg, GPIO46 can
> be on GPIO46 or DF_nWE. So refering to GPIO46 does not give you
> sufficient information for exactly what pin is to be configured here.
>
> To complicate matters, MFP configuration on PXA deals with stuff like
> low power mode configuration, drive strength, etc which is completely
> independent of the GPIO layer.
I agree with Russell. The muxing goes way beyond GPIO use.
On the i.MX for example, the iomux is used to configured what the pads on the
chip are used for. The pad configuration might be gpio related or it could be
setting the pad for use by one of the internal peripherals.
For instance, on the i.MX35 the uart3 rxd signal can appear on the pads: RTS2,
SD2_DATA0, ATA_DATA10, or FEC_TX_CLK. All of these pads can also be configured
as a gpio or for use by some other on-board peripheral.
Also, like Russell mentions for the PXA930, some of the gpios can be routed to
multiple pins. And some of the pads have no gpio use but multiple other uses.
Regards,
Hartley-