[PATCH v3] pinctrl: baytrail: Replace WARN with dev_info_once when setting direct-irq pin to output

[PATCH v3] pinctrl: baytrail: Replace WARN with dev_info_once when setting direct-irq pin to output

[Hans de Goede]

Suspending Goodix touchscreens requires changing the interrupt pin to
output before sending them a power-down command. Followed by wiggling
the interrupt pin to wake the device up, after which it is put back
in input mode.

On Cherry Trail device the interrupt pin is listed as a GpioInt ACPI
resource so we can do this without problems as long as we release the
irq before changing the pin to output mode.

On Bay Trail devices with a Goodix touchscreen direct-irq mode is used
in combination with listing the pin as a normal GpioIo resource. This
works fine, but this triggers the WARN in byt_gpio_set_direction-s output
path because direct-irq support is enabled on the pin.

This commit replaces the WARN call with a dev_info_once call, fixing a
bunch of WARN splats in dmesg on each suspend/resume cycle.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v3:
- Replace WARN with a dev_info_once call, instead of dropping it

Changes in v2:
- Drop now unused conf_ref local variable
---
 drivers/pinctrl/intel/pinctrl-baytrail.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
index c6f53ed626c9..17e6740a36c5 100644
--- a/drivers/pinctrl/intel/pinctrl-baytrail.c
+++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
@@ -811,15 +811,15 @@ static int byt_gpio_set_direction(struct pinctrl_dev *pctl_dev,
 	value &= ~BYT_DIR_MASK;
 	if (input)
 		value |= BYT_OUTPUT_EN;
-	else
+	else if (readl(conf_reg) & BYT_DIRECT_IRQ_EN)
 		/*
 		 * Before making any direction modifications, do a check if gpio
 		 * is set for direct IRQ.  On baytrail, setting GPIO to output
 		 * does not make sense, so let's at least warn the caller before
 		 * they shoot themselves in the foot.
 		 */
-		WARN(readl(conf_reg) & BYT_DIRECT_IRQ_EN,
-		     "Potential Error: Setting GPIO with direct_irq_en to output");
+		dev_info_once(vg->dev, "Potential Error: Setting GPIO with direct_irq_en to output");
+
 	writel(value, val_reg);
 
 	raw_spin_unlock_irqrestore(&byt_lock, flags);
-- 
2.24.1

Re: [PATCH v3] pinctrl: baytrail: Replace WARN with dev_info_once when setting direct-irq pin to output

[Mika Westerberg]

On Wed, Jan 01, 2020 at 03:52:43PM +0100, Hans de Goede wrote:
> Suspending Goodix touchscreens requires changing the interrupt pin to
> output before sending them a power-down command. Followed by wiggling
> the interrupt pin to wake the device up, after which it is put back
> in input mode.
> 
> On Cherry Trail device the interrupt pin is listed as a GpioInt ACPI
> resource so we can do this without problems as long as we release the
> irq before changing the pin to output mode.
> 
> On Bay Trail devices with a Goodix touchscreen direct-irq mode is used
> in combination with listing the pin as a normal GpioIo resource. This
> works fine, but this triggers the WARN in byt_gpio_set_direction-s output
> path because direct-irq support is enabled on the pin.
> 
> This commit replaces the WARN call with a dev_info_once call, fixing a
> bunch of WARN splats in dmesg on each suspend/resume cycle.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v3] pinctrl: baytrail: Replace WARN with dev_info_once when setting direct-irq pin to output

[Linus Walleij]

On Wed, Jan 1, 2020 at 3:52 PM Hans de Goede <hdegoede@redhat.com> wrote:

> Suspending Goodix touchscreens requires changing the interrupt pin to
> output before sending them a power-down command. Followed by wiggling
> the interrupt pin to wake the device up, after which it is put back
> in input mode.
>
> On Cherry Trail device the interrupt pin is listed as a GpioInt ACPI
> resource so we can do this without problems as long as we release the
> irq before changing the pin to output mode.
>
> On Bay Trail devices with a Goodix touchscreen direct-irq mode is used
> in combination with listing the pin as a normal GpioIo resource. This
> works fine, but this triggers the WARN in byt_gpio_set_direction-s output
> path because direct-irq support is enabled on the pin.
>
> This commit replaces the WARN call with a dev_info_once call, fixing a
> bunch of WARN splats in dmesg on each suspend/resume cycle.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v3:
> - Replace WARN with a dev_info_once call, instead of dropping it

Patch applied with Mika's ACK!

Yours,
Linus Walleij

