On Saturday, August 2, 2025 at 1:37 PM Sam Protsenko [off-list ref] wrote:

Not a strong point, but I'd break this patch into two:
  1. Add 32-bit counter feature (without enabling it in exynosautov920
implementation)
  2. Enable 32-bit counter feature in exynosautov920

I'll break this patch into two in the next patch set.

quoted

 #define S3C2410_WTCNT_MAXCNT   0xffff

Suggest renaming this to S3C2410_WTCNT_MAXCNT_16, to emphasize the fact
this value is for 16-bit counters. And for consistency with the below one.

quoted

+#define S3C2410_WTCNT_MAXCNT_32        0xffffffff

I'll rename this to S3C2410_WTCNT_MAXCNT_16 in the next patch set.

quoted

+ * %QUIRK_HAS_32BIT_MAXCNT: WTDAT and WTCNT are 32-bit registers.
+ With these

Why not name it like QUIRK_HAS_32BIT_CNT or QUIRK_HAS_32BIT_COUNTER?
As I understand, the quirk means that the chip has 32-bit counter, so MAX
bit is not really needed?

quoted

+ * 32-bit registers, larger values to be set, which means that larger
+ timeouts

Spelling: "to be set" -> "will be set" (or "have to be set").

I'll modify this in the next patch set.

quoted

+       unsigned int            max_cnt;

Maybe make it u32? It definitely refers to a 32-bit register value, so
will be more explicit that way. Not a strong opinion though.

I'll change this to u32 in the next patch set.

quoted

 };

 static const struct s3c2410_wdt_variant drv_data_s3c2410 = {

@@ -349,7 +356,7 @@ static const struct s3c2410_wdt_variant

drv_data_exynosautov920_cl0 = {

quoted

        .cnt_en_bit = 8,
        .quirks = QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_MASK_RESET |
                  QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_CNT_EN |
-                 QUIRK_HAS_DBGACK_BIT,
+                 QUIRK_HAS_DBGACK_BIT | QUIRK_HAS_32BIT_MAXCNT,
 };

 static const struct s3c2410_wdt_variant drv_data_exynosautov920_cl1 = {

@@ -362,7 +369,7 @@ static const struct s3c2410_wdt_variant

drv_data_exynosautov920_cl1 = {

quoted

        .cnt_en_bit = 8,
        .quirks = QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_MASK_RESET |
                  QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_CNT_EN |
-                 QUIRK_HAS_DBGACK_BIT,
+                 QUIRK_HAS_DBGACK_BIT | QUIRK_HAS_32BIT_MAXCNT,

Yeah, I think it would be easier to review and handle further if this
exynosautov920 enablement is extracted into a separate patch.

I'll break this patch into two in the next patch set.

quoted

-       if (count >= 0x10000) {
-               divisor = DIV_ROUND_UP(count, 0xffff);
+       if (count > wdt->max_cnt) {

wdt->max_cnt + 1?

Yes, 0x10000 represented 'wdt->max_cnt + 1.'
Would you like to suggest any revisions?

quoted

+       wdt->max_cnt = S3C2410_WTCNT_MAXCNT;
+       if ((wdt->drv_data->quirks & QUIRK_HAS_32BIT_MAXCNT))

Double braces don't seem to be needed.

quoted

+               wdt->max_cnt = S3C2410_WTCNT_MAXCNT_32;
+

Style (minor nitpick): this block can be more explicit, i.e.:

       if ((wdt->drv_data->quirks & QUIRK_HAS_32BIT_MAXCNT))
               wdt->max_cnt = S3C2410_WTCNT_MAXCNT_32;
       else
               wdt->max_cnt = S3C2410_WTCNT_MAXCNT;

I'll fix these in the next patch set.