[PATCH 1/3] pinctrl: at91: correct a few typos

[PATCH 1/3] pinctrl: at91: correct a few typos

[Alexandre Belloni]

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 drivers/pinctrl/pinctrl-at91.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
index a7549c4c83b4..6446dc804aa7 100644
--- a/drivers/pinctrl/pinctrl-at91.c
+++ b/drivers/pinctrl/pinctrl-at91.c
@@ -118,7 +118,7 @@ struct at91_pin_group {
 };
 
 /**
- * struct at91_pinctrl_mux_ops - describes an At91 mux ops group
+ * struct at91_pinctrl_mux_ops - describes an AT91 mux ops group
  * on new IP with support for periph C and D the way to mux in
  * periph A and B has changed
  * So provide the right call back
@@ -1396,7 +1396,7 @@ static void gpio_irq_handler(unsigned irq, struct irq_desc *desc)
 	chained_irq_enter(chip, desc);
 	for (;;) {
 		/* Reading ISR acks pending (edge triggered) GPIO interrupts.
-		 * When there none are pending, we're finished unless we need
+		 * When there are none pending, we're finished unless we need
 		 * to process multiple banks (like ID_PIOCDE on sam9263).
 		 */
 		isr = readl_relaxed(pio + PIO_ISR) & readl_relaxed(pio + PIO_IMR);