Re: [PATCH v3] pinctrl: baytrail: Replace WARN with dev_info_once when setting direct-irq pin to output

[Andy Shevchenko]

On Wed, Jan 01, 2020 at 03:52:43PM +0100, Hans de Goede wrote:
> Suspending Goodix touchscreens requires changing the interrupt pin to
> output before sending them a power-down command. Followed by wiggling
> the interrupt pin to wake the device up, after which it is put back
> in input mode.
> 
> On Cherry Trail device the interrupt pin is listed as a GpioInt ACPI
> resource so we can do this without problems as long as we release the
> irq before changing the pin to output mode.
> 
> On Bay Trail devices with a Goodix touchscreen direct-irq mode is used
> in combination with listing the pin as a normal GpioIo resource. This
> works fine, but this triggers the WARN in byt_gpio_set_direction-s output
> path because direct-irq support is enabled on the pin.
> 
> This commit replaces the WARN call with a dev_info_once call, fixing a
> bunch of WARN splats in dmesg on each suspend/resume cycle.

Hmm...

> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v3:
> - Replace WARN with a dev_info_once call, instead of dropping it
> 
> Changes in v2:
> - Drop now unused conf_ref local variable
> ---
>  drivers/pinctrl/intel/pinctrl-baytrail.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
> index c6f53ed626c9..17e6740a36c5 100644
> --- a/drivers/pinctrl/intel/pinctrl-baytrail.c
> +++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
> @@ -811,15 +811,15 @@ static int byt_gpio_set_direction(struct pinctrl_dev *pctl_dev,
>  	value &= ~BYT_DIR_MASK;
>  	if (input)
>  		value |= BYT_OUTPUT_EN;
> -	else
> +	else if (readl(conf_reg) & BYT_DIRECT_IRQ_EN)
>  		/*
>  		 * Before making any direction modifications, do a check if gpio
>  		 * is set for direct IRQ.  On baytrail, setting GPIO to output
>  		 * does not make sense, so let's at least warn the caller before

...if it's a warning, perhaps do a warning instead of info?
Otherwise, we probably need to change a comment here.

>  		 * they shoot themselves in the foot.
>  		 */
> -		WARN(readl(conf_reg) & BYT_DIRECT_IRQ_EN,
> -		     "Potential Error: Setting GPIO with direct_irq_en to output");
> +		dev_info_once(vg->dev, "Potential Error: Setting GPIO with direct_irq_en to output");
> +
>  	writel(value, val_reg);
>  
>  	raw_spin_unlock_irqrestore(&byt_lock, flags);

P.S. I have applied it for bots to play with, but will wait for answer to the
above.

> -- 
> 2.24.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] pinctrl: baytrail: Replace WARN with dev_info_once when setting direct-irq pin to output

[Hans de Goede]

Hi,

