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

[Joakim Zhang]

Hello, Philipp

> -----Original Message-----
> From: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
> Sent: Wednesday, June 24, 2026 4:30 PM
> To: Joakim Zhang <joakim.zhang@xxxxxxxxxxx>; mturquette@xxxxxxxxxxxx;
> sboyd@xxxxxxxxxx; bmasney@xxxxxxxxxx; robh@xxxxxxxxxx;
> krzk+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx; Gary Yang
> <gary.yang@xxxxxxxxxxx>
> Cc: cix-kernel-upstream <cix-kernel-upstream@xxxxxxxxxxx>; linux-
> clk@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v6 3/4] reset: cix: add sky1 audss auxiliary reset driver
> 
> EXTERNAL EMAIL
> 
> CAUTION: Suspicious Email from unusual domain.
> 
> 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 ...
I'll propagate the return value in the ops callbacks.


> > +}
> > +
> > +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.
Yes, will add.


> > +     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.
Will remove both .reset and .status.


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


> > +
> > +     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.
Done. rcdev.of_node now uses dev->of_node.


> > +     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.
Removed dev_set_drvdata() and the extra blank line.

Thanks,
Joakim