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

> 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 <[email protected]>
> 
> 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?
> 

This is intentional.

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

Possibly, will wait for maintainer input.

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

Again, will wait for maintainer input.

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