Re: [PATCH v2 2/2] watchdog: s3c2410_wdt: Add exynos5515-wdt compatible data

Aiden,

On 8/18/26 08:53, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] The `drv_data_exynos5515` variant includes `QUIRK_HAS_PMU_AUTO_DISABLE` but fails to define `.disable_reg`, resulting in PMU register corruption at offset 0x0.
> - [High] The `drv_data_exynos5515` variant uses an INT_EN register for masking but fails to set `.mask_reset_inv = true`, inverting the watchdog masking logic.
> --
> 
Please either address or explain why those are non-issues.

Guenter

> --- Patch [2]: [PATCH v2 2/2] watchdog: s3c2410_wdt: Add exynos5515-wdt compatible data ---
> commit fd223d0c16451bec63089d85a77c3b012b8f28ed
> Author: Aiden Isik <[email protected]>
> 
> watchdog: s3c2410_wdt: Add exynos5515-wdt compatible data
>      
> Add driver data for the Exynos5515 SoC's watchdog timer.
> 
>> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
>> index e31f93db05096..7299dfdb0c099 100644
>> --- a/drivers/watchdog/s3c2410_wdt.c
>> +++ b/drivers/watchdog/s3c2410_wdt.c
>> @@ -232,6 +232,16 @@ static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
>>   		  QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_AUTO_DISABLE,
>>   };
>>   
>> +static const struct s3c2410_wdt_variant drv_data_exynos5515 = {
>> +	.mask_reset_reg = EXYNOSAUTOV920_CLUSTER0_NONCPU_INT_EN,
> 
> [Severity: High]
> This variant uses an INT_EN register for masking but omits .mask_reset_inv.
> Could this cause the watchdog masking logic to be inverted?
> 
> Looking at s3c2410wdt_mask_wdt_reset(), the logic evaluates to false when
> unmasking if .mask_reset_inv is not set to true:
> 
> s3c2410wdt_mask_wdt_reset() {
> 	...
> 	const bool val_inv = wdt->drv_data->mask_reset_inv;
> 	const u32 val = (mask ^ val_inv) ? mask_val : 0;
> 	...
> 	ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->mask_reset_reg,
> 				 mask_val, val);
> 
> Does this mean it will write a 0 when unmasking, disabling the reset output
> instead of enabling it?
> 
>> +	.mask_bit = 2,
>> +	.rst_stat_reg = EXYNOS5_RST_STAT_REG_OFFSET,
>> +	.rst_stat_bit = 24,
>> +	.quirks = QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_MASK_RESET | \
>> +		  QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_AUTO_DISABLE | \
> 
> [Severity: High]
> Since QUIRK_HAS_PMU_AUTO_DISABLE is set but .disable_reg is omitted, will
> .disable_reg default to 0x0?
> 
> During initialization and suspend, s3c2410wdt_disable_wdt_reset() applies
> updates to wdt->drv_data->disable_reg:
> 
> s3c2410wdt_disable_wdt_reset() {
> 	...
> 	ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->disable_reg,
> 				 mask_val, val);
> 
> Could this unintentionally corrupt bit 2 at PMU offset 0x0?
> 
>> +		  QUIRK_HAS_DBGACK_BIT,
>> +};
>> +
> 
> [ ... ]
>