RE: [PATCH v9 04/12] reset: realtek: Add RTD1625-ISO reset controller driver

[Yu-Chun Lin [林祐君]]

Hi Philipp,

> On Mi, 2026-06-24 at 19:29 +0800, Yu-Chun Lin wrote:
> > From: Cheng-Yu Lee <cylee12@xxxxxxxxxxx>
> >
> > Add support for the ISO (Isolation) domain reset controller on the
> > Realtek
> > RTD1625 SoC.
> >
> > The reset controller shares the same register space with the ISO clock
> > controller. To handle this shared register space, the reset driver is
> > implemented as an auxiliary driver. It will be instantiated and probed
> > via the auxiliary bus by the RTD1625-ISO clock controller driver.
> >
> > Signed-off-by: Cheng-Yu Lee <cylee12@xxxxxxxxxxx>
> > Co-developed-by: Yu-Chun Lin <eleanor.lin@xxxxxxxxxxx>
> > Signed-off-by: Yu-Chun Lin <eleanor.lin@xxxxxxxxxxx>
> > ---
> > Changes in v9:
> > - Extract reset-related code from the previous clock driver patch
> > (formerly patch 9 in v8).
> > ---
> >  drivers/reset/realtek/Makefile            |  2 +-
> >  drivers/reset/realtek/reset-rtd1625-iso.c | 99
> > +++++++++++++++++++++++
> >  2 files changed, 100 insertions(+), 1 deletion(-)  create mode 100644
> > drivers/reset/realtek/reset-rtd1625-iso.c
> >
> > diff --git a/drivers/reset/realtek/Makefile
> > b/drivers/reset/realtek/Makefile index c3f605ffb11c..9007c9d5683b
> > 100644
> > --- a/drivers/reset/realtek/Makefile
> > +++ b/drivers/reset/realtek/Makefile
> > @@ -1,3 +1,3 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> >  obj-$(CONFIG_RESET_RTK_COMMON) += reset-rtk-common.o
> > -obj-$(CONFIG_RESET_RTD1625) += reset-rtd1625-crt.o
> > +obj-$(CONFIG_RESET_RTD1625) += reset-rtd1625-crt.o
> > +reset-rtd1625-iso.o
> 
> Is there any benefit to these two being separate modules?
> I suggest you merge them into one: reset-rtd1625.o
> 

If I merge them into a single 'reset-rtd1625' module,
both the 'crt' and 'iso' clock drivers would trigger the probe
process for the same reset driver name, which would lead to a
duplicate driver registration error.

Therefore, I would prefer to keep them separate.

> > diff --git a/drivers/reset/realtek/reset-rtd1625-iso.c
> > b/drivers/reset/realtek/reset-rtd1625-iso.c
> > new file mode 100644
> > index 000000000000..78eaabb408f0
> > --- /dev/null
> > +++ b/drivers/reset/realtek/reset-rtd1625-iso.c
> > @@ -0,0 +1,99 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2026 Realtek Semiconductor Corporation  */
> > +
> > +#include <dt-bindings/reset/realtek,rtd1625.h>
> > +#include <linux/auxiliary_bus.h>
> > +#include <linux/device.h>
> > +#include <linux/errno.h>
> > +#include <linux/of.h>
> > +#include <linux/slab.h>
> > +#include "reset-rtk-common.h"
> > +
> > +#define RTD1625_ISO_RSTN_MAX 29
> > +#define RTD1625_ISO_S_RSTN_MAX       5
> 
> These are not necessary, just use ARRAY_SIZE() for nr_resets.
> 

Ack.

> > +

[...]

> > +
> > +static int rtd1625_iso_reset_probe(struct auxiliary_device *adev,
> > +                                const struct auxiliary_device_id *id)
> > +{
> > +     struct device *dev = &adev->dev;
> > +     struct device *parent = dev->parent;
> > +     struct rtk_reset_data *data;
> > +
> > +     data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > +     if (!data)
> > +             return -ENOMEM;
> > +
> > +     if (of_device_is_compatible(parent->of_node,
> "realtek,rtd1625-iso-s-clk")) {
> > +             data->descs           = rtd1625_iso_s_reset_descs;
> > +             data->rcdev.nr_resets = RTD1625_ISO_S_RSTN_MAX;
> > +     } else {
> > +             data->descs           = rtd1625_iso_reset_descs;
> > +             data->rcdev.nr_resets = RTD1625_ISO_RSTN_MAX;
> > +     }
> 
> No need to parse OF compatible again. Store these in a struct, point
> auxiliary_device_id::driver_data to it, and use that here.
> 
> regards
> Philipp

Agreed, I will do it in v10. Thanks.

Best Regards,
Yu-Chun