@@ -1505,7 +1505,7 @@ static int at91_gpio_of_irq_setup(struct device_node *node,
 		prev = gpio_chips[at91_gpio->pioc_idx - 1];
 
 	/* The top level handler handles one bank of GPIOs, except
-	 * on some SoC it can handles up to three...
+	 * on some SoC it can handle up to three...
 	 * We only set up the handler for the first of the list.
 	 */
 	if (prev && prev->next == at91_gpio)
-- 
1.8.3.2

[PATCH 2/3] pinctrl: at91: initialize config parameter to 0

[Alexandre Belloni]

When passing a not initialized config parameter, at91_pinconf_get() would return
a bogus value. Fix that by initializing it to zero before using it.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 drivers/pinctrl/pinctrl-at91.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
index 6446dc804aa7..b0b78f3468ae 100644
--- a/drivers/pinctrl/pinctrl-at91.c
+++ b/drivers/pinctrl/pinctrl-at91.c
@@ -722,7 +722,8 @@ static int at91_pinconf_get(struct pinctrl_dev *pctldev,
 	unsigned pin;
 	int div;
 
-	dev_dbg(info->dev, "%s:%d, pin_id=%d, config=0x%lx", __func__, __LINE__, pin_id, *config);
+	*config = 0;
+	dev_dbg(info->dev, "%s:%d, pin_id=%d", __func__, __LINE__, pin_id);
 	pio = pin_to_controller(info, pin_to_bank(pin_id));
 	pin = pin_id % MAX_NB_GPIO_PER_BANK;
 
-- 
1.8.3.2

[PATCH 2/3] pinctrl: at91: initialize config parameter to 0

[Nicolas Ferre]

On 07/12/2013 14:08, Alexandre Belloni :
> When passing a not initialized config parameter, at91_pinconf_get() would return
> a bogus value. Fix that by initializing it to zero before using it.
>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
>   drivers/pinctrl/pinctrl-at91.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
> index 6446dc804aa7..b0b78f3468ae 100644
> --- a/drivers/pinctrl/pinctrl-at91.c
> +++ b/drivers/pinctrl/pinctrl-at91.c
> @@ -722,7 +722,8 @@ static int at91_pinconf_get(struct pinctrl_dev *pctldev,
>   	unsigned pin;
>   	int div;
>
> -	dev_dbg(info->dev, "%s:%d, pin_id=%d, config=0x%lx", __func__, __LINE__, pin_id, *config);
> +	*config = 0;
> +	dev_dbg(info->dev, "%s:%d, pin_id=%d", __func__, __LINE__, pin_id);
>   	pio = pin_to_controller(info, pin_to_bank(pin_id));
>   	pin = pin_id % MAX_NB_GPIO_PER_BANK;

Beyond this patch, I must say that I am puzzled by this function.

What I read from the prototype documentation and what I see in different 
implementations is different...

Linus, can we have a review of this function because it seems not in 
line with what is used for u300 (but on the other hand looks like the 
what is returned by pinctrl-exynos5440.c driver for example).

What would be the consequences if we change this function's behavior: I 
mean use of -EINVAL for pin configuration "available but disabled" as 
said in include/linux/pinctrl/pinconf.h?

Bye,
-- 
Nicolas Ferre

[PATCH 2/3] pinctrl: at91: initialize config parameter to 0

[Alexandre Belloni]

Hi,

On 09/12/2013 09:24, Nicolas Ferre wrote:
> On 07/12/2013 14:08, Alexandre Belloni :
>> When passing a not initialized config parameter, at91_pinconf_get()
>> would return
>> a bogus value. Fix that by initializing it to zero before using it.
>>
>> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
>> ---
>>   drivers/pinctrl/pinctrl-at91.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pinctrl/pinctrl-at91.c
>> b/drivers/pinctrl/pinctrl-at91.c
>> index 6446dc804aa7..b0b78f3468ae 100644
>> --- a/drivers/pinctrl/pinctrl-at91.c
>> +++ b/drivers/pinctrl/pinctrl-at91.c
>> @@ -722,7 +722,8 @@ static int at91_pinconf_get(struct pinctrl_dev
>> *pctldev,
>>       unsigned pin;
>>       int div;
>>
>> -    dev_dbg(info->dev, "%s:%d, pin_id=%d, config=0x%lx", __func__,
>> __LINE__, pin_id, *config);
>> +    *config = 0;
>> +    dev_dbg(info->dev, "%s:%d, pin_id=%d", __func__, __LINE__, pin_id);
>>       pio = pin_to_controller(info, pin_to_bank(pin_id));
>>       pin = pin_id % MAX_NB_GPIO_PER_BANK;
>
> Beyond this patch, I must say that I am puzzled by this function.
>
> What I read from the prototype documentation and what I see in
> different implementations is different...
>
> Linus, can we have a review of this function because it seems not in
> line with what is used for u300 (but on the other hand looks like the
> what is returned by pinctrl-exynos5440.c driver for example).
>
> What would be the consequences if we change this function's behavior:
> I mean use of -EINVAL for pin configuration "available but disabled"
> as said in include/linux/pinctrl/pinconf.h?
>

>From what I understand, it doesn't really matter because that function
is never used. I guess the best implementation is the tegra one ;)

It is only called in one specific case:
 - you have ops->is_generic == true (that is only true for a few
implmentations)
 - and you are dumping the pinconf state using debugfs

I'm actually wondering if the checks for the ops->pin_config_get are not
a bit overkill. We check for that function in:
 - pinconf_check_ops()
 - before calling it in pin_config_get_for_pin() which is only used
once, in the same path : dump using debugfs and having ops->is_generic
== true
 - in pinconf_pins_show() which is the function calling also in the same
path

What I would do is:
 - remove all the checks in pinconf_check_ops() and pinconf_pins_show()
so that people are not pressured to implement a function that is simply
never used.
 - modify pin_config_get_for_pin() by removing the dev_err() call and
returning -ENOTSUPP instead of -EINVAL (it doesn't change the behaviour
but I feel -ENOTSUPP is more appropriate)

I have a patch ready but I can't test it as I don't own any of the
is_generic platforms.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 2/3] pinctrl: at91: initialize config parameter to 0

[Linus Walleij]

On Mon, Dec 9, 2013 at 10:55 AM, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:

> I'm actually wondering if the checks for the ops->pin_config_get are not
> a bit overkill. We check for that function in:
>  - pinconf_check_ops()
>  - before calling it in pin_config_get_for_pin() which is only used
> once, in the same path : dump using debugfs and having ops->is_generic
> == true
>  - in pinconf_pins_show() which is the function calling also in the same
> path
>
> What I would do is:
>  - remove all the checks in pinconf_check_ops() and pinconf_pins_show()
> so that people are not pressured to implement a function that is simply
> never used.
>  - modify pin_config_get_for_pin() by removing the dev_err() call and
> returning -ENOTSUPP instead of -EINVAL (it doesn't change the behaviour
> but I feel -ENOTSUPP is more appropriate)
>
> I have a patch ready but I can't test it as I don't own any of the
> is_generic platforms.

Mail it out with a [CFT: ] "call for testing" prefix.

Yours,
Linus Walleij

[PATCH 2/3] pinctrl: at91: initialize config parameter to 0

[Alexandre Belloni]

On 12/12/2013 15:40, Linus Walleij wrote:
> Mail it out with a [CFT: ] "call for testing" prefix.
>

I actually already sent it but without the CFT. Do you want me to resend ?
Regards,

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 2/3] pinctrl: at91: initialize config parameter to 0

[Linus Walleij]

On Mon, Dec 9, 2013 at 9:24 AM, Nicolas Ferre <nicolas.ferre@atmel.com> wrote:

>> -       dev_dbg(info->dev, "%s:%d, pin_id=%d, config=0x%lx", __func__,
>> __LINE__, pin_id, *config);
>> +       *config = 0;
>> +       dev_dbg(info->dev, "%s:%d, pin_id=%d", __func__, __LINE__,
>> pin_id);
>>         pio = pin_to_controller(info, pin_to_bank(pin_id));
>>         pin = pin_id % MAX_NB_GPIO_PER_BANK;
>
>
> Beyond this patch, I must say that I am puzzled by this function.
>
> What I read from the prototype documentation and what I see in different
> implementations is different...


Yeah, we need to fix this mess.

> Linus, can we have a review of this function because it seems not in line
> with what is used for u300 (but on the other hand looks like the what is
> returned by pinctrl-exynos5440.c driver for example).

It is supposed to read out one config at the time, if and only if used
with the generic pin config.

Typically:

enum pin_config_param param = (enum pin_config_param) *config;

switch (param) {
    case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
            *config = 0;
            if (biasmode)
                    return 0;
            else
                    return -EINVAL;
            break;
(...)

return -ENOTSUPP;

However AT91 is *not* using generic pin config, so the semantics of
this call is driver-dependent. In your case the implementation get all
the configs at once, which is an efficient shortcut if you don't need
to be general and enumerat all possible configs.

> What would be the consequences if we change this function's behavior: I mean
> use of -EINVAL for pin configuration "available but disabled" as said in
> include/linux/pinctrl/pinconf.h?

That code will propagate back ... I guess you'd have to test it :-/

Yours,
Linus Walleij

[PATCH 2/3] pinctrl: at91: initialize config parameter to 0

[Linus Walleij]

On Sat, Dec 7, 2013 at 2:08 PM, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:

> When passing a not initialized config parameter, at91_pinconf_get() would return
> a bogus value. Fix that by initializing it to zero before using it.
>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>

Patch applied.

Yours,
Linus Walleij

[PATCH 3/3] pinctrl: at91: implement at91_pinconf_dbg_show

[Alexandre Belloni]

This allows to get the pin configuration by using debugfs. On my system:
 # cat /sys/kernel/debug/pinctrl/pinctrl.3/pinconf-pins

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 drivers/pinctrl/pinctrl-at91.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
index b0b78f3468ae..bcfc8a2eebca 100644
--- a/drivers/pinctrl/pinctrl-at91.c
+++ b/drivers/pinctrl/pinctrl-at91.c
@@ -784,10 +784,35 @@ static int at91_pinconf_set(struct pinctrl_dev *pctldev,
 	return 0;
 }
 
+#define DBG_SHOW_FLAG(flag) do {		\
+	if (config & flag) {			\
+		if (num_conf)			\
+			seq_puts(s, "|");	\
+		seq_puts(s, #flag);		\
+		num_conf++;			\
+	}					\
+} while (0)
+
 static void at91_pinconf_dbg_show(struct pinctrl_dev *pctldev,
 				   struct seq_file *s, unsigned pin_id)
 {
+	unsigned long config;
+	int ret, val, num_conf = 0;
+
+	ret = at91_pinconf_get(pctldev, pin_id, &config);
+
+	DBG_SHOW_FLAG(MULTI_DRIVE);
+	DBG_SHOW_FLAG(PULL_UP);
+	DBG_SHOW_FLAG(PULL_DOWN);
+	DBG_SHOW_FLAG(DIS_SCHMIT);
+	DBG_SHOW_FLAG(DEGLITCH);
+	DBG_SHOW_FLAG(DEBOUNCE);
+	if (config & DEBOUNCE) {
+		val = config >> DEBOUNCE_VAL_SHIFT;
+		seq_printf(s, "(%d)", val);
+	}
 
+	return;
 }
 
 static void at91_pinconf_group_dbg_show(struct pinctrl_dev *pctldev,
-- 
1.8.3.2

[PATCH 3/3] pinctrl: at91: implement at91_pinconf_dbg_show

[Linus Walleij]

On Sat, Dec 7, 2013 at 2:08 PM, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:

> This allows to get the pin configuration by using debugfs. On my system:
>  # cat /sys/kernel/debug/pinctrl/pinctrl.3/pinconf-pins
>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>

This is a more invasive patch so I prefer if Laurent ACK this before
I apply it.

Yours,
Linus Walleij

[PATCH 3/3] pinctrl: at91: implement at91_pinconf_dbg_show

[Alexandre Belloni]

On 12/12/2013 15:45, Linus Walleij wrote:
> On Sat, Dec 7, 2013 at 2:08 PM, Alexandre Belloni
> <alexandre.belloni@free-electrons.com> wrote:
>
>> This allows to get the pin configuration by using debugfs. On my system:
>>  # cat /sys/kernel/debug/pinctrl/pinctrl.3/pinconf-pins
>>
>> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> This is a more invasive patch so I prefer if Laurent ACK this before
> I apply it.

Hum, just to be clear, this is an Atmel chip and the Atmel maintainers
were already in copy. I believe Laurent already has a lot on his plate
with CDF ;)


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 3/3] pinctrl: at91: implement at91_pinconf_dbg_show

[Laurent Pinchart]

On Thursday 12 December 2013 16:50:20 Alexandre Belloni wrote:
> On 12/12/2013 15:45, Linus Walleij wrote:
> > On Sat, Dec 7, 2013 at 2:08 PM, Alexandre Belloni
> > 
> > <alexandre.belloni@free-electrons.com> wrote:
> >> This allows to get the pin configuration by using debugfs. On my system:
> >>  # cat /sys/kernel/debug/pinctrl/pinctrl.3/pinconf-pins
> >> 
> >> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> > 
> > This is a more invasive patch so I prefer if Laurent ACK this before
> > I apply it.
> 
> Hum, just to be clear, this is an Atmel chip and the Atmel maintainers
> were already in copy. I believe Laurent already has a lot on his plate
> with CDF ;)

