Re: [PATCH v7 2/2] mfd: ac200: Add X-Powers AC200 support

[James Hilliard]

On Wed, Sep 2, 2026 at 8:31 AM Lee Jones <lee@xxxxxxxxxx> wrote:
>
> On Tue, 11 Aug 2026, James Hilliard wrote:
>
> > The X-Powers AC200 is a mixed-signal companion IC with a paged register
> > map accessed over I2C.
> >
> > Enable the shared input clock and prevent its rate from changing. Match
> > the vendor driver's 40 ms wait before the first register access,
> > initialize the paged regmap, report the chip and package revision, and
> > apply common reset.
> >
> > The Ethernet PHY link endpoint is independently enumerated on its MDIO
> > bus rather than created as an MFD platform child. Keep the regmap attached
> > to the AC200 I2C device so a separately enumerated function can resolve
> > that device, establish its required lifetime relationship and retrieve
> > the regmap from the provider.
> >
> > Cache only the common page selector. Individual functions can reset
> > independently and invalidate their other registers without regmap's
> > knowledge, so leave all functional registers volatile.
> >
> > Reset the chip during managed teardown and system shutdown.
> >
> > Signed-off-by: James Hilliard <james.hilliard1@xxxxxxxxx>
> > ---
> >  drivers/mfd/Kconfig  |  11 ++++
> >  drivers/mfd/Makefile |   1 +
> >  drivers/mfd/ac200.c  | 163 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 175 insertions(+)
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index e4fd4572472f..cac3fff5aee9 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -205,6 +205,17 @@ config MFD_AC100
> >         This driver include only the core APIs. You have to select individual
> >         components like codecs or RTC under the corresponding menus.
> >
> > +config MFD_AC200
> > +     tristate "X-Powers AC200"
> > +     depends on I2C
> > +     depends on OF
> > +     select REGMAP_I2C
> > +     help
> > +       Support for the X-Powers AC200 mixed-signal companion IC. The AC200
> > +       contains audio, video, RTC and Fast Ethernet PHY functions and is
> > +       co-packaged with some Allwinner H6 and H616 SoCs. This driver provides
> > +       the shared register access used by the individual function drivers.
> > +
> >  config MFD_AXP20X
> >       tristate
> >       select MFD_CORE
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 72d3944b0ad8..f8101d2a9ce9 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -150,6 +150,7 @@ obj-$(CONFIG_MFD_DA9052_SPI)      += da9052-spi.o
> >  obj-$(CONFIG_MFD_DA9052_I2C) += da9052-i2c.o
> >
> >  obj-$(CONFIG_MFD_AC100)              += ac100.o
> > +obj-$(CONFIG_MFD_AC200)              += ac200.o
> >  obj-$(CONFIG_MFD_AXP20X)     += axp20x.o
> >  obj-$(CONFIG_MFD_AXP20X_I2C) += axp20x-i2c.o
> >  obj-$(CONFIG_MFD_AXP20X_RSB) += axp20x-rsb.o
> > diff --git a/drivers/mfd/ac200.c b/drivers/mfd/ac200.c
> > new file mode 100644
> > index 000000000000..0964e637afef
> > --- /dev/null
> > +++ b/drivers/mfd/ac200.c
> > @@ -0,0 +1,163 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * MFD core driver for the X-Powers AC200
> > + *
> > + * Copyright (C) 2019 Jernej Skrabec <jernej.skrabec@xxxxxxxxx>
> > + * Copyright (C) 2026 James Hilliard <james.hilliard1@xxxxxxxxx>
> > + *
> > + * Based on the AC100 driver:
> > + * Copyright (C) 2016 Chen-Yu Tsai
>
> Drop this.  Every driver tends to be based on something else.

Dropped in v8.

>
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
>
> Why aren't you using the MFD API?
>
> If you don't need it, then why is this in drivers/mfd?

The EPHY remains enumerated on its primary MDIO bus, so it is not an MFD
child. For non-EPHY use cases, the AC200 audio, video and RTC functions
are intended to be added as MFD children.

There are no such child drivers in this series, so I have not added
unused MFD cells. Those can be introduced with their corresponding
bindings and drivers.

>
> > +#define AC200_SYS_VERSION_REG                        0x0000
> > +#define AC200_SYS_VERSION_PACKAGE_MASK               GENMASK(15, 14)
> > +#define AC200_SYS_VERSION_CHIP_MASK          GENMASK(11, 0)
> > +
> > +#define AC200_SYS_CONTROL_REG                        0x0002
> > +#define AC200_SYS_CONTROL_CHIP_RESET_DEASSERT        BIT(0)
> > +
> > +/* Interface register accessible from every register page. */
> > +#define AC200_TWI_REG_ADDR_H 0x00fe
> > +#define AC200_MAX_REG                0xa1f2
> > +
> > +struct ac200 {
> > +     struct regmap *regmap;
> > +};
>
> Why not just pass 'regmap' directly?

Changed to store the regmap directly as the I2C driver data.

