[PATCH v2] watchdog: designware: make reset really optional

[Quentin Schulz]

Hi Sean,

On 11/15/22 14:22, Sean Anderson wrote:
> On 11/15/22 05:20, Quentin Schulz wrote:
>> From: Quentin Schulz <quentin.schulz at theobroma-systems.com>
>>
>> Checking for DM_RESET is not enough since not all watchdog
>> implementations use a reset lane. Such is the case for Rockchip
>> implementation for example. Since reset_assert_bulk will only succeed if
>> the resets property exists in the watchdog DT node, it needs to be
>> called only if a reset property is present.
>>
>> This adds a condition on the resets property presence in the watchdog DT
>> node before assuming a reset lane needs to be fetched with
>> reset_assert_bulk, by calling ofnode_read_prop.
>>
>> Cc: Quentin Schulz <foss+uboot at 0leil.net>
>> Reviewed-by: Stefan Roese <sr at denx.de>
>> Signed-off-by: Quentin Schulz <quentin.schulz at theobroma-systems.com>
>> ---
>> To: Stefan Roese <sr at denx.de>
>> Cc: u-boot at lists.denx.de
>> ---
>> Changes in v2:
>> - added Rb,
>> - changed indentation from space to tabs while adding condition,
>> - Link to v1: 
>> https://urldefense.com/v3/__https://lore.kernel.org/r/20221114-dw-wdt-no-reset-v1-0-15a62faba4cc@theobroma-systems.com__;!!OOPJP91ZZw!hjXLDLa7xWHE9MH_lM74A55aTSs47Xxu3cwp4EcZ3v4FkZ2uEyTzqWAgUpPTEGaX8kTKOCHSHC7FuHFN9deZmXuY08tg-Q$ ---
>>   drivers/watchdog/designware_wdt.c | 9 +++++----
>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/watchdog/designware_wdt.c 
>> b/drivers/watchdog/designware_wdt.c
>> index cad756aeaf..f8df1916b5 100644
>> --- a/drivers/watchdog/designware_wdt.c
>> +++ b/drivers/watchdog/designware_wdt.c
>> @@ -72,13 +72,13 @@ static int designware_wdt_reset(struct udevice *dev)
>>   static int designware_wdt_stop(struct udevice *dev)
>>   {
>>       struct designware_wdt_priv *priv = dev_get_priv(dev);
>> +    __maybe_unused int ret;
>>       designware_wdt_reset(dev);
>>       writel(0, priv->base + DW_WDT_CR);
>> -        if (CONFIG_IS_ENABLED(DM_RESET)) {
>> -        int ret;
>> -
>> +    if (CONFIG_IS_ENABLED(DM_RESET) &&
>> +        ofnode_read_prop(dev_ofnode(dev), "resets", &ret)) {
>>           ret = reset_assert_bulk(&priv->resets);
>>           if (ret)
>>               return ret;
>> @@ -135,7 +135,8 @@ static int designware_wdt_probe(struct udevice *dev)
>>       priv->clk_khz = CONFIG_DW_WDT_CLOCK_KHZ;
>>   #endif
>> -    if (CONFIG_IS_ENABLED(DM_RESET)) {
>> +    if (CONFIG_IS_ENABLED(DM_RESET) &&
>> +        ofnode_read_prop(dev_ofnode(dev), "resets", &ret)) {
>>           ret = reset_get_bulk(dev, &priv->resets);
>>           if (ret)
> 
> Can't we just check for -ENOENT here?
> 

__of_parse_phandle_with_args/fdtdec_parse_phandle_with_args has three 
possible meanings for -ENOENT, two of which aren't matching the "no such 
property" behavior I'd expect.

And that's before digging into the multiple other function calls in 
reset_get_by_index_tail.

I'm not against it, not sure it's safe enough that is all.

Cheers,
Quentin