Re: [PATCH v2 1/1] watchdog: realtek-otto: Change to use regmap API

[Guenter Roeck]

On 5/19/26 12:00, Sander Vanheule wrote:
> Hi Rustam,
>
> On Tue, 2026-05-19 at 23:23 +0500, Rustam Adilov wrote:
> [...]
> > @@ -74,24 +75,17 @@ struct otto_wdt_ctrl {
> >   static int otto_wdt_start(struct watchdog_device *wdev)
> >   {
> >   	struct otto_wdt_ctrl *ctrl = watchdog_get_drvdata(wdev);
> > -	u32 v;
> > -
> > -	v = ioread32(ctrl->base + OTTO_WDT_REG_CTRL);
> > -	v |= OTTO_WDT_CTRL_ENABLE;
> > -	iowrite32(v, ctrl->base + OTTO_WDT_REG_CTRL);
> >   
> > +	regmap_set_bits(ctrl->regmap, OTTO_WDT_REG_CTRL,
> > OTTO_WDT_CTRL_ENABLE);
> >   	return 0;
>
> iowrite32() doesn't return anything, but regmap_set_bits() does.
>
> You can turn this into
>      return regmap_set_bits(...);
>
> >   }
> >   
> >   static int otto_wdt_stop(struct watchdog_device *wdev)
> >   {
> >   	struct otto_wdt_ctrl *ctrl = watchdog_get_drvdata(wdev);
> > -	u32 v;
> > -
> > -	v = ioread32(ctrl->base + OTTO_WDT_REG_CTRL);
> > -	v &= ~OTTO_WDT_CTRL_ENABLE;
> > -	iowrite32(v, ctrl->base + OTTO_WDT_REG_CTRL);
> >   
> > +	regmap_clear_bits(ctrl->regmap, OTTO_WDT_REG_CTRL,
> > +			  OTTO_WDT_CTRL_ENABLE);
> >   	return 0;
>
> Same as above.
> This is probably short enough to keep is on one line? (< 100 characters)
>
> >   }
> >   
> > @@ -99,8 +93,7 @@ static int otto_wdt_ping(struct watchdog_device *wdev)
> >   {
> >   	struct otto_wdt_ctrl *ctrl = watchdog_get_drvdata(wdev);
> >   
> > -	iowrite32(OTTO_WDT_CNTR_PING, ctrl->base + OTTO_WDT_REG_CNTR);
> > -
> > +	regmap_write(ctrl->regmap, OTTO_WDT_REG_CNTR, OTTO_WDT_CNTR_PING);
> >   	return 0;
>
> Same as above.
>
> >   }
> >   
> > @@ -126,7 +119,7 @@ static int otto_wdt_determine_timeouts(struct
> > watchdog_device *wdev, unsigned in
> >   	unsigned int total_ticks;
> >   	unsigned int prescale;
> >   	unsigned int tick_ms;
> > -	u32 v;
> > +	u32 mask, val;
> >   
> >   	do {
> >   		prescale = prescale_next;
> > @@ -142,14 +135,11 @@ static int otto_wdt_determine_timeouts(struct
> > watchdog_device *wdev, unsigned in
> >   	} while (phase1_ticks > OTTO_WDT_PHASE_TICKS_MAX
> >   		|| phase2_ticks > OTTO_WDT_PHASE_TICKS_MAX);
> >   
> > -	v = ioread32(ctrl->base + OTTO_WDT_REG_CTRL);
> > -
> > -	v &= ~(OTTO_WDT_CTRL_PRESCALE | OTTO_WDT_CTRL_PHASE1 |
> > OTTO_WDT_CTRL_PHASE2);
> > -	v |= FIELD_PREP(OTTO_WDT_CTRL_PHASE1, phase1_ticks - 1);
> > -	v |= FIELD_PREP(OTTO_WDT_CTRL_PHASE2, phase2_ticks - 1);
> > -	v |= FIELD_PREP(OTTO_WDT_CTRL_PRESCALE, prescale);
> > -
> > -	iowrite32(v, ctrl->base + OTTO_WDT_REG_CTRL);
> > +	mask = OTTO_WDT_CTRL_PRESCALE | OTTO_WDT_CTRL_PHASE1 |
> > OTTO_WDT_CTRL_PHASE2;
> > +	val = FIELD_PREP(OTTO_WDT_CTRL_PHASE1, phase1_ticks - 1);
> > +	val |= FIELD_PREP(OTTO_WDT_CTRL_PHASE2, phase2_ticks - 1);
> > +	val |= FIELD_PREP(OTTO_WDT_CTRL_PRESCALE, prescale);
> > +	regmap_update_bits(ctrl->regmap, OTTO_WDT_REG_CTRL, mask, val);
>
> To be consistent, you would also need to propagate the result of
> regmap_update_bits() if it's an error. Same for the other regmap_*() calls.

I am now inclined to reject this patch. regmap mmio never returns errors
unless it is associated with clocks, and now useless error handling is
requested. That defeats the idea of simplifying the code. Instead, its
complexity is increased by adding unnecessary error handling.

So this is now a NACK if unnecessary error handling is indeed added,
unless someone convinces me that this would add some benefits that I
am unable to see.

Guenter