Re: [PATCH 2/2] watchdog: s3c2410_wdt: Add support for Exynos8855 SoC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] watchdog: confusing reuse of CLUSTER0 macro for CLUSTER1
- [Low] watchdog: space-based alignment for EXYNOS8855_RST_STAT_REG_OFFSET
--

commit fec6f94918dfbe88ae7146d760500426231edbc3
Author: Alim Akhtar <alim.akhtar@samsung.com>

watchdog: s3c2410_wdt: Add support for Exynos8855 SoC

Watchdog IP on this SoC is similar to Exynos850/ExynosAutov{9/920}.
This does not have CNT_EN_WDT bit in PMU and offset of RST_STAT is
different then the previously support SoC.
Reuse the offset as much as from already supported SoC

> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> index 7053963fb0ac6..8ce4ca3e2aa81 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
> @@ -58,6 +58,7 @@
>  #define S3C2410_WATCHDOG_DEFAULT_TIME	(15)
>  
>  #define EXYNOS5_RST_STAT_REG_OFFSET		0x0404
> +#define EXYNOS8855_RST_STAT_REG_OFFSET          0x10
                                          ^^^^^^^^^^
[Severity: Low]
Does this new macro intentionally use space-based alignment here?

The surrounding macros seem to use tabs for alignment. Using spaces deviates
from the standard tab-based formatting.

>  #define EXYNOS5_WDT_DISABLE_REG_OFFSET		0x0408
>  #define EXYNOS5_WDT_MASK_RESET_REG_OFFSET	0x040c

[ ... ]

> @@ -281,6 +282,26 @@ static const struct s3c2410_wdt_variant drv_data_exynos850_cl1 = {
>  		  QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_CNT_EN,
>  };
>  
> +static const struct s3c2410_wdt_variant drv_data_exynos8855_cl0 = {
> +	.mask_reset_reg = EXYNOS850_CLUSTER0_NONCPU_INT_EN,
> +	.mask_bit = 2,
> +	.mask_reset_inv = true,
> +	.rst_stat_reg = EXYNOS8855_RST_STAT_REG_OFFSET,
> +	.rst_stat_bit = EXYNOS850_CLUSTER0_WDTRESET_BIT,
> +	.quirks = QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_MASK_RESET |
> +		  QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_DBGACK_BIT,
> +};
> +
> +static const struct s3c2410_wdt_variant drv_data_exynos8855_cl1 = {
> +	.mask_reset_reg = EXYNOSAUTOV920_CLUSTER1_NONCPU_INT_EN,
> +	.mask_bit = 2,
> +	.mask_reset_inv = true,
> +	.rst_stat_reg = EXYNOS8855_RST_STAT_REG_OFFSET,
> +	.rst_stat_bit = EXYNOSAUTOV9_CLUSTER0_WDTRESET_BIT,
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Low]
Is it intentional to use a CLUSTER0 macro from an unrelated SoC to define the
reset status bit for a cluster 1 configuration?

Even if the underlying hardware bit value is correct and allows reuse, applying
a CLUSTER0 macro here is misleading and looks like a potential copy-paste typo.
Could this result in a functional logic error if the status bit for cluster 1
is actually supposed to be different?

> +	.quirks = QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_MASK_RESET |
> +		  QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_DBGACK_BIT,
> +};
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260927143100.3576195-1-alim.akhtar@samsung.com?part=2