Re: [PATCH v6 3/4] reset: cix: add sky1 audss auxiliary reset driver

[Philipp Zabel]

On Di, 2026-06-23 at 15:08 +0800, joakim.zhang@xxxxxxxxxxx wrote:
> From: Joakim Zhang <joakim.zhang@xxxxxxxxxxx>
> 
> Add an auxiliary reset controller driver for the AUDSS CRU. Sixteen
> software reset lines for audio subsystem peripherals are controlled
> through one register in the CRU register map.
> 
> The driver is created by the AUDSS clock platform driver and registers
> the reset controller on the CRU device node.
> 
> Signed-off-by: Joakim Zhang <joakim.zhang@xxxxxxxxxxx>
> ---
>  drivers/reset/Kconfig            |  14 +++
>  drivers/reset/Makefile           |   1 +
>  drivers/reset/reset-sky1-audss.c | 192 +++++++++++++++++++++++++++++++
>  3 files changed, 207 insertions(+)
>  create mode 100644 drivers/reset/reset-sky1-audss.c
> 
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index d009eb0849a3..f74859b292ae 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -300,6 +300,20 @@ config RESET_SKY1
>  	help
>  	  This enables the reset controller for Cix Sky1.
>  
> +config RESET_SKY1_AUDSS
> +	tristate "Cix Sky1 Audio Subsystem reset controller"
> +	depends on ARCH_CIX || COMPILE_TEST
> +	select AUXILIARY_BUS
> +	select REGMAP_MMIO
> +	default CLK_SKY1_AUDSS
> +	help
> +	  Support for block-level software reset lines in the Cix Sky1
> +	  Audio Subsystem (AUDSS) Clock and Reset Unit. Sixteen reset
> +	  outputs for audio peripherals are controlled through the CRU
> +	  register map. The driver binds as an auxiliary device from
> +	  the AUDSS clock driver. Say M or Y here if you want to build
> +	  this driver.
> +
>  config RESET_SOCFPGA
>  	bool "SoCFPGA Reset Driver" if COMPILE_TEST && (!ARM || !ARCH_INTEL_SOCFPGA)
>  	default ARM && ARCH_INTEL_SOCFPGA
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 3e52569bd276..e81407ea3e29 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -39,6 +39,7 @@ obj-$(CONFIG_RESET_RZV2H_USB2PHY) += reset-rzv2h-usb2phy.o
>  obj-$(CONFIG_RESET_SCMI) += reset-scmi.o
>  obj-$(CONFIG_RESET_SIMPLE) += reset-simple.o
>  obj-$(CONFIG_RESET_SKY1) += reset-sky1.o
> +obj-$(CONFIG_RESET_SKY1_AUDSS) += reset-sky1-audss.o
>  obj-$(CONFIG_RESET_SOCFPGA) += reset-socfpga.o
>  obj-$(CONFIG_RESET_SUNPLUS) += reset-sunplus.o
>  obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o
> diff --git a/drivers/reset/reset-sky1-audss.c b/drivers/reset/reset-sky1-audss.c
> new file mode 100644
> index 000000000000..20870f37d7d7
> --- /dev/null
> +++ b/drivers/reset/reset-sky1-audss.c
> @@ -0,0 +1,192 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Cix Sky1 Audio Subsystem reset controller driver
> + *
> + * Copyright 2026 Cix Technology Group Co., Ltd.
> + */
> +
> +#include <dt-bindings/reset/cix,sky1-audss-cru.h>
> +
> +#include <linux/auxiliary_bus.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/regmap.h>
> +#include <linux/reset-controller.h>
> +
> +#define SKY1_RESET_SLEEP_MIN_US		50
> +#define SKY1_RESET_SLEEP_MAX_US		100
> +
> +#define AUDSS_SW_RST			0x78
> +
> +struct sky1_audss_reset_map {
> +	unsigned int offset;
> +	unsigned int mask;
> +};
> +
> +struct sky1_audss_reset {
> +	struct reset_controller_dev rcdev;
> +	struct regmap *regmap;
> +	const struct sky1_audss_reset_map *map;
> +};
> +
> +static const struct sky1_audss_reset_map sky1_audss_reset_map[] = {
> +	[AUDSS_I2S0_SW_RST]   = { AUDSS_SW_RST, BIT(0) },
> +	[AUDSS_I2S1_SW_RST]   = { AUDSS_SW_RST, BIT(1) },
> +	[AUDSS_I2S2_SW_RST]   = { AUDSS_SW_RST, BIT(2) },
> +	[AUDSS_I2S3_SW_RST]   = { AUDSS_SW_RST, BIT(3) },
> +	[AUDSS_I2S4_SW_RST]   = { AUDSS_SW_RST, BIT(4) },
> +	[AUDSS_I2S5_SW_RST]   = { AUDSS_SW_RST, BIT(5) },
> +	[AUDSS_I2S6_SW_RST]   = { AUDSS_SW_RST, BIT(6) },
> +	[AUDSS_I2S7_SW_RST]   = { AUDSS_SW_RST, BIT(7) },
> +	[AUDSS_I2S8_SW_RST]   = { AUDSS_SW_RST, BIT(8) },
> +	[AUDSS_I2S9_SW_RST]   = { AUDSS_SW_RST, BIT(9) },
> +	[AUDSS_WDT_SW_RST]    = { AUDSS_SW_RST, BIT(10) },
> +	[AUDSS_TIMER_SW_RST]  = { AUDSS_SW_RST, BIT(11) },
> +	[AUDSS_MB0_SW_RST]    = { AUDSS_SW_RST, BIT(12) },
> +	[AUDSS_MB1_SW_RST]    = { AUDSS_SW_RST, BIT(13) },
> +	[AUDSS_HDA_SW_RST]    = { AUDSS_SW_RST, BIT(14) },
> +	[AUDSS_DMAC_SW_RST]   = { AUDSS_SW_RST, BIT(15) },
> +};
> +
> +static struct sky1_audss_reset *to_sky1_audss_reset(struct reset_controller_dev *rcdev)
> +{
> +	return container_of(rcdev, struct sky1_audss_reset, rcdev);
> +}
> +
> +static int sky1_audss_reset_set(struct reset_controller_dev *rcdev,
> +				unsigned long id, bool assert)
> +{
> +	struct sky1_audss_reset *priv = to_sky1_audss_reset(rcdev);
> +	const struct sky1_audss_reset_map *signal = &priv->map[id];
> +	unsigned int value = assert ? 0 : signal->mask;
> +
> +	return regmap_update_bits(priv->regmap, signal->offset, signal->mask, value);

Why does this propagate the return value ...

> +}
> +
> +static int sky1_audss_reset_assert(struct reset_controller_dev *rcdev,
> +				   unsigned long id)
> +{
> +	sky1_audss_reset_set(rcdev, id, true);

... only to be ignored? It'd be better to pass it on.

> +	usleep_range(SKY1_RESET_SLEEP_MIN_US, SKY1_RESET_SLEEP_MAX_US);
> +	return 0;
> +}
> +
> +static int sky1_audss_reset_deassert(struct reset_controller_dev *rcdev,
> +				     unsigned long id)
> +{
> +	sky1_audss_reset_set(rcdev, id, false);
> +	usleep_range(SKY1_RESET_SLEEP_MIN_US, SKY1_RESET_SLEEP_MAX_US);
> +	return 0;
> +}
> +
> +static int sky1_audss_reset(struct reset_controller_dev *rcdev,
> +			    unsigned long id)
> +{
> +	sky1_audss_reset_assert(rcdev, id);
> +	sky1_audss_reset_deassert(rcdev, id);
> +	return 0;
> +}

