[PATCH] pinctrl: at91: add the config GPIO_OUTPUT_x

[PATCH] pinctrl: at91: add the config GPIO_OUTPUT_x

[Wenyou Yang]

In order to support the pinctrl sleep state.

Signed-off-by: Wenyou Yang <wenyou.yang-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
---
Hi Linus,

The patch is based on branch: for-next
git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl

Best Regards,
Wenyou Yang

 drivers/pinctrl/pinctrl-at91.c     |   31 +++++++++++++++++++++++++++++++
 include/dt-bindings/pinctrl/at91.h |    2 ++
 2 files changed, 33 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
index 5d24aae..fc51e59 100644
--- a/drivers/pinctrl/pinctrl-at91.c
+++ b/drivers/pinctrl/pinctrl-at91.c
@@ -62,6 +62,8 @@ static int gpio_banks;
 #define DEGLITCH	(1 << 2)
 #define PULL_DOWN	(1 << 3)
 #define DIS_SCHMIT	(1 << 4)
+#define GPIO_OUTPUT_HIGH	(1 << 5)
+#define GPIO_OUTPUT_LOW		(1 << 6)
 #define DEBOUNCE	(1 << 16)
 #define DEBOUNCE_VAL_SHIFT	17
 #define DEBOUNCE_VAL	(0x3fff << DEBOUNCE_VAL_SHIFT)
@@ -152,12 +154,15 @@ struct at91_pinctrl_mux_ops {
 	void (*set_pulldown)(void __iomem *pio, unsigned mask, bool is_on);
 	bool (*get_schmitt_trig)(void __iomem *pio, unsigned pin);
 	void (*disable_schmitt_trig)(void __iomem *pio, unsigned mask);
+	bool (*get_gpio_output)(void __iomem *pio, unsigned mask);
+	void (*set_gpio_output)(void __iomem *pio, unsigned mask, bool is_high);
 	/* irq */
 	int (*irq_type)(struct irq_data *d, unsigned type);
 };
 
 static int gpio_irq_type(struct irq_data *d, unsigned type);
 static int alt_gpio_irq_type(struct irq_data *d, unsigned type);
+static void at91_mux_gpio_enable(void __iomem *pio, unsigned mask, bool input);
 
 struct at91_pinctrl {
 	struct device		*dev;
@@ -472,6 +477,20 @@ static bool at91_mux_pio3_get_schmitt_trig(void __iomem *pio, unsigned pin)
 	return (__raw_readl(pio + PIO_SCHMITT) >> pin) & 0x1;
 }
 