Not to mention the rest. I'd love to help with at91 pinctrl, but I think I 
have to draw the line somewhere. I hope it won't come as a shock that I can't 
review the whole kernel :-D

-- 
Regards,

Laurent Pinchart

[PATCH 3/3] pinctrl: at91: implement at91_pinconf_dbg_show

[Linus Walleij]

On Thu, Dec 12, 2013 at 5:04 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Thursday 12 December 2013 16:50:20 Alexandre Belloni wrote:
>> On 12/12/2013 15:45, Linus Walleij wrote:
>> > On Sat, Dec 7, 2013 at 2:08 PM, Alexandre Belloni
>> >
>> > <alexandre.belloni@free-electrons.com> wrote:
>> >> This allows to get the pin configuration by using debugfs. On my system:
>> >>  # cat /sys/kernel/debug/pinctrl/pinctrl.3/pinconf-pins
>> >>
>> >> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
>> >
>> > This is a more invasive patch so I prefer if Laurent ACK this before
>> > I apply it.
>>
>> Hum, just to be clear, this is an Atmel chip and the Atmel maintainers
>> were already in copy. I believe Laurent already has a lot on his plate
>> with CDF ;)
>
> Not to mention the rest. I'd love to help with at91 pinctrl, but I think I
> have to draw the line somewhere. I hope it won't come as a shock that I can't
> review the whole kernel :-D

