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

[Philipp Zabel]

On Mo, 2026-07-20 at 13:15 +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            |  13 +++
>  drivers/reset/Makefile           |   1 +
>  drivers/reset/reset-sky1-audss.c | 137 +++++++++++++++++++++++++++++++
>  3 files changed, 151 insertions(+)
>  create mode 100644 drivers/reset/reset-sky1-audss.c
> 
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index d009eb0849a3..b19e719f2abe 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -300,6 +300,19 @@ 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
> +	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..d31d80e1251a
> --- /dev/null
> +++ b/drivers/reset/reset-sky1-audss.c
> @@ -0,0 +1,137 @@
> +// 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/module.h>
> +#include <linux/of.h>

What is this used for?

> +#include <linux/regmap.h>
> +#include <linux/reset-controller.h>
> +
> +#define SKY1_RESET_SLEEP_MIN_US		50
> +#define SKY1_RESET_SLEEP_MAX_US		100

Is this delay compatible with all possible modules being reset?

Are min and max from a spec or is this just some safe value?
Maybe you could only define the _MIN_US and use fsleep() below.

> +
> +#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);

Consider using regmap_assign_bits() instead.

> +}
> +
> +static int sky1_audss_reset_assert(struct reset_controller_dev *rcdev,
> +				   unsigned long id)
> +{
> +	int ret;
> +
> +	ret = sky1_audss_reset_set(rcdev, id, true);
> +	if (ret)
> +		return ret;
> +
> +	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)
> +{
> +	int ret;
> +
> +	ret = sky1_audss_reset_set(rcdev, id, false);
> +	if (ret)
> +		return ret;
> +
> +	usleep_range(SKY1_RESET_SLEEP_MIN_US, SKY1_RESET_SLEEP_MAX_US);

This is the same in both assert and deassert. You could move it into
the helper function.

regards
Philipp