[PATCH v2 0/1] watchdog: realtek-otto: Make use of regmap API

[PATCH v2 0/1] watchdog: realtek-otto: Make use of regmap API

[Rustam Adilov]

This patch series changes the driver to use regmap API for all of the
register access stuff instead of the ioread32 and iowrite32 with __iomem.

---
Changelog in v2:
- removed patches 2 and 3 that add rtl9607c device.
- included driver changes made by Sander in [1].
- added disable_locking to regmap_config and set it to true.
- Link to v1: https://lore.kernel.org/all/20260509163101.722793-1-adilov@disroot.org/

[1] - https://lore.kernel.org/all/20260515212351.752054-1-sander@svanheule.net/

Rustam Adilov (1):
  watchdog: realtek-otto: Change to use regmap API

 drivers/watchdog/Kconfig            |  1 +
 drivers/watchdog/realtek_otto_wdt.c | 74 +++++++++++++++--------------
 2 files changed, 39 insertions(+), 36 deletions(-)

-- 
2.54.0

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

[Rustam Adilov]

Change all of the register access stuff to done through regmap API.
This helps us to simplify the code and allows to make use of specific
regmap functions like regmap_update_bits to replace read/modify/write
instances.

Add the REGMAP_MMIO as a select to REALTEK_OTTO_WDT now that the regmap
is used.

Signed-off-by: Rustam Adilov <adilov@disroot.org>
---
 drivers/watchdog/Kconfig            |  1 +
 drivers/watchdog/realtek_otto_wdt.c | 74 +++++++++++++++--------------
 2 files changed, 39 insertions(+), 36 deletions(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index dc78729ba2a5..5c32d79b126c 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1076,6 +1076,7 @@ config REALTEK_OTTO_WDT
 	depends on MACH_REALTEK_RTL || COMPILE_TEST
 	depends on COMMON_CLK
 	select WATCHDOG_CORE
+	select REGMAP_MMIO
 	default MACH_REALTEK_RTL
 	help
 	  Say Y here to include support for the watchdog timer on Realtek
diff --git a/drivers/watchdog/realtek_otto_wdt.c b/drivers/watchdog/realtek_otto_wdt.c
index 01b3ef89bacf..504a7d379f4a 100644
--- a/drivers/watchdog/realtek_otto_wdt.c
+++ b/drivers/watchdog/realtek_otto_wdt.c
@@ -28,6 +28,7 @@
 #include <linux/platform_device.h>
 #include <linux/property.h>
 #include <linux/reboot.h>
+#include <linux/regmap.h>
 #include <linux/watchdog.h>
 
 #define OTTO_WDT_REG_CNTR		0x0
@@ -66,7 +67,7 @@
 struct otto_wdt_ctrl {
 	struct watchdog_device wdev;
 	struct device *dev;
-	void __iomem *base;
+	struct regmap *regmap;
 	unsigned int clk_rate_khz;
 	int irq_phase1;
 };
@@ -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;
 }
 
 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;
 }
 
@@ -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;
 }
 
@@ -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);
 
 	timeout_ms = total_ticks * tick_ms;
 	ctrl->wdev.timeout = timeout_ms / 1000;
