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

[Bartosz Golaszewski]

On Sat, 25 Jul 2026 14:34:29 +0200, Biju <biju.das.au@xxxxxxxxx> said:
> From: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
>
> 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 <biju.das.jz@xxxxxxxxxxxxxx>
> ---
>  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.

> +}
> +
> +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");
> +
> +	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.

It wouldn't also hurt to use a compound literal like so:

	config = (struct pwrseq_config){
		.parent = dev,
		...
	};

> +	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.

> +	return 0;
> +}
> +
> +static const struct auxiliary_device_id pwrseq_rzg3l_pwrrdy_id_table[] = {
> +	{ .name = "rz_sysc.pwrseq-pwrrdy" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(auxiliary, pwrseq_rzg3l_pwrrdy_id_table);
> +
> +static struct auxiliary_driver pwrseq_rzg3l_pwrrdy_driver = {
> +	.driver = {
> +		.name = "pwrseq-rzg3l-pwrrdy",
> +	},
> +	.probe = pwrseq_rzg3l_pwrrdy_probe,
> +	.id_table = pwrseq_rzg3l_pwrrdy_id_table,
> +};
> +module_auxiliary_driver(pwrseq_rzg3l_pwrrdy_driver);
> +
> +MODULE_AUTHOR("Biju Das <biju.das.jz@xxxxxxxxxxxxxx>");
> +MODULE_DESCRIPTION("Renesas RZ/G3L Power Ready Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.43.0
>
>