[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

[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