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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The `drv_data_exynos5515` variant declares `QUIRK_HAS_PMU_AUTO_DISABLE` but fails to initialize `.disable_reg`, causing unintentional writes to PMU offset 0x0000.
- [High] The `drv_data_exynos5515` variant uses an 'INT_EN' register for masking watchdog reset, but omits the `.mask_reset_inv = true` flag, inverting the WDT reset mask logic.
- [Medium] Incorrect use of an Exynos Auto v920 register macro (`EXYNOSAUTOV920_CLUSTER0_NONCPU_INT_EN`) in an Exynos 5515 variant.
--

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

watchdog: s3c2410_wdt: Add exynos5515-wdt compatible data

This commit adds driver data and a compatible string 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: Medium]
Is this intentionally using an Exynos Auto v920 register macro for an
Exynos 5515 variant, or could this be a copy-paste error?

[Severity: High]
Since this variant uses an INT_EN register for masking the reset, should
the .mask_reset_inv = true flag be set here?

Without the inversion flag, the bitwise masking logic in
s3c2410wdt_mask_wdt_reset() evaluates to (mask ^ false). For INT_EN
registers, this typically writes 1 when intending to mask the reset, and 0
when intending to unmask it, effectively inverting the logic.

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

[Severity: High]
Will this cause unintentional writes to offset 0x0000 of the PMU regmap?

This variant declares QUIRK_HAS_PMU_AUTO_DISABLE but omits .disable_reg.
Since struct fields default to zero, when s3c2410wdt_enable() checks for
this quirk and calls s3c2410wdt_disable_wdt_reset() during driver probe:

s3c2410wdt_disable_wdt_reset()
    regmap_update_bits(wdt->pmureg, wdt->drv_data->disable_reg, mask_val, val);

regmap_update_bits() will blindly write to offset 0x0000.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260817-for-next-lucky7-watchdog-v1-0-a14370e20b92@member.fsf.org?part=2