Re: [PATCH v9 5/6] reset: rzv2h-usb2phy: Convert to regmap API

[Philipp Zabel <p.zabel@pengutronix.de>]

On Fr, 2026-03-27 at 19:08 +0100, Tommaso Merciai wrote:
> Replace raw MMIO accesses (void __iomem *, readl/writel) with
> regmap_read/regmap_write via devm_regmap_init_mmio(). Regmap
> provides its own internal locking, so the manual spinlock and
> scoped_guard() wrappers are no longer needed.
> 
> Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
> ---
> v8->v9:
>  - New patch
> 
>  drivers/reset/Kconfig               |  1 +
>  drivers/reset/reset-rzv2h-usb2phy.c | 42 ++++++++++++++++-------------
>  2 files changed, 24 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 5165006be693..c539ca88518f 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -257,6 +257,7 @@ config RESET_RZG2L_USBPHY_CTRL
>  config RESET_RZV2H_USB2PHY
>  	tristate "Renesas RZ/V2H(P) (and similar SoCs) USB2PHY Reset driver"
>  	depends on ARCH_RENESAS || COMPILE_TEST
> +	select REGMAP_MMIO
>  	help
>  	  Support for USB2PHY Port reset Control found on the RZ/V2H(P) SoC
>  	  (and similar SoCs).
> diff --git a/drivers/reset/reset-rzv2h-usb2phy.c b/drivers/reset/reset-rzv2h-usb2phy.c
> index 5bdd39274612..4014eff0f017 100644
> --- a/drivers/reset/reset-rzv2h-usb2phy.c
> +++ b/drivers/reset/reset-rzv2h-usb2phy.c
> @@ -5,13 +5,13 @@
>   * Copyright (C) 2025 Renesas Electronics Corporation
>   */
>  
> -#include <linux/cleanup.h>
>  #include <linux/delay.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
>  #include <linux/reset.h>
>  #include <linux/reset-controller.h>
>  
> @@ -37,10 +37,9 @@ struct rzv2h_usb2phy_reset_of_data {
>  
>  struct rzv2h_usb2phy_reset_priv {
>  	const struct rzv2h_usb2phy_reset_of_data *data;
> -	void __iomem *base;
> +	struct regmap *regmap;
>  	struct device *dev;
>  	struct reset_controller_dev rcdev;
> -	spinlock_t lock; /* protects register accesses */
>  };
>  
>  static inline struct rzv2h_usb2phy_reset_priv
> @@ -55,10 +54,8 @@ static int rzv2h_usbphy_reset_assert(struct reset_controller_dev *rcdev,
>  	struct rzv2h_usb2phy_reset_priv *priv = rzv2h_usbphy_rcdev_to_priv(rcdev);
>  	const struct rzv2h_usb2phy_reset_of_data *data = priv->data;
>  
> -	scoped_guard(spinlock, &priv->lock) {
> -		writel(data->reset2_acquire_val, priv->base + data->reset2_reg);
> -		writel(data->reset_assert_val, priv->base + data->reset_reg);
> -	}
> +	regmap_write(priv->regmap, data->reset2_reg, data->reset2_acquire_val);
> +	regmap_write(priv->regmap, data->reset_reg, data->reset_assert_val);

What is the spinlock protecting? acquire/assert registers being set
together, without another acquire/assert or deassert/release register
access pair interleaving?
In that case you still need the lock. Or use regmap_multi_reg_write().
You could even directly store the sequences as struct reg_sequence in
rzv2h_usb2phy_reset_of_data.

>  	usleep_range(11, 20);
>  
> @@ -71,11 +68,9 @@ static int rzv2h_usbphy_reset_deassert(struct reset_controller_dev *rcdev,
>  	struct rzv2h_usb2phy_reset_priv *priv = rzv2h_usbphy_rcdev_to_priv(rcdev);
>  	const struct rzv2h_usb2phy_reset_of_data *data = priv->data;
>  
> -	scoped_guard(spinlock, &priv->lock) {
> -		writel(data->reset_deassert_val, priv->base + data->reset_reg);
> -		writel(data->reset2_release_val, priv->base + data->reset2_reg);
> -		writel(data->reset_release_val, priv->base + data->reset_reg);
> -	}
> +	regmap_write(priv->regmap, data->reset_reg, data->reset_deassert_val);
> +	regmap_write(priv->regmap, data->reset2_reg, data->reset2_release_val);
> +	regmap_write(priv->regmap, data->reset_reg, data->reset_release_val);

Same as above.

[...]

> @@ -149,7 +153,7 @@ static int rzv2h_usb2phy_reset_probe(struct platform_device *pdev)
>  		return dev_err_probe(dev, error, "unable to register cleanup action\n");
>  
>  	for (unsigned int i = 0; i < data->init_val_count; i++)
> -		writel(data->init_vals[i].val, priv->base + data->init_vals[i].reg);
> +		regmap_write(priv->regmap, data->init_vals[i].reg, data->init_vals[i].val);

Not required for locking, but this could use regmap_multi_reg_write()
as well.

regards
Philipp