>
> > +static const struct regmap_range_cfg ac200_range_cfg[] = {
> > +     {
> > +             .range_max = AC200_MAX_REG,
> > +             .selector_reg = AC200_TWI_REG_ADDR_H,
> > +             .selector_mask = 0xff,
> > +             .window_len = 256,
> > +     },
> > +};
> > +
> > +/*
> > + * Each AC200 sub-block can reset independently, invalidating its register
> > + * contents without regmap's knowledge. Cache only the common page selector;
> > + * this avoids a selector read-modify-write for every access on the same page
> > + * without ever returning stale functional-register values.
> > + */
> > +static bool ac200_volatile_reg(struct device *dev, unsigned int reg)
> > +{
> > +     return reg != AC200_TWI_REG_ADDR_H;
> > +}
> > +
> > +static const struct regmap_config ac200_regmap_config = {
> > +     .name = "ac200",
> > +     .reg_bits = 8,
> > +     .reg_stride = 2,
> > +     .val_bits = 16,
> > +     .ranges = ac200_range_cfg,
> > +     .num_ranges = ARRAY_SIZE(ac200_range_cfg),
> > +     .max_register = AC200_MAX_REG,
> > +     .volatile_reg = ac200_volatile_reg,
> > +     .cache_type = REGCACHE_MAPLE,
> > +};
> > +
> > +static void ac200_disable(void *data)
> > +{
> > +     struct ac200 *ddata = data;
> > +
> > +     regmap_write(ddata->regmap, AC200_SYS_CONTROL_REG, 0);
> > +}
>
> You can't do this in .remove()?

Changed to reset the chip from the I2C remove callback.

>
> > +static int ac200_probe(struct i2c_client *client)
> > +{
> > +     struct device *dev = &client->dev;
> > +     unsigned int version;
> > +     struct ac200 *ddata;
> > +     struct clk *clk;
> > +     int ret;
> > +
> > +     ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
> > +     if (!ddata)
> > +             return -ENOMEM;
> > +
> > +     clk = devm_clk_get_enabled(dev, NULL);
> > +     if (IS_ERR(clk))
> > +             return dev_err_probe(dev, PTR_ERR(clk),
> > +                                  "failed to enable input clock\n");
> > +
> > +     ret = devm_clk_rate_exclusive_get(dev, clk);
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "failed to lock clock rate\n");
> > +
> > +     ddata->regmap = devm_regmap_init_i2c(client, &ac200_regmap_config);
> > +     if (IS_ERR(ddata->regmap))
> > +             return dev_err_probe(dev, PTR_ERR(ddata->regmap),
> > +                                  "failed to initialize regmap\n");
> > +
> > +     i2c_set_clientdata(client, ddata);
> > +
> > +     /*
> > +      * No minimum delay is documented. Match the vendor driver's 40 ms delay
> > +      * before its first AC200 register access after enabling the input clock.
> > +      */
> > +     msleep(40);
> > +
> > +     ret = regmap_read(ddata->regmap, AC200_SYS_VERSION_REG, &version);
> > +     if (ret)
> > +             return dev_err_probe(dev, ret,
> > +                                  "failed to read chip version\n");
> > +
> > +     dev_info(dev, "AC200 revision %#lx in package %lu\n",
> > +              FIELD_GET(AC200_SYS_VERSION_CHIP_MASK, version),
> > +              FIELD_GET(AC200_SYS_VERSION_PACKAGE_MASK, version));
>
> We support all versions, so why print it out at all?

Removed the revision read and log message.

> > +     /* Reset the chip after dependent function drivers have unbound. */
> > +     ret = devm_add_action_or_reset(dev, ac200_disable, ddata);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = regmap_write(ddata->regmap, AC200_SYS_CONTROL_REG, 0);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = regmap_write(ddata->regmap, AC200_SYS_CONTROL_REG,
> > +                        AC200_SYS_CONTROL_CHIP_RESET_DEASSERT);
> > +     if (ret)
> > +             return ret;
>
> Okay, now what?  What uses this regmap?

The current consumer is the separately submitted AC200/AC300 PHY driver.
It creates a managed device link to the AC200 I2C device and obtains the
regmap with dev_get_regmap() for ancillary package-control access.

v8:
https://patch.msgid.link/20260902-submit-ac200-mfd-v8-0-2aa06720b8ac@xxxxxxxxx

>
> > +     return 0;
> > +}
> > +
> > +static void ac200_shutdown(struct i2c_client *client)
> > +{
> > +     ac200_disable(i2c_get_clientdata(client));
> > +}
> > +
> > +static const struct of_device_id ac200_of_match[] = {
> > +     { .compatible = "x-powers,ac200" },
> > +     { }
> > +};
> > +MODULE_DEVICE_TABLE(of, ac200_of_match);
> > +
> > +static const struct i2c_device_id ac200_i2c_ids[] = {
> > +     { .name = "ac200" },
> > +     { }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, ac200_i2c_ids);
> > +
> > +static struct i2c_driver ac200_driver = {
> > +     .driver = {
> > +             .name = "ac200",
> > +             .of_match_table = ac200_of_match,
> > +     },
> > +     .probe = ac200_probe,
> > +     .shutdown = ac200_shutdown,
> > +     .id_table = ac200_i2c_ids,
> > +};
> > +module_i2c_driver(ac200_driver);
> > +
> > +MODULE_AUTHOR("James Hilliard <james.hilliard1@xxxxxxxxx>");
> > +MODULE_DESCRIPTION("X-Powers AC200 MFD core driver");
> > +MODULE_LICENSE("GPL");
> >
> > --
> > 2.53.0
> >
>
> --
> Lee Jones