[PATCH 0/6] pinctrl: st: Fix disparity between Pinctrl & GPIO in /sysfs

[PATCH 0/6] pinctrl: st: Fix disparity between Pinctrl & GPIO in /sysfs

[Lee Jones]

ST's hardware differentiates between GPIO mode and Pinctrl alternate
functions.  When a pin is in GPIO mode, there are dedicated registers
to set and obtain direction status.  However, If a pin's alternate
function is in use then the direction is set and status is derived
from a bunch of syscon registers.  The issue is; until now there was
a lack of parity between the two.

Further explanation with examples can be found in the commit logs.

Karim BEN BELGACEM (1):
  ARM: STi: DT: STiH407: Fix retime pin mask for PIO5 and PIO35

Lee Jones (5):
  pinctrl: st: Introduce a 'get pin function' call
  pinctrl: st: Move st_get_pio_control() further up the source file
  pinctrl: st: Supply a GPIO get_direction() call-back
  pinctrl: st: Show correct pin direction -- even when in GPIO mode
  pinctrl: st: Display pin's function when printing pinctrl debug
    information

 arch/arm/boot/dts/stih407-pinctrl.dtsi |  2 +
 drivers/pinctrl/pinctrl-st.c           | 83 +++++++++++++++++++++++++++++-----
 2 files changed, 73 insertions(+), 12 deletions(-)

-- 
1.9.1

[PATCH 1/6] ARM: STi: DT: STiH407: Fix retime pin mask for PIO5 and PIO35

[Lee Jones]

From: Karim BEN BELGACEM <karim.ben-belgacem@st.com>

This will avoid programming the retime registers when not implemented

- PIO5  : no retime registers assigned to pins 6 and 7
- PIO35 : pin 7 is reserved so no retime register assigned to it

Signed-off-by: Karim BEN BELGACEM <karim.ben-belgacem@st.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/stih407-pinctrl.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/stih407-pinctrl.dtsi b/arch/arm/boot/dts/stih407-pinctrl.dtsi
index 402844c..0a754f2 100644
--- a/arch/arm/boot/dts/stih407-pinctrl.dtsi
+++ b/arch/arm/boot/dts/stih407-pinctrl.dtsi
@@ -104,6 +104,7 @@
 				#interrupt-cells = <2>;
 				reg = <0x5000 0x100>;
 				st,bank-name = "PIO5";