Will any AUDSS reset consumer use the reset_control_reset() API?
If not, no need to implement this.

> +
> +static int sky1_audss_reset_status(struct reset_controller_dev *rcdev,
> +				   unsigned long id)
> +{
> +	struct sky1_audss_reset *priv = to_sky1_audss_reset(rcdev);
> +	const struct sky1_audss_reset_map *signal = &priv->map[id];
> +	unsigned int value;
> +
> +	regmap_read(priv->regmap, signal->offset, &value);
> +	return !!(value & signal->mask);
> +}
> +
> +static const struct reset_control_ops sky1_audss_reset_ops = {
> +	.reset    = sky1_audss_reset,
> +	.assert   = sky1_audss_reset_assert,
> +	.deassert = sky1_audss_reset_deassert,
> +	.status   = sky1_audss_reset_status,
> +};
> +
> +static const struct regmap_config sky1_audss_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +};
> +
> +static void sky1_audss_reset_iounmap(void *data)
> +{
> +	iounmap(data);
> +}
> +
> +static int sky1_audss_reset_get_regmap(struct sky1_audss_reset *priv)
> +{
> +	struct device *dev = priv->rcdev.dev;
> +	void __iomem *base;
> +	int ret;
> +
> +	priv->regmap = dev_get_regmap(dev->parent, NULL);
> +	if (priv->regmap)
> +		return 0;
> +
> +	base = of_iomap(dev->parent->of_node, 0);
> +	if (!base)
> +		return dev_err_probe(dev, -ENOMEM, "failed to iomap address space\n");
> +
> +	ret = devm_add_action_or_reset(dev, sky1_audss_reset_iounmap, base);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register iounmap action\n");
> +
> +	priv->regmap = devm_regmap_init_mmio(dev, base, &sky1_audss_regmap_config);
> +	if (IS_ERR(priv->regmap))
> +		return dev_err_probe(dev, PTR_ERR(priv->regmap),
> +				     "failed to initialize regmap\n");

Why is there a fallback path? The clock driver creates the regmap
before creating the reset aux device, so dev_get_regmap() can never
fail.

> +
> +	return 0;
> +}
> +
> +static int sky1_audss_reset_probe(struct auxiliary_device *adev,
> +				  const struct auxiliary_device_id *id)
> +{
> +	struct sky1_audss_reset *priv;
> +	struct device *dev = &adev->dev;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->map = sky1_audss_reset_map;
> +	priv->rcdev.owner = THIS_MODULE;
> +	priv->rcdev.nr_resets = ARRAY_SIZE(sky1_audss_reset_map);
> +	priv->rcdev.ops = &sky1_audss_reset_ops;
> +	priv->rcdev.of_node = dev->parent->of_node;

auxiliary_device_create() uses device_set_of_node_from_dev() to inherit
the parent of_node, so you can use dev->of_node here.

> +	priv->rcdev.dev = dev;
> +	priv->rcdev.of_reset_n_cells = 1;

No need to set of_reset_n_cells.

> +
> +	dev_set_drvdata(dev, priv);

This seems unnecessary as well.

> +
> +	ret = sky1_audss_reset_get_regmap(priv);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to get regmap\n");
> +
> +	return devm_reset_controller_register(dev, &priv->rcdev);
> +}
> +
> +static const struct auxiliary_device_id sky1_audss_reset_ids[] = {
> +	{ .name = "clk_sky1_audss.reset" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(auxiliary, sky1_audss_reset_ids);
> +
> +static struct auxiliary_driver sky1_audss_reset_driver = {
> +	.probe = sky1_audss_reset_probe,
> +	.id_table = sky1_audss_reset_ids,
> +};
> +

Drop this empty line.

> +module_auxiliary_driver(sky1_audss_reset_driver);
> +
> +MODULE_AUTHOR("Joakim Zhang <joakim.zhang@xxxxxxxxxxx>");
> +MODULE_DESCRIPTION("Cix Sky1 Audio Subsystem reset driver");
> +MODULE_LICENSE("GPL");

regards
Philipp