Re: [PATCH v4] watchdog: qcom: support pre-timeout when the bark irq is available

[Jorge Ramirez]

>>>
>>>>  static const u32 reg_offset_data_apcs_tmr[] = {
>>>>  	[WDT_RST] = 0x38,
>>>>  	[WDT_EN] = 0x40,
>>>> @@ -54,15 +58,38 @@ struct qcom_wdt *to_qcom_wdt(struct watchdog_device *wdd)
>>>>  	return container_of(wdd, struct qcom_wdt, wdd);
>>>>  }
>>>>  
>>>> +static inline int qcom_get_enable(struct watchdog_device *wdd)
>>>> +{
>>>> +	int enable = QCOM_WDT_ENABLE;
>>>> +
>>>> +	if (wdd->info->options & WDIOF_PRETIMEOUT)
>>>> +		enable |= QCOM_WDT_ENABLE_IRQ;
>>>> +
>>>
>>> Again, the condition needs to be that pretimeout != 0,
>>> not that it is supported.
>>
>> no I dont think so. doing that would propagate a possible error in some
>> pretimeout setup code which would end up enabling an interrupt when it
>> shouldnt. so I dont think that doing that would be correct.
>>
> If the pretimeout setup code is buggy, it needs to be fixed.

the condition whether to enable the HW interrupts (IMO) should be
controlled by the DTS as part of the static configuration.

> 
>> The interrupt should only be enabled if WDIOF_PRETIMEOUT is configured
>> (independently of the pretimeout value); as a matter of fact, if
>> pretimeout is 0, the interrupt will trigger at the same time than bark
>> (which is what the original code used to do).
>>
> The original code did not set bit 1 of the WDT_EN register,

sure, this is true

> and it did not set the bark time.

actually no, unless we are looking at different files, the original code
did set the bark time even though PRETIMEOUT was not enabled... so yes
bark was being set to bite. Maybe I am misunderstanding your point.

> 
>> so I'd rather keep this condition unless you strongly oppose to it.
>>
> 
> Please feel free to petition  to Wim.

I'll change to your recomendation and repost v5 - I thought
WDIOF_PRETIMEOUT was formally correct but functionally there is little
difference (if the hardware works as expected)

thanks for all your comments Guenter.