On 08-01-2020 18:47, Andy Shevchenko wrote:
> On Wed, Jan 01, 2020 at 03:52:43PM +0100, Hans de Goede wrote:
>> Suspending Goodix touchscreens requires changing the interrupt pin to
>> output before sending them a power-down command. Followed by wiggling
>> the interrupt pin to wake the device up, after which it is put back
>> in input mode.
>>
>> On Cherry Trail device the interrupt pin is listed as a GpioInt ACPI
>> resource so we can do this without problems as long as we release the
>> irq before changing the pin to output mode.
>>
>> On Bay Trail devices with a Goodix touchscreen direct-irq mode is used
>> in combination with listing the pin as a normal GpioIo resource. This
>> works fine, but this triggers the WARN in byt_gpio_set_direction-s output
>> path because direct-irq support is enabled on the pin.
>>
>> This commit replaces the WARN call with a dev_info_once call, fixing a
>> bunch of WARN splats in dmesg on each suspend/resume cycle.
> 
> Hmm...
> 
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v3:
>> - Replace WARN with a dev_info_once call, instead of dropping it
>>
>> Changes in v2:
>> - Drop now unused conf_ref local variable
>> ---
>>   drivers/pinctrl/intel/pinctrl-baytrail.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
>> index c6f53ed626c9..17e6740a36c5 100644
>> --- a/drivers/pinctrl/intel/pinctrl-baytrail.c
>> +++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
>> @@ -811,15 +811,15 @@ static int byt_gpio_set_direction(struct pinctrl_dev *pctl_dev,
>>   	value &= ~BYT_DIR_MASK;
>>   	if (input)
>>   		value |= BYT_OUTPUT_EN;
>> -	else
>> +	else if (readl(conf_reg) & BYT_DIRECT_IRQ_EN)
>>   		/*
>>   		 * Before making any direction modifications, do a check if gpio
>>   		 * is set for direct IRQ.  On baytrail, setting GPIO to output
>>   		 * does not make sense, so let's at least warn the caller before
> 
> ...if it's a warning, perhaps do a warning instead of info?
> Otherwise, we probably need to change a comment here.

I went with info on purpose since this will trigger on all BYT devices with
a Goodix touchscreens of which we have quite a few, so my vote goes to maybe
the comment. Note that although the log level is info (because it is somewhat
expected to happen), the content of the log msg is still warning-ish.

I guess we could replace "let's at least warn the caller before" with
"let's at least let the caller know before"

Regards,

Hans



> 
>>   		 * they shoot themselves in the foot.
>>   		 */
>> -		WARN(readl(conf_reg) & BYT_DIRECT_IRQ_EN,
>> -		     "Potential Error: Setting GPIO with direct_irq_en to output");
>> +		dev_info_once(vg->dev, "Potential Error: Setting GPIO with direct_irq_en to output");
>> +
>>   	writel(value, val_reg);
>>   
>>   	raw_spin_unlock_irqrestore(&byt_lock, flags);
> 
> P.S. I have applied it for bots to play with, but will wait for answer to the
> above.
> 
>> -- 
>> 2.24.1
>>
> 

Re: [PATCH v3] pinctrl: baytrail: Replace WARN with dev_info_once when setting direct-irq pin to output

[Andy Shevchenko]

On Thu, Jan 09, 2020 at 10:05:47AM +0100, Hans de Goede wrote:
> On 08-01-2020 18:47, Andy Shevchenko wrote:
> > On Wed, Jan 01, 2020 at 03:52:43PM +0100, Hans de Goede wrote:

...

> > >   		/*
> > >   		 * Before making any direction modifications, do a check if gpio
> > >   		 * is set for direct IRQ.  On baytrail, setting GPIO to output
> > >   		 * does not make sense, so let's at least warn the caller before
> > 
> > ...if it's a warning, perhaps do a warning instead of info?
> > Otherwise, we probably need to change a comment here.
> 
> I went with info on purpose since this will trigger on all BYT devices with
> a Goodix touchscreens of which we have quite a few, so my vote goes to maybe
> the comment. Note that although the log level is info (because it is somewhat
> expected to happen), the content of the log msg is still warning-ish.
> 
> I guess we could replace "let's at least warn the caller before" with
> "let's at least let the caller know before"

What about simple 'warn -> inform' ?
(I can update myself)

> > >   		 * they shoot themselves in the foot.
> > >   		 */
> > > -		WARN(readl(conf_reg) & BYT_DIRECT_IRQ_EN,
> > > -		     "Potential Error: Setting GPIO with direct_irq_en to output");
> > > +		dev_info_once(vg->dev, "Potential Error: Setting GPIO with direct_irq_en to output");

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] pinctrl: baytrail: Replace WARN with dev_info_once when setting direct-irq pin to output

[Hans de Goede]

Hi,

On 09-01-2020 11:21, Andy Shevchenko wrote:
> On Thu, Jan 09, 2020 at 10:05:47AM +0100, Hans de Goede wrote:
>> On 08-01-2020 18:47, Andy Shevchenko wrote:
>>> On Wed, Jan 01, 2020 at 03:52:43PM +0100, Hans de Goede wrote:
> 
> ...
> 
>>>>    		/*
>>>>    		 * Before making any direction modifications, do a check if gpio
>>>>    		 * is set for direct IRQ.  On baytrail, setting GPIO to output
>>>>    		 * does not make sense, so let's at least warn the caller before
>>>
>>> ...if it's a warning, perhaps do a warning instead of info?
>>> Otherwise, we probably need to change a comment here.
>>
>> I went with info on purpose since this will trigger on all BYT devices with
>> a Goodix touchscreens of which we have quite a few, so my vote goes to maybe
>> the comment. Note that although the log level is info (because it is somewhat
>> expected to happen), the content of the log msg is still warning-ish.
>>
>> I guess we could replace "let's at least warn the caller before" with
>> "let's at least let the caller know before"
> 
> What about simple 'warn -> inform' ?
> (I can update myself)

Works for me, thanks.

Regards,

Hans



> 
>>>>    		 * they shoot themselves in the foot.
>>>>    		 */
>>>> -		WARN(readl(conf_reg) & BYT_DIRECT_IRQ_EN,
>>>> -		     "Potential Error: Setting GPIO with direct_irq_en to output");
>>>> +		dev_info_once(vg->dev, "Potential Error: Setting GPIO with direct_irq_en to output");
>