Sorry about this mess, forget about it, I don't know what is wrong
we me today, probably trying to process too many patches :-)

Yours,
Linus Walleij

[PATCH 3/3] pinctrl: at91: implement at91_pinconf_dbg_show

[Nicolas Ferre]

On 07/12/2013 14:08, Alexandre Belloni :
> This allows to get the pin configuration by using debugfs. On my system:
>   # cat /sys/kernel/debug/pinctrl/pinctrl.3/pinconf-pins
>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>

I am fine with these helpers:

Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>

But still a little disappointed that Laurent announced that he wouldn't 
review the whole kernel ;-)

Bye guys and thanks a lot!

> ---
>   drivers/pinctrl/pinctrl-at91.c | 25 +++++++++++++++++++++++++
>   1 file changed, 25 insertions(+)
>
> diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
> index b0b78f3468ae..bcfc8a2eebca 100644
> --- a/drivers/pinctrl/pinctrl-at91.c
> +++ b/drivers/pinctrl/pinctrl-at91.c
> @@ -784,10 +784,35 @@ static int at91_pinconf_set(struct pinctrl_dev *pctldev,
>   	return 0;
>   }
>
> +#define DBG_SHOW_FLAG(flag) do {		\
> +	if (config & flag) {			\
> +		if (num_conf)			\
> +			seq_puts(s, "|");	\
> +		seq_puts(s, #flag);		\
> +		num_conf++;			\
> +	}					\
> +} while (0)
> +
>   static void at91_pinconf_dbg_show(struct pinctrl_dev *pctldev,
>   				   struct seq_file *s, unsigned pin_id)
>   {
> +	unsigned long config;
> +	int ret, val, num_conf = 0;
> +
> +	ret = at91_pinconf_get(pctldev, pin_id, &config);
> +
> +	DBG_SHOW_FLAG(MULTI_DRIVE);
> +	DBG_SHOW_FLAG(PULL_UP);
> +	DBG_SHOW_FLAG(PULL_DOWN);
> +	DBG_SHOW_FLAG(DIS_SCHMIT);
> +	DBG_SHOW_FLAG(DEGLITCH);
> +	DBG_SHOW_FLAG(DEBOUNCE);
> +	if (config & DEBOUNCE) {
> +		val = config >> DEBOUNCE_VAL_SHIFT;
> +		seq_printf(s, "(%d)", val);
> +	}
>
> +	return;
>   }
>
>   static void at91_pinconf_group_dbg_show(struct pinctrl_dev *pctldev,
>


-- 
Nicolas Ferre

[PATCH 3/3] pinctrl: at91: implement at91_pinconf_dbg_show

[Linus Walleij]

On Sat, Dec 7, 2013 at 2:08 PM, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:

> This allows to get the pin configuration by using debugfs. On my system:
>  # cat /sys/kernel/debug/pinctrl/pinctrl.3/pinconf-pins
>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>

Patch applied with Nicolas' ACK.

Yours,
Linus Walleij

[PATCH 1/3] pinctrl: at91: correct a few typos

[Linus Walleij]

On Sat, Dec 7, 2013 at 2:08 PM, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:

> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>

Patch applied.

Please include Laurent Pinchart on the To: line on AT91 pin control
patches.

Yours,
Linus Walleij

[PATCH 1/3] pinctrl: at91: correct a few typos

[Laurent Pinchart]

Hi Linus,

On Thursday 12 December 2013 15:42:28 Linus Walleij wrote:
> On Sat, Dec 7, 2013 at 2:08 PM, Alexandre Belloni wrote:
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> 
> Patch applied.
> 
> Please include Laurent Pinchart on the To: line on AT91 pin control
> patches.

I understand that you would like patch to have a wide review coverage, but why 
me ? :-)

-- 
Regards,

Laurent Pinchart

[PATCH 1/3] pinctrl: at91: correct a few typos

[Linus Walleij]

On Thu, Dec 12, 2013 at 3:47 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Linus,
>
> On Thursday 12 December 2013 15:42:28 Linus Walleij wrote:
>> On Sat, Dec 7, 2013 at 2:08 PM, Alexandre Belloni wrote:
>> > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
>>
>> Patch applied.
>>
>> Please include Laurent Pinchart on the To: line on AT91 pin control
>> patches.
>
> I understand that you would like patch to have a wide review coverage, but why
> me ? :-)

Bah nevermind, too much in my head, I confused AT91 for
SH mobile nothing else... Nicolas and Jean-Christophe is on the
list and all is well.

Yours,
Linus Walleij