@@ -193,7 +183,7 @@ static int otto_wdt_restart(struct watchdog_device *wdev, unsigned long reboot_m
 
 	/* Configure for shortest timeout and wait for reset to occur */
 	v = FIELD_PREP(OTTO_WDT_CTRL_RST_MODE, reset_mode) | OTTO_WDT_CTRL_ENABLE;
-	iowrite32(v, ctrl->base + OTTO_WDT_REG_CTRL);
+	regmap_write(ctrl->regmap, OTTO_WDT_REG_CTRL, v);
 
 	mdelay(3 * otto_wdt_tick_ms(ctrl, 0));
 
@@ -204,7 +194,7 @@ static irqreturn_t otto_wdt_phase1_isr(int irq, void *dev_id)
 {
 	struct otto_wdt_ctrl *ctrl = dev_id;
 
-	iowrite32(OTTO_WDT_INTR_PHASE_1, ctrl->base + OTTO_WDT_REG_INTR);
+	regmap_write(ctrl->regmap, OTTO_WDT_REG_INTR, OTTO_WDT_INTR_PHASE_1);
 	dev_crit(ctrl->dev, "phase 1 timeout\n");
 	watchdog_notify_pretimeout(&ctrl->wdev);
 
@@ -250,7 +240,6 @@ static int otto_wdt_probe_reset_mode(struct otto_wdt_ctrl *ctrl)
 	const struct fwnode_handle *node = ctrl->dev->fwnode;
 	int mode_count;
 	u32 mode;
-	u32 v;
 
 	if (!node)
 		return -ENXIO;
@@ -272,19 +261,25 @@ static int otto_wdt_probe_reset_mode(struct otto_wdt_ctrl *ctrl)
 	else
 		return -EINVAL;
 
-	v = ioread32(ctrl->base + OTTO_WDT_REG_CTRL);
-	v &= ~OTTO_WDT_CTRL_RST_MODE;
-	v |= FIELD_PREP(OTTO_WDT_CTRL_RST_MODE, mode);
-	iowrite32(v, ctrl->base + OTTO_WDT_REG_CTRL);
-
+	regmap_update_bits(ctrl->regmap, OTTO_WDT_REG_CTRL,
+			   OTTO_WDT_CTRL_RST_MODE,
+			   FIELD_PREP(OTTO_WDT_CTRL_RST_MODE, mode));
 	return 0;
 }
 
+static const struct regmap_config realtek_otto_wdt_regmap_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.disable_locking = true,
+};
+
 static int otto_wdt_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct otto_wdt_ctrl *ctrl;
 	unsigned int max_tick_ms;
+	void __iomem *base;
 	int ret;
 
 	ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
@@ -292,18 +287,25 @@ static int otto_wdt_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	ctrl->dev = dev;
-	ctrl->base = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(ctrl->base))
-		return PTR_ERR(ctrl->base);
+	base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	ctrl->regmap = devm_regmap_init_mmio(dev, base,
+					     &realtek_otto_wdt_regmap_config);
+	if (IS_ERR(ctrl->regmap)) {
+		dev_err(dev, "regmap init failed\n");
+		return PTR_ERR(ctrl->regmap);
+	}
 
 	ret = otto_wdt_probe_clk(ctrl);
 	if (ret)
 		return ret;
 
 	/* Clear any old interrupts and reset initial state */
-	iowrite32(OTTO_WDT_INTR_PHASE_1 | OTTO_WDT_INTR_PHASE_2,
-			ctrl->base + OTTO_WDT_REG_INTR);
-	iowrite32(OTTO_WDT_CTRL_DEFAULT, ctrl->base + OTTO_WDT_REG_CTRL);
+	regmap_write(ctrl->regmap, OTTO_WDT_REG_INTR,
+		     OTTO_WDT_INTR_PHASE_1 | OTTO_WDT_INTR_PHASE_2);
+	regmap_write(ctrl->regmap, OTTO_WDT_REG_CTRL, OTTO_WDT_CTRL_DEFAULT);
 
 	ctrl->irq_phase1 = platform_get_irq_byname(pdev, "phase1");
 	if (ctrl->irq_phase1 < 0)
-- 
2.54.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Calling `disable_irq()` in the watchdog restart handler can cause a system deadlock and sleeps in an atomic context during reboot.
--

commit 31e0a644ed16d3bca39871dd569420fedb50663e
Author: Rustam Adilov <adilov@disroot.org>

watchdog: realtek-otto: Change to use regmap API

This commit converts the Realtek Otto watchdog driver to use the regmap API
for register access instead of direct MMIO. It simplifies the code by
replacing read/modify/write sequences with regmap-specific functions like
regmap_update_bits.

