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

[sashiko-bot@kernel.org]

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

--- Patch [2]: [PATCH v2 2/2] watchdog: s3c2410_wdt: Add exynos5515-wdt compatible data ---
commit fd223d0c16451bec63089d85a77c3b012b8f28ed
Author: Aiden Isik <aidenisik@member.fsf.org>

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,
> +};
> +

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818-for-next-lucky7-watchdog-v2-0-6743f479a47f@member.fsf.org?part=2