RE: [PATCH v4 3/4] watchdog: s3c2410_wdt: Increase max timeout value of watchdog

[<sw617.shin@samsung.com>]

On Saturday, August 2, 2025 at 1:37 PM Sam Protsenko <semen.protsenko@linaro.org> 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.

> >  #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.
> 
> > +#define S3C2410_WTCNT_MAXCNT_32        0xffffffff
> >

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

> > + * %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?
> 
> > + * 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.

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

> >  };
> >
> >  static const struct s3c2410_wdt_variant drv_data_s3c2410 = {
> > @@ -349,7 +356,7 @@ static const struct s3c2410_wdt_variant
> drv_data_exynosautov920_cl0 = {
> >         .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 = {
> >         .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.

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

> > +       wdt->max_cnt = S3C2410_WTCNT_MAXCNT;
> > +       if ((wdt->drv_data->quirks & QUIRK_HAS_32BIT_MAXCNT))
> 
> Double braces don't seem to be needed.
> 
> > +               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.