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

[Hans de Goede <hdegoede@redhat.com>]

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
>>
>