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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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