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

[Rustam Adilov <adilov@disroot.org>]

On 2026-05-19 19:25, Guenter Roeck wrote:
> 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

Hello,
Sorry for taking so long to reply.

Just to be clear, i didn't have a chance to do anything about the requested
error handling. Looking at the existing watchdog drivers that use regmap, some
implement the error handling and some don't and that's a bit confusing to me.
I would like to see if Sander has something more to say here.

While i am here, this whole patch is not specifically for simplifying the code.
It is also about fixing an issue where the CONFIG_SWAP_IO_SPACE is enabled. It
makes it so that ioread32 and iowrite32 perform byte swap and since this driver
expects them not to byte swap, things break. RTL9607C is expected to have it for
USB support.
Alternatively i could have used __raw_readl and __raw_writel instead of regmap but
i felt like regmap would be more accepted.

Best,
Rustam