> diff --git a/drivers/watchdog/realtek_otto_wdt.c b/drivers/watchdog/realtek_otto_wdt.c
> index 01b3ef89bacfc..504a7d379f4ae 100644
> --- a/drivers/watchdog/realtek_otto_wdt.c
> +++ b/drivers/watchdog/realtek_otto_wdt.c

[ ... ]

> @@ -193,7 +183,7 @@ static int otto_wdt_restart(struct watchdog_device *wdev, unsigned long reboot_m
>  
>  	/* Configure for shortest timeout and wait for reset to occur */
>  	v = FIELD_PREP(OTTO_WDT_CTRL_RST_MODE, reset_mode) | OTTO_WDT_CTRL_ENABLE;
> -	iowrite32(v, ctrl->base + OTTO_WDT_REG_CTRL);
> +	regmap_write(ctrl->regmap, OTTO_WDT_REG_CTRL, v);
>  
>  	mdelay(3 * otto_wdt_tick_ms(ctrl, 0));
>  

[Severity: High]
This isn't a bug introduced by this patch, but since you are modifying this
function, could the disable_irq() call earlier in otto_wdt_restart() cause a
deadlock during reboot?

Looking at the full otto_wdt_restart() function, it disables the interrupt
before triggering the hardware reset:

static int otto_wdt_restart(...)
{
    ...
    disable_irq(ctrl->irq_phase1);
    ...

Because otto_wdt_restart() acts as a system restart handler, it is executed
via machine_restart() in an atomic context with local interrupts disabled and
other CPUs halted.

Calling disable_irq() invokes synchronize_irq(), which waits for any
in-progress interrupt handler to complete. If the otto_wdt_phase1_isr handler
was running on another CPU that was just halted via smp_send_stop(),
synchronize_irq() will spin indefinitely, deadlocking the restart process.

Would it be safer to use disable_irq_nosync() here instead to avoid waiting?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519182329.24472-2-adilov@disroot.org?part=1

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

[Sander Vanheule]

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.


Best,
Sander

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

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

[Rustam Adilov]

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

Re: [PATCH v2 0/1] watchdog: realtek-otto: Make use of regmap API

[Rustam Adilov]

Hello,

I have noticed the status for this patch series is "Changes Requested"
in the patchwork site but the maintainer opposes the requested changes
by Sander in [1], so what do i do here? It is just one the roadblocks
to get USB stuff supported for Realtek chips.

I have already sent a reply week later in [2] but didn't get a response
back. Maybe this email could be served as a gentle remainder.

[1] - https://lore.kernel.org/linux-watchdog/1d5091ff-e497-4b5a-a906-7cc02c31c4c0@roeck-us.net/
[2] - https://lore.kernel.org/linux-watchdog/b4b09be5ff456a23d15a5af3237b9b46@disroot.org/

Best,
Rustam

Re: [PATCH v2 0/1] watchdog: realtek-otto: Make use of regmap API

[Guenter Roeck]

On 6/20/26 04:23, Rustam Adilov wrote:
> Hello,
> 
> I have noticed the status for this patch series is "Changes Requested"
> in the patchwork site but the maintainer opposes the requested changes
> by Sander in [1], so what do i do here? It is just one the roadblocks
> to get USB stuff supported for Realtek chips.
> 
> I have already sent a reply week later in [2] but didn't get a response
> back. Maybe this email could be served as a gentle remainder.
> 

I said:
	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.

Your response did not explain the benefits of adding the unnecessary error
handling. I did not see a rationale from Sander either.

On top of that, your patch did not explain the real reason for the patch,
as stated in your reply. This means that, for me, it was just a patch making
no functional changes for no good reason other than to potentially _increase_
code complexity by adding unnecessary error handling while at the same time
claiming that code complexity would be reduced.

Also, I do not recall even an attempt to address (or even comment on) the
actual problem with the driver as reported by Sashiko.

Guenter

> [1] - https://lore.kernel.org/linux-watchdog/1d5091ff-e497-4b5a-a906-7cc02c31c4c0@roeck-us.net/
> [2] - https://lore.kernel.org/linux-watchdog/b4b09be5ff456a23d15a5af3237b9b46@disroot.org/
> 
> Best,
> Rustam

Re: [PATCH v2 0/1] watchdog: realtek-otto: Make use of regmap API

[Sander Vanheule]

On Tue, 2026-06-23 at 11:44 -0700, Guenter Roeck wrote:
> On 6/20/26 04:23, Rustam Adilov wrote:
> > Hello,
> > 
> > I have noticed the status for this patch series is "Changes Requested"
> > in the patchwork site but the maintainer opposes the requested changes
> > by Sander in [1], so what do i do here? It is just one the roadblocks
> > to get USB stuff supported for Realtek chips.
> > 
> > I have already sent a reply week later in [2] but didn't get a response
> > back. Maybe this email could be served as a gentle remainder.
> > 
> 
> I said:
> 	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.
> 
> Your response did not explain the benefits of adding the unnecessary error
> handling. I did not see a rationale from Sander either.

I mainly brought it up because I have been asked elsewhere [1] to check the
regmap return codes. Although that was for a device on an MDIO bus, which I
suppose is more likely to actually generate errors.

[1] https://lore.kernel.org/linux-leds/CAMRc=Mdb7CWB9PmzXJyfvGjvG0iwuwUgfLuKJuMweRFvAhAoHg@mail.gmail.com/

> On top of that, your patch did not explain the real reason for the patch,
> as stated in your reply. This means that, for me, it was just a patch making
> no functional changes for no good reason other than to potentially _increase_
> code complexity by adding unnecessary error handling while at the same time
> claiming that code complexity would be reduced.

Given the reason is endianess issues, does the GPIO driver (gpio-realtek-otto.c)
using ioread32()/iowrite32() still work correctly? If you have the wrong
endianess there, you would only really see issues with the GPIO interrupt
handling.

If GPIO works correctly with CONFIG_SWAP_IO_SPACE enabled, then I suppose the
watchdog driver needs to be amended. Otherwise perhaps the USB peripheral driver
should be compensating for its endianess?

> Also, I do not recall even an attempt to address (or even comment on) the
> actual problem with the driver as reported by Sashiko.

I did have a look at this report [2], but I didn't manage to figure out if the
behavior I see matches the reasoning by the bot. Since I didn't get any
feedback, I left it as is.

[2] https://lore.kernel.org/linux-watchdog/d0b159eefa6bc5abf0d1531acde568396f480500.camel@svanheule.net/

Best,
Sander

Re: [PATCH v2 0/1] watchdog: realtek-otto: Make use of regmap API

[Rustam Adilov]

Hello,
On 2026-06-23 20:44, Sander Vanheule wrote:
> On Tue, 2026-06-23 at 11:44 -0700, Guenter Roeck wrote:
>> On 6/20/26 04:23, Rustam Adilov wrote:
>> > Hello,
>> > 
>> > I have noticed the status for this patch series is "Changes Requested"
>> > in the patchwork site but the maintainer opposes the requested changes
>> > by Sander in [1], so what do i do here? It is just one the roadblocks
>> > to get USB stuff supported for Realtek chips.
>> > 
>> > I have already sent a reply week later in [2] but didn't get a response
>> > back. Maybe this email could be served as a gentle remainder.
>> > 
>> 
>> I said:
>> 	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.
>> 
>> Your response did not explain the benefits of adding the unnecessary error
>> handling. I did not see a rationale from Sander either.
> 
> I mainly brought it up because I have been asked elsewhere [1] to check the
> regmap return codes. Although that was for a device on an MDIO bus, which I
> suppose is more likely to actually generate errors.
> 
> [1] https://lore.kernel.org/linux-leds/CAMRc=Mdb7CWB9PmzXJyfvGjvG0iwuwUgfLuKJuMweRFvAhAoHg@mail.gmail.com/

Thanks for clarification. I will take it as i don't need to add the error
handling.

As for why my response didn't explain it. I didn't want to speak for Sander so
i decided to just wait on it.

>> On top of that, your patch did not explain the real reason for the patch,
>> as stated in your reply. This means that, for me, it was just a patch making
>> no functional changes for no good reason other than to potentially _increase_
>> code complexity by adding unnecessary error handling while at the same time
>> claiming that code complexity would be reduced.

I can change the commit message to mention this main reason in the next patch
version.

> Given the reason is endianess issues, does the GPIO driver (gpio-realtek-otto.c)
> using ioread32()/iowrite32() still work correctly? If you have the wrong
> endianess there, you would only really see issues with the GPIO interrupt
> handling.
> 
> If GPIO works correctly with CONFIG_SWAP_IO_SPACE enabled, then I suppose the
> watchdog driver needs to be amended. Otherwise perhaps the USB peripheral driver
> should be compensating for its endianess?

Actually, it is other way around. GPIO works correctly when CONFIG_SWAP_IO_SPACE
is not enabled. When i do enable it, i need to patch the driver to make it work.
The dirty patch is here [1], which simply changes ioread32()/iowrite32() to
their __raw variants inside gpio_bank_read and gpio_bank_write the and what is
also important, the GPIO_GENERIC_BIG_ENDIAN_BYTE_ORDER flag needs to be set.
And also, i can't simply use the compatibles without GPIO_PORTS_REVERSED because
the realtek_gpio_line_imr_pos is required for correct functionality.

This patch obviously won't cut as it is going to break rtl9300 without SWAP_IO_SPACE.
Maybe we could make of gpio-regmap to handle swapping and stuff? I don't know of
any other elegant solutions to this problem.

[1] - https://github.com/jameywine/openwrt/blob/bb94712cb6faccf082c5a9fcebfabddf837a16bb/target/linux/realtek/patches-6.18/814-gpio-realtek-otto-change-read-write-functions.patch

>> Also, I do not recall even an attempt to address (or even comment on) the
>> actual problem with the driver as reported by Sashiko.
> 
> I did have a look at this report [2], but I didn't manage to figure out if the
> behavior I see matches the reasoning by the bot. Since I didn't get any
> feedback, I left it as is.
> 
> [2] https://lore.kernel.org/linux-watchdog/d0b159eefa6bc5abf0d1531acde568396f480500.camel@svanheule.net/

Oh yes, thanks for bumping it back. I'll leave it to Guenter Roeck to leave a
feedback as i am not even nearly qualified to understand it.

> Best,
> Sander

Best,
Rustam

Re: [PATCH v2 0/1] watchdog: realtek-otto: Make use of regmap API

[Sander Vanheule]

On Wed, 2026-07-01 at 18:48 +0000, Rustam Adilov wrote:
> Hello,
> On 2026-06-23 20:44, Sander Vanheule wrote:
> > Given the reason is endianess issues, does the GPIO driver (gpio-realtek-
> > otto.c)
> > using ioread32()/iowrite32() still work correctly? If you have the wrong
> > endianess there, you would only really see issues with the GPIO interrupt
> > handling.
> > 
> > If GPIO works correctly with CONFIG_SWAP_IO_SPACE enabled, then I suppose
> > the
> > watchdog driver needs to be amended. Otherwise perhaps the USB peripheral
> > driver
> > should be compensating for its endianess?
> 
> Actually, it is other way around. GPIO works correctly when
> CONFIG_SWAP_IO_SPACE
> is not enabled. When i do enable it, i need to patch the driver to make it
> work.
> The dirty patch is here [1], which simply changes ioread32()/iowrite32() to
> their __raw variants inside gpio_bank_read and gpio_bank_write the and what is
> also important, the GPIO_GENERIC_BIG_ENDIAN_BYTE_ORDER flag needs to be set.
> And also, i can't simply use the compatibles without GPIO_PORTS_REVERSED
> because
> the realtek_gpio_line_imr_pos is required for correct functionality.
> 
> This patch obviously won't cut as it is going to break rtl9300 without
> SWAP_IO_SPACE.
> Maybe we could make of gpio-regmap to handle swapping and stuff? I don't know
> of
> any other elegant solutions to this problem.
> 
> [1] -
> https://github.com/jameywine/openwrt/blob/bb94712cb6faccf082c5a9fcebfabddf837a16bb/target/linux/realtek/patches-6.18/814-gpio-realtek-otto-change-read-write-functions.patch

Looking at the full patch (*), I see you're not only modifying the GPIO,
watchdog and timer drivers, but also some very basic reset and pinctrl code to
have a functional system with SWAP_IO_SPACE. If SWAP_IO_SPACE requires you to
avoid byte-swapping in so many drivers, then you're just working around
SWAP_IO_SPACE. In other words, AFAICT it shouldn't be selected.

(*) https://github.com/jameywine/openwrt/commit/053aba620b12c6215e6760d8a613b4ebe05d452b

If there is a single new USB peripheral driver that only works with
SWAP_IO_SPACE, then I suggest you update that driver instead of enabling and
bypassing the effects of SWAP_IO_SPACE everywhere else.

Best,
Sander

Re: [PATCH v2 0/1] watchdog: realtek-otto: Make use of regmap API

[Rustam Adilov]

On 2026-07-04 14:00, Sander Vanheule wrote:
> On Wed, 2026-07-01 at 18:48 +0000, Rustam Adilov wrote:
>> Hello,
>> On 2026-06-23 20:44, Sander Vanheule wrote:
>> > Given the reason is endianess issues, does the GPIO driver (gpio-realtek-
>> > otto.c)
>> > using ioread32()/iowrite32() still work correctly? If you have the wrong
>> > endianess there, you would only really see issues with the GPIO interrupt
>> > handling.
>> > 
>> > If GPIO works correctly with CONFIG_SWAP_IO_SPACE enabled, then I suppose
>> > the
>> > watchdog driver needs to be amended. Otherwise perhaps the USB peripheral
>> > driver
>> > should be compensating for its endianess?
>> 
>> Actually, it is other way around. GPIO works correctly when
>> CONFIG_SWAP_IO_SPACE
>> is not enabled. When i do enable it, i need to patch the driver to make it
>> work.
>> The dirty patch is here [1], which simply changes ioread32()/iowrite32() to
>> their __raw variants inside gpio_bank_read and gpio_bank_write the and what is
>> also important, the GPIO_GENERIC_BIG_ENDIAN_BYTE_ORDER flag needs to be set.
>> And also, i can't simply use the compatibles without GPIO_PORTS_REVERSED
>> because
>> the realtek_gpio_line_imr_pos is required for correct functionality.
>> 
>> This patch obviously won't cut as it is going to break rtl9300 without
>> SWAP_IO_SPACE.
>> Maybe we could make of gpio-regmap to handle swapping and stuff? I don't know
>> of
>> any other elegant solutions to this problem.
>> 
>> [1] -
>> https://github.com/jameywine/openwrt/blob/bb94712cb6faccf082c5a9fcebfabddf837a16bb/target/linux/realtek/patches-6.18/814-gpio-realtek-otto-change-read-write-functions.patch
> 
> Looking at the full patch (*), I see you're not only modifying the GPIO,
> watchdog and timer drivers, but also some very basic reset and pinctrl code to
> have a functional system with SWAP_IO_SPACE. If SWAP_IO_SPACE requires you to
> avoid byte-swapping in so many drivers, then you're just working around
> SWAP_IO_SPACE. In other words, AFAICT it shouldn't be selected.
>
> (*) https://github.com/jameywine/openwrt/commit/053aba620b12c6215e6760d8a613b4ebe05d452b

I would say it is more of working around limitation of big endian MIPS with little
endian USB controller but more on that in the message below.

> If there is a single new USB peripheral driver that only works with
> SWAP_IO_SPACE, then I suggest you update that driver instead of enabling and
> bypassing the effects of SWAP_IO_SPACE everywhere else.

Sadly it is not much to do with a single USB driver (technically yes if you count
ohci-platform and ehci-platform) but everything to do with the reads and writes
functions in ehci.h and ohci.h files. If we don't select SWAP_IO_SPACE, we have to
basically change them to perform a byte swap as we can't use readl nor readl_be
functions for that. You can check how that would look like in [1] but that would
not be acceptable by upstream nor any other similar modifications to those header
files i don't think.

And that is a crux of the problem. That same can be seen with rtl930x and rtl931x
in [2]. SWAP_IO_SPACE as far as i can see, is the only solution to be upstreamable.

[1] https://github.com/openwrt/openwrt/commit/ba48e61e9da58b38d69fc58f90f9fcd93c11862b
[2] https://github.com/openwrt/openwrt/pull/23508

Best,
Rustam

Re: [PATCH v2 0/1] watchdog: realtek-otto: Make use of regmap API

[Sander Vanheule]

Hi Rustam,

On Sat, 2026-07-04 at 15:13 +0000, Rustam Adilov wrote:
> On 2026-07-04 14:00, Sander Vanheule wrote:
> > Looking at the full patch (*), I see you're not only modifying the GPIO,
> > watchdog and timer drivers, but also some very basic reset and pinctrl code
> > to
> > have a functional system with SWAP_IO_SPACE. If SWAP_IO_SPACE requires you
> > to
> > avoid byte-swapping in so many drivers, then you're just working around
> > SWAP_IO_SPACE. In other words, AFAICT it shouldn't be selected.
> > 
> > (*)
> > https://github.com/jameywine/openwrt/commit/053aba620b12c6215e6760d8a613b4ebe05d452b
> 
> I would say it is more of working around limitation of big endian MIPS with
> little
> endian USB controller but more on that in the message below.
> 
> > If there is a single new USB peripheral driver that only works with
> > SWAP_IO_SPACE, then I suggest you update that driver instead of enabling and
> > bypassing the effects of SWAP_IO_SPACE everywhere else.
> 
> Sadly it is not much to do with a single USB driver (technically yes if you
> count
> ohci-platform and ehci-platform) but everything to do with the reads and
> writes
> functions in ehci.h and ohci.h files. If we don't select SWAP_IO_SPACE, we
> have to
> basically change them to perform a byte swap as we can't use readl nor
> readl_be
> functions for that. You can check how that would look like in [1] but that
> would
> not be acceptable by upstream nor any other similar modifications to those
> header
> files i don't think.
> 
> And that is a crux of the problem. That same can be seen with rtl930x and
> rtl931x
> in [2]. SWAP_IO_SPACE as far as i can see, is the only solution to be
> upstreamable.
> 
> [1]
> https://github.com/openwrt/openwrt/commit/ba48e61e9da58b38d69fc58f90f9fcd93c11862b
> [2] https://github.com/openwrt/openwrt/pull/23508

Thanks for the extra details. From a quick glance, I have to say that the extra
OHCI/EHCI quirks from [1] don't look too offensive, but I'm not familiar with
the USB subsystem.

Those USB frameworks already have quirks for mixed-endian systems, so maybe the
issue is more that nobody has had the need to support your situation yet. If you
(or Carlo) are not sure how to clean that up, you can submit those changes as an
RFC to get some feedback without a full blown review. Maybe also involve linux-
mips then.

Best,
Sander