+static bool at91_mux_pio3_get_gpio_output(void __iomem *pio, unsigned pin)
+{
+	return (__raw_readl(pio + PIO_ODSR) >> pin) & 0x1;
+}
+
+static void at91_mux_pio3_set_gpio_output(void __iomem *pio,
+						unsigned mask,
+						bool is_high)
+{
+	at91_mux_gpio_enable(pio, mask, 0);
+	writel_relaxed(mask, pio + (is_high ? PIO_SODR : PIO_CODR));
+}
+
+
 static struct at91_pinctrl_mux_ops at91rm9200_ops = {
 	.get_periph	= at91_mux_get_periph,
 	.mux_A_periph	= at91_mux_set_A_periph,
@@ -495,6 +514,8 @@ static struct at91_pinctrl_mux_ops at91sam9x5_ops = {
 	.set_pulldown	= at91_mux_pio3_set_pulldown,
 	.get_schmitt_trig = at91_mux_pio3_get_schmitt_trig,
 	.disable_schmitt_trig = at91_mux_pio3_disable_schmitt_trig,
+	.get_gpio_output = at91_mux_pio3_get_gpio_output,
+	.set_gpio_output = at91_mux_pio3_set_gpio_output,
 	.irq_type	= alt_gpio_irq_type,
 };
 
@@ -741,6 +762,10 @@ static int at91_pinconf_get(struct pinctrl_dev *pctldev,
 		*config |= PULL_DOWN;
 	if (info->ops->get_schmitt_trig && info->ops->get_schmitt_trig(pio, pin))
 		*config |= DIS_SCHMIT;
+	if (info->ops->get_gpio_output) {
+		*config |= info->ops->get_gpio_output(pio, pin) ?
+					GPIO_OUTPUT_HIGH : GPIO_OUTPUT_LOW;
+	}
 
 	return 0;
 }
@@ -778,6 +803,12 @@ static int at91_pinconf_set(struct pinctrl_dev *pctldev,
 			info->ops->set_pulldown(pio, mask, config & PULL_DOWN);
 		if (info->ops->disable_schmitt_trig && config & DIS_SCHMIT)
 			info->ops->disable_schmitt_trig(pio, mask);
+		if (info->ops->set_gpio_output) {
+			if (config & GPIO_OUTPUT_HIGH)
+				info->ops->set_gpio_output(pio, mask, 1);
+			if (config & GPIO_OUTPUT_LOW)
+				info->ops->set_gpio_output(pio, mask, 0);
+		};
 
 	} /* for each config */
 
diff --git a/include/dt-bindings/pinctrl/at91.h b/include/dt-bindings/pinctrl/at91.h
index 0fee6ff..e799268 100644
--- a/include/dt-bindings/pinctrl/at91.h
+++ b/include/dt-bindings/pinctrl/at91.h
@@ -15,6 +15,8 @@
 #define AT91_PINCTRL_DEGLITCH		(1 << 2)
 #define AT91_PINCTRL_PULL_DOWN		(1 << 3)
 #define AT91_PINCTRL_DIS_SCHMIT		(1 << 4)
+#define AT91_PINCTRL_OUTPUT_HIGH	(1 << 5)
+#define AT91_PINCTRL_OUTPUT_LOW		(1 << 6)
 #define AT91_PINCTRL_DEBOUNCE		(1 << 16)
 #define AT91_PINCTRL_DEBOUNCE_VAL(x)	(x << 17)
 
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pinctrl: at91: add the config GPIO_OUTPUT_x

[Jean-Christophe PLAGNIOL-VILLARD]

[-- Attachment #1.1: Type: text/plain, Size: 4497 bytes --]


On Mar 5, 2014, at 9:53 AM, Wenyou Yang <wenyou.yang@atmel.com> wrote:

> In order to support the pinctrl sleep state.

As I said before NACK

this is not the job of the pinctrl to describe gpio output or input state

Best Regards,
J.
> 
> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> ---
> Hi Linus,
> 
> The patch is based on branch: for-next
> git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl
> 
> Best Regards,
> Wenyou Yang
> 
> drivers/pinctrl/pinctrl-at91.c     |   31 +++++++++++++++++++++++++++++++
> include/dt-bindings/pinctrl/at91.h |    2 ++
> 2 files changed, 33 insertions(+)
> 
> diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
> index 5d24aae..fc51e59 100644
> --- a/drivers/pinctrl/pinctrl-at91.c
> +++ b/drivers/pinctrl/pinctrl-at91.c
> @@ -62,6 +62,8 @@ static int gpio_banks;
> #define DEGLITCH	(1 << 2)
> #define PULL_DOWN	(1 << 3)
> #define DIS_SCHMIT	(1 << 4)
> +#define GPIO_OUTPUT_HIGH	(1 << 5)
> +#define GPIO_OUTPUT_LOW		(1 << 6)
> #define DEBOUNCE	(1 << 16)
> #define DEBOUNCE_VAL_SHIFT	17
> #define DEBOUNCE_VAL	(0x3fff << DEBOUNCE_VAL_SHIFT)
> @@ -152,12 +154,15 @@ struct at91_pinctrl_mux_ops {
> 	void (*set_pulldown)(void __iomem *pio, unsigned mask, bool is_on);
> 	bool (*get_schmitt_trig)(void __iomem *pio, unsigned pin);
> 	void (*disable_schmitt_trig)(void __iomem *pio, unsigned mask);
> +	bool (*get_gpio_output)(void __iomem *pio, unsigned mask);
> +	void (*set_gpio_output)(void __iomem *pio, unsigned mask, bool is_high);
> 	/* irq */
> 	int (*irq_type)(struct irq_data *d, unsigned type);
> };
> 
> static int gpio_irq_type(struct irq_data *d, unsigned type);
> static int alt_gpio_irq_type(struct irq_data *d, unsigned type);
> +static void at91_mux_gpio_enable(void __iomem *pio, unsigned mask, bool input);
> 
> struct at91_pinctrl {
> 	struct device		*dev;
> @@ -472,6 +477,20 @@ static bool at91_mux_pio3_get_schmitt_trig(void __iomem *pio, unsigned pin)
> 	return (__raw_readl(pio + PIO_SCHMITT) >> pin) & 0x1;
> }
> 
> +static bool at91_mux_pio3_get_gpio_output(void __iomem *pio, unsigned pin)
> +{
> +	return (__raw_readl(pio + PIO_ODSR) >> pin) & 0x1;
> +}
> +
> +static void at91_mux_pio3_set_gpio_output(void __iomem *pio,
> +						unsigned mask,
> +						bool is_high)
> +{
> +	at91_mux_gpio_enable(pio, mask, 0);
> +	writel_relaxed(mask, pio + (is_high ? PIO_SODR : PIO_CODR));
> +}
> +
> +
> static struct at91_pinctrl_mux_ops at91rm9200_ops = {
> 	.get_periph	= at91_mux_get_periph,
> 	.mux_A_periph	= at91_mux_set_A_periph,
> @@ -495,6 +514,8 @@ static struct at91_pinctrl_mux_ops at91sam9x5_ops = {
> 	.set_pulldown	= at91_mux_pio3_set_pulldown,
> 	.get_schmitt_trig = at91_mux_pio3_get_schmitt_trig,
> 	.disable_schmitt_trig = at91_mux_pio3_disable_schmitt_trig,
> +	.get_gpio_output = at91_mux_pio3_get_gpio_output,
> +	.set_gpio_output = at91_mux_pio3_set_gpio_output,
> 	.irq_type	= alt_gpio_irq_type,
> };
> 
> @@ -741,6 +762,10 @@ static int at91_pinconf_get(struct pinctrl_dev *pctldev,
> 		*config |= PULL_DOWN;
> 	if (info->ops->get_schmitt_trig && info->ops->get_schmitt_trig(pio, pin))
> 		*config |= DIS_SCHMIT;
> +	if (info->ops->get_gpio_output) {
> +		*config |= info->ops->get_gpio_output(pio, pin) ?
> +					GPIO_OUTPUT_HIGH : GPIO_OUTPUT_LOW;
> +	}
> 
> 	return 0;
> }
> @@ -778,6 +803,12 @@ static int at91_pinconf_set(struct pinctrl_dev *pctldev,
> 			info->ops->set_pulldown(pio, mask, config & PULL_DOWN);
> 		if (info->ops->disable_schmitt_trig && config & DIS_SCHMIT)
> 			info->ops->disable_schmitt_trig(pio, mask);
> +		if (info->ops->set_gpio_output) {
> +			if (config & GPIO_OUTPUT_HIGH)
> +				info->ops->set_gpio_output(pio, mask, 1);
> +			if (config & GPIO_OUTPUT_LOW)
> +				info->ops->set_gpio_output(pio, mask, 0);
> +		};
> 
> 	} /* for each config */
> 
> diff --git a/include/dt-bindings/pinctrl/at91.h b/include/dt-bindings/pinctrl/at91.h
> index 0fee6ff..e799268 100644
> --- a/include/dt-bindings/pinctrl/at91.h
> +++ b/include/dt-bindings/pinctrl/at91.h
> @@ -15,6 +15,8 @@
> #define AT91_PINCTRL_DEGLITCH		(1 << 2)
> #define AT91_PINCTRL_PULL_DOWN		(1 << 3)
> #define AT91_PINCTRL_DIS_SCHMIT		(1 << 4)
> +#define AT91_PINCTRL_OUTPUT_HIGH	(1 << 5)
> +#define AT91_PINCTRL_OUTPUT_LOW		(1 << 6)
> #define AT91_PINCTRL_DEBOUNCE		(1 << 16)
> #define AT91_PINCTRL_DEBOUNCE_VAL(x)	(x << 17)
> 
> -- 
> 1.7.9.5
> 


[-- Attachment #1.2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 842 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH] pinctrl: at91: add the config GPIO_OUTPUT_x

[Yang, Wenyou]

Hi JC,

> -----Original Message-----
> From: Jean-Christophe PLAGNIOL-VILLARD [mailto:plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org]
> Sent: Wednesday, March 05, 2014 12:58 PM
> To: Yang, Wenyou
> Cc: Jean-Christophe PLAGNIOL-VILLARD; linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org;
> b.brezillon-ZNYIgs0QAGpBDgjK7y7TUQ@public.gmane.org; <linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org> mailing
> list; Linux Kernel list; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org;
> pawel.moll-5wv7dgnIgG8@public.gmane.org; mark.rutland-5wv7dgnIgG8@public.gmane.org; ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org;
> galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org
> Subject: Re: [PATCH] pinctrl: at91: add the config GPIO_OUTPUT_x
> 
> 
> On Mar 5, 2014, at 9:53 AM, Wenyou Yang <wenyou.yang-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:
> 
> > In order to support the pinctrl sleep state.
> 
> As I said before NACK
> 
> this is not the job of the pinctrl to describe gpio output or input
> state
But according to what said in the section "GPIO mode pitfalls" of Documentation/pinctrl.txt.
It should be handle by the pinctrl.

If not, to deal with the sleep state will be very complicated. 
Muxing the pins for FUNCTION to enable peripheral, then twist them over to GPIO mode
and use gpio_direction_output() to drive it HIGH or LOW during sleep.

--->8 ------------
The solution is to not think that what the datasheet calls "GPIO mode"
has to be handled by the <linux/gpio.h> interface. Instead view this as
a certain pin config setting. Look in e.g. <linux/pinctrl/pinconf-generic.h>
and you find this in the documentation:

  PIN_CONFIG_OUTPUT: this will configure the pin in output, use argument
     1 to indicate high level, argument 0 to indicate low level.

So it is perfectly possible to push a pin into "GPIO mode" and drive the
line low as part of the usual pin control map.
---<8 ------------

Best Regards,
Wenyou Yang

> 
> Best Regards,
> J.
> >
> > Signed-off-by: Wenyou Yang <wenyou.yang-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> > ---
> > Hi Linus,
> >
> > The patch is based on branch: for-next
> > git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl
> >
> > Best Regards,
> > Wenyou Yang
> >
> > drivers/pinctrl/pinctrl-at91.c     |   31
> +++++++++++++++++++++++++++++++
> > include/dt-bindings/pinctrl/at91.h |    2 ++
> > 2 files changed, 33 insertions(+)
> >
> > diff --git a/drivers/pinctrl/pinctrl-at91.c
> > b/drivers/pinctrl/pinctrl-at91.c index 5d24aae..fc51e59 100644
> > --- a/drivers/pinctrl/pinctrl-at91.c
> > +++ b/drivers/pinctrl/pinctrl-at91.c
> > @@ -62,6 +62,8 @@ static int gpio_banks;
> > #define DEGLITCH	(1 << 2)
> > #define PULL_DOWN	(1 << 3)
> > #define DIS_SCHMIT	(1 << 4)
> > +#define GPIO_OUTPUT_HIGH	(1 << 5)
> > +#define GPIO_OUTPUT_LOW		(1 << 6)
> > #define DEBOUNCE	(1 << 16)
> > #define DEBOUNCE_VAL_SHIFT	17
> > #define DEBOUNCE_VAL	(0x3fff << DEBOUNCE_VAL_SHIFT)
> > @@ -152,12 +154,15 @@ struct at91_pinctrl_mux_ops {
> > 	void (*set_pulldown)(void __iomem *pio, unsigned mask, bool is_on);
> > 	bool (*get_schmitt_trig)(void __iomem *pio, unsigned pin);
> > 	void (*disable_schmitt_trig)(void __iomem *pio, unsigned mask);
> > +	bool (*get_gpio_output)(void __iomem *pio, unsigned mask);
> > +	void (*set_gpio_output)(void __iomem *pio, unsigned mask, bool
> > +is_high);
> > 	/* irq */
> > 	int (*irq_type)(struct irq_data *d, unsigned type); };
> >
> > static int gpio_irq_type(struct irq_data *d, unsigned type); static
> > int alt_gpio_irq_type(struct irq_data *d, unsigned type);
> > +static void at91_mux_gpio_enable(void __iomem *pio, unsigned mask,
> > +bool input);
> >
> > struct at91_pinctrl {
> > 	struct device		*dev;
> > @@ -472,6 +477,20 @@ static bool at91_mux_pio3_get_schmitt_trig(void
> __iomem *pio, unsigned pin)
> > 	return (__raw_readl(pio + PIO_SCHMITT) >> pin) & 0x1; }
> >
> > +static bool at91_mux_pio3_get_gpio_output(void __iomem *pio, unsigned
> > +pin) {
> > +	return (__raw_readl(pio + PIO_ODSR) >> pin) & 0x1; }
> > +
> > +static void at91_mux_pio3_set_gpio_output(void __iomem *pio,
> > +						unsigned mask,
> > +						bool is_high)
> > +{
> > +	at91_mux_gpio_enable(pio, mask, 0);
> > +	writel_relaxed(mask, pio + (is_high ? PIO_SODR : PIO_CODR)); }
> > +
> > +
> > static struct at91_pinctrl_mux_ops at91rm9200_ops = {
> > 	.get_periph	= at91_mux_get_periph,
> > 	.mux_A_periph	= at91_mux_set_A_periph,
> > @@ -495,6 +514,8 @@ static struct at91_pinctrl_mux_ops at91sam9x5_ops
> = {
> > 	.set_pulldown	= at91_mux_pio3_set_pulldown,
> > 	.get_schmitt_trig = at91_mux_pio3_get_schmitt_trig,
> > 	.disable_schmitt_trig = at91_mux_pio3_disable_schmitt_trig,
> > +	.get_gpio_output = at91_mux_pio3_get_gpio_output,
> > +	.set_gpio_output = at91_mux_pio3_set_gpio_output,
> > 	.irq_type	= alt_gpio_irq_type,
> > };
> >
> > @@ -741,6 +762,10 @@ static int at91_pinconf_get(struct pinctrl_dev
> *pctldev,
> > 		*config |= PULL_DOWN;
> > 	if (info->ops->get_schmitt_trig && info->ops->get_schmitt_trig(pio,
> pin))
> > 		*config |= DIS_SCHMIT;
> > +	if (info->ops->get_gpio_output) {
> > +		*config |= info->ops->get_gpio_output(pio, pin) ?
> > +					GPIO_OUTPUT_HIGH : GPIO_OUTPUT_LOW;
> > +	}
> >
> > 	return 0;
> > }
> > @@ -778,6 +803,12 @@ static int at91_pinconf_set(struct pinctrl_dev
> *pctldev,
> > 			info->ops->set_pulldown(pio, mask, config & PULL_DOWN);
> > 		if (info->ops->disable_schmitt_trig && config & DIS_SCHMIT)
> > 			info->ops->disable_schmitt_trig(pio, mask);
> > +		if (info->ops->set_gpio_output) {
> > +			if (config & GPIO_OUTPUT_HIGH)
> > +				info->ops->set_gpio_output(pio, mask, 1);
> > +			if (config & GPIO_OUTPUT_LOW)
> > +				info->ops->set_gpio_output(pio, mask, 0);
> > +		};
> >
> > 	} /* for each config */
> >
> > diff --git a/include/dt-bindings/pinctrl/at91.h
> > b/include/dt-bindings/pinctrl/at91.h
> > index 0fee6ff..e799268 100644
> > --- a/include/dt-bindings/pinctrl/at91.h
> > +++ b/include/dt-bindings/pinctrl/at91.h
> > @@ -15,6 +15,8 @@
> > #define AT91_PINCTRL_DEGLITCH		(1 << 2)
> > #define AT91_PINCTRL_PULL_DOWN		(1 << 3)
> > #define AT91_PINCTRL_DIS_SCHMIT		(1 << 4)
> > +#define AT91_PINCTRL_OUTPUT_HIGH	(1 << 5)
> > +#define AT91_PINCTRL_OUTPUT_LOW		(1 << 6)
> > #define AT91_PINCTRL_DEBOUNCE		(1 << 16)
> > #define AT91_PINCTRL_DEBOUNCE_VAL(x)	(x << 17)
> >
> > --
> > 1.7.9.5
> >

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] pinctrl: at91: add the config GPIO_OUTPUT_x

[Yang, Wenyou]

Hi JC,

> -----Original Message-----
> From: Yang, Wenyou
> Sent: Wednesday, March 05, 2014 1:32 PM
> To: Jean-Christophe PLAGNIOL-VILLARD
> Cc: linus.walleij@linaro.org; b.brezillon@overkiz.com; <linux-arm-
> kernel@lists.infradead.org> mailing list; Linux Kernel list;
> devicetree@vger.kernel.org; robh+dt@kernel.org; pawel.moll@arm.com;
> mark.rutland@arm.com; ijc+devicetree@hellion.org.uk;
> galak@codeaurora.org
> Subject: RE: [PATCH] pinctrl: at91: add the config GPIO_OUTPUT_x
> 
> Hi JC,
> 
> > -----Original Message-----
> > From: Jean-Christophe PLAGNIOL-VILLARD [mailto:plagnioj@jcrosoft.com]
> > Sent: Wednesday, March 05, 2014 12:58 PM
> > To: Yang, Wenyou
> > Cc: Jean-Christophe PLAGNIOL-VILLARD; linus.walleij@linaro.org;
> > b.brezillon@overkiz.com; <linux-arm-kernel@lists.infradead.org>
> > mailing list; Linux Kernel list; devicetree@vger.kernel.org;
> > robh+dt@kernel.org; pawel.moll@arm.com; mark.rutland@arm.com;
> > ijc+devicetree@hellion.org.uk; galak@codeaurora.org
> > Subject: Re: [PATCH] pinctrl: at91: add the config GPIO_OUTPUT_x
> >
> >
> > On Mar 5, 2014, at 9:53 AM, Wenyou Yang <wenyou.yang@atmel.com> wrote:
> >
> > > In order to support the pinctrl sleep state.
> >
> > As I said before NACK
> >
> > this is not the job of the pinctrl to describe gpio output or input
> > state
> But according to what said in the section "GPIO mode pitfalls" of
> Documentation/pinctrl.txt.
> It should be handle by the pinctrl.
> 
> If not, to deal with the sleep state will be very complicated.
> Muxing the pins for FUNCTION to enable peripheral, then twist them over
> to GPIO mode and use gpio_direction_output() to drive it HIGH or LOW
> during sleep.
> 
> --->8 ------------
> The solution is to not think that what the datasheet calls "GPIO mode"
> has to be handled by the <linux/gpio.h> interface. Instead view this as
> a certain pin config setting. Look in e.g. <linux/pinctrl/pinconf-
> generic.h> and you find this in the documentation:
> 
>   PIN_CONFIG_OUTPUT: this will configure the pin in output, use argument
>      1 to indicate high level, argument 0 to indicate low level.
> 
> So it is perfectly possible to push a pin into "GPIO mode" and drive the
> line low as part of the usual pin control map.
> ---<8 ------------

Do you have any feedback?

Thanks,
Wenyou Yang

> 
> >
> > Best Regards,
> > J.
> > >
> > > Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> > > ---
> > > Hi Linus,
> > >
> > > The patch is based on branch: for-next
> > > git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl
> > >
> > > Best Regards,
> > > Wenyou Yang
> > >
> > > drivers/pinctrl/pinctrl-at91.c     |   31
> > +++++++++++++++++++++++++++++++
> > > include/dt-bindings/pinctrl/at91.h |    2 ++
> > > 2 files changed, 33 insertions(+)
> > >
> > > diff --git a/drivers/pinctrl/pinctrl-at91.c
> > > b/drivers/pinctrl/pinctrl-at91.c index 5d24aae..fc51e59 100644
> > > --- a/drivers/pinctrl/pinctrl-at91.c
> > > +++ b/drivers/pinctrl/pinctrl-at91.c
> > > @@ -62,6 +62,8 @@ static int gpio_banks;
> > > #define DEGLITCH	(1 << 2)
> > > #define PULL_DOWN	(1 << 3)
> > > #define DIS_SCHMIT	(1 << 4)
> > > +#define GPIO_OUTPUT_HIGH	(1 << 5)
> > > +#define GPIO_OUTPUT_LOW		(1 << 6)
> > > #define DEBOUNCE	(1 << 16)
> > > #define DEBOUNCE_VAL_SHIFT	17
> > > #define DEBOUNCE_VAL	(0x3fff << DEBOUNCE_VAL_SHIFT)
> > > @@ -152,12 +154,15 @@ struct at91_pinctrl_mux_ops {
> > > 	void (*set_pulldown)(void __iomem *pio, unsigned mask, bool is_on);
> > > 	bool (*get_schmitt_trig)(void __iomem *pio, unsigned pin);
> > > 	void (*disable_schmitt_trig)(void __iomem *pio, unsigned mask);
> > > +	bool (*get_gpio_output)(void __iomem *pio, unsigned mask);
> > > +	void (*set_gpio_output)(void __iomem *pio, unsigned mask, bool
> > > +is_high);
> > > 	/* irq */
> > > 	int (*irq_type)(struct irq_data *d, unsigned type); };
> > >
> > > static int gpio_irq_type(struct irq_data *d, unsigned type); static
> > > int alt_gpio_irq_type(struct irq_data *d, unsigned type);
> > > +static void at91_mux_gpio_enable(void __iomem *pio, unsigned mask,
> > > +bool input);
> > >
> > > struct at91_pinctrl {
> > > 	struct device		*dev;
> > > @@ -472,6 +477,20 @@ static bool at91_mux_pio3_get_schmitt_trig(void
> > __iomem *pio, unsigned pin)
> > > 	return (__raw_readl(pio + PIO_SCHMITT) >> pin) & 0x1; }
> > >
> > > +static bool at91_mux_pio3_get_gpio_output(void __iomem *pio,
> > > +unsigned
> > > +pin) {
> > > +	return (__raw_readl(pio + PIO_ODSR) >> pin) & 0x1; }
> > > +
> > > +static void at91_mux_pio3_set_gpio_output(void __iomem *pio,
> > > +						unsigned mask,
> > > +						bool is_high)
> > > +{
> > > +	at91_mux_gpio_enable(pio, mask, 0);
> > > +	writel_relaxed(mask, pio + (is_high ? PIO_SODR : PIO_CODR)); }
> > > +
> > > +
> > > static struct at91_pinctrl_mux_ops at91rm9200_ops = {
> > > 	.get_periph	= at91_mux_get_periph,
> > > 	.mux_A_periph	= at91_mux_set_A_periph,
> > > @@ -495,6 +514,8 @@ static struct at91_pinctrl_mux_ops
> > > at91sam9x5_ops
> > = {
> > > 	.set_pulldown	= at91_mux_pio3_set_pulldown,
> > > 	.get_schmitt_trig = at91_mux_pio3_get_schmitt_trig,
> > > 	.disable_schmitt_trig = at91_mux_pio3_disable_schmitt_trig,
> > > +	.get_gpio_output = at91_mux_pio3_get_gpio_output,
> > > +	.set_gpio_output = at91_mux_pio3_set_gpio_output,
> > > 	.irq_type	= alt_gpio_irq_type,
> > > };
> > >
> > > @@ -741,6 +762,10 @@ static int at91_pinconf_get(struct pinctrl_dev
> > *pctldev,
> > > 		*config |= PULL_DOWN;
> > > 	if (info->ops->get_schmitt_trig && info->ops->get_schmitt_trig(pio,
> > pin))
> > > 		*config |= DIS_SCHMIT;
> > > +	if (info->ops->get_gpio_output) {
> > > +		*config |= info->ops->get_gpio_output(pio, pin) ?
> > > +					GPIO_OUTPUT_HIGH : GPIO_OUTPUT_LOW;
> > > +	}
> > >
> > > 	return 0;
> > > }
> > > @@ -778,6 +803,12 @@ static int at91_pinconf_set(struct pinctrl_dev
> > *pctldev,
> > > 			info->ops->set_pulldown(pio, mask, config & PULL_DOWN);
> > > 		if (info->ops->disable_schmitt_trig && config & DIS_SCHMIT)
> > > 			info->ops->disable_schmitt_trig(pio, mask);
> > > +		if (info->ops->set_gpio_output) {
> > > +			if (config & GPIO_OUTPUT_HIGH)
> > > +				info->ops->set_gpio_output(pio, mask, 1);
> > > +			if (config & GPIO_OUTPUT_LOW)
> > > +				info->ops->set_gpio_output(pio, mask, 0);
> > > +		};
> > >
> > > 	} /* for each config */
> > >
> > > diff --git a/include/dt-bindings/pinctrl/at91.h
> > > b/include/dt-bindings/pinctrl/at91.h
> > > index 0fee6ff..e799268 100644
> > > --- a/include/dt-bindings/pinctrl/at91.h
> > > +++ b/include/dt-bindings/pinctrl/at91.h
> > > @@ -15,6 +15,8 @@
> > > #define AT91_PINCTRL_DEGLITCH		(1 << 2)
> > > #define AT91_PINCTRL_PULL_DOWN		(1 << 3)
> > > #define AT91_PINCTRL_DIS_SCHMIT		(1 << 4)
> > > +#define AT91_PINCTRL_OUTPUT_HIGH	(1 << 5)
> > > +#define AT91_PINCTRL_OUTPUT_LOW		(1 << 6)
> > > #define AT91_PINCTRL_DEBOUNCE		(1 << 16)
> > > #define AT91_PINCTRL_DEBOUNCE_VAL(x)	(x << 17)
> > >
> > > --
> > > 1.7.9.5
> > >

Re: [PATCH] pinctrl: at91: add the config GPIO_OUTPUT_x

[Jean-Christophe PLAGNIOL-VILLARD]

On Mar 11, 2014, at 9:28 AM, Yang, Wenyou <Wenyou.Yang@atmel.com> wrote:

> Hi JC,
> 
>> -----Original Message-----
>> From: Yang, Wenyou
>> Sent: Wednesday, March 05, 2014 1:32 PM
>> To: Jean-Christophe PLAGNIOL-VILLARD
>> Cc: linus.walleij@linaro.org; b.brezillon@overkiz.com; <linux-arm-
>> kernel@lists.infradead.org> mailing list; Linux Kernel list;
>> devicetree@vger.kernel.org; robh+dt@kernel.org; pawel.moll@arm.com;
>> mark.rutland@arm.com; ijc+devicetree@hellion.org.uk;
>> galak@codeaurora.org
>> Subject: RE: [PATCH] pinctrl: at91: add the config GPIO_OUTPUT_x
>> 
>> Hi JC,
>> 
>>> -----Original Message-----
>>> From: Jean-Christophe PLAGNIOL-VILLARD [mailto:plagnioj@jcrosoft.com]
>>> Sent: Wednesday, March 05, 2014 12:58 PM
>>> To: Yang, Wenyou
>>> Cc: Jean-Christophe PLAGNIOL-VILLARD; linus.walleij@linaro.org;
>>> b.brezillon@overkiz.com; <linux-arm-kernel@lists.infradead.org>
>>> mailing list; Linux Kernel list; devicetree@vger.kernel.org;
>>> robh+dt@kernel.org; pawel.moll@arm.com; mark.rutland@arm.com;
>>> ijc+devicetree@hellion.org.uk; galak@codeaurora.org
>>> Subject: Re: [PATCH] pinctrl: at91: add the config GPIO_OUTPUT_x
>>> 
>>> 
>>> On Mar 5, 2014, at 9:53 AM, Wenyou Yang <wenyou.yang@atmel.com> wrote:
>>> 
>>>> In order to support the pinctrl sleep state.
>>> 
>>> As I said before NACK
>>> 
>>> this is not the job of the pinctrl to describe gpio output or input
>>> state
>> But according to what said in the section "GPIO mode pitfalls" of
>> Documentation/pinctrl.txt.
>> It should be handle by the pinctrl.
>> 
>> If not, to deal with the sleep state will be very complicated.
>> Muxing the pins for FUNCTION to enable peripheral, then twist them over
>> to GPIO mode and use gpio_direction_output() to drive it HIGH or LOW
>> during sleep.
>> 
>> --->8 ------------
>> The solution is to not think that what the datasheet calls "GPIO mode"
>> has to be handled by the <linux/gpio.h> interface. Instead view this as
>> a certain pin config setting. Look in e.g. <linux/pinctrl/pinconf-
>> generic.h> and you find this in the documentation:
>> 
>>  PIN_CONFIG_OUTPUT: this will configure the pin in output, use argument
>>     1 to indicate high level, argument 0 to indicate low level.
>> 
>> So it is perfectly possible to push a pin into "GPIO mode" and drive the
>> line low as part of the usual pin control map.
>> ---<8 ------------
> 
> Do you have any feedback?

Here the issue is that you do drive the gpio and use as a gpio

which means you request it as a GPIO so after you can not reswitch it to alternative function
and the gpio is already requested as alternative function

So basically you mess-up with the gpio API and pinctrl API by bypassing both

Best Regards,
J.

> 
> Thanks,
> Wenyou Yang
> 
>> 
>>> 
>>> Best Regards,
>>> J.
>>>> 
>>>> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
>>>> ---
>>>> Hi Linus,
>>>> 
>>>> The patch is based on branch: for-next
>>>> git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl
>>>> 
>>>> Best Regards,
>>>> Wenyou Yang
>>>> 
>>>> drivers/pinctrl/pinctrl-at91.c     |   31
>>> +++++++++++++++++++++++++++++++
>>>> include/dt-bindings/pinctrl/at91.h |    2 ++
>>>> 2 files changed, 33 insertions(+)
>>>> 
>>>> diff --git a/drivers/pinctrl/pinctrl-at91.c
>>>> b/drivers/pinctrl/pinctrl-at91.c index 5d24aae..fc51e59 100644
>>>> --- a/drivers/pinctrl/pinctrl-at91.c
>>>> +++ b/drivers/pinctrl/pinctrl-at91.c
>>>> @@ -62,6 +62,8 @@ static int gpio_banks;
>>>> #define DEGLITCH	(1 << 2)
>>>> #define PULL_DOWN	(1 << 3)
>>>> #define DIS_SCHMIT	(1 << 4)
>>>> +#define GPIO_OUTPUT_HIGH	(1 << 5)
>>>> +#define GPIO_OUTPUT_LOW		(1 << 6)
>>>> #define DEBOUNCE	(1 << 16)
>>>> #define DEBOUNCE_VAL_SHIFT	17
>>>> #define DEBOUNCE_VAL	(0x3fff << DEBOUNCE_VAL_SHIFT)
>>>> @@ -152,12 +154,15 @@ struct at91_pinctrl_mux_ops {
>>>> 	void (*set_pulldown)(void __iomem *pio, unsigned mask, bool is_on);
>>>> 	bool (*get_schmitt_trig)(void __iomem *pio, unsigned pin);
>>>> 	void (*disable_schmitt_trig)(void __iomem *pio, unsigned mask);
>>>> +	bool (*get_gpio_output)(void __iomem *pio, unsigned mask);
>>>> +	void (*set_gpio_output)(void __iomem *pio, unsigned mask, bool
>>>> +is_high);
>>>> 	/* irq */
>>>> 	int (*irq_type)(struct irq_data *d, unsigned type); };
>>>> 
>>>> static int gpio_irq_type(struct irq_data *d, unsigned type); static
>>>> int alt_gpio_irq_type(struct irq_data *d, unsigned type);
>>>> +static void at91_mux_gpio_enable(void __iomem *pio, unsigned mask,
>>>> +bool input);
>>>> 
>>>> struct at91_pinctrl {
>>>> 	struct device		*dev;
>>>> @@ -472,6 +477,20 @@ static bool at91_mux_pio3_get_schmitt_trig(void
>>> __iomem *pio, unsigned pin)
>>>> 	return (__raw_readl(pio + PIO_SCHMITT) >> pin) & 0x1; }
>>>> 
>>>> +static bool at91_mux_pio3_get_gpio_output(void __iomem *pio,
>>>> +unsigned
>>>> +pin) {
>>>> +	return (__raw_readl(pio + PIO_ODSR) >> pin) & 0x1; }
>>>> +
>>>> +static void at91_mux_pio3_set_gpio_output(void __iomem *pio,
>>>> +						unsigned mask,
>>>> +						bool is_high)
>>>> +{
>>>> +	at91_mux_gpio_enable(pio, mask, 0);
>>>> +	writel_relaxed(mask, pio + (is_high ? PIO_SODR : PIO_CODR)); }
>>>> +
>>>> +
>>>> static struct at91_pinctrl_mux_ops at91rm9200_ops = {
>>>> 	.get_periph	= at91_mux_get_periph,
>>>> 	.mux_A_periph	= at91_mux_set_A_periph,
>>>> @@ -495,6 +514,8 @@ static struct at91_pinctrl_mux_ops
>>>> at91sam9x5_ops
>>> = {
>>>> 	.set_pulldown	= at91_mux_pio3_set_pulldown,
>>>> 	.get_schmitt_trig = at91_mux_pio3_get_schmitt_trig,
>>>> 	.disable_schmitt_trig = at91_mux_pio3_disable_schmitt_trig,
>>>> +	.get_gpio_output = at91_mux_pio3_get_gpio_output,
>>>> +	.set_gpio_output = at91_mux_pio3_set_gpio_output,
>>>> 	.irq_type	= alt_gpio_irq_type,
>>>> };
>>>> 
>>>> @@ -741,6 +762,10 @@ static int at91_pinconf_get(struct pinctrl_dev
>>> *pctldev,
>>>> 		*config |= PULL_DOWN;
>>>> 	if (info->ops->get_schmitt_trig && info->ops->get_schmitt_trig(pio,
>>> pin))
>>>> 		*config |= DIS_SCHMIT;
>>>> +	if (info->ops->get_gpio_output) {
>>>> +		*config |= info->ops->get_gpio_output(pio, pin) ?
>>>> +					GPIO_OUTPUT_HIGH : GPIO_OUTPUT_LOW;
>>>> +	}
>>>> 
>>>> 	return 0;
>>>> }
>>>> @@ -778,6 +803,12 @@ static int at91_pinconf_set(struct pinctrl_dev
>>> *pctldev,
>>>> 			info->ops->set_pulldown(pio, mask, config & PULL_DOWN);
>>>> 		if (info->ops->disable_schmitt_trig && config & DIS_SCHMIT)
>>>> 			info->ops->disable_schmitt_trig(pio, mask);
>>>> +		if (info->ops->set_gpio_output) {
>>>> +			if (config & GPIO_OUTPUT_HIGH)
>>>> +				info->ops->set_gpio_output(pio, mask, 1);
>>>> +			if (config & GPIO_OUTPUT_LOW)
>>>> +				info->ops->set_gpio_output(pio, mask, 0);
>>>> +		};
>>>> 
>>>> 	} /* for each config */
>>>> 
>>>> diff --git a/include/dt-bindings/pinctrl/at91.h
>>>> b/include/dt-bindings/pinctrl/at91.h
>>>> index 0fee6ff..e799268 100644
>>>> --- a/include/dt-bindings/pinctrl/at91.h
>>>> +++ b/include/dt-bindings/pinctrl/at91.h
>>>> @@ -15,6 +15,8 @@
>>>> #define AT91_PINCTRL_DEGLITCH		(1 << 2)
>>>> #define AT91_PINCTRL_PULL_DOWN		(1 << 3)
>>>> #define AT91_PINCTRL_DIS_SCHMIT		(1 << 4)
>>>> +#define AT91_PINCTRL_OUTPUT_HIGH	(1 << 5)
>>>> +#define AT91_PINCTRL_OUTPUT_LOW		(1 << 6)
>>>> #define AT91_PINCTRL_DEBOUNCE		(1 << 16)
>>>> #define AT91_PINCTRL_DEBOUNCE_VAL(x)	(x << 17)
>>>> 
>>>> --
>>>> 1.7.9.5
>>>> 
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH] pinctrl: at91: add the config GPIO_OUTPUT_x

[Yang, Wenyou]

> -----Original Message-----
> From: Jean-Christophe PLAGNIOL-VILLARD [mailto:plagnioj@jcrosoft.com]
> Sent: Tuesday, March 11, 2014 12:16 PM
> To: Yang, Wenyou
> Cc: Jean-Christophe PLAGNIOL-VILLARD; mark.rutland@arm.com;
> devicetree@vger.kernel.org; pawel.moll@arm.com;
> ijc+devicetree@hellion.org.uk; linus.walleij@linaro.org; Linux Kernel
> list; b.brezillon@overkiz.com; robh+dt@kernel.org; galak@codeaurora.org;
> <linux-arm-kernel@lists.infradead.org> mailing list
> Subject: Re: [PATCH] pinctrl: at91: add the config GPIO_OUTPUT_x
> 
> 
> On Mar 11, 2014, at 9:28 AM, Yang, Wenyou <Wenyou.Yang@atmel.com> wrote:
> 
> > Hi JC,
> >
> >> -----Original Message-----
> >> From: Yang, Wenyou
> >> Sent: Wednesday, March 05, 2014 1:32 PM
> >> To: Jean-Christophe PLAGNIOL-VILLARD
> >> Cc: linus.walleij@linaro.org; b.brezillon@overkiz.com; <linux-arm-
> >> kernel@lists.infradead.org> mailing list; Linux Kernel list;
> >> devicetree@vger.kernel.org; robh+dt@kernel.org; pawel.moll@arm.com;
> >> mark.rutland@arm.com; ijc+devicetree@hellion.org.uk;
> >> galak@codeaurora.org
> >> Subject: RE: [PATCH] pinctrl: at91: add the config GPIO_OUTPUT_x
> >>
> >> Hi JC,
> >>
> >>> -----Original Message-----
> >>> From: Jean-Christophe PLAGNIOL-VILLARD
> >>> [mailto:plagnioj@jcrosoft.com]
> >>> Sent: Wednesday, March 05, 2014 12:58 PM
> >>> To: Yang, Wenyou
> >>> Cc: Jean-Christophe PLAGNIOL-VILLARD; linus.walleij@linaro.org;
> >>> b.brezillon@overkiz.com; <linux-arm-kernel@lists.infradead.org>
> >>> mailing list; Linux Kernel list; devicetree@vger.kernel.org;
> >>> robh+dt@kernel.org; pawel.moll@arm.com; mark.rutland@arm.com;
> >>> ijc+devicetree@hellion.org.uk; galak@codeaurora.org
> >>> Subject: Re: [PATCH] pinctrl: at91: add the config GPIO_OUTPUT_x
> >>>
> >>>
> >>> On Mar 5, 2014, at 9:53 AM, Wenyou Yang <wenyou.yang@atmel.com>
> wrote:
> >>>
> >>>> In order to support the pinctrl sleep state.
> >>>
> >>> As I said before NACK
> >>>
> >>> this is not the job of the pinctrl to describe gpio output or input
> >>> state
> >> But according to what said in the section "GPIO mode pitfalls" of
> >> Documentation/pinctrl.txt.
> >> It should be handle by the pinctrl.
> >>
> >> If not, to deal with the sleep state will be very complicated.
> >> Muxing the pins for FUNCTION to enable peripheral, then twist them
> >> over to GPIO mode and use gpio_direction_output() to drive it HIGH or
> >> LOW during sleep.
> >>
> >> --->8 ------------
> >> The solution is to not think that what the datasheet calls "GPIO
> mode"
> >> has to be handled by the <linux/gpio.h> interface. Instead view this
> >> as a certain pin config setting. Look in e.g. <linux/pinctrl/pinconf-
> >> generic.h> and you find this in the documentation:
> >>
> >>  PIN_CONFIG_OUTPUT: this will configure the pin in output, use
> argument
> >>     1 to indicate high level, argument 0 to indicate low level.
> >>
> >> So it is perfectly possible to push a pin into "GPIO mode" and drive
> >> the line low as part of the usual pin control map.
> >> ---<8 ------------
> >
> > Do you have any feedback?
> 
> Here the issue is that you do drive the gpio and use as a gpio
> 
> which means you request it as a GPIO so after you can not reswitch it to
> alternative function and the gpio is already requested as alternative
> function
> 
> So basically you mess-up with the gpio API and pinctrl API by bypassing
> both

But I don't agree with you.

As I known, in mainline the other SoC vendors provide such function, configure the gpio mode with output config by pinctrl.

For example, ST. nomadisk.
You can see it in the function: nmk_pin_config_set() of the file, pinctrl-nomadik.c.
It provides GPIO mode and OUTPUT config.

This is a good solution, as what said in Documentation/pinctrl.txt,

it is perfectly possible to push a pin into "GPIO mode" and drive the line low as part of the usual pin control map.


Best Regards,
Wenyou Yang
> 
> Best Regards,
> J.
> 
> >
> > Thanks,
> > Wenyou Yang
> >
> >>
> >>>
> >>> Best Regards,
> >>> J.
> >>>>
> >>>> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> >>>> ---
> >>>> Hi Linus,
> >>>>
> >>>> The patch is based on branch: for-next
> >>>> git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl
> >>>>
> >>>> Best Regards,
> >>>> Wenyou Yang
> >>>>
> >>>> drivers/pinctrl/pinctrl-at91.c     |   31
> >>> +++++++++++++++++++++++++++++++
> >>>> include/dt-bindings/pinctrl/at91.h |    2 ++
> >>>> 2 files changed, 33 insertions(+)
> >>>>
> >>>> diff --git a/drivers/pinctrl/pinctrl-at91.c
> >>>> b/drivers/pinctrl/pinctrl-at91.c index 5d24aae..fc51e59 100644
> >>>> --- a/drivers/pinctrl/pinctrl-at91.c
> >>>> +++ b/drivers/pinctrl/pinctrl-at91.c
> >>>> @@ -62,6 +62,8 @@ static int gpio_banks;
> >>>> #define DEGLITCH	(1 << 2)
> >>>> #define PULL_DOWN	(1 << 3)
> >>>> #define DIS_SCHMIT	(1 << 4)
> >>>> +#define GPIO_OUTPUT_HIGH	(1 << 5)
> >>>> +#define GPIO_OUTPUT_LOW		(1 << 6)
> >>>> #define DEBOUNCE	(1 << 16)
> >>>> #define DEBOUNCE_VAL_SHIFT	17
> >>>> #define DEBOUNCE_VAL	(0x3fff << DEBOUNCE_VAL_SHIFT)
> >>>> @@ -152,12 +154,15 @@ struct at91_pinctrl_mux_ops {
> >>>> 	void (*set_pulldown)(void __iomem *pio, unsigned mask, bool is_on);
> >>>> 	bool (*get_schmitt_trig)(void __iomem *pio, unsigned pin);
> >>>> 	void (*disable_schmitt_trig)(void __iomem *pio, unsigned mask);
> >>>> +	bool (*get_gpio_output)(void __iomem *pio, unsigned mask);
> >>>> +	void (*set_gpio_output)(void __iomem *pio, unsigned mask,
> bool
> >>>> +is_high);
> >>>> 	/* irq */
> >>>> 	int (*irq_type)(struct irq_data *d, unsigned type); };
> >>>>
> >>>> static int gpio_irq_type(struct irq_data *d, unsigned type); static
> >>>> int alt_gpio_irq_type(struct irq_data *d, unsigned type);
> >>>> +static void at91_mux_gpio_enable(void __iomem *pio, unsigned mask,
> >>>> +bool input);
> >>>>
> >>>> struct at91_pinctrl {
> >>>> 	struct device		*dev;
> >>>> @@ -472,6 +477,20 @@ static bool
> >>>> at91_mux_pio3_get_schmitt_trig(void
> >>> __iomem *pio, unsigned pin)
> >>>> 	return (__raw_readl(pio + PIO_SCHMITT) >> pin) & 0x1; }
> >>>>
> >>>> +static bool at91_mux_pio3_get_gpio_output(void __iomem *pio,
> >>>> +unsigned
> >>>> +pin) {
> >>>> +	return (__raw_readl(pio + PIO_ODSR) >> pin) & 0x1; }
> >>>> +
> >>>> +static void at91_mux_pio3_set_gpio_output(void __iomem *pio,
> >>>> +						unsigned mask,
> >>>> +						bool is_high)
> >>>> +{
> >>>> +	at91_mux_gpio_enable(pio, mask, 0);
> >>>> +	writel_relaxed(mask, pio + (is_high ? PIO_SODR : PIO_CODR)); }
> >>>> +
> >>>> +
> >>>> static struct at91_pinctrl_mux_ops at91rm9200_ops = {
> >>>> 	.get_periph	= at91_mux_get_periph,
> >>>> 	.mux_A_periph	= at91_mux_set_A_periph,
> >>>> @@ -495,6 +514,8 @@ static struct at91_pinctrl_mux_ops
> >>>> at91sam9x5_ops
> >>> = {
> >>>> 	.set_pulldown	= at91_mux_pio3_set_pulldown,
> >>>> 	.get_schmitt_trig = at91_mux_pio3_get_schmitt_trig,
> >>>> 	.disable_schmitt_trig = at91_mux_pio3_disable_schmitt_trig,
> >>>> +	.get_gpio_output = at91_mux_pio3_get_gpio_output,
> >>>> +	.set_gpio_output = at91_mux_pio3_set_gpio_output,
> >>>> 	.irq_type	= alt_gpio_irq_type,
> >>>> };
> >>>>
> >>>> @@ -741,6 +762,10 @@ static int at91_pinconf_get(struct pinctrl_dev
> >>> *pctldev,
> >>>> 		*config |= PULL_DOWN;
> >>>> 	if (info->ops->get_schmitt_trig &&
> >>>> info->ops->get_schmitt_trig(pio,
> >>> pin))
> >>>> 		*config |= DIS_SCHMIT;
> >>>> +	if (info->ops->get_gpio_output) {
> >>>> +		*config |= info->ops->get_gpio_output(pio, pin) ?
> >>>> +					GPIO_OUTPUT_HIGH : GPIO_OUTPUT_LOW;
> >>>> +	}
> >>>>
> >>>> 	return 0;
> >>>> }
> >>>> @@ -778,6 +803,12 @@ static int at91_pinconf_set(struct pinctrl_dev
> >>> *pctldev,
> >>>> 			info->ops->set_pulldown(pio, mask, config & PULL_DOWN);
> >>>> 		if (info->ops->disable_schmitt_trig && config & DIS_SCHMIT)
> >>>> 			info->ops->disable_schmitt_trig(pio, mask);
> >>>> +		if (info->ops->set_gpio_output) {
> >>>> +			if (config & GPIO_OUTPUT_HIGH)
> >>>> +				info->ops->set_gpio_output(pio, mask, 1);
> >>>> +			if (config & GPIO_OUTPUT_LOW)
> >>>> +				info->ops->set_gpio_output(pio, mask, 0);
> >>>> +		};
> >>>>
> >>>> 	} /* for each config */
> >>>>
> >>>> diff --git a/include/dt-bindings/pinctrl/at91.h
> >>>> b/include/dt-bindings/pinctrl/at91.h
> >>>> index 0fee6ff..e799268 100644
> >>>> --- a/include/dt-bindings/pinctrl/at91.h
> >>>> +++ b/include/dt-bindings/pinctrl/at91.h
> >>>> @@ -15,6 +15,8 @@
> >>>> #define AT91_PINCTRL_DEGLITCH		(1 << 2)
> >>>> #define AT91_PINCTRL_PULL_DOWN		(1 << 3)
> >>>> #define AT91_PINCTRL_DIS_SCHMIT		(1 << 4)
> >>>> +#define AT91_PINCTRL_OUTPUT_HIGH	(1 << 5)
> >>>> +#define AT91_PINCTRL_OUTPUT_LOW		(1 << 6)
> >>>> #define AT91_PINCTRL_DEBOUNCE		(1 << 16)
> >>>> #define AT91_PINCTRL_DEBOUNCE_VAL(x)	(x << 17)
> >>>>
> >>>> --
> >>>> 1.7.9.5
> >>>>
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] pinctrl: at91: add the config GPIO_OUTPUT_x

[Jean-Christophe PLAGNIOL-VILLARD]

On Mar 11, 2014, at 2:54 PM, Yang, Wenyou <Wenyou.Yang@atmel.com> wrote:

> 
> 
>> -----Original Message-----
>> From: Jean-Christophe PLAGNIOL-VILLARD [mailto:plagnioj@jcrosoft.com]
>> Sent: Tuesday, March 11, 2014 12:16 PM
>> To: Yang, Wenyou
>> Cc: Jean-Christophe PLAGNIOL-VILLARD; mark.rutland@arm.com;
>> devicetree@vger.kernel.org; pawel.moll@arm.com;
>> ijc+devicetree@hellion.org.uk; linus.walleij@linaro.org; Linux Kernel
>> list; b.brezillon@overkiz.com; robh+dt@kernel.org; galak@codeaurora.org;
>> <linux-arm-kernel@lists.infradead.org> mailing list
>> Subject: Re: [PATCH] pinctrl: at91: add the config GPIO_OUTPUT_x
>> 
>> 
>> On Mar 11, 2014, at 9:28 AM, Yang, Wenyou <Wenyou.Yang@atmel.com> wrote:
>> 
>>> Hi JC,
>>> 
>>>> -----Original Message-----
>>>> From: Yang, Wenyou
>>>> Sent: Wednesday, March 05, 2014 1:32 PM
>>>> To: Jean-Christophe PLAGNIOL-VILLARD
>>>> Cc: linus.walleij@linaro.org; b.brezillon@overkiz.com; <linux-arm-
>>>> kernel@lists.infradead.org> mailing list; Linux Kernel list;
>>>> devicetree@vger.kernel.org; robh+dt@kernel.org; pawel.moll@arm.com;
>>>> mark.rutland@arm.com; ijc+devicetree@hellion.org.uk;
>>>> galak@codeaurora.org
>>>> Subject: RE: [PATCH] pinctrl: at91: add the config GPIO_OUTPUT_x
>>>> 
>>>> Hi JC,
>>>> 
>>>>> -----Original Message-----
>>>>> From: Jean-Christophe PLAGNIOL-VILLARD
>>>>> [mailto:plagnioj@jcrosoft.com]
>>>>> Sent: Wednesday, March 05, 2014 12:58 PM
>>>>> To: Yang, Wenyou
>>>>> Cc: Jean-Christophe PLAGNIOL-VILLARD; linus.walleij@linaro.org;
>>>>> b.brezillon@overkiz.com; <linux-arm-kernel@lists.infradead.org>
>>>>> mailing list; Linux Kernel list; devicetree@vger.kernel.org;
>>>>> robh+dt@kernel.org; pawel.moll@arm.com; mark.rutland@arm.com;
>>>>> ijc+devicetree@hellion.org.uk; galak@codeaurora.org
>>>>> Subject: Re: [PATCH] pinctrl: at91: add the config GPIO_OUTPUT_x
>>>>> 
>>>>> 
>>>>> On Mar 5, 2014, at 9:53 AM, Wenyou Yang <wenyou.yang@atmel.com>
>> wrote:
>>>>> 
>>>>>> In order to support the pinctrl sleep state.
>>>>> 
>>>>> As I said before NACK
>>>>> 
>>>>> this is not the job of the pinctrl to describe gpio output or input
>>>>> state
>>>> But according to what said in the section "GPIO mode pitfalls" of
>>>> Documentation/pinctrl.txt.
>>>> It should be handle by the pinctrl.
>>>> 
>>>> If not, to deal with the sleep state will be very complicated.
>>>> Muxing the pins for FUNCTION to enable peripheral, then twist them
>>>> over to GPIO mode and use gpio_direction_output() to drive it HIGH or
>>>> LOW during sleep.
>>>> 
>>>> --->8 ------------
>>>> The solution is to not think that what the datasheet calls "GPIO
>> mode"
>>>> has to be handled by the <linux/gpio.h> interface. Instead view this
>>>> as a certain pin config setting. Look in e.g. <linux/pinctrl/pinconf-
>>>> generic.h> and you find this in the documentation:
>>>> 
>>>> PIN_CONFIG_OUTPUT: this will configure the pin in output, use
>> argument
>>>>    1 to indicate high level, argument 0 to indicate low level.
>>>> 
>>>> So it is perfectly possible to push a pin into "GPIO mode" and drive
>>>> the line low as part of the usual pin control map.
>>>> ---<8 ------------
>>> 
>>> Do you have any feedback?
>> 
>> Here the issue is that you do drive the gpio and use as a gpio
>> 
>> which means you request it as a GPIO so after you can not reswitch it to
>> alternative function and the gpio is already requested as alternative
>> function
>> 
>> So basically you mess-up with the gpio API and pinctrl API by bypassing
>> both
> 
> But I don't agree with you.
> 
> As I known, in mainline the other SoC vendors provide such function, configure the gpio mode with output config by pinctrl.
> 
> For example, ST. nomadisk.
> You can see it in the function: nmk_pin_config_set() of the file, pinctrl-nomadik.c.
> It provides GPIO mode and OUTPUT config.
> 
> This is a good solution, as what said in Documentation/pinctrl.txt,
> 
> it is perfectly possible to push a pin into "GPIO mode" and drive the line low as part of the usual pin control map.
> 

This is simple if we use gpio mode as pinctrl tomorrow we will see gpio configuration in pinctrl
such as chip reset, default led state etc…

The pinctrl is not here to control a *GPIO* but to describe the configuration of pin.

> 
> Best Regards,
> Wenyou Yang
>> 
>> Best Regards,
>> J.
>> 
>>> 
>>> Thanks,
>>> Wenyou Yang
>>> 
>>>> 
>>>>> 
>>>>> Best Regards,
>>>>> J.
>>>>>> 
>>>>>> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
>>>>>> ---
>>>>>> Hi Linus,
>>>>>> 
>>>>>> The patch is based on branch: for-next
>>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl
>>>>>> 
>>>>>> Best Regards,
>>>>>> Wenyou Yang
>>>>>> 
>>>>>> drivers/pinctrl/pinctrl-at91.c     |   31
>>>>> +++++++++++++++++++++++++++++++
>>>>>> include/dt-bindings/pinctrl/at91.h |    2 ++
>>>>>> 2 files changed, 33 insertions(+)
>>>>>> 
>>>>>> diff --git a/drivers/pinctrl/pinctrl-at91.c
>>>>>> b/drivers/pinctrl/pinctrl-at91.c index 5d24aae..fc51e59 100644
>>>>>> --- a/drivers/pinctrl/pinctrl-at91.c
>>>>>> +++ b/drivers/pinctrl/pinctrl-at91.c
>>>>>> @@ -62,6 +62,8 @@ static int gpio_banks;
>>>>>> #define DEGLITCH	(1 << 2)
>>>>>> #define PULL_DOWN	(1 << 3)
>>>>>> #define DIS_SCHMIT	(1 << 4)
>>>>>> +#define GPIO_OUTPUT_HIGH	(1 << 5)
>>>>>> +#define GPIO_OUTPUT_LOW		(1 << 6)
>>>>>> #define DEBOUNCE	(1 << 16)
>>>>>> #define DEBOUNCE_VAL_SHIFT	17
>>>>>> #define DEBOUNCE_VAL	(0x3fff << DEBOUNCE_VAL_SHIFT)
>>>>>> @@ -152,12 +154,15 @@ struct at91_pinctrl_mux_ops {
>>>>>> 	void (*set_pulldown)(void __iomem *pio, unsigned mask, bool is_on);
>>>>>> 	bool (*get_schmitt_trig)(void __iomem *pio, unsigned pin);
>>>>>> 	void (*disable_schmitt_trig)(void __iomem *pio, unsigned mask);
>>>>>> +	bool (*get_gpio_output)(void __iomem *pio, unsigned mask);
>>>>>> +	void (*set_gpio_output)(void __iomem *pio, unsigned mask,
>> bool
>>>>>> +is_high);
>>>>>> 	/* irq */
>>>>>> 	int (*irq_type)(struct irq_data *d, unsigned type); };
>>>>>> 
>>>>>> static int gpio_irq_type(struct irq_data *d, unsigned type); static
>>>>>> int alt_gpio_irq_type(struct irq_data *d, unsigned type);
>>>>>> +static void at91_mux_gpio_enable(void __iomem *pio, unsigned mask,
>>>>>> +bool input);
>>>>>> 
>>>>>> struct at91_pinctrl {
>>>>>> 	struct device		*dev;
>>>>>> @@ -472,6 +477,20 @@ static bool
>>>>>> at91_mux_pio3_get_schmitt_trig(void
>>>>> __iomem *pio, unsigned pin)
>>>>>> 	return (__raw_readl(pio + PIO_SCHMITT) >> pin) & 0x1; }
>>>>>> 
>>>>>> +static bool at91_mux_pio3_get_gpio_output(void __iomem *pio,
>>>>>> +unsigned
>>>>>> +pin) {
>>>>>> +	return (__raw_readl(pio + PIO_ODSR) >> pin) & 0x1; }
>>>>>> +
>>>>>> +static void at91_mux_pio3_set_gpio_output(void __iomem *pio,
>>>>>> +						unsigned mask,
>>>>>> +						bool is_high)
>>>>>> +{
>>>>>> +	at91_mux_gpio_enable(pio, mask, 0);
>>>>>> +	writel_relaxed(mask, pio + (is_high ? PIO_SODR : PIO_CODR)); }
>>>>>> +
>>>>>> +
>>>>>> static struct at91_pinctrl_mux_ops at91rm9200_ops = {
>>>>>> 	.get_periph	= at91_mux_get_periph,
>>>>>> 	.mux_A_periph	= at91_mux_set_A_periph,
>>>>>> @@ -495,6 +514,8 @@ static struct at91_pinctrl_mux_ops
>>>>>> at91sam9x5_ops
>>>>> = {
>>>>>> 	.set_pulldown	= at91_mux_pio3_set_pulldown,
>>>>>> 	.get_schmitt_trig = at91_mux_pio3_get_schmitt_trig,
>>>>>> 	.disable_schmitt_trig = at91_mux_pio3_disable_schmitt_trig,
>>>>>> +	.get_gpio_output = at91_mux_pio3_get_gpio_output,
>>>>>> +	.set_gpio_output = at91_mux_pio3_set_gpio_output,
>>>>>> 	.irq_type	= alt_gpio_irq_type,
>>>>>> };
>>>>>> 
>>>>>> @@ -741,6 +762,10 @@ static int at91_pinconf_get(struct pinctrl_dev
>>>>> *pctldev,
>>>>>> 		*config |= PULL_DOWN;
>>>>>> 	if (info->ops->get_schmitt_trig &&
>>>>>> info->ops->get_schmitt_trig(pio,
>>>>> pin))
>>>>>> 		*config |= DIS_SCHMIT;
>>>>>> +	if (info->ops->get_gpio_output) {
>>>>>> +		*config |= info->ops->get_gpio_output(pio, pin) ?
>>>>>> +					GPIO_OUTPUT_HIGH : GPIO_OUTPUT_LOW;
>>>>>> +	}
>>>>>> 
>>>>>> 	return 0;
>>>>>> }
>>>>>> @@ -778,6 +803,12 @@ static int at91_pinconf_set(struct pinctrl_dev
>>>>> *pctldev,
>>>>>> 			info->ops->set_pulldown(pio, mask, config & PULL_DOWN);
>>>>>> 		if (info->ops->disable_schmitt_trig && config & DIS_SCHMIT)
>>>>>> 			info->ops->disable_schmitt_trig(pio, mask);
>>>>>> +		if (info->ops->set_gpio_output) {
>>>>>> +			if (config & GPIO_OUTPUT_HIGH)
>>>>>> +				info->ops->set_gpio_output(pio, mask, 1);
>>>>>> +			if (config & GPIO_OUTPUT_LOW)
>>>>>> +				info->ops->set_gpio_output(pio, mask, 0);
>>>>>> +		};
>>>>>> 
>>>>>> 	} /* for each config */
>>>>>> 
>>>>>> diff --git a/include/dt-bindings/pinctrl/at91.h
>>>>>> b/include/dt-bindings/pinctrl/at91.h
>>>>>> index 0fee6ff..e799268 100644
>>>>>> --- a/include/dt-bindings/pinctrl/at91.h
>>>>>> +++ b/include/dt-bindings/pinctrl/at91.h
>>>>>> @@ -15,6 +15,8 @@
>>>>>> #define AT91_PINCTRL_DEGLITCH		(1 << 2)
>>>>>> #define AT91_PINCTRL_PULL_DOWN		(1 << 3)
>>>>>> #define AT91_PINCTRL_DIS_SCHMIT		(1 << 4)
>>>>>> +#define AT91_PINCTRL_OUTPUT_HIGH	(1 << 5)
>>>>>> +#define AT91_PINCTRL_OUTPUT_LOW		(1 << 6)
>>>>>> #define AT91_PINCTRL_DEBOUNCE		(1 << 16)
>>>>>> #define AT91_PINCTRL_DEBOUNCE_VAL(x)	(x << 17)
>>>>>> 
>>>>>> --
>>>>>> 1.7.9.5
>>>>>> 
>>> 
>>> 
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH] pinctrl: at91: add the config GPIO_OUTPUT_x

[Yang, Wenyou]

Hi JC,

> -----Original Message-----
> From: Jean-Christophe PLAGNIOL-VILLARD [mailto:plagnioj@jcrosoft.com]
> Sent: Tuesday, March 11, 2014 7:14 PM
> To: Yang, Wenyou
> Cc: Jean-Christophe PLAGNIOL-VILLARD; mark.rutland@arm.com;
> devicetree@vger.kernel.org; pawel.moll@arm.com;
> ijc+devicetree@hellion.org.uk; linus.walleij@linaro.org; Linux Kernel
> list; b.brezillon@overkiz.com; robh+dt@kernel.org; galak@codeaurora.org;
> <linux-arm-kernel@lists.infradead.org> mailing list; Lin, JM
> Subject: Re: [PATCH] pinctrl: at91: add the config GPIO_OUTPUT_x
> 
> 
> On Mar 11, 2014, at 2:54 PM, Yang, Wenyou <Wenyou.Yang@atmel.com> wrote:
> 
> >
> >
> >> -----Original Message-----
> >> From: Jean-Christophe PLAGNIOL-VILLARD [mailto:plagnioj@jcrosoft.com]
> >> Sent: Tuesday, March 11, 2014 12:16 PM
> >> To: Yang, Wenyou
> >> Cc: Jean-Christophe PLAGNIOL-VILLARD; mark.rutland@arm.com;
> >> devicetree@vger.kernel.org; pawel.moll@arm.com;
> >> ijc+devicetree@hellion.org.uk; linus.walleij@linaro.org; Linux Kernel
> >> list; b.brezillon@overkiz.com; robh+dt@kernel.org;
> >> galak@codeaurora.org; <linux-arm-kernel@lists.infradead.org> mailing
> >> list
> >> Subject: Re: [PATCH] pinctrl: at91: add the config GPIO_OUTPUT_x
> >>
> >>
> >> On Mar 11, 2014, at 9:28 AM, Yang, Wenyou <Wenyou.Yang@atmel.com>
> wrote:
> >>
> >>> Hi JC,
> >>>
> >>>> -----Original Message-----
> >>>> From: Yang, Wenyou
> >>>> Sent: Wednesday, March 05, 2014 1:32 PM
> >>>> To: Jean-Christophe PLAGNIOL-VILLARD
> >>>> Cc: linus.walleij@linaro.org; b.brezillon@overkiz.com; <linux-arm-
> >>>> kernel@lists.infradead.org> mailing list; Linux Kernel list;
> >>>> devicetree@vger.kernel.org; robh+dt@kernel.org; pawel.moll@arm.com;
> >>>> mark.rutland@arm.com; ijc+devicetree@hellion.org.uk;
> >>>> galak@codeaurora.org
> >>>> Subject: RE: [PATCH] pinctrl: at91: add the config GPIO_OUTPUT_x
> >>>>
> >>>> Hi JC,
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Jean-Christophe PLAGNIOL-VILLARD
> >>>>> [mailto:plagnioj@jcrosoft.com]
> >>>>> Sent: Wednesday, March 05, 2014 12:58 PM
> >>>>> To: Yang, Wenyou
> >>>>> Cc: Jean-Christophe PLAGNIOL-VILLARD; linus.walleij@linaro.org;
> >>>>> b.brezillon@overkiz.com; <linux-arm-kernel@lists.infradead.org>
> >>>>> mailing list; Linux Kernel list; devicetree@vger.kernel.org;
> >>>>> robh+dt@kernel.org; pawel.moll@arm.com; mark.rutland@arm.com;
> >>>>> ijc+devicetree@hellion.org.uk; galak@codeaurora.org
> >>>>> Subject: Re: [PATCH] pinctrl: at91: add the config GPIO_OUTPUT_x
> >>>>>
> >>>>>
> >>>>> On Mar 5, 2014, at 9:53 AM, Wenyou Yang <wenyou.yang@atmel.com>
> >> wrote:
> >>>>>
> >>>>>> In order to support the pinctrl sleep state.
> >>>>>
> >>>>> As I said before NACK
> >>>>>
> >>>>> this is not the job of the pinctrl to describe gpio output or
> >>>>> input state
> >>>> But according to what said in the section "GPIO mode pitfalls" of
> >>>> Documentation/pinctrl.txt.
> >>>> It should be handle by the pinctrl.
> >>>>
> >>>> If not, to deal with the sleep state will be very complicated.
> >>>> Muxing the pins for FUNCTION to enable peripheral, then twist them
> >>>> over to GPIO mode and use gpio_direction_output() to drive it HIGH
> >>>> or LOW during sleep.
> >>>>
> >>>> --->8 ------------
> >>>> The solution is to not think that what the datasheet calls "GPIO
> >> mode"
> >>>> has to be handled by the <linux/gpio.h> interface. Instead view
> >>>> this as a certain pin config setting. Look in e.g.
> >>>> <linux/pinctrl/pinconf- generic.h> and you find this in the
> documentation:
> >>>>
> >>>> PIN_CONFIG_OUTPUT: this will configure the pin in output, use
> >> argument
> >>>>    1 to indicate high level, argument 0 to indicate low level.
> >>>>
> >>>> So it is perfectly possible to push a pin into "GPIO mode" and
> >>>> drive the line low as part of the usual pin control map.
> >>>> ---<8 ------------
> >>>
> >>> Do you have any feedback?
> >>
> >> Here the issue is that you do drive the gpio and use as a gpio
> >>
> >> which means you request it as a GPIO so after you can not reswitch it
> >> to alternative function and the gpio is already requested as
> >> alternative function
> >>
> >> So basically you mess-up with the gpio API and pinctrl API by
> >> bypassing both
> >
> > But I don't agree with you.
> >
> > As I known, in mainline the other SoC vendors provide such function,
> configure the gpio mode with output config by pinctrl.
> >
> > For example, ST. nomadisk.
> > You can see it in the function: nmk_pin_config_set() of the file,
> pinctrl-nomadik.c.
> > It provides GPIO mode and OUTPUT config.
> >
> > This is a good solution, as what said in Documentation/pinctrl.txt,
> >
> > it is perfectly possible to push a pin into "GPIO mode" and drive the
> line low as part of the usual pin control map.
> >
> 
> This is simple if we use gpio mode as pinctrl tomorrow we will see gpio
> configuration in pinctrl such as chip reset, default led state etc...
> 
> The pinctrl is not here to control a *GPIO* but to describe the
> configuration of pin.
What you said is the usual use case.

But, in the Power Management scenario, there are the following requirements to meet:
When running time, the pins are configured as the PERIPH functions by the pinctrl default state as usaul.
When suspending to memory, the pins are configured as GPIO output low, output high or input by the pinctrl sleep state.
When resuming from suspended, the pins are configured back as the PERIPH functions.

I just found the same implementation by Boris which is better.
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-August/194975.html

Boris, Could you have something to say?

> >>>>>>
> >>>>>> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> >>>>>> ---
> >>>>>> Hi Linus,
> >>>>>>
> >>>>>> The patch is based on branch: for-next
> >>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctr
> >>>>>> l
> >>>>>>
> >>>>>> Best Regards,
> >>>>>> Wenyou Yang
> >>>>>>
> >>>>>> drivers/pinctrl/pinctrl-at91.c     |   31
> >>>>> +++++++++++++++++++++++++++++++
> >>>>>> include/dt-bindings/pinctrl/at91.h |    2 ++
> >>>>>> 2 files changed, 33 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/pinctrl/pinctrl-at91.c
> >>>>>> b/drivers/pinctrl/pinctrl-at91.c index 5d24aae..fc51e59 100644
> >>>>>> --- a/drivers/pinctrl/pinctrl-at91.c
> >>>>>> +++ b/drivers/pinctrl/pinctrl-at91.c
> >>>>>> @@ -62,6 +62,8 @@ static int gpio_banks;
> >>>>>> #define DEGLITCH	(1 << 2)
> >>>>>> #define PULL_DOWN	(1 << 3)
> >>>>>> #define DIS_SCHMIT	(1 << 4)
> >>>>>> +#define GPIO_OUTPUT_HIGH	(1 << 5)
> >>>>>> +#define GPIO_OUTPUT_LOW		(1 << 6)
> >>>>>> #define DEBOUNCE	(1 << 16)
> >>>>>> #define DEBOUNCE_VAL_SHIFT	17
> >>>>>> #define DEBOUNCE_VAL	(0x3fff << DEBOUNCE_VAL_SHIFT)
> >>>>>> @@ -152,12 +154,15 @@ struct at91_pinctrl_mux_ops {
> >>>>>> 	void (*set_pulldown)(void __iomem *pio, unsigned mask, bool
> is_on);
> >>>>>> 	bool (*get_schmitt_trig)(void __iomem *pio, unsigned pin);
> >>>>>> 	void (*disable_schmitt_trig)(void __iomem *pio, unsigned
> mask);
> >>>>>> +	bool (*get_gpio_output)(void __iomem *pio, unsigned mask);
> >>>>>> +	void (*set_gpio_output)(void __iomem *pio, unsigned mask,
> >> bool
> >>>>>> +is_high);
> >>>>>> 	/* irq */
> >>>>>> 	int (*irq_type)(struct irq_data *d, unsigned type); };
> >>>>>>
> >>>>>> static int gpio_irq_type(struct irq_data *d, unsigned type);
> >>>>>> static int alt_gpio_irq_type(struct irq_data *d, unsigned type);
> >>>>>> +static void at91_mux_gpio_enable(void __iomem *pio, unsigned
> >>>>>> +mask, bool input);
> >>>>>>
> >>>>>> struct at91_pinctrl {
> >>>>>> 	struct device		*dev;
> >>>>>> @@ -472,6 +477,20 @@ static bool
> >>>>>> at91_mux_pio3_get_schmitt_trig(void
> >>>>> __iomem *pio, unsigned pin)
> >>>>>> 	return (__raw_readl(pio + PIO_SCHMITT) >> pin) & 0x1; }
> >>>>>>
> >>>>>> +static bool at91_mux_pio3_get_gpio_output(void __iomem *pio,
> >>>>>> +unsigned
> >>>>>> +pin) {
> >>>>>> +	return (__raw_readl(pio + PIO_ODSR) >> pin) & 0x1; }
> >>>>>> +
> >>>>>> +static void at91_mux_pio3_set_gpio_output(void __iomem *pio,
> >>>>>> +						unsigned mask,
> >>>>>> +						bool is_high)
> >>>>>> +{
> >>>>>> +	at91_mux_gpio_enable(pio, mask, 0);
> >>>>>> +	writel_relaxed(mask, pio + (is_high ? PIO_SODR : PIO_CODR)); }
> >>>>>> +
> >>>>>> +
> >>>>>> static struct at91_pinctrl_mux_ops at91rm9200_ops = {
> >>>>>> 	.get_periph	= at91_mux_get_periph,
> >>>>>> 	.mux_A_periph	= at91_mux_set_A_periph,
> >>>>>> @@ -495,6 +514,8 @@ static struct at91_pinctrl_mux_ops
> >>>>>> at91sam9x5_ops
> >>>>> = {
> >>>>>> 	.set_pulldown	= at91_mux_pio3_set_pulldown,
> >>>>>> 	.get_schmitt_trig = at91_mux_pio3_get_schmitt_trig,
> >>>>>> 	.disable_schmitt_trig = at91_mux_pio3_disable_schmitt_trig,
> >>>>>> +	.get_gpio_output = at91_mux_pio3_get_gpio_output,
> >>>>>> +	.set_gpio_output = at91_mux_pio3_set_gpio_output,
> >>>>>> 	.irq_type	= alt_gpio_irq_type,
> >>>>>> };
> >>>>>>
> >>>>>> @@ -741,6 +762,10 @@ static int at91_pinconf_get(struct
> >>>>>> pinctrl_dev
> >>>>> *pctldev,
> >>>>>> 		*config |= PULL_DOWN;
> >>>>>> 	if (info->ops->get_schmitt_trig &&
> >>>>>> info->ops->get_schmitt_trig(pio,
> >>>>> pin))
> >>>>>> 		*config |= DIS_SCHMIT;
> >>>>>> +	if (info->ops->get_gpio_output) {
> >>>>>> +		*config |= info->ops->get_gpio_output(pio, pin) ?
> >>>>>> +					GPIO_OUTPUT_HIGH : GPIO_OUTPUT_LOW;
> >>>>>> +	}
> >>>>>>
> >>>>>> 	return 0;
> >>>>>> }
> >>>>>> @@ -778,6 +803,12 @@ static int at91_pinconf_set(struct
> >>>>>> pinctrl_dev
> >>>>> *pctldev,
> >>>>>> 			info->ops->set_pulldown(pio, mask, config &
> PULL_DOWN);
> >>>>>> 		if (info->ops->disable_schmitt_trig && config &
> DIS_SCHMIT)
> >>>>>> 			info->ops->disable_schmitt_trig(pio, mask);
> >>>>>> +		if (info->ops->set_gpio_output) {
> >>>>>> +			if (config & GPIO_OUTPUT_HIGH)
> >>>>>> +				info->ops->set_gpio_output(pio, mask, 1);
> >>>>>> +			if (config & GPIO_OUTPUT_LOW)
> >>>>>> +				info->ops->set_gpio_output(pio, mask, 0);
> >>>>>> +		};
> >>>>>>
> >>>>>> 	} /* for each config */
> >>>>>>
> >>>>>> diff --git a/include/dt-bindings/pinctrl/at91.h
> >>>>>> b/include/dt-bindings/pinctrl/at91.h
> >>>>>> index 0fee6ff..e799268 100644
> >>>>>> --- a/include/dt-bindings/pinctrl/at91.h
> >>>>>> +++ b/include/dt-bindings/pinctrl/at91.h
> >>>>>> @@ -15,6 +15,8 @@
> >>>>>> #define AT91_PINCTRL_DEGLITCH		(1 << 2)
> >>>>>> #define AT91_PINCTRL_PULL_DOWN		(1 << 3)
> >>>>>> #define AT91_PINCTRL_DIS_SCHMIT		(1 << 4)
> >>>>>> +#define AT91_PINCTRL_OUTPUT_HIGH	(1 << 5)
> >>>>>> +#define AT91_PINCTRL_OUTPUT_LOW		(1 << 6)
> >>>>>> #define AT91_PINCTRL_DEBOUNCE		(1 << 16)
> >>>>>> #define AT91_PINCTRL_DEBOUNCE_VAL(x)	(x << 17)
> >>>>>>
> >>>>>> --
> >>>>>> 1.7.9.5
> >>>>>>
> >>>
> >>>
> >>> _______________________________________________
> >>> linux-arm-kernel mailing list
> >>> linux-arm-kernel@lists.infradead.org
> >>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel


Bes Regards,
Wenyou Yang

Re: [PATCH] pinctrl: at91: add the config GPIO_OUTPUT_x

[Boris BREZILLON]

Le 13/03/2014 07:00, Yang, Wenyou a écrit :
> Hi JC,
>
>> -----Original Message-----
>> From: Jean-Christophe PLAGNIOL-VILLARD [mailto:plagnioj@jcrosoft.com]
>> Sent: Tuesday, March 11, 2014 7:14 PM
>> To: Yang, Wenyou
>> Cc: Jean-Christophe PLAGNIOL-VILLARD; mark.rutland@arm.com;
>> devicetree@vger.kernel.org; pawel.moll@arm.com;
>> ijc+devicetree@hellion.org.uk; linus.walleij@linaro.org; Linux Kernel
>> list; b.brezillon@overkiz.com; robh+dt@kernel.org; galak@codeaurora.org;
>> <linux-arm-kernel@lists.infradead.org> mailing list; Lin, JM
>> Subject: Re: [PATCH] pinctrl: at91: add the config GPIO_OUTPUT_x
>>
>>
>> On Mar 11, 2014, at 2:54 PM, Yang, Wenyou <Wenyou.Yang@atmel.com> wrote:
>>
>>>
>>>> -----Original Message-----
>>>> From: Jean-Christophe PLAGNIOL-VILLARD [mailto:plagnioj@jcrosoft.com]
>>>> Sent: Tuesday, March 11, 2014 12:16 PM
>>>> To: Yang, Wenyou
>>>> Cc: Jean-Christophe PLAGNIOL-VILLARD; mark.rutland@arm.com;
>>>> devicetree@vger.kernel.org; pawel.moll@arm.com;
>>>> ijc+devicetree@hellion.org.uk; linus.walleij@linaro.org; Linux Kernel
>>>> list; b.brezillon@overkiz.com; robh+dt@kernel.org;
>>>> galak@codeaurora.org; <linux-arm-kernel@lists.infradead.org> mailing
>>>> list
>>>> Subject: Re: [PATCH] pinctrl: at91: add the config GPIO_OUTPUT_x
>>>>
>>>>
>>>> On Mar 11, 2014, at 9:28 AM, Yang, Wenyou <Wenyou.Yang@atmel.com>
>> wrote:
>>>>> Hi JC,
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Yang, Wenyou
>>>>>> Sent: Wednesday, March 05, 2014 1:32 PM
>>>>>> To: Jean-Christophe PLAGNIOL-VILLARD
>>>>>> Cc: linus.walleij@linaro.org; b.brezillon@overkiz.com; <linux-arm-
>>>>>> kernel@lists.infradead.org> mailing list; Linux Kernel list;
>>>>>> devicetree@vger.kernel.org; robh+dt@kernel.org; pawel.moll@arm.com;
>>>>>> mark.rutland@arm.com; ijc+devicetree@hellion.org.uk;
>>>>>> galak@codeaurora.org
>>>>>> Subject: RE: [PATCH] pinctrl: at91: add the config GPIO_OUTPUT_x
>>>>>>
>>>>>> Hi JC,
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Jean-Christophe PLAGNIOL-VILLARD
>>>>>>> [mailto:plagnioj@jcrosoft.com]
>>>>>>> Sent: Wednesday, March 05, 2014 12:58 PM
>>>>>>> To: Yang, Wenyou
>>>>>>> Cc: Jean-Christophe PLAGNIOL-VILLARD; linus.walleij@linaro.org;
>>>>>>> b.brezillon@overkiz.com; <linux-arm-kernel@lists.infradead.org>
>>>>>>> mailing list; Linux Kernel list; devicetree@vger.kernel.org;
>>>>>>> robh+dt@kernel.org; pawel.moll@arm.com; mark.rutland@arm.com;
>>>>>>> ijc+devicetree@hellion.org.uk; galak@codeaurora.org
>>>>>>> Subject: Re: [PATCH] pinctrl: at91: add the config GPIO_OUTPUT_x
>>>>>>>
>>>>>>>
>>>>>>> On Mar 5, 2014, at 9:53 AM, Wenyou Yang <wenyou.yang@atmel.com>
>>>> wrote:
>>>>>>>> In order to support the pinctrl sleep state.
>>>>>>> As I said before NACK
>>>>>>>
>>>>>>> this is not the job of the pinctrl to describe gpio output or
>>>>>>> input state
>>>>>> But according to what said in the section "GPIO mode pitfalls" of
>>>>>> Documentation/pinctrl.txt.
>>>>>> It should be handle by the pinctrl.
>>>>>>
>>>>>> If not, to deal with the sleep state will be very complicated.
>>>>>> Muxing the pins for FUNCTION to enable peripheral, then twist them
>>>>>> over to GPIO mode and use gpio_direction_output() to drive it HIGH
>>>>>> or LOW during sleep.
>>>>>>
>>>>>> --->8 ------------
>>>>>> The solution is to not think that what the datasheet calls "GPIO
>>>> mode"
>>>>>> has to be handled by the <linux/gpio.h> interface. Instead view
>>>>>> this as a certain pin config setting. Look in e.g.
>>>>>> <linux/pinctrl/pinconf- generic.h> and you find this in the
>> documentation:
>>>>>> PIN_CONFIG_OUTPUT: this will configure the pin in output, use
>>>> argument
>>>>>>     1 to indicate high level, argument 0 to indicate low level.
>>>>>>
>>>>>> So it is perfectly possible to push a pin into "GPIO mode" and
>>>>>> drive the line low as part of the usual pin control map.
>>>>>> ---<8 ------------
>>>>> Do you have any feedback?
>>>> Here the issue is that you do drive the gpio and use as a gpio
>>>>
>>>> which means you request it as a GPIO so after you can not reswitch it
>>>> to alternative function and the gpio is already requested as
>>>> alternative function
>>>>
>>>> So basically you mess-up with the gpio API and pinctrl API by
>>>> bypassing both
>>> But I don't agree with you.
>>>
>>> As I known, in mainline the other SoC vendors provide such function,
>> configure the gpio mode with output config by pinctrl.
>>> For example, ST. nomadisk.
>>> You can see it in the function: nmk_pin_config_set() of the file,
>> pinctrl-nomadik.c.
>>> It provides GPIO mode and OUTPUT config.
>>>
>>> This is a good solution, as what said in Documentation/pinctrl.txt,
>>>
>>> it is perfectly possible to push a pin into "GPIO mode" and drive the
>> line low as part of the usual pin control map.
>> This is simple if we use gpio mode as pinctrl tomorrow we will see gpio
>> configuration in pinctrl such as chip reset, default led state etc...
>>
>> The pinctrl is not here to control a *GPIO* but to describe the
>> configuration of pin.
> What you said is the usual use case.
>
> But, in the Power Management scenario, there are the following requirements to meet:
> When running time, the pins are configured as the PERIPH functions by the pinctrl default state as usaul.
> When suspending to memory, the pins are configured as GPIO output low, output high or input by the pinctrl sleep state.
> When resuming from suspended, the pins are configured back as the PERIPH functions.
>
> I just found the same implementation by Boris which is better.
> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-August/194975.html
>
> Boris, Could you have something to say?

Yes.

I still think the pinconf OUTPUT state is useful in some cases, and the
one you described is obviously one of these (I needed it for a different
reason : https://lkml.org/lkml/2013/8/28/203 which appeared to be a wrong
one).

But I'm not the pinctrl subsystem or the at91 pinctrl driver maintainer...

Linus, do you have an opinion on this feature ?

>>>>>>>> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
>>>>>>>> ---
>>>>>>>> Hi Linus,
>>>>>>>>
>>>>>>>> The patch is based on branch: for-next
>>>>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctr
>>>>>>>> l
>>>>>>>>
>>>>>>>> Best Regards,
>>>>>>>> Wenyou Yang
>>>>>>>>
>>>>>>>> drivers/pinctrl/pinctrl-at91.c     |   31
>>>>>>> +++++++++++++++++++++++++++++++
>>>>>>>> include/dt-bindings/pinctrl/at91.h |    2 ++
>>>>>>>> 2 files changed, 33 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/pinctrl/pinctrl-at91.c
>>>>>>>> b/drivers/pinctrl/pinctrl-at91.c index 5d24aae..fc51e59 100644
>>>>>>>> --- a/drivers/pinctrl/pinctrl-at91.c
>>>>>>>> +++ b/drivers/pinctrl/pinctrl-at91.c
>>>>>>>> @@ -62,6 +62,8 @@ static int gpio_banks;
>>>>>>>> #define DEGLITCH	(1 << 2)
>>>>>>>> #define PULL_DOWN	(1 << 3)
>>>>>>>> #define DIS_SCHMIT	(1 << 4)
>>>>>>>> +#define GPIO_OUTPUT_HIGH	(1 << 5)
>>>>>>>> +#define GPIO_OUTPUT_LOW		(1 << 6)
>>>>>>>> #define DEBOUNCE	(1 << 16)
>>>>>>>> #define DEBOUNCE_VAL_SHIFT	17
>>>>>>>> #define DEBOUNCE_VAL	(0x3fff << DEBOUNCE_VAL_SHIFT)
>>>>>>>> @@ -152,12 +154,15 @@ struct at91_pinctrl_mux_ops {
>>>>>>>> 	void (*set_pulldown)(void __iomem *pio, unsigned mask, bool
>> is_on);
>>>>>>>> 	bool (*get_schmitt_trig)(void __iomem *pio, unsigned pin);
>>>>>>>> 	void (*disable_schmitt_trig)(void __iomem *pio, unsigned
>> mask);
>>>>>>>> +	bool (*get_gpio_output)(void __iomem *pio, unsigned mask);
>>>>>>>> +	void (*set_gpio_output)(void __iomem *pio, unsigned mask,
>>>> bool
>>>>>>>> +is_high);
>>>>>>>> 	/* irq */
>>>>>>>> 	int (*irq_type)(struct irq_data *d, unsigned type); };
>>>>>>>>
>>>>>>>> static int gpio_irq_type(struct irq_data *d, unsigned type);
>>>>>>>> static int alt_gpio_irq_type(struct irq_data *d, unsigned type);
>>>>>>>> +static void at91_mux_gpio_enable(void __iomem *pio, unsigned
>>>>>>>> +mask, bool input);
>>>>>>>>
>>>>>>>> struct at91_pinctrl {
>>>>>>>> 	struct device		*dev;
>>>>>>>> @@ -472,6 +477,20 @@ static bool
>>>>>>>> at91_mux_pio3_get_schmitt_trig(void
>>>>>>> __iomem *pio, unsigned pin)
>>>>>>>> 	return (__raw_readl(pio + PIO_SCHMITT) >> pin) & 0x1; }
>>>>>>>>
>>>>>>>> +static bool at91_mux_pio3_get_gpio_output(void __iomem *pio,
>>>>>>>> +unsigned
>>>>>>>> +pin) {
>>>>>>>> +	return (__raw_readl(pio + PIO_ODSR) >> pin) & 0x1; }
>>>>>>>> +
>>>>>>>> +static void at91_mux_pio3_set_gpio_output(void __iomem *pio,
>>>>>>>> +						unsigned mask,
>>>>>>>> +						bool is_high)
>>>>>>>> +{
>>>>>>>> +	at91_mux_gpio_enable(pio, mask, 0);
>>>>>>>> +	writel_relaxed(mask, pio + (is_high ? PIO_SODR : PIO_CODR)); }
>>>>>>>> +
>>>>>>>> +
>>>>>>>> static struct at91_pinctrl_mux_ops at91rm9200_ops = {
>>>>>>>> 	.get_periph	= at91_mux_get_periph,
>>>>>>>> 	.mux_A_periph	= at91_mux_set_A_periph,
>>>>>>>> @@ -495,6 +514,8 @@ static struct at91_pinctrl_mux_ops
>>>>>>>> at91sam9x5_ops
>>>>>>> = {
>>>>>>>> 	.set_pulldown	= at91_mux_pio3_set_pulldown,
>>>>>>>> 	.get_schmitt_trig = at91_mux_pio3_get_schmitt_trig,
>>>>>>>> 	.disable_schmitt_trig = at91_mux_pio3_disable_schmitt_trig,
>>>>>>>> +	.get_gpio_output = at91_mux_pio3_get_gpio_output,
>>>>>>>> +	.set_gpio_output = at91_mux_pio3_set_gpio_output,
>>>>>>>> 	.irq_type	= alt_gpio_irq_type,
>>>>>>>> };
>>>>>>>>
>>>>>>>> @@ -741,6 +762,10 @@ static int at91_pinconf_get(struct
>>>>>>>> pinctrl_dev
>>>>>>> *pctldev,
>>>>>>>> 		*config |= PULL_DOWN;
>>>>>>>> 	if (info->ops->get_schmitt_trig &&
>>>>>>>> info->ops->get_schmitt_trig(pio,
>>>>>>> pin))
>>>>>>>> 		*config |= DIS_SCHMIT;
>>>>>>>> +	if (info->ops->get_gpio_output) {
>>>>>>>> +		*config |= info->ops->get_gpio_output(pio, pin) ?
>>>>>>>> +					GPIO_OUTPUT_HIGH : GPIO_OUTPUT_LOW;
>>>>>>>> +	}
>>>>>>>>
>>>>>>>> 	return 0;
>>>>>>>> }
>>>>>>>> @@ -778,6 +803,12 @@ static int at91_pinconf_set(struct
>>>>>>>> pinctrl_dev
>>>>>>> *pctldev,
>>>>>>>> 			info->ops->set_pulldown(pio, mask, config &
>> PULL_DOWN);
>>>>>>>> 		if (info->ops->disable_schmitt_trig && config &
>> DIS_SCHMIT)
>>>>>>>> 			info->ops->disable_schmitt_trig(pio, mask);
>>>>>>>> +		if (info->ops->set_gpio_output) {
>>>>>>>> +			if (config & GPIO_OUTPUT_HIGH)
>>>>>>>> +				info->ops->set_gpio_output(pio, mask, 1);
>>>>>>>> +			if (config & GPIO_OUTPUT_LOW)
>>>>>>>> +				info->ops->set_gpio_output(pio, mask, 0);
>>>>>>>> +		};
>>>>>>>>
>>>>>>>> 	} /* for each config */
>>>>>>>>
>>>>>>>> diff --git a/include/dt-bindings/pinctrl/at91.h
>>>>>>>> b/include/dt-bindings/pinctrl/at91.h
>>>>>>>> index 0fee6ff..e799268 100644
>>>>>>>> --- a/include/dt-bindings/pinctrl/at91.h
>>>>>>>> +++ b/include/dt-bindings/pinctrl/at91.h
>>>>>>>> @@ -15,6 +15,8 @@
>>>>>>>> #define AT91_PINCTRL_DEGLITCH		(1 << 2)
>>>>>>>> #define AT91_PINCTRL_PULL_DOWN		(1 << 3)
>>>>>>>> #define AT91_PINCTRL_DIS_SCHMIT		(1 << 4)
>>>>>>>> +#define AT91_PINCTRL_OUTPUT_HIGH	(1 << 5)
>>>>>>>> +#define AT91_PINCTRL_OUTPUT_LOW		(1 << 6)
>>>>>>>> #define AT91_PINCTRL_DEBOUNCE		(1 << 16)
>>>>>>>> #define AT91_PINCTRL_DEBOUNCE_VAL(x)	(x << 17)
>>>>>>>>
>>>>>>>> --
>>>>>>>> 1.7.9.5
>>>>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> linux-arm-kernel mailing list
>>>>> linux-arm-kernel@lists.infradead.org
>>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
> Bes Regards,
> Wenyou Yang
>

Re: [PATCH] pinctrl: at91: add the config GPIO_OUTPUT_x

[Nicolas Ferre]

On 05/03/2014 02:53, Wenyou Yang :
> In order to support the pinctrl sleep state.
> 
> Signed-off-by: Wenyou Yang <wenyou.yang-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>

Wenyou,

This patch has been discussed here:
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-August/194975.html

I even gave my Acknowledgement on Boris' patch. Can you please check the
differences between Boris' patch and yours?

Linus,

For the nature of this feature, I would like to have your clear advice
as this is needed and has been proposed twice (at least). This feature
is needed for power management states of GPIOs.

Best regards,


> ---
> Hi Linus,
> 
> The patch is based on branch: for-next
> git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl
> 
> Best Regards,
> Wenyou Yang
> 
>  drivers/pinctrl/pinctrl-at91.c     |   31 +++++++++++++++++++++++++++++++
>  include/dt-bindings/pinctrl/at91.h |    2 ++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
> index 5d24aae..fc51e59 100644
> --- a/drivers/pinctrl/pinctrl-at91.c
> +++ b/drivers/pinctrl/pinctrl-at91.c
> @@ -62,6 +62,8 @@ static int gpio_banks;
>  #define DEGLITCH	(1 << 2)
>  #define PULL_DOWN	(1 << 3)
>  #define DIS_SCHMIT	(1 << 4)
> +#define GPIO_OUTPUT_HIGH	(1 << 5)
> +#define GPIO_OUTPUT_LOW		(1 << 6)
>  #define DEBOUNCE	(1 << 16)
>  #define DEBOUNCE_VAL_SHIFT	17
>  #define DEBOUNCE_VAL	(0x3fff << DEBOUNCE_VAL_SHIFT)
> @@ -152,12 +154,15 @@ struct at91_pinctrl_mux_ops {
>  	void (*set_pulldown)(void __iomem *pio, unsigned mask, bool is_on);
>  	bool (*get_schmitt_trig)(void __iomem *pio, unsigned pin);
>  	void (*disable_schmitt_trig)(void __iomem *pio, unsigned mask);
> +	bool (*get_gpio_output)(void __iomem *pio, unsigned mask);
> +	void (*set_gpio_output)(void __iomem *pio, unsigned mask, bool is_high);
>  	/* irq */
>  	int (*irq_type)(struct irq_data *d, unsigned type);
>  };
>  
>  static int gpio_irq_type(struct irq_data *d, unsigned type);
>  static int alt_gpio_irq_type(struct irq_data *d, unsigned type);
> +static void at91_mux_gpio_enable(void __iomem *pio, unsigned mask, bool input);
>  
>  struct at91_pinctrl {
>  	struct device		*dev;
> @@ -472,6 +477,20 @@ static bool at91_mux_pio3_get_schmitt_trig(void __iomem *pio, unsigned pin)
>  	return (__raw_readl(pio + PIO_SCHMITT) >> pin) & 0x1;
>  }
>  
> +static bool at91_mux_pio3_get_gpio_output(void __iomem *pio, unsigned pin)
> +{
> +	return (__raw_readl(pio + PIO_ODSR) >> pin) & 0x1;
> +}
> +
> +static void at91_mux_pio3_set_gpio_output(void __iomem *pio,
> +						unsigned mask,
> +						bool is_high)
> +{
> +	at91_mux_gpio_enable(pio, mask, 0);
> +	writel_relaxed(mask, pio + (is_high ? PIO_SODR : PIO_CODR));
> +}
> +
> +
>  static struct at91_pinctrl_mux_ops at91rm9200_ops = {
>  	.get_periph	= at91_mux_get_periph,
>  	.mux_A_periph	= at91_mux_set_A_periph,
> @@ -495,6 +514,8 @@ static struct at91_pinctrl_mux_ops at91sam9x5_ops = {
>  	.set_pulldown	= at91_mux_pio3_set_pulldown,
>  	.get_schmitt_trig = at91_mux_pio3_get_schmitt_trig,
>  	.disable_schmitt_trig = at91_mux_pio3_disable_schmitt_trig,
> +	.get_gpio_output = at91_mux_pio3_get_gpio_output,
> +	.set_gpio_output = at91_mux_pio3_set_gpio_output,
>  	.irq_type	= alt_gpio_irq_type,
>  };
>  
> @@ -741,6 +762,10 @@ static int at91_pinconf_get(struct pinctrl_dev *pctldev,
>  		*config |= PULL_DOWN;
>  	if (info->ops->get_schmitt_trig && info->ops->get_schmitt_trig(pio, pin))
>  		*config |= DIS_SCHMIT;
> +	if (info->ops->get_gpio_output) {
> +		*config |= info->ops->get_gpio_output(pio, pin) ?
> +					GPIO_OUTPUT_HIGH : GPIO_OUTPUT_LOW;
> +	}
>  
>  	return 0;
>  }
> @@ -778,6 +803,12 @@ static int at91_pinconf_set(struct pinctrl_dev *pctldev,
>  			info->ops->set_pulldown(pio, mask, config & PULL_DOWN);
>  		if (info->ops->disable_schmitt_trig && config & DIS_SCHMIT)
>  			info->ops->disable_schmitt_trig(pio, mask);
> +		if (info->ops->set_gpio_output) {
> +			if (config & GPIO_OUTPUT_HIGH)
> +				info->ops->set_gpio_output(pio, mask, 1);
> +			if (config & GPIO_OUTPUT_LOW)
> +				info->ops->set_gpio_output(pio, mask, 0);
> +		};
>  
>  	} /* for each config */
>  
> diff --git a/include/dt-bindings/pinctrl/at91.h b/include/dt-bindings/pinctrl/at91.h
> index 0fee6ff..e799268 100644
> --- a/include/dt-bindings/pinctrl/at91.h
> +++ b/include/dt-bindings/pinctrl/at91.h
> @@ -15,6 +15,8 @@
>  #define AT91_PINCTRL_DEGLITCH		(1 << 2)
>  #define AT91_PINCTRL_PULL_DOWN		(1 << 3)
>  #define AT91_PINCTRL_DIS_SCHMIT		(1 << 4)
> +#define AT91_PINCTRL_OUTPUT_HIGH	(1 << 5)
> +#define AT91_PINCTRL_OUTPUT_LOW		(1 << 6)
>  #define AT91_PINCTRL_DEBOUNCE		(1 << 16)
>  #define AT91_PINCTRL_DEBOUNCE_VAL(x)	(x << 17)
>  
> 


-- 
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pinctrl: at91: add the config GPIO_OUTPUT_x

[Linus Walleij]

On Tue, Mar 11, 2014 at 6:37 PM, Nicolas Ferre <nicolas.ferre@atmel.com> wrote:

> Linus,
>
> For the nature of this feature, I would like to have your clear advice
> as this is needed and has been proposed twice (at least). This feature
> is needed for power management states of GPIOs.

It seems that this is aligned with what other pin control drivers do
to move pins to low power states. As long as the same pins are
never used for "ordinary" GPIO through gpiolib I think doing this is
just some electric aspect of the pin and controlled by the device using
it as part of its normal low-power operation. No different from e.g.
setting a different biasing or decoupling or grounding the pin or whatever.

The fact that the register or bit(s) are named ...gpio-something... is
irrelevant if they cannot be accessed as such.

If you want to be *really* picky you can start go about to edit
your gpio-ranges so that it is impossible to cross-reference one of
these GPIOs to a pin, and thus impossible to use them from the
GPIO subsystem. (Right now the at91 driver is using
pinctrl_add_gpio_range() so switch this to using ranges defined
from the device tree if you are fully migrated to DT.)

Basically this is a system issue, how you guys want to handle the
AT91 low-power modes.

I think you are three maintainers of AT91:

M:      Andrew Victor <linux@maxim.org.za>
M:      Nicolas Ferre <nicolas.ferre@atmel.com>
M:      Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>

Can you get a majority vote on how you want this to work on AT91?
If two out of three AT91 maintainers ACK the patch I will apply it, and
then you three can fight amongst yourselves ;-)

Yours,
Linus Walleij