+				st,retime-pin-mask = <0x3f>;
 			};
 
 			rc {
@@ -519,6 +520,7 @@
 				#interrupt-cells = <2>;
 				reg = <0x5000 0x100>;
 				st,bank-name = "PIO35";
+				st,retime-pin-mask = <0x7f>;
 			};
 
 			i2c4 {
-- 
1.9.1

[PATCH 2/6] pinctrl: st: Introduce a 'get pin function' call

[Lee Jones]

This call fetches the numerical function value a specified pin is
currently operating in.  Function zero is more often than not the
GPIO function.  Greater than zero values represent an alternative
function.  You'd need to either look those up in the Device Tree
sources or the Programmer's Manual.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/pinctrl/pinctrl-st.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-st.c b/drivers/pinctrl/pinctrl-st.c
index 9e5ec00..5362e45 100644
--- a/drivers/pinctrl/pinctrl-st.c
+++ b/drivers/pinctrl/pinctrl-st.c
@@ -460,6 +460,20 @@ static void st_pctl_set_function(struct st_pio_control *pc,
 	regmap_field_write(alt, val);
 }
 
+static unsigned int st_pctl_get_pin_function(struct st_pio_control *pc, int pin)
+{
+	struct regmap_field *alt = pc->alt;
+	unsigned int val;
+	int offset = pin * 4;
+
+	if (!alt)
+		return 0;
+
+	regmap_field_read(alt, &val);
+
+	return (val >> offset) & 0xf;
+}
+
 static unsigned long st_pinconf_delay_to_bit(unsigned int delay,
 	const struct st_pctl_data *data, unsigned long config)
 {
-- 
1.9.1

[PATCH 3/6] pinctrl: st: Move st_get_pio_control() further up the source file

[Lee Jones]

st_get_pio_control() will be used by subsequent calls which are
to be located above its original position.  This is required to
prevent the need for an unnecessary forward-declaration/prototype.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/pinctrl/pinctrl-st.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-st.c b/drivers/pinctrl/pinctrl-st.c
index 5362e45..10ad19c 100644
--- a/drivers/pinctrl/pinctrl-st.c
+++ b/drivers/pinctrl/pinctrl-st.c
@@ -398,6 +398,16 @@ static const struct st_pctl_data stih407_flashdata = {
 	.rt = 100,
 };
 
+static struct st_pio_control *st_get_pio_control(
+			struct pinctrl_dev *pctldev, int pin)
+{
+	struct pinctrl_gpio_range *range =
+			 pinctrl_find_gpio_range_from_pin(pctldev, pin);
+	struct st_gpio_bank *bank = gpio_range_to_bank(range);
+
+	return &bank->pc;
+}
+
 /* Low level functions.. */
 static inline int st_gpio_bank(int gpio)
 {
@@ -918,16 +928,6 @@ static int st_pmx_get_groups(struct pinctrl_dev *pctldev,
 	return 0;
 }
 
-static struct st_pio_control *st_get_pio_control(
-			struct pinctrl_dev *pctldev, int pin)
-{
-	struct pinctrl_gpio_range *range =
-			 pinctrl_find_gpio_range_from_pin(pctldev, pin);
-	struct st_gpio_bank *bank = gpio_range_to_bank(range);
-
-	return &bank->pc;
-}
-
 static int st_pmx_set_mux(struct pinctrl_dev *pctldev, unsigned fselector,
 			unsigned group)
 {
-- 
1.9.1

[PATCH 4/6] pinctrl: st: Supply a GPIO get_direction() call-back

[Lee Jones]

ST's hardware differentiates between GPIO mode and Pinctrl alternate
functions.  When a pin is in GPIO mode, there are dedicated registers
to set and obtain direction status.  However, If a pin's alternate
function is in use then the direction is set and status is derived
from a bunch of syscon registers.  The issue is; until now there was
a lack of parity between the two.

For example:

Catting the two following information sources could result in
conflicting information (output has been snipped for simplicity):

 $ cat /sys/kernel/debug/gpio
  GPIOs 32-39, platform/961f080.pin-controller-sbc, PIO4:
   gpio-33  (?                   ) out hi

 $ cat /sys/kernel/debug/pinctrl/<pin-controller>/pinconf-pins
  pin 33 (PIO4[1]):[OE:0,PU:0,OD:0]
         [retime:0,invclk:0,clknotdat:0,de:0,rt-clk:0,rt-delay:0]

In this example GPIO-33 is a GPIO controlled LED, which is set for
output, as you'd expect.  However, when the same information is
drafted from Pinctrl, it clearly states that OE (Output Enable) is
not set i.e. the pin is set for input.  This is because OE normally
only represents alternate functions and has no bearing on how the
pin operates when in Alt-0 (GPIO mode).

This patch changes the current semantics and provides a parity link
between the two subsystems.  The get_direction() call-back firstly
determines which function a pin is operating in, then uses the
appropriate helpers for that mode.

Reported-by: Olivier Clergeaud <olivier.clergeaud@st.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/pinctrl/pinctrl-st.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/pinctrl-st.c b/drivers/pinctrl/pinctrl-st.c
index 10ad19c..52a4377 100644
--- a/drivers/pinctrl/pinctrl-st.c
+++ b/drivers/pinctrl/pinctrl-st.c
@@ -206,7 +206,6 @@
 #define gpio_chip_to_bank(chip) \
 		container_of(chip, struct st_gpio_bank, gpio_chip)
 
-
 enum st_retime_style {
 	st_retime_style_none,
 	st_retime_style_packed,
@@ -781,6 +780,35 @@ static int st_gpio_direction_output(struct gpio_chip *chip,
 	return 0;
 }
 
+static int st_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
+{
+	struct st_gpio_bank *bank = gpio_chip_to_bank(chip);
+	struct st_pio_control pc = bank->pc;
+	unsigned long config;
+	unsigned int direction = 0;
+	unsigned int function;
+	unsigned int value;
+	int i = 0;
+
+	/* Alternate function direction is handled by Pinctrl */
+	function = st_pctl_get_pin_function(&pc, offset);
+	if (function) {
+		st_pinconf_get_direction(&pc, offset, &config);
+		return !ST_PINCONF_UNPACK_OE(config);
+	}
+
+	/*
+	 * GPIO direction is handled differently
+	 * - See st_gpio_direction() above for an explanation
+	 */
+	for (i = 0; i <= 2; i++) {
+		value = readl(bank->base + REG_PIO_PC(i));
+		direction |= ((value >> offset) & 0x1) << i;
+	}
+
+	return (direction == ST_GPIO_DIRECTION_IN);
+}
+
 static int st_gpio_xlate(struct gpio_chip *gc,
 			const struct of_phandle_args *gpiospec, u32 *flags)
 {
@@ -1452,6 +1480,7 @@ static struct gpio_chip st_gpio_template = {
 	.set			= st_gpio_set,
 	.direction_input	= st_gpio_direction_input,
 	.direction_output	= st_gpio_direction_output,
+	.get_direction		= st_gpio_get_direction,
 	.ngpio			= ST_GPIO_PINS_PER_BANK,
 	.of_gpio_n_cells	= 1,
 	.of_xlate		= st_gpio_xlate,
-- 
1.9.1

[PATCH 5/6] pinctrl: st: Show correct pin direction -- even when in GPIO mode

[Lee Jones]

Until now ST's pinconf_dbg_show() call-back has displayed the PIO
alternate function direction, which is only relevant if a pin is
operating in an alternate function mode i.e not GPIO mode.  If a
pin is in GPIO mode its direction is both set and status is
obtained by a completely different/unrelated bunch of registers.

This change ensures that the correct pin direction is shown, even
if a pin is operating in GPIO mode.

Reported-by: Olivier Clergeaud <olivier.clergeaud@st.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/pinctrl/pinctrl-st.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/pinctrl-st.c b/drivers/pinctrl/pinctrl-st.c
index 52a4377..1cda40e 100644
--- a/drivers/pinctrl/pinctrl-st.c
+++ b/drivers/pinctrl/pinctrl-st.c
@@ -206,6 +206,9 @@
 #define gpio_chip_to_bank(chip) \
 		container_of(chip, struct st_gpio_bank, gpio_chip)
 
+#define pc_to_bank(pc) \
+		container_of(pc, struct st_gpio_bank, pc)
+
 enum st_retime_style {
 	st_retime_style_none,
 	st_retime_style_packed,
@@ -1053,15 +1056,18 @@ static int st_pinconf_get(struct pinctrl_dev *pctldev,
 static void st_pinconf_dbg_show(struct pinctrl_dev *pctldev,
 				   struct seq_file *s, unsigned pin_id)
 {
+	struct st_pio_control *pc;
 	unsigned long config;
+	int offset = st_gpio_pin(pin_id);
 
 	mutex_unlock(&pctldev->mutex);
+	pc = st_get_pio_control(pctldev, pin_id);
 	st_pinconf_get(pctldev, pin_id, &config);
 	mutex_lock(&pctldev->mutex);
 	seq_printf(s, "[OE:%ld,PU:%ld,OD:%ld]\n"
 		"\t\t[retime:%ld,invclk:%ld,clknotdat:%ld,"
 		"de:%ld,rt-clk:%ld,rt-delay:%ld]",
-		ST_PINCONF_UNPACK_OE(config),
+		!st_gpio_get_direction(&pc_to_bank(pc)->gpio_chip, offset),
 		ST_PINCONF_UNPACK_PU(config),
 		ST_PINCONF_UNPACK_OD(config),
 		ST_PINCONF_UNPACK_RT(config),
-- 
1.9.1

[PATCH 6/6] pinctrl: st: Display pin's function when printing pinctrl debug information

[Lee Jones]

Great for easily determining which mode a pin is operating in.
This patch was particularly helpful when debugging a recent GPIO/
Pinctrl disparity issue.

Before:
    $ cat /sys/kernel/debug/pinctrl/<pin-controller>/pinconf-pins
      pin 33 (PIO4[1]):[OE:0,PU:0,OD:0]
             [retime:0,invclk:0,clknotdat:0,de:0,rt-clk:0,rt-delay:0]

After [GPIO]:
    $ cat /sys/kernel/debug/pinctrl/<pin-controller>/pinconf-pins
      pin 33 (PIO4[1]):[OE:0,PU:0,OD:0] GPIO
             [retime:0,invclk:0,clknotdat:0,de:0,rt-clk:0,rt-delay:0]

After [Alt]:
    $ cat /sys/kernel/debug/pinctrl/<pin-controller>/pinconf-pins
      pin 33 (PIO4[1]):[OE:0,PU:0,OD:0] Alt Fn 2
             [retime:0,invclk:0,clknotdat:0,de:0,rt-clk:0,rt-delay:0]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/pinctrl/pinctrl-st.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/pinctrl-st.c b/drivers/pinctrl/pinctrl-st.c
index 1cda40e..d5b2ffa 100644
--- a/drivers/pinctrl/pinctrl-st.c
+++ b/drivers/pinctrl/pinctrl-st.c
@@ -1058,18 +1058,28 @@ static void st_pinconf_dbg_show(struct pinctrl_dev *pctldev,
 {
 	struct st_pio_control *pc;
 	unsigned long config;
+	unsigned int function;
 	int offset = st_gpio_pin(pin_id);
+	char f[64];
 
 	mutex_unlock(&pctldev->mutex);
 	pc = st_get_pio_control(pctldev, pin_id);
 	st_pinconf_get(pctldev, pin_id, &config);
 	mutex_lock(&pctldev->mutex);
-	seq_printf(s, "[OE:%ld,PU:%ld,OD:%ld]\n"
+
+	function = st_pctl_get_pin_function(pc, offset);
+	if (function)
+		sprintf(f, "Alt Fn %d", function);
+	else
+		sprintf(f, "GPIO");
+
+	seq_printf(s, "[OE:%d,PU:%ld,OD:%ld]\t%s\n"
 		"\t\t[retime:%ld,invclk:%ld,clknotdat:%ld,"
 		"de:%ld,rt-clk:%ld,rt-delay:%ld]",
 		!st_gpio_get_direction(&pc_to_bank(pc)->gpio_chip, offset),
 		ST_PINCONF_UNPACK_PU(config),
 		ST_PINCONF_UNPACK_OD(config),
+		f,
 		ST_PINCONF_UNPACK_RT(config),
 		ST_PINCONF_UNPACK_RT_INVERTCLK(config),
 		ST_PINCONF_UNPACK_RT_CLKNOTDATA(config),
-- 
1.9.1

[STLinux Kernel] [PATCH 6/6] pinctrl: st: Display pin's function when printing pinctrl debug information

[Maxime Coquelin]

Hi Lee,

On 03/18/2015 11:51 AM, Lee Jones wrote:
> Great for easily determining which mode a pin is operating in.
> This patch was particularly helpful when debugging a recent GPIO/
> Pinctrl disparity issue.
>
> Before:
>      $ cat /sys/kernel/debug/pinctrl/<pin-controller>/pinconf-pins
>        pin 33 (PIO4[1]):[OE:0,PU:0,OD:0]
>               [retime:0,invclk:0,clknotdat:0,de:0,rt-clk:0,rt-delay:0]
>
> After [GPIO]:
>      $ cat /sys/kernel/debug/pinctrl/<pin-controller>/pinconf-pins
>        pin 33 (PIO4[1]):[OE:0,PU:0,OD:0] GPIO
>               [retime:0,invclk:0,clknotdat:0,de:0,rt-clk:0,rt-delay:0]
>
> After [Alt]:
>      $ cat /sys/kernel/debug/pinctrl/<pin-controller>/pinconf-pins
>        pin 33 (PIO4[1]):[OE:0,PU:0,OD:0] Alt Fn 2
>               [retime:0,invclk:0,clknotdat:0,de:0,rt-clk:0,rt-delay:0]
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>   drivers/pinctrl/pinctrl-st.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
A couple of comments below.
Once fixed, you can add my:
Acked-by: Maxime Coquelin <maxime.coquelin@st.com>



>
> diff --git a/drivers/pinctrl/pinctrl-st.c b/drivers/pinctrl/pinctrl-st.c
> index 1cda40e..d5b2ffa 100644
> --- a/drivers/pinctrl/pinctrl-st.c
> +++ b/drivers/pinctrl/pinctrl-st.c
> @@ -1058,18 +1058,28 @@ static void st_pinconf_dbg_show(struct pinctrl_dev *pctldev,
>   {
>   	struct st_pio_control *pc;
>   	unsigned long config;
> +	unsigned int function;
>   	int offset = st_gpio_pin(pin_id);
> +	char f[64];
64 bytes is way too big here
>   
>   	mutex_unlock(&pctldev->mutex);
>   	pc = st_get_pio_control(pctldev, pin_id);
>   	st_pinconf_get(pctldev, pin_id, &config);
>   	mutex_lock(&pctldev->mutex);
> -	seq_printf(s, "[OE:%ld,PU:%ld,OD:%ld]\n"
> +
> +	function = st_pctl_get_pin_function(pc, offset);
> +	if (function)
> +		sprintf(f, "Alt Fn %d", function);
Please use snprintf.

> +	else
> +		sprintf(f, "GPIO");
> +
> +	seq_printf(s, "[OE:%d,PU:%ld,OD:%ld]\t%s\n"
>   		"\t\t[retime:%ld,invclk:%ld,clknotdat:%ld,"
>   		"de:%ld,rt-clk:%ld,rt-delay:%ld]",
>   		!st_gpio_get_direction(&pc_to_bank(pc)->gpio_chip, offset),
>   		ST_PINCONF_UNPACK_PU(config),
>   		ST_PINCONF_UNPACK_OD(config),
> +		f,
>   		ST_PINCONF_UNPACK_RT(config),
>   		ST_PINCONF_UNPACK_RT_INVERTCLK(config),
>   		ST_PINCONF_UNPACK_RT_CLKNOTDATA(config),

Thanks,
Maxime

[STLinux Kernel] [PATCH 1/6] ARM: STi: DT: STiH407: Fix retime pin mask for PIO5 and PIO35

[Maxime Coquelin]

On 03/18/2015 11:51 AM, Lee Jones wrote:
> From: Karim BEN BELGACEM <karim.ben-belgacem@st.com>
>
> This will avoid programming the retime registers when not implemented
>
> - PIO5  : no retime registers assigned to pins 6 and 7
> - PIO35 : pin 7 is reserved so no retime register assigned to it
>
> Signed-off-by: Karim BEN BELGACEM <karim.ben-belgacem@st.com>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>   arch/arm/boot/dts/stih407-pinctrl.dtsi | 2 ++
>   1 file changed, 2 insertions(+)
>

Acked-by: Maxime Coquelin <maxime.coquelin@st.com>

Thanks,
Maxime

[STLinux Kernel] [PATCH 2/6] pinctrl: st: Introduce a 'get pin function' call

[Maxime Coquelin]

On 03/18/2015 11:51 AM, Lee Jones wrote:
> This call fetches the numerical function value a specified pin is
> currently operating in.  Function zero is more often than not the
> GPIO function.  Greater than zero values represent an alternative
> function.  You'd need to either look those up in the Device Tree
> sources or the Programmer's Manual.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>   drivers/pinctrl/pinctrl-st.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
>
> diff --git a/drivers/pinctrl/pinctrl-st.c b/drivers/pinctrl/pinctrl-st.c
> index 9e5ec00..5362e45 100644
> --- a/drivers/pinctrl/pinctrl-st.c
> +++ b/drivers/pinctrl/pinctrl-st.c
> @@ -460,6 +460,20 @@ static void st_pctl_set_function(struct st_pio_control *pc,
>   	regmap_field_write(alt, val);
>   }
>   
> +static unsigned int st_pctl_get_pin_function(struct st_pio_control *pc, int pin)
> +{
> +	struct regmap_field *alt = pc->alt;
> +	unsigned int val;
> +	int offset = pin * 4;
> +
> +	if (!alt)
> +		return 0;
Shouldn't we print something if alt is NULL?
Else we can think we are on alternate 0.

> +
> +	regmap_field_read(alt, &val);
> +
> +	return (val >> offset) & 0xf;
> +}
> +
>   static unsigned long st_pinconf_delay_to_bit(unsigned int delay,
>   	const struct st_pctl_data *data, unsigned long config)
>   {

[STLinux Kernel] [PATCH 3/6] pinctrl: st: Move st_get_pio_control() further up the source file

[Maxime Coquelin]

On 03/18/2015 11:51 AM, Lee Jones wrote:
> st_get_pio_control() will be used by subsequent calls which are
> to be located above its original position.  This is required to
> prevent the need for an unnecessary forward-declaration/prototype.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>   drivers/pinctrl/pinctrl-st.c | 20 ++++++++++----------
>   1 file changed, 10 insertions(+), 10 deletions(-)
>
>

Acked-by: Maxime Coquelin <maxime.coquelin@st.com>

Thanks,
Maxime

[STLinux Kernel] [PATCH 4/6] pinctrl: st: Supply a GPIO get_direction() call-back

[Maxime Coquelin]

On 03/18/2015 11:51 AM, Lee Jones wrote:
> ST's hardware differentiates between GPIO mode and Pinctrl alternate
> functions.  When a pin is in GPIO mode, there are dedicated registers
> to set and obtain direction status.  However, If a pin's alternate
> function is in use then the direction is set and status is derived
> from a bunch of syscon registers.  The issue is; until now there was
> a lack of parity between the two.
>
> For example:
>
> Catting the two following information sources could result in
> conflicting information (output has been snipped for simplicity):
>
>   $ cat /sys/kernel/debug/gpio
>    GPIOs 32-39, platform/961f080.pin-controller-sbc, PIO4:
>     gpio-33  (?                   ) out hi
>
>   $ cat /sys/kernel/debug/pinctrl/<pin-controller>/pinconf-pins
>    pin 33 (PIO4[1]):[OE:0,PU:0,OD:0]
>           [retime:0,invclk:0,clknotdat:0,de:0,rt-clk:0,rt-delay:0]
>
> In this example GPIO-33 is a GPIO controlled LED, which is set for
> output, as you'd expect.  However, when the same information is
> drafted from Pinctrl, it clearly states that OE (Output Enable) is
> not set i.e. the pin is set for input.  This is because OE normally
> only represents alternate functions and has no bearing on how the
> pin operates when in Alt-0 (GPIO mode).
>
> This patch changes the current semantics and provides a parity link
> between the two subsystems.  The get_direction() call-back firstly
> determines which function a pin is operating in, then uses the
> appropriate helpers for that mode.
>
> Reported-by: Olivier Clergeaud <olivier.clergeaud@st.com>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>   drivers/pinctrl/pinctrl-st.c | 31 ++++++++++++++++++++++++++++++-
>   1 file changed, 30 insertions(+), 1 deletion(-)
>

Acked-by: Maxime Coquelin <maxime.coquelin@st.com>

Thanks,
Maxime

[STLinux Kernel] [PATCH 5/6] pinctrl: st: Show correct pin direction -- even when in GPIO mode

[Maxime Coquelin]

On 03/18/2015 11:51 AM, Lee Jones wrote:
> Until now ST's pinconf_dbg_show() call-back has displayed the PIO
> alternate function direction, which is only relevant if a pin is
> operating in an alternate function mode i.e not GPIO mode.  If a
> pin is in GPIO mode its direction is both set and status is
> obtained by a completely different/unrelated bunch of registers.
>
> This change ensures that the correct pin direction is shown, even
> if a pin is operating in GPIO mode.
>
> Reported-by: Olivier Clergeaud <olivier.clergeaud@st.com>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>   drivers/pinctrl/pinctrl-st.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
>
>


Acked-by: Maxime Coquelin <maxime.coquelin@st.com>

Thanks,
Maxime

[STLinux Kernel] [PATCH 2/6] pinctrl: st: Introduce a 'get pin function' call

[Lee Jones]

On Wed, 18 Mar 2015, Maxime Coquelin wrote:

> 
> 
> On 03/18/2015 11:51 AM, Lee Jones wrote:
> >This call fetches the numerical function value a specified pin is
> >currently operating in.  Function zero is more often than not the
> >GPIO function.  Greater than zero values represent an alternative
> >function.  You'd need to either look those up in the Device Tree
> >sources or the Programmer's Manual.
> >
> >Signed-off-by: Lee Jones <lee.jones@linaro.org>
> >---
> >  drivers/pinctrl/pinctrl-st.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> >diff --git a/drivers/pinctrl/pinctrl-st.c b/drivers/pinctrl/pinctrl-st.c
> >index 9e5ec00..5362e45 100644
> >--- a/drivers/pinctrl/pinctrl-st.c
> >+++ b/drivers/pinctrl/pinctrl-st.c
> >@@ -460,6 +460,20 @@ static void st_pctl_set_function(struct st_pio_control *pc,
> >  	regmap_field_write(alt, val);
> >  }
> >+static unsigned int st_pctl_get_pin_function(struct st_pio_control *pc, int pin)
> >+{
> >+	struct regmap_field *alt = pc->alt;
> >+	unsigned int val;
> >+	int offset = pin * 4;
> >+
> >+	if (!alt)
> >+		return 0;
> Shouldn't we print something if alt is NULL?
> Else we can think we are on alternate 0.

That is the assumption that I've made.  Is there isn't an alt, then a
pin can only be on Alt-0.  Have I made the incorrect assumption here?

> >+
> >+	regmap_field_read(alt, &val);
> >+
> >+	return (val >> offset) & 0xf;
> >+}
> >+
> >  static unsigned long st_pinconf_delay_to_bit(unsigned int delay,
> >  	const struct st_pctl_data *data, unsigned long config)
> >  {
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[STLinux Kernel] [PATCH 6/6] pinctrl: st: Display pin's function when printing pinctrl debug information

[Lee Jones]

On Wed, 18 Mar 2015, Maxime Coquelin wrote:

> Hi Lee,
> 
> On 03/18/2015 11:51 AM, Lee Jones wrote:
> >Great for easily determining which mode a pin is operating in.
> >This patch was particularly helpful when debugging a recent GPIO/
> >Pinctrl disparity issue.
> >
> >Before:
> >     $ cat /sys/kernel/debug/pinctrl/<pin-controller>/pinconf-pins
> >       pin 33 (PIO4[1]):[OE:0,PU:0,OD:0]
> >              [retime:0,invclk:0,clknotdat:0,de:0,rt-clk:0,rt-delay:0]
> >
> >After [GPIO]:
> >     $ cat /sys/kernel/debug/pinctrl/<pin-controller>/pinconf-pins
> >       pin 33 (PIO4[1]):[OE:0,PU:0,OD:0] GPIO
> >              [retime:0,invclk:0,clknotdat:0,de:0,rt-clk:0,rt-delay:0]
> >
> >After [Alt]:
> >     $ cat /sys/kernel/debug/pinctrl/<pin-controller>/pinconf-pins
> >       pin 33 (PIO4[1]):[OE:0,PU:0,OD:0] Alt Fn 2
> >              [retime:0,invclk:0,clknotdat:0,de:0,rt-clk:0,rt-delay:0]
> >
> >Signed-off-by: Lee Jones <lee.jones@linaro.org>
> >---
> >  drivers/pinctrl/pinctrl-st.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> A couple of comments below.
> Once fixed, you can add my:
> Acked-by: Maxime Coquelin <maxime.coquelin@st.com>
> 
> 
> 
> >
> >diff --git a/drivers/pinctrl/pinctrl-st.c b/drivers/pinctrl/pinctrl-st.c
> >index 1cda40e..d5b2ffa 100644
> >--- a/drivers/pinctrl/pinctrl-st.c
> >+++ b/drivers/pinctrl/pinctrl-st.c
> >@@ -1058,18 +1058,28 @@ static void st_pinconf_dbg_show(struct pinctrl_dev *pctldev,
> >  {
> >  	struct st_pio_control *pc;
> >  	unsigned long config;
> >+	unsigned int function;
> >  	int offset = st_gpio_pin(pin_id);
> >+	char f[64];
> 64 bytes is way too big here

Ah yes, this is legacy from when the message was much larger.

Initially, it was "WARN: Pin in GPIO mode -- this status is invalid"

But this incarnation is much nicer I thought.

Good spot.  Will fix.

> >  	mutex_unlock(&pctldev->mutex);
> >  	pc = st_get_pio_control(pctldev, pin_id);
> >  	st_pinconf_get(pctldev, pin_id, &config);
> >  	mutex_lock(&pctldev->mutex);
> >-	seq_printf(s, "[OE:%ld,PU:%ld,OD:%ld]\n"
> >+
> >+	function = st_pctl_get_pin_function(pc, offset);
> >+	if (function)
> >+		sprintf(f, "Alt Fn %d", function);
> Please use snprintf.

Of course.  Will fix.

> >+	else
> >+		sprintf(f, "GPIO");
> >+
> >+	seq_printf(s, "[OE:%d,PU:%ld,OD:%ld]\t%s\n"
> >  		"\t\t[retime:%ld,invclk:%ld,clknotdat:%ld,"
> >  		"de:%ld,rt-clk:%ld,rt-delay:%ld]",
> >  		!st_gpio_get_direction(&pc_to_bank(pc)->gpio_chip, offset),
> >  		ST_PINCONF_UNPACK_PU(config),
> >  		ST_PINCONF_UNPACK_OD(config),
> >+		f,
> >  		ST_PINCONF_UNPACK_RT(config),
> >  		ST_PINCONF_UNPACK_RT_INVERTCLK(config),
> >  		ST_PINCONF_UNPACK_RT_CLKNOTDATA(config),
> 
> Thanks,
> Maxime

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[STLinux Kernel] [PATCH 2/6] pinctrl: st: Introduce a 'get pin function' call

[Maxime Coquelin]

On 03/18/2015 05:51 PM, Lee Jones wrote:
> On Wed, 18 Mar 2015, Maxime Coquelin wrote:
>
>>
>> On 03/18/2015 11:51 AM, Lee Jones wrote:
>>> This call fetches the numerical function value a specified pin is
>>> currently operating in.  Function zero is more often than not the
>>> GPIO function.  Greater than zero values represent an alternative
>>> function.  You'd need to either look those up in the Device Tree
>>> sources or the Programmer's Manual.
>>>
>>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>>> ---
>>>   drivers/pinctrl/pinctrl-st.c | 14 ++++++++++++++
>>>   1 file changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/pinctrl/pinctrl-st.c b/drivers/pinctrl/pinctrl-st.c
>>> index 9e5ec00..5362e45 100644
>>> --- a/drivers/pinctrl/pinctrl-st.c
>>> +++ b/drivers/pinctrl/pinctrl-st.c
>>> @@ -460,6 +460,20 @@ static void st_pctl_set_function(struct st_pio_control *pc,
>>>   	regmap_field_write(alt, val);
>>>   }
>>> +static unsigned int st_pctl_get_pin_function(struct st_pio_control *pc, int pin)
>>> +{
>>> +	struct regmap_field *alt = pc->alt;
>>> +	unsigned int val;
>>> +	int offset = pin * 4;
>>> +
>>> +	if (!alt)
>>> +		return 0;
>> Shouldn't we print something if alt is NULL?
>> Else we can think we are on alternate 0.
> That is the assumption that I've made.  Is there isn't an alt, then a
> pin can only be on Alt-0.  Have I made the incorrect assumption here?
Just re-checked the code, and yes you are right.
No alt here means alt field of struct st_pctl_data is -1, which in turn 
means the register is not available.

You can forget my remark, and add my:
Acked-by: Maxime Coquelin <maxime.coquelin@st.com>

Thanks,
Maxime