[PATCH] pinctrl/amd: Configure GPIO register using BIOS settings

[PATCH] pinctrl/amd: Configure GPIO register using BIOS settings

[Agrawal, Nitesh-kumar]

In the function amd_gpio_irq_set_type, use the settings provided by
the BIOS,when the LevelTrig is Edge and activeLevel is HIGH, to configure
the GPIO registers. Ignore the settings from client.

Reviewed-by: Pankaj Sen <Pankaj.Sen@amd.com>
Signed-off-by: Nitesh Kumar Agrawal <Nitesh-kumar.Agrawal@amd.com>
---
 drivers/pinctrl/pinctrl-amd.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index 828148d..6408dda 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -384,6 +384,8 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 {
 	int ret = 0;
 	u32 pin_reg;
+	bool level_trig;
+	u32 active_level;
 	unsigned long flags;
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct amd_gpio *gpio_dev = gpiochip_get_data(gc);
@@ -391,6 +393,18 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 	spin_lock_irqsave(&gpio_dev->lock, flags);
 	pin_reg = readl(gpio_dev->base + (d->hwirq)*4);
 
+	/*
+	* Use the settings provided by the BIOS, when the LevelTrig is
+	* EDGE and the activeLevel is HIGH, ignore the settings coming
+	* from the client to configure the GPIO register.
+	*/
+	level_trig = !(pin_reg & (LEVEL_TRIGGER << LEVEL_TRIG_OFF));
+	active_level = pin_reg & (ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF);
+
+	if (level_trig && ((active_level >> ACTIVE_LEVEL_OFF) == ACTIVE_HIGH)) {
+		type = IRQ_TYPE_EDGE_FALLING;
+	}
+
 	switch (type & IRQ_TYPE_SENSE_MASK) {
 	case IRQ_TYPE_EDGE_RISING:
 		pin_reg &= ~BIT(LEVEL_TRIG_OFF);
-- 
2.7.4

Re: [PATCH] pinctrl/amd: Configure GPIO register using BIOS settings

[Linus Walleij]

On Fri, Sep 9, 2016 at 5:18 PM, Agrawal, Nitesh-kumar
<Nitesh-kumar.Agrawal@amd.com> wrote:

> In the function amd_gpio_irq_set_type, use the settings provided by
> the BIOS,when the LevelTrig is Edge and activeLevel is HIGH, to configure
> the GPIO registers. Ignore the settings from client.
>
> Reviewed-by: Pankaj Sen <Pankaj.Sen@amd.com>
> Signed-off-by: Nitesh Kumar Agrawal <Nitesh-kumar.Agrawal@amd.com>

Thanks! It turns out I left a partly resolved commit in the tree
so applied parts of this patch as a fixup.

Sorry for my screw-ups.

Yours,
Linus Walleij

Re: pinctrl/amd: Configure GPIO register using BIOS settings

[Dmitry Torokhov]

On Fri, Sep 09, 2016 at 03:18:09PM +0000, Agrawal, Nitesh-kumar wrote:
> In the function amd_gpio_irq_set_type, use the settings provided by
> the BIOS,when the LevelTrig is Edge and activeLevel is HIGH, to configure
> the GPIO registers. Ignore the settings from client.
> 
> Reviewed-by: Pankaj Sen <Pankaj.Sen@amd.com>
> Signed-off-by: Nitesh Kumar Agrawal <Nitesh-kumar.Agrawal@amd.com>

Just found this patch in the tree. Can you please explain why it is
needed (the patch description unfortunately tells what the patch does,
but not why).

I would expect that we either allow reprogramming the trigger as client
wishes or would error out and let the upper layers know. Silently
"fixing" the settings is wrong course of action in my opinion.

If this was trying to work around Elan touchpad drivers not working on
AMD platforms it needs to be solved in Elan driver, not here.

Thanks!

> ---
>  drivers/pinctrl/pinctrl-amd.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
> index 828148d..6408dda 100644
> --- a/drivers/pinctrl/pinctrl-amd.c
> +++ b/drivers/pinctrl/pinctrl-amd.c
> @@ -384,6 +384,8 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>  {
>  	int ret = 0;
>  	u32 pin_reg;
> +	bool level_trig;
> +	u32 active_level;
>  	unsigned long flags;
>  	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>  	struct amd_gpio *gpio_dev = gpiochip_get_data(gc);
> @@ -391,6 +393,18 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>  	spin_lock_irqsave(&gpio_dev->lock, flags);
>  	pin_reg = readl(gpio_dev->base + (d->hwirq)*4);
>  
> +	/*
> +	* Use the settings provided by the BIOS, when the LevelTrig is
> +	* EDGE and the activeLevel is HIGH, ignore the settings coming
> +	* from the client to configure the GPIO register.
> +	*/
> +	level_trig = !(pin_reg & (LEVEL_TRIGGER << LEVEL_TRIG_OFF));
> +	active_level = pin_reg & (ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF);
> +
> +	if (level_trig && ((active_level >> ACTIVE_LEVEL_OFF) == ACTIVE_HIGH)) {
> +		type = IRQ_TYPE_EDGE_FALLING;
> +	}
> +
>  	switch (type & IRQ_TYPE_SENSE_MASK) {
>  	case IRQ_TYPE_EDGE_RISING:
>  		pin_reg &= ~BIT(LEVEL_TRIG_OFF);

-- 
Dmitry

RE: pinctrl/amd: Configure GPIO register using BIOS settings

[Agrawal, Nitesh-kumar]

-----Original Message-----
From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com] 
Sent: Thursday, September 28, 2017 12:28 PM
To: Agrawal, Nitesh-kumar <Nitesh-kumar.Agrawal@amd.com>
Cc: linus.walleij@linaro.org; Sen, Pankaj <Pankaj.Sen@amd.com>; S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>; linux-gpio@vger.kernel.org
Subject: Re: pinctrl/amd: Configure GPIO register using BIOS settings

On Fri, Sep 09, 2016 at 03:18:09PM +0000, Agrawal, Nitesh-kumar wrote:
> In the function amd_gpio_irq_set_type, use the settings provided by 
> the BIOS,when the LevelTrig is Edge and activeLevel is HIGH, to 
> configure the GPIO registers. Ignore the settings from client.
> 
> Reviewed-by: Pankaj Sen <Pankaj.Sen@amd.com>
> Signed-off-by: Nitesh Kumar Agrawal <Nitesh-kumar.Agrawal@amd.com>

Just found this patch in the tree. Can you please explain why it is needed (the patch description unfortunately tells what the patch does, but not why).

I would expect that we either allow reprogramming the trigger as client wishes or would error out and let the upper layers know. Silently "fixing" the settings is wrong course of action in my opinion.

If this was trying to work around Elan touchpad drivers not working on AMD platforms it needs to be solved in Elan driver, not here.

[Nitesh]We have reverted the patch in the later version of the patch.

Re: pinctrl/amd: Configure GPIO register using BIOS settings

[Dmitry Torokhov]

Hi Nitesh,

On Thu, Sep 28, 2017 at 12:46 AM, Agrawal, Nitesh-kumar
<Nitesh-kumar.Agrawal@amd.com> wrote:
> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: Thursday, September 28, 2017 12:28 PM
> To: Agrawal, Nitesh-kumar <Nitesh-kumar.Agrawal@amd.com>
> Cc: linus.walleij@linaro.org; Sen, Pankaj <Pankaj.Sen@amd.com>; S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>; linux-gpio@vger.kernel.org
> Subject: Re: pinctrl/amd: Configure GPIO register using BIOS settings
>
> On Fri, Sep 09, 2016 at 03:18:09PM +0000, Agrawal, Nitesh-kumar wrote:
>> In the function amd_gpio_irq_set_type, use the settings provided by
>> the BIOS,when the LevelTrig is Edge and activeLevel is HIGH, to
>> configure the GPIO registers. Ignore the settings from client.
>>
>> Reviewed-by: Pankaj Sen <Pankaj.Sen@amd.com>
>> Signed-off-by: Nitesh Kumar Agrawal <Nitesh-kumar.Agrawal@amd.com>
>
> Just found this patch in the tree. Can you please explain why it is needed (the patch description unfortunately tells what the patch does, but not why).
>
> I would expect that we either allow reprogramming the trigger as client wishes or would error out and let the upper layers know. Silently "fixing" the settings is wrong course of action in my opinion.
>
> If this was trying to work around Elan touchpad drivers not working on AMD platforms it needs to be solved in Elan driver, not here.
>
> [Nitesh]We have reverted the patch in the later version of the patch.
>

Where was it reverted? I am looking at today's linux-next, and I am
still seeing this comment in pinctrl-amd.c:

        /* Ignore the settings coming from the client and
        * read the values from the ACPI tables
        * while setting the trigger type
        */

Thanks.

-- 
Dmitry

RE: pinctrl/amd: Configure GPIO register using BIOS settings

[Agrawal, Nitesh-kumar]

Hi Dmitry,

On Thu, Sep 28, 2017 at 12:46 AM, Agrawal, Nitesh-kumar <Nitesh-kumar.Agrawal@amd.com> wrote:
> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: Thursday, September 28, 2017 12:28 PM
> To: Agrawal, Nitesh-kumar <Nitesh-kumar.Agrawal@amd.com>
> Cc: linus.walleij@linaro.org; Sen, Pankaj <Pankaj.Sen@amd.com>; S-k, 
> Shyam-sundar <Shyam-sundar.S-k@amd.com>; linux-gpio@vger.kernel.org
> Subject: Re: pinctrl/amd: Configure GPIO register using BIOS settings
>
> On Fri, Sep 09, 2016 at 03:18:09PM +0000, Agrawal, Nitesh-kumar wrote:
>> In the function amd_gpio_irq_set_type, use the settings provided by 
>> the BIOS,when the LevelTrig is Edge and activeLevel is HIGH, to 
>> configure the GPIO registers. Ignore the settings from client.
>>
>> Reviewed-by: Pankaj Sen <Pankaj.Sen@amd.com>
>> Signed-off-by: Nitesh Kumar Agrawal <Nitesh-kumar.Agrawal@amd.com>
>
> Just found this patch in the tree. Can you please explain why it is needed (the patch description unfortunately tells what the patch does, but not why).
>
> I would expect that we either allow reprogramming the trigger as client wishes or would error out and let the upper layers know. Silently "fixing" the settings is wrong course of action in my opinion.
>
> If this was trying to work around Elan touchpad drivers not working on AMD platforms it needs to be solved in Elan driver, not here.
>
> [Nitesh]We have reverted the patch in the later version of the patch.
>

Where was it reverted? I am looking at today's linux-next, and I am still seeing this comment in pinctrl-amd.c:

        /* Ignore the settings coming from the client and
        * read the values from the ACPI tables
        * while setting the trigger type
        */


Ok. We have reverted the code but the comments are still there. We will submit a patch to revert the comments also. Thanks for reviewing the patch.

Re: [PATCH] pinctrl/amd: Configure GPIO register using BIOS settings

[Linus Walleij]

On Wed, Aug 31, 2016 at 10:50 AM, Agrawal, Nitesh-kumar
<Nitesh-kumar.Agrawal@amd.com> wrote:

I'm adding Wei and Wang to the patch as they worked on the driver in the
past and I'd like to hear what they say. Could you folks provide
some ACK or comments?

The indentation is horrible in the patch, I don't know if it is a result
of the mailer though, the it's not your fault and I can fix it up for sure
if you have a problem getting it right.

Just make sure you run scripts/checkpatch.pl before sending the patches.

> In the function amd_gpio_irq_set_type, use the settings provided by
> the BIOS,when the LevelTrig is Edge and activeLevel is HIGH, to configure
> the GPIO registers. Ignore the settings from client.
>
> Reviewed-by:Pankaj Sen <Pankaj.Sen@amd.com>
> Signed-off-by:Nitesh Kumar Agrawal <Nitesh-kumar.Agrawal@amd.com>
> ---
>  drivers/pinctrl/pinctrl-amd.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
> index 828148d..a645082 100644
> --- a/drivers/pinctrl/pinctrl-amd.c
> +++ b/drivers/pinctrl/pinctrl-amd.c
> @@ -385,12 +385,26 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>         int ret = 0;
>         u32 pin_reg;
>         unsigned long flags;
> +        u32 levelTrig;

Please refrain from using CamelCase, just call it level_trig.

Also: this seems to be a bool.

> +        u32 activeLevel;

No CamelCase. This doesn't seem to be a bool though, can't really tell.

>         struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>         struct amd_gpio *gpio_dev = gpiochip_get_data(gc);
>
>         spin_lock_irqsave(&gpio_dev->lock, flags);
>         pin_reg = readl(gpio_dev->base + (d->hwirq)*4);
>
> +        /*
> +         When LevelTrig is set EDGE and activeLevel is set HIGH in BIOS
> +         default settings, ignore incoming settings from client and use
> +         BIOS settings to configure GPIO register.
> +        */

/*
 * Please comment like this
 * with a star in the beginning on every line
 * I know I am picky.
 */

> +        levelTrig = pin_reg & (LEVEL_TRIGGER << LEVEL_TRIG_OFF);

So if this was a bool it would be:

level_trig = !!(pin_reg & (LEVEL_TRIGGER << LEVEL_TRIG_OFF));

Notice how the double-bang (!!) clamps the result to a bool.

> +        activeLevel = pin_reg & (ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF);
> +
> +        if((!levelTrig)&&((activeLevel>> ACTIVE_LEVEL_OFF) == ACTIVE_HIGH)) {
> +           type = IRQ_TYPE_EDGE_FALLING;
> +        }

The thing looks a bit unorthodox but I guess you know what you're doing.

Yours,
Linus Walleij

RE: [PATCH] pinctrl/amd: Configure GPIO register using BIOS settings

[Agrawal, Nitesh-kumar]

In the function amd_gpio_irq_set_type, use the settings provided by
the BIOS,when the LevelTrig is Edge and activeLevel is HIGH, to configure
the GPIO registers. Ignore the settings from client.

Reviewed-by:Pankaj Sen <Pankaj.Sen@amd.com>
Signed-off-by:Nitesh Kumar Agrawal <Nitesh-kumar.Agrawal@amd.com>
---
 drivers/pinctrl/pinctrl-amd.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index 828148d..a645082 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -385,12 +385,26 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 	int ret = 0;
 	u32 pin_reg;
 	unsigned long flags;
+        u32 levelTrig;
+        u32 activeLevel;
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct amd_gpio *gpio_dev = gpiochip_get_data(gc);
 
 	spin_lock_irqsave(&gpio_dev->lock, flags);
 	pin_reg = readl(gpio_dev->base + (d->hwirq)*4);
 
+        /*
+         When LevelTrig is set EDGE and activeLevel is set HIGH in BIOS
+         default settings, ignore incoming settings from client and use
+         BIOS settings to configure GPIO register.
+        */
+        levelTrig = pin_reg & (LEVEL_TRIGGER << LEVEL_TRIG_OFF);
+        activeLevel = pin_reg & (ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF);
+
+        if((!levelTrig)&&((activeLevel>> ACTIVE_LEVEL_OFF) == ACTIVE_HIGH)) {
+           type = IRQ_TYPE_EDGE_FALLING;
+        }
+
 	switch (type & IRQ_TYPE_SENSE_MASK) {
 	case IRQ_TYPE_EDGE_RISING:
 		pin_reg &= ~BIT(LEVEL_TRIG_OFF);
-- 
2.7.4

Any comments or feedback on the above patch

[PATCH] pinctrl/amd: Configure GPIO register using BIOS settings

[Agrawal, Nitesh-kumar]

In the function amd_gpio_irq_set_type, use the settings provided by
the BIOS,when the LevelTrig is Edge and activeLevel is HIGH, to configure
the GPIO registers. Ignore the settings from client.

Reviewed-by:Pankaj Sen <Pankaj.Sen@amd.com>
Signed-off-by:Nitesh Kumar Agrawal <Nitesh-kumar.Agrawal@amd.com>
---
 drivers/pinctrl/pinctrl-amd.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index 828148d..a645082 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -385,12 +385,26 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 	int ret = 0;
 	u32 pin_reg;
 	unsigned long flags;
+        u32 levelTrig;
+        u32 activeLevel;
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct amd_gpio *gpio_dev = gpiochip_get_data(gc);
 
 	spin_lock_irqsave(&gpio_dev->lock, flags);
 	pin_reg = readl(gpio_dev->base + (d->hwirq)*4);
 
+        /*
+         When LevelTrig is set EDGE and activeLevel is set HIGH in BIOS
+         default settings, ignore incoming settings from client and use
+         BIOS settings to configure GPIO register.
+        */
+        levelTrig = pin_reg & (LEVEL_TRIGGER << LEVEL_TRIG_OFF);
+        activeLevel = pin_reg & (ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF);
+
+        if((!levelTrig)&&((activeLevel>> ACTIVE_LEVEL_OFF) == ACTIVE_HIGH)) {
+           type = IRQ_TYPE_EDGE_FALLING;
+        }
+
 	switch (type & IRQ_TYPE_SENSE_MASK) {
 	case IRQ_TYPE_EDGE_RISING:
 		pin_reg &= ~BIT(LEVEL_TRIG_OFF);
-- 
2.7.4