RE: [PATCH 2/3] power: sequencing: Add Renesas RZ/G3L Power Ready driver

Hi Bartosz Golaszewski,

Thanks for the feedback.

> -----Original Message-----
> From: Bartosz Golaszewski <[email protected]>
> Sent: 28 July 2026 09:45
> Subject: Re: [PATCH 2/3] power: sequencing: Add Renesas RZ/G3L Power Ready driver
> 
> On Sat, 25 Jul 2026 14:34:29 +0200, Biju <[email protected]> said:
> > From: Biju Das <[email protected]>
> >
> > Add a power sequencing driver for the Renesas RZ/G3L PWRRDY module,
> > which signals power readiness for various IPs (USB, DSI, CSI etc.) on
> > the SoC. The driver binds as an auxiliary device to the parent SYSC
> > driver, using its regmap to toggle the SYS_PWRRDY_N register bits, and
> > exposes {usb,dsi,csi}-pwrrdy pwrseq targets.
> >
> > Signed-off-by: Biju Das <[email protected]>
> > ---
> >  drivers/power/sequencing/Kconfig              |   8 +
> >  drivers/power/sequencing/Makefile             |   1 +
> >  .../power/sequencing/pwrseq-renesas-pwrrdy.c  | 141
> > ++++++++++++++++++
> >  3 files changed, 150 insertions(+)
> >  create mode 100644 drivers/power/sequencing/pwrseq-renesas-pwrrdy.c
> >
> > diff --git a/drivers/power/sequencing/Kconfig
> > b/drivers/power/sequencing/Kconfig
> > index 1c5f5820f5b7..245961cc8123 100644
> > --- a/drivers/power/sequencing/Kconfig
> > +++ b/drivers/power/sequencing/Kconfig
> > @@ -27,6 +27,14 @@ config POWER_SEQUENCING_QCOM_WCN
> >  	  this driver is needed for correct power control or else we'd risk not
> >  	  respecting the required delays between enabling Bluetooth and WLAN.
> >
> > +config POWER_SEQUENCING_RENESAS_PWRRDY
> > +	tristate "Renesas Power Ready sequencing driver"
> > +	depends on SYSC_RZ || COMPILE_TEST
> > +	help
> > +	  Say Y here to enable the power sequencing driver for the Renesas
> > +	  Power Ready signals. This driver handles the power ready signals
> > +	  required to power on the various IP's on RZ/G3L platform.
> > +
> >  config POWER_SEQUENCING_TH1520_GPU
> >  	tristate "T-HEAD TH1520 GPU power sequencing driver"
> >  	depends on (ARCH_THEAD && AUXILIARY_BUS) || COMPILE_TEST diff --git
> > a/drivers/power/sequencing/Makefile
> > b/drivers/power/sequencing/Makefile
> > index 0911d4618298..b33d08d82f43 100644
> > --- a/drivers/power/sequencing/Makefile
> > +++ b/drivers/power/sequencing/Makefile
> > @@ -4,5 +4,6 @@ obj-$(CONFIG_POWER_SEQUENCING)		+= pwrseq-core.o
> >  pwrseq-core-y				:= core.o
> >
> >  obj-$(CONFIG_POWER_SEQUENCING_QCOM_WCN)	+= pwrseq-qcom-wcn.o
> > +obj-$(CONFIG_POWER_SEQUENCING_RENESAS_PWRRDY) +=
> > +pwrseq-renesas-pwrrdy.o
> >  obj-$(CONFIG_POWER_SEQUENCING_TH1520_GPU) += pwrseq-thead-gpu.o
> >  obj-$(CONFIG_POWER_SEQUENCING_PCIE_M2)	+= pwrseq-pcie-m2.o
> > diff --git a/drivers/power/sequencing/pwrseq-renesas-pwrrdy.c
> > b/drivers/power/sequencing/pwrseq-renesas-pwrrdy.c
> > new file mode 100644
> > index 000000000000..a3d187dd3247
> > --- /dev/null
> > +++ b/drivers/power/sequencing/pwrseq-renesas-pwrrdy.c
> > @@ -0,0 +1,141 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Renesas RZ/G3L Power Ready driver
> > + *
> > + */
> > +
> > +#include <linux/auxiliary_bus.h>
> > +#include <linux/module.h>
> > +#include <linux/pwrseq/provider.h>
> > +#include <linux/regmap.h>
> > +
> > +#define SYS_PWRRDY_N		0xd70
> > +#define SYS_PWRRDY_N_USB_MASK	BIT(0)
> > +#define SYS_PWRRDY_N_DSI_MASK	BIT(1)
> > +#define SYS_PWRRDY_N_CSI_MASK	BIT(2)
> > +
> > +static int pwrseq_rzg3l_set_pwrrdy(struct pwrseq_device *pwrseq, u32
> > +mask, u32 val) {
> > +	struct regmap *regmap = pwrseq_device_get_drvdata(pwrseq);
> > +
> > +	return regmap_update_bits(regmap, SYS_PWRRDY_N, mask, val); }
> > +
> > +static int pwrseq_rzg3l_usb_pwrrdy_enable(struct pwrseq_device
> > +*pwrseq) {
> > +	return pwrseq_rzg3l_set_pwrrdy(pwrseq, SYS_PWRRDY_N_USB_MASK, 0); }
> > +
> > +static int pwrseq_rzg3l_usb_pwrrdy_disable(struct pwrseq_device
> > +*pwrseq) {
> > +	return pwrseq_rzg3l_set_pwrrdy(pwrseq, SYS_PWRRDY_N_USB_MASK, 1); }
> > +
> > +static const struct pwrseq_unit_data pwrseq_rzg3l_usb_pwrrdy_unit = {
> > +	.name = "usb-pwrrdy-power-sequence",
> > +	.enable = pwrseq_rzg3l_usb_pwrrdy_enable,
> > +	.disable = pwrseq_rzg3l_usb_pwrrdy_disable, };
> > +
> > +static int pwrseq_rzg3l_dsi_pwrrdy_enable(struct pwrseq_device
> > +*pwrseq) {
> > +	return pwrseq_rzg3l_set_pwrrdy(pwrseq, SYS_PWRRDY_N_DSI_MASK, 0); }
> > +
> > +static int pwrseq_rzg3l_dsi_pwrrdy_disable(struct pwrseq_device
> > +*pwrseq) {
> > +	return pwrseq_rzg3l_set_pwrrdy(pwrseq, SYS_PWRRDY_N_DSI_MASK, 1); }
> > +
> > +static const struct pwrseq_unit_data pwrseq_rzg3l_dsi_pwrrdy_unit = {
> > +	.name = "dsi-pwrrdy-sequence",
> > +	.enable = pwrseq_rzg3l_dsi_pwrrdy_enable,
> > +	.disable = pwrseq_rzg3l_dsi_pwrrdy_disable, };
> > +
> > +static int pwrseq_rzg3l_csi_pwrrdy_enable(struct pwrseq_device
> > +*pwrseq) {
> > +	return pwrseq_rzg3l_set_pwrrdy(pwrseq, SYS_PWRRDY_N_CSI_MASK, 0); }
> > +
> > +static int pwrseq_rzg3l_csi_pwrrdy_disable(struct pwrseq_device
> > +*pwrseq) {
> > +	return pwrseq_rzg3l_set_pwrrdy(pwrseq, SYS_PWRRDY_N_CSI_MASK, 1); }
> > +
> > +static const struct pwrseq_unit_data pwrseq_rzg3l_csi_pwrrdy_unit = {
> > +	.name = "csi-pwrrdy-power-sequence",
> > +	.enable = pwrseq_rzg3l_csi_pwrrdy_enable,
> > +	.disable = pwrseq_rzg3l_csi_pwrrdy_disable, };
> > +
> > +static const struct pwrseq_target_data pwrseq_rzg3l_usb_pwrrdy_target = {
> > +	.name = "usb-pwrrdy",
> > +	.unit = &pwrseq_rzg3l_usb_pwrrdy_unit, };
> > +
> > +static const struct pwrseq_target_data pwrseq_rzg3l_dsi_pwrrdy_target = {
> > +	.name = "dsi-pwrrdy",
> > +	.unit = &pwrseq_rzg3l_dsi_pwrrdy_unit, };
> > +
> > +static const struct pwrseq_target_data pwrseq_rzg3l_csi_pwrrdy_target = {
> > +	.name = "csi-pwrrdy",
> > +	.unit = &pwrseq_rzg3l_csi_pwrrdy_unit, };
> > +
> > +static const struct pwrseq_target_data *pwrseq_rzg3l_pwrrdy_targets[] = {
> > +	&pwrseq_rzg3l_usb_pwrrdy_target,
> > +	&pwrseq_rzg3l_dsi_pwrrdy_target,
> > +	&pwrseq_rzg3l_csi_pwrrdy_target,
> > +	NULL
> > +};
> > +
> > +static int pwrseq_rzg3l_pwrrdy_match(struct pwrseq_device *pwrseq,
> > +				     struct device *dev)
> > +{
> > +	return PWRSEQ_MATCH_OK;
> 
> When I see an always-tru match() callback, it always raises an alarm bell.
> Typically, I'd expect there to be some validation of the consumer happening.
> 
> Please at least provide an explanation of why it's ok.

Without the match function the driver is not probing.

sysc(driver) ->powerseq(driver) ->{DSI,CSI,USB}(driver)
there is no property that I can match here as the resource is a register on
the SYSC IP and powerseq driver is instantiated by the SYSC driver.

Maybe I can provide a comment here saying that??
"No DT property to match here, so it is OK always"


> 
> > +}
> > +
> > +static int pwrseq_rzg3l_pwrrdy_probe(struct auxiliary_device *adev,
> > +				     const struct auxiliary_device_id *id) {
> > +	struct device *dev = &adev->dev;
> > +	struct pwrseq_config config = {};
> > +	struct pwrseq_device *pwrseq;
> > +	struct regmap *regmap;
> > +
> > +	regmap = dev_get_regmap(adev->dev.parent, NULL);
> > +	if (!regmap)
> > +		return dev_err_probe(dev, -ENODEV, "Failed to retrieve parent
> > +regmap\n");

I will remove this in next version as regmap can be passed from

devm_auxiliary_device_create() and

config.drvdata = adev->dev.platform_data;

> > +
> > +	config.parent = dev;
> > +	config.owner = THIS_MODULE;
> > +	config.drvdata = regmap;
> > +	config.match = pwrseq_rzg3l_pwrrdy_match;
> > +	config.targets = pwrseq_rzg3l_pwrrdy_targets;
> 
> Add newline here.

OK.

> 
> It wouldn't also hurt to use a compound literal like so:
> 
> 	config = (struct pwrseq_config){
> 		.parent = dev,
> 		...
> 	};
> 

Agreed.

> > +	pwrseq = devm_pwrseq_device_register(dev, &config);
> > +	if (IS_ERR(pwrseq))
> > +		return dev_err_probe(dev, PTR_ERR(pwrseq), "Failed to register
> > +power sequencer\n");
> > +
> 
> I'd just return devm_pwrseq_device_register() here.


OK.

Cheers,
Biju