Re: [PATCH 06/10] mfd: Add core driver for AD242x A2B transceivers

[Lee Jones <lee.jones@linaro.org>]

On Mon, 09 Dec 2019, Daniel Mack wrote:

> The core driver for these devices is split into several parts.
> 
> The master node driver is an I2C client. It is responsible for
> bringing up the bus topology and discovering the slave nodes.
> This process requries some knowlegde of the slave node configuration
> to program the bus timings correctly, so the master drivers walks
> the tree of nodes in the devicetree. The slave driver handles platform
> devices that are instantiated by the master node driver after
> discovery has finished.
> 
> Master nodes expose two addresses on the I2C bus, one (referred to as
> 'BASE' in the datasheet) for accessing registers on the transceiver
> node itself, and one (referred to as 'BUS') for accessing remote
> registers, either on the remote transceiver itself, or on I2C hardware
> connected to that remote transceiver, which then acts as a remote I2C
> bus master.
> 
> In order to allow MFD sub-devices to be registered as children of
> either the master or any slave node, the details on how to access the
> registers are hidden behind a regmap config. A pointer to the regmap
> is then exposed in the struct shared with the sub-devices.
> 
> The ad242x-bus driver is a simple proxy that occupies the BUS I2C
> address and which is referred to through a devicetree handle by the
> master driver.
> 
> For the discovery process, the driver has to wait for an interrupt
> to occur. In case no interrupt is configured in DT, the driver falls
> back to interrupt polling. After the discovery phase is completed,
> interrupts are only needed for error handling and GPIO handling,
> both of which is not currenty implemented.
> 
> Code common to both the master and the slave driver lives in
> 'ad242x-node.c'.
> 
> Signed-off-by: Daniel Mack <daniel@zonque.org>
> 
> mfd

?

> ---
>  drivers/mfd/Kconfig         |  11 +
>  drivers/mfd/Makefile        |   1 +
>  drivers/mfd/ad242x-bus.c    |  42 +++
>  drivers/mfd/ad242x-master.c | 611 ++++++++++++++++++++++++++++++++++++
>  drivers/mfd/ad242x-node.c   | 262 ++++++++++++++++
>  drivers/mfd/ad242x-slave.c  | 234 ++++++++++++++
>  include/linux/mfd/ad242x.h  | 400 +++++++++++++++++++++++

This device, or at least the way it's been coded is batty!

It's going to need a lot of massaging before being accepted.

Let's start with a quick (it's taken 2 hours already!) glance.

See below ...

>  7 files changed, 1561 insertions(+)
>  create mode 100644 drivers/mfd/ad242x-bus.c
>  create mode 100644 drivers/mfd/ad242x-master.c
>  create mode 100644 drivers/mfd/ad242x-node.c
>  create mode 100644 drivers/mfd/ad242x-slave.c
>  create mode 100644 include/linux/mfd/ad242x.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 420900852166..727a35053d8c 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -99,6 +99,17 @@ config PMIC_ADP5520
>  	  individual components like LCD backlight, LEDs, GPIOs and Kepad
>  	  under the corresponding menus.
>  
> +config MFD_AD242X
> +	bool "Analog Devices AD242x A2B support"
> +	select MFD_CORE
> +	select REGMAP_I2C
> +	depends on I2C=y && OF
> +	help
> +	  If you say yes here, you get support for devices from the AD242x
> +	  familiy. This driver provides common support for accessing the
> +	  devices, additional drivers must be enabled in order to use the
> +	  functionality of the devices.
> +
>  config MFD_AAT2870_CORE
>  	bool "AnalogicTech AAT2870"
>  	select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index aed99f08739f..2361c676f6c8 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -203,6 +203,7 @@ obj-$(CONFIG_MFD_SPMI_PMIC)	+= qcom-spmi-pmic.o
>  obj-$(CONFIG_TPS65911_COMPARATOR)	+= tps65911-comparator.o
>  obj-$(CONFIG_MFD_TPS65090)	+= tps65090.o
>  obj-$(CONFIG_MFD_AAT2870_CORE)	+= aat2870-core.o
> +obj-$(CONFIG_MFD_AD242X)	+= ad242x-master.o ad242x-slave.o ad242x-bus.o ad242x-node.o
>  obj-$(CONFIG_MFD_AT91_USART)	+= at91-usart.o
>  obj-$(CONFIG_MFD_ATMEL_FLEXCOM)	+= atmel-flexcom.o
>  obj-$(CONFIG_MFD_ATMEL_HLCDC)	+= atmel-hlcdc.o
> diff --git a/drivers/mfd/ad242x-bus.c b/drivers/mfd/ad242x-bus.c
> new file mode 100644
> index 000000000000..6660e13ce43d
> --- /dev/null
> +++ b/drivers/mfd/ad242x-bus.c
> @@ -0,0 +1,42 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/mfd/ad242x.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +
> +static int ad242x_bus_i2c_probe(struct i2c_client *i2c,
> +				const struct i2c_device_id *id)
> +{
> +	dev_set_drvdata(&i2c->dev, i2c);
> +	i2c_set_clientdata(i2c, &i2c->dev);

Please explain to me what you think is happening here.

> +	return 0;
> +}

What does this driver do?  Seems kinda pointless?

> +static const struct of_device_id ad242x_bus_of_match[] = {
> +	{ .compatible = "adi,ad2428w-bus" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, ad242x_bus_of_match);
> +
> +static const struct i2c_device_id ad242x_bus_i2c_id[] = {
> +	{ "ad242x_bus", 0 },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(i2c, ad242x_bus_i2c_id);

This table cam be removed if you use probe_new.

> +static struct i2c_driver ad242x_bus_i2c_driver = {
> +	.driver = {
> +		.name = "ad242x-bus",
> +		.of_match_table = ad242x_bus_of_match,
> +	},
> +	.probe = ad242x_bus_i2c_probe,
> +	.id_table = ad242x_bus_i2c_id,
> +};
> +
> +module_i2c_driver(ad242x_bus_i2c_driver);
> +
> +MODULE_DESCRIPTION("AD242x bus driver");
> +MODULE_AUTHOR("Daniel Mack <daniel@zonque.org>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/mfd/ad242x-master.c b/drivers/mfd/ad242x-master.c
> new file mode 100644
> index 000000000000..1b0bf90442a2
> --- /dev/null
> +++ b/drivers/mfd/ad242x-master.c
> @@ -0,0 +1,611 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/mfd/ad242x.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
> +
> +struct ad242x_master {
> +	struct ad242x_node	node;
> +	struct clk		*sync_clk;
> +	struct completion	run_completion;
> +	struct completion	discover_completion;
> +	struct ad242x_i2c_bus	bus;
> +	unsigned int		up_slot_size;
> +	unsigned int		dn_slot_size;
> +	bool			up_slot_alt_fmt;
> +	bool			dn_slot_alt_fmt;
> +	unsigned int		sync_clk_rate;
> +	int			irq;
> +	u8			response_cycles;
> +};

> +struct ad242x_node *ad242x_master_get_node(struct ad242x_master *master)
> +{
> +	return &master->node;
> +}
> +EXPORT_SYMBOL_GPL(ad242x_master_get_node);
> +
> +struct ad242x_i2c_bus *ad242x_master_get_bus(struct ad242x_master *master)
> +{
> +	return &master->bus;
> +}
> +EXPORT_SYMBOL_GPL(ad242x_master_get_bus);
> +
> +const char *ad242x_master_get_clk_name(struct ad242x_master *master)
> +{
> +	return __clk_get_name(master->sync_clk);
> +}
> +EXPORT_SYMBOL_GPL(ad242x_master_get_clk_name);
> +
> +unsigned int ad242x_master_get_clk_rate(struct ad242x_master *master)
> +{
> +	return master->sync_clk_rate;
> +}
> +EXPORT_SYMBOL_GPL(ad242x_master_get_clk_rate);

All of these functions provide abstraction for the sake of
abstraction.  They should be removed and replaced with the code
contained within them.

> +static int ad242x_read_one_irq(struct ad242x_master *master)
> +{
> +	struct regmap *regmap = master->node.regmap;
> +	struct device *dev = master->node.dev;
> +	unsigned int val, inttype;
> +	int ret;
> +
> +	ret = regmap_read(regmap, AD242X_INTSTAT, &val);
> +	if (ret < 0) {
> +		dev_err(dev, "unable to read INTSTAT register: %d\n", ret);

Users do not care about registers.

"Failed to obtain interrupt status"

> +		return ret;
> +	}
> +
> +	if (!(val & AD242X_INTSTAT_IRQ))
> +		return -ENOENT;

What happened here?  No interrupts fired?

IRQ_NONE would be better than "No such file or directory".

> +	ret = regmap_read(regmap, AD242X_INTTYPE, &inttype);
> +	if (ret < 0) {
> +		dev_err(dev, "unable to read INTTYPE register: %d\n", ret);

Same for all log messages throughout this patch-set.

> +		return ret;
> +	}
> +
> +	ret = regmap_read(regmap, AD242X_INTSRC, &val);
> +	if (ret < 0) {
> +		dev_err(dev, "unable to read INTSRC register: %d\n", ret);
> +		return ret;
> +	}

What does this prove?  Why aren't you doing anything with the value?

> +	ret = regmap_read(regmap, AD242X_INTPND0, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_write(regmap, AD242X_INTPND0, val);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_read(regmap, AD242X_INTPND1, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_write(regmap, AD242X_INTPND1, val);
> +	if (ret < 0)
> +		return ret;

What does writing back the value do?  Comments please.

> +	if (val & AD242X_INTSRC_MSTINT) {
> +		ret = regmap_read(regmap, AD242X_INTPND2, &val);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = regmap_write(regmap, AD242X_INTPND2, val);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	dev_err(dev, "%s() inttype: 0x%02x\n", __func__, inttype);

No debugging type 'func's please.

What makes this an error?

> +	switch (inttype) {
> +	case AD242X_INTTYPE_DSCDONE:
> +		complete(&master->discover_completion);
> +		break;
> +	case AD242X_INTTYPE_MSTR_RUNNING:
> +		complete(&master->run_completion);
> +		break;
> +	default:
> +		dev_info(dev, "Unhandled interrupt type 0x%02x\n", inttype);
> +	}
> +
> +	return 0;
> +}
> +
> +static int ad242x_read_irqs(struct ad242x_master *master)
> +{
> +	int ret;
> +	bool first = true;
> +
> +	while (true) {
> +		ret = ad242x_read_one_irq(master);
> +		if (ret < 0)
> +			return ret;
> +		if (ret == -ENOENT)
> +			return first ? ret : 0;
> +
> +		first = false;
> +	}
> +}
> +
> +static irqreturn_t ad242x_handle_irq(int irq, void *devid)
> +{
> +	struct ad242x_master *master = devid;
> +	int ret;
> +
> +	ret = ad242x_read_irqs(master);
> +	if (ret == -ENOENT)
> +		return IRQ_NONE;
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ad242x_wait_for_irq(struct ad242x_master *master,
> +			       struct completion *completion,
> +			       unsigned int timeout)
> +{
> +	int ret;
> +
> +	if (master->irq > 0) {
> +		ret = wait_for_completion_timeout(completion,
> +						  msecs_to_jiffies(timeout));
> +	} else {
> +		usleep_range(timeout * 1000, timeout * 1500);
> +		ad242x_read_irqs(master);
> +		ret = completion_done(completion);
> +	}

What are the semantics of this function.  Comments please.

> +	return ret == 0 ? -ETIMEDOUT : 0;
> +}
> +
> +/* See Table 3-2 in the datasheet */

Do you provide a link to the datasheet anywhere?

All I can find is a 1 page overview.

Please provide a description to what you're doing *here*.

> +static unsigned int ad242x_bus_bits(unsigned int slot_size, bool alt_fmt)
> +{
> +	int alt_bits[8] = { 0, 13, 17, 21, 30, 0, 39, 0 };
> +	int idx = AD242X_SLOTFMT_DNSIZE(slot_size);
> +
> +	return alt_fmt ? alt_bits[idx] : slot_size + 1;
> +}
> +
> +/* See Table 9-1 in the datasheet */

It's okay to reference the datasheet, but tell us what you're doing
here as well.

> +static unsigned int ad242x_master_respoffs(struct ad242x_node *node)
> +{
> +	if (node->tdm_mode == 2 && node->tdm_slot_size == 16)
> +		return 238;
> +
> +	if ((node->tdm_mode == 2 && node->tdm_slot_size == 32) ||
> +	    (node->tdm_mode == 4 && node->tdm_slot_size == 16))
> +		return 245;
> +
> +	return 248;

No magic numbers please.  You need to define them.

> +}
> +
> +static int ad242x_discover(struct ad242x_master *master,
> +			   struct device_node *nodes_np)
> +{
> +	struct regmap *regmap = master->node.regmap;
> +	struct device *dev = master->node.dev;
> +	struct device_node *child_np;
> +	unsigned int val, n = 0, i, respoffs, respcycs;

> +	unsigned int respcycs_up_min = UINT_MAX;
> +	unsigned int respcycs_dn_max = 0;

What are these?

> +	unsigned int master_up_slots = 0;
> +	unsigned int master_dn_slots = 0;
> +	bool up_enabled = false, dn_enabled = false;
> +	uint8_t slave_control = 0;
> +	int ret;
> +
> +	respoffs = ad242x_master_respoffs(&master->node);
> +
> +	for_each_available_child_of_node(nodes_np, child_np) {

What are we discovering here?  Child devices, or something else?

> +		unsigned int dnslot_activity, upslot_activity;
> +		unsigned int slave_dn_slots, slave_up_slots;
> +		unsigned int respcycs_dn, respcycs_up;
> +		struct ad242x_slot_config slot_config;
> +
> +		ret = ad242x_read_slot_config(dev, child_np, &slot_config);
> +		if (ret < 0) {
> +			dev_err(dev, "slot config of slave %d is invalid\n", n);
> +			return ret;
> +		}

What is a 'slot' defined as?

> +		/* See section 3-18 in the datasheet */

Give us a quick explanation.

> +		slave_dn_slots = max_t(int, slot_config.dn_n_forward_slots,
> +				       fls(slot_config.dn_rx_slots));
> +		slave_up_slots = max_t(int, slot_config.up_n_forward_slots,
> +				       fls(slot_config.up_rx_slots));
> +
> +		if (n == 0) {
> +			master_up_slots = slave_up_slots;
> +			master_dn_slots = slave_dn_slots;
> +		}
> +
> +		/* See Appendix B in the datasheet */

Give us a quick explanation.

> +		dnslot_activity = slave_dn_slots *
> +			ad242x_bus_bits(master->dn_slot_size,
> +					master->dn_slot_alt_fmt);
> +		upslot_activity = slave_up_slots *
> +			ad242x_bus_bits(master->up_slot_size,
> +					master->up_slot_alt_fmt);
> +
> +		respcycs_dn = DIV_ROUND_UP(64 + dnslot_activity, 4) + 4*n + 2;

Spaces around the '*'.  If it's not clear, use brackets.

> +		respcycs_up = respoffs -
> +			      (DIV_ROUND_UP(64 + upslot_activity, 4) + 1);

No idea what's going on here.

You need to define these magic numbers to make it clear.

> +		if (respcycs_dn > respcycs_dn_max)
> +			respcycs_dn_max = respcycs_dn;
> +
> +		if (respcycs_up < respcycs_up_min)
> +			respcycs_up_min = respcycs_up;
> +
> +		if (slave_dn_slots > 0)
> +			dn_enabled = true;
> +
> +		if (slave_up_slots > 0)
> +			up_enabled = true;
> +
> +		n++;
> +	}
> +
> +	if (n == 0) {
> +		dev_err(dev, "No child nodes specified\n");
> +		return -EINVAL;
> +	}
> +
> +	if (of_property_read_bool(dev->of_node, "adi,invert-xcvr-b")) {
> +		ret = regmap_update_bits(regmap, AD242X_CONTROL,
> +					 AD242X_CONTROL_XCVRBINV,
> +					 AD242X_CONTROL_XCVRBINV);
> +		if (ret < 0)
> +			return ret;
> +
> +		slave_control = AD242X_CONTROL_XCVRBINV;
> +	}
> +
> +	if (respcycs_dn_max > respcycs_up_min) {
> +		dev_err(dev, "Unsupported bus topology\n");
> +		return -EINVAL;
> +	}
> +
> +	respcycs = (respcycs_dn_max + respcycs_up_min) / 2;
> +	ret = regmap_write(regmap, AD242X_RESPCYCS, respcycs);
> +	if (ret < 0)
> +		return ret;

Comments please.

In fact, comments throughout please.

Anything that isn't absolutely crystal clear should have at least a
little one liner to clarify what is being calculated/set.

> +	ret = regmap_update_bits(regmap, AD242X_CONTROL,
> +				 AD242X_CONTROL_NEWSTRCT,
> +				 AD242X_CONTROL_NEWSTRCT);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_write(regmap, AD242X_SWCTL, AD242X_SWCTL_ENSW);
> +	if (ret < 0)
> +		return ret;
> +
> +	for (i = 0; i < n; i++) {
> +		ret = regmap_write(regmap, AD242X_DISCVRY, respcycs - (4*i));

Spaces.

What is 4?

> +		if (ret < 0)
> +			return ret;
> +
> +		ret = ad242x_wait_for_irq(master,
> +					  &master->discover_completion, 35);

Define magic numbers throughout.

> +		if (ret < 0) {
> +			dev_err(dev, "Discovery of node %d timed out\n", i);
> +			return ret;
> +		}
> +
> +		val = AD242X_SWCTL_MODE(2) | AD242X_SWCTL_ENSW;
> +
> +		if (i == 0)
> +			ret = regmap_write(regmap, AD242X_SWCTL, val);
> +		else
> +			ret = ad242x_slave_write(&master->bus, regmap, i,
> +						 AD242X_SWCTL, val);
> +
> +		if (ret < 0)
> +			return ret;
> +
> +		dev_info(dev, "Node %d discovered\n", i);
> +
> +		/* Last node? */
> +		if (i == n - 1)
> +			break;
> +
> +		ret = ad242x_slave_write(&master->bus, regmap, i,
> +					 AD242X_INTMSK2,
> +					 AD242X_INTMSK2_DSCDIEN);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = ad242x_slave_write(&master->bus, regmap, i,
> +					 AD242X_CONTROL, slave_control);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = ad242x_slave_write(&master->bus, regmap, i,
> +					 AD242X_SWCTL, AD242X_SWCTL_ENSW);
> +		if (ret < 0)
> +			return ret;
> +
> +		reinit_completion(&master->discover_completion);
> +	}
> +
> +	ret = regmap_write(regmap, AD242X_DNSLOTS, master_dn_slots);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_write(regmap, AD242X_UPSLOTS, master_up_slots);
> +	if (ret < 0)
> +		return ret;
> +
> +	val = 0;
> +	if (dn_enabled)
> +		val |= AD242X_DATCTL_DNS;
> +
> +	if (up_enabled)
> +		val |= AD242X_DATCTL_UPS;
> +
> +	ret = regmap_write(regmap, AD242X_DATCTL, val);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int ad242x_init_irq(struct ad242x_master *master)
> +{
> +	struct regmap *regmap = master->node.regmap;
> +	struct device *dev = master->node.dev;
> +	int ret;
> +
> +	if (master->irq > 0) {
> +		ret = devm_request_threaded_irq(dev, master->irq, NULL,
> +						ad242x_handle_irq, IRQF_ONESHOT,
> +						dev_name(dev), master);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	ret = regmap_write(regmap, AD242X_INTMSK0,
> +			   AD242X_INTMSK0_SRFEIEN | AD242X_INTMSK0_BECIEN |
> +			   AD242X_INTMSK0_PWREIEN | AD242X_INTMSK0_CRCEIEN |
> +			   AD242X_INTMSK0_DDEIEN  | AD242X_INTMSK0_HCEIEN);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_write(regmap, AD242X_INTMSK2,
> +			   AD242X_INTMSK2_DSCDIEN | AD242X_INTMSK2_SLVIRQEN);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static const struct regmap_config ad242x_regmap_config = {
> +	.reg_bits	= 8,
> +	.val_bits	= 8,
> +	.volatile_reg	= ad242x_is_volatile_reg,
> +	.writeable_reg	= ad242x_is_writeable_reg,
> +	.max_register	= AD242X_MAX_REG,
> +	.cache_type	= REGCACHE_RBTREE,
> +};
> +
> +static int ad242x_master_probe(struct i2c_client *i2c,
> +			       const struct i2c_device_id *id)
> +{
> +	struct device_node *bus_np, *nodes_np, *np;
> +	struct device *busdev, *dev = &i2c->dev;
> +	struct ad242x_master *master;
> +	struct regmap *regmap;
> +	unsigned int val;
> +	int ret;
> +
> +	nodes_np = of_get_child_by_name(dev->of_node, "nodes");
> +	if (!nodes_np) {
> +		dev_err(dev, "no 'nodes' property given\n");
> +		return -EINVAL;
> +	}
> +
> +	bus_np = of_parse_phandle(dev->of_node, "adi,a2b-bus", 0);
> +	if (!bus_np) {
> +		dev_err(dev, "no 'adi,a2b-bus' handle specified for master node\n");
> +		return -EINVAL;
> +	}
> +
> +	busdev = bus_find_device_by_of_node(&i2c_bus_type, bus_np);
> +	if (!busdev) {
> +		dev_err(dev, "'adi,a2b-bus' handle invalid\n");
> +		return -EINVAL;
> +	}
> +
> +	master = devm_kzalloc(dev, sizeof(struct ad242x_master), GFP_KERNEL);

sizeof(*master)

> +	if (!master)
> +		return -ENOMEM;
> +
> +	mutex_init(&master->bus.mutex);
> +	init_completion(&master->run_completion);
> +	init_completion(&master->discover_completion);

> +	dev_set_drvdata(dev, &master->node);
> +	i2c_set_clientdata(i2c, master);

What do you think is happening here?

> +	regmap = devm_regmap_init_i2c(i2c, &ad242x_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		ret = PTR_ERR(regmap);
> +		dev_err(dev, "regmap init failed: %d\n", ret);

"initialisation"

Or even better "Failed to initialise I2C Regmap"

> +		return ret;
> +	}
> +
> +	master->bus.client = to_i2c_client(busdev);

What does 'bus' do in this context?

> +	master->node.regmap = regmap;
> +	master->node.dev = dev;
> +	master->node.master = master;
> +	master->node.id = AD242X_MASTER_ID;
> +	master->irq = i2c->irq;
> +
> +	master->sync_clk = devm_clk_get(dev, "sync");
> +	if (IS_ERR(master->sync_clk)) {
> +		ret = PTR_ERR(master->sync_clk);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "failed to get sync clk: %d\n", ret);
> +
> +		return ret;
> +	}
> +
> +	if (of_property_read_u32(dev->of_node, "clock-frequency",
> +				 &master->sync_clk_rate)) {
> +		ret = clk_set_rate(master->sync_clk, master->sync_clk_rate);
> +		if (ret < 0) {
> +			dev_err(dev, "Cannot set sync clock rate: %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	master->sync_clk_rate = clk_get_rate(master->sync_clk);
> +	if (master->sync_clk_rate != 44100 && master->sync_clk_rate != 48000) {

Please define these magic numbers.

Something descriptive that tells us what the different clock speeds
do.

> +		dev_err(dev, "SYNC clock rate %d is invalid\n",
> +			master->sync_clk_rate);
> +		return -EINVAL;
> +	}
> +
> +	ret = clk_prepare_enable(master->sync_clk);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to enable sync clk: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Master node setup */
> +
> +	ret = regmap_write(regmap, AD242X_CONTROL,
> +			   AD242X_CONTROL_MSTR | AD242X_CONTROL_SOFTRST);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = ad242x_wait_for_irq(master, &master->run_completion, 10);
> +	if (ret < 0) {
> +		dev_err(dev, "timeout waiting for PLL sync: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_update_bits(regmap, AD242X_CONTROL,
> +				 AD242X_CONTROL_SOFTRST, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = ad242x_node_probe(&master->node);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = ad242x_init_irq(master);
> +	if (ret < 0) {
> +		dev_err(dev, "Unable to set up IRQ: %d", ret);
> +		return ret;
> +	}
> +
> +	/* Slot format setup */
> +
> +	of_property_read_u32(dev->of_node, "adi,upstream-slot-size", &val);
> +	if (val < 8 || val > 32 || (val % 4 != 0)) {
> +		dev_err(dev, "invalid upstream-slot-size %d\n", val);
> +		return -EINVAL;
> +	}
> +	master->up_slot_size = val;
> +
> +	of_property_read_u32(dev->of_node, "adi,downstream-slot-size", &val);
> +	if (val < 8 || val > 32 || (val % 4 != 0)) {
> +		dev_err(dev, "invalid downstream-slot-size %d\n", val);
> +		return -EINVAL;
> +	}
> +	master->dn_slot_size = val;
> +
> +	master->dn_slot_alt_fmt =
> +		of_property_read_bool(dev->of_node,
> +				      "adi,alternate-downstream-slot-format");
> +	master->up_slot_alt_fmt =
> +		of_property_read_bool(dev->of_node,
> +				      "adi,alternate-upstream-slot-format");

Obviously this all needs to be run past the DT maintainer(s).

> +	val = AD242X_SLOTFMT_DNSIZE(master->dn_slot_size) |
> +	      AD242X_SLOTFMT_UPSIZE(master->up_slot_size);
> +
> +	if (master->dn_slot_alt_fmt)
> +		val |= AD242X_SLOTFMT_DNFMT;
> +
> +	if (master->up_slot_alt_fmt)
> +		val |= AD242X_SLOTFMT_UPFMT;
> +
> +	ret = regmap_write(regmap, AD242X_SLOTFMT, val);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Node discovery and MFD setup */
> +
> +	ret = ad242x_discover(master, nodes_np);
> +	if (ret < 0) {
> +		dev_err(dev, "error discovering nodes: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = ad242x_node_add_mfd_cells(dev);

Why is this called twice with the same children?

> +	if (ret < 0) {
> +		dev_err(dev, "failed to add MFD devices %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Register platform devices for nodes */
> +
> +	for_each_available_child_of_node(nodes_np, np)
> +		of_platform_device_create(np, NULL, dev);

What are you doing here?

Either use OF to register all child devices OR use MFD, not a mixture
of both.

> +	of_node_put(nodes_np);
> +
> +	return 0;
> +}
> +
> +static int ad242x_master_remove(struct i2c_client *i2c)
> +{
> +	struct ad242x_master *master = i2c_get_clientdata(i2c);
> +
> +	if (master->sync_clk)
> +		clk_disable_unprepare(master->sync_clk);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ad242x_master_of_match[] = {
> +	{ .compatible = "adi,ad2428w-master" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ad242x_master_of_match);
> +
> +static const struct i2c_device_id ad242x_master_i2c_id[] = {
> +	{"ad242x-master", 0},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, ad242x_master_i2c_id);

If you one the OF match table, you don't need this empty I2C table.
Grep for probe_new.

> +static struct i2c_driver ad242x_master_i2c_driver = {
> +	.driver	= {
> +		.name = "ad242x-master",
> +		.of_match_table	= ad242x_master_of_match,
> +	},
> +	.probe = ad242x_master_probe,
> +	.remove = ad242x_master_remove,
> +	.id_table = ad242x_master_i2c_id,
> +};
> +

Remove this line.

> +module_i2c_driver(ad242x_master_i2c_driver);
> +
> +MODULE_DESCRIPTION("AD242x master master driver");

Typo.

> +MODULE_AUTHOR("Daniel Mack <daniel@zonque.org>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/mfd/ad242x-node.c b/drivers/mfd/ad242x-node.c
> new file mode 100644
> index 000000000000..f9db689380a7
> --- /dev/null
> +++ b/drivers/mfd/ad242x-node.c
> @@ -0,0 +1,262 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/ad242x.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
> +
> +/* See Table 7-43 in the datasheet */

More information please.

> +static int ad242x_tdmmode_index(unsigned int mode, bool slave)
> +{
> +	switch (mode) {
> +	case 2:
> +		return 0;
> +	case 4:
> +		return 1;
> +	case 8:
> +		return 2;
> +	case 12:
> +		return slave ? -EINVAL : 3;
> +	case 16:
> +		return 4;
> +	case 20:
> +		return slave ? -EINVAL : 5;
> +	case 24:
> +		return slave ? -EINVAL : 6;
> +	case 32:
> +		return 7;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +int ad242x_node_probe(struct ad242x_node *node)
> +{
> +	struct device_node *np = node->dev->of_node;
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(node->regmap, AD242X_VENDOR, &val);
> +	if (ret < 0) {
> +		dev_err(node->dev, "failed to read VENDOR register %d\n", ret);

Please re-write all of your kernel log messages to be user friendly.

> +		return ret;
> +	}
> +
> +	if (val != 0xad) {

No magic numbers - please define them all.

> +		dev_err(node->dev, "bogus value 0x%02x in VENDOR register\n",
> +			val);
> +		return -ENODEV;
> +	}
> +
> +	ret = regmap_read(node->regmap, AD242X_PRODUCT, &val);
> +	if (ret < 0) {
> +		dev_err(node->dev, "failed to read PRODUCT register %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	if (val != 0x28) {
> +		dev_err(node->dev, "bogus value 0x%02x in PRODUCT register\n",
> +			val);
> +		return -ENODEV;
> +	}
> +
> +	ret = regmap_read(node->regmap, AD242X_VERSION, &val);
> +	if (ret < 0) {
> +		dev_err(node->dev, "failed to read VERSION register %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (node->id == AD242X_MASTER_ID)
> +		dev_info(node->dev,
> +			 "Detected AD242x master node, version %d.%d\n",
> +			 val >> 4, val & 0xf);
> +	else
> +		dev_info(node->dev,
> +			 "Detected AD242x slave node, version %d.%d, id %d\n",
> +			 val >> 4, val & 0xf, node->id);
> +
> +	ret = regmap_read(node->regmap, AD242X_CAPABILITY, &val);
> +	if (ret < 0) {
> +		dev_err(node->dev, "failed to read CAPABILITY register %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	node->caps = val;
> +
> +	val = 0;
> +
> +	if (of_property_read_bool(np, "adi,spread-a2b-clock"))
> +		val |= AD242X_PLLCTL_SSMODE_AB;
> +	else if (of_property_read_bool(np, "adi,spread-a2b-i2s-clock"))
> +		val |= AD242X_PLLCTL_SSMODE_AB_I2S;
> +
> +	if (of_property_read_bool(np, "adi,spread-spectrum-high"))
> +		val |= AD242X_PLLCTL_SSDEPTH;
> +
> +	ret = regmap_write(node->regmap, AD242X_PLLCTL, val);
> +	if (ret < 0) {
> +		dev_err(node->dev, "failed to write PLLCTL register %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* I2S global setup */
> +
> +	of_property_read_u32(np, "adi,tdm-mode", &node->tdm_mode);
> +	of_property_read_u32(np, "adi,tdm-slot-size", &node->tdm_slot_size);
> +
> +	ret = ad242x_tdmmode_index(node->tdm_mode, false);
> +	if (ret < 0) {
> +		dev_err(node->dev, "invalid TDM mode %d\n", node->tdm_mode);
> +		return -EINVAL;
> +	}
> +
> +	val = AD242X_I2SGCTL_TDMMODE(ret);
> +
> +	if (node->tdm_slot_size == 16) {
> +		val |= AD242X_I2SGCTL_TDMSS;
> +	} else if (node->tdm_slot_size != 32) {
> +		dev_err(node->dev, "invalid TDM slot size %d\n",
> +			node->tdm_slot_size);
> +		return -EINVAL;
> +	}
> +
> +	if (of_property_read_bool(np, "adi,alternating-sync"))
> +		val |= AD242X_I2SGCTL_ALT;
> +
> +	if (of_property_read_bool(np, "adi,early-sync"))
> +		val |= AD242X_I2SGCTL_EARLY;
> +
> +	if (of_property_read_bool(np, "adi,invert-sync"))
> +		val |= AD242X_I2SGCTL_INV;
> +
> +	ret = regmap_write(node->regmap, AD242X_I2SGCTL, val);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static const struct mfd_cell ad242x_mfd_cells[] = {
> +	{
> +		.of_compatible	= "adi,ad2428w-i2c",
> +		.name		= "ad242x-i2c",

Swap these around.  Or better still, use the macros found in:

  include/linux/mfd/core.h

> +	},
> +	{
> +		.of_compatible	= "adi,ad2428w-gpio",
> +		.name		= "ad242x-gpio",
> +	},
> +	{
> +		.of_compatible	= "adi,ad2428w-clk",
> +		.name		= "ad242x-clk",
> +	},
> +	{
> +		.of_compatible	= "adi,ad2428w-codec",
> +		.name		= "ad242x-codec",
> +	},
> +};
> +
> +int ad242x_node_add_mfd_cells(struct device *dev)
> +{
> +	return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO,
> +				    ad242x_mfd_cells,
> +				    ARRAY_SIZE(ad242x_mfd_cells),
> +				    NULL, 0, NULL);
> +}
> +
> +static int ad242x_get_slot_mask(const struct device_node *np,
> +				const char *propname, u32 *mask)
> +{
> +	unsigned int i, num;
> +	int ret, proplen;
> +	u32 slots[32];

You should define 32 as the maximum number of slots available, then
use it again for most of the random '32's below.

> +	if (!of_get_property(np, propname, &proplen))
> +		return -ENOENT;

This whole piece becomes simpler if you use:

 of_property_read_variable_u32_array()

> +	num = proplen / sizeof(u32);
> +
> +	if (num > ARRAY_SIZE(slots))
> +		return -EOVERFLOW;
> +
> +	ret = of_property_read_u32_array(np, propname, slots, num);
> +	if (ret < 0)
> +		return ret;
> +
> +	*mask = 0;
> +
> +	for (i = 0; i < num; i++) {
> +		if (slots[i] >= 32)
> +			return -EINVAL;
> +
> +		*mask |= BIT(slots[i]);
> +	}
> +
> +	return 0;
> +}
> +
> +int ad242x_read_slot_config(struct device *dev,
> +			    struct device_node *np,
> +			    struct ad242x_slot_config *config)
> +{
> +	struct device_node *dn_np, *up_np;
> +	int ret;
> +
> +	dn_np = of_get_child_by_name(np, "downstream");
> +	if (!dn_np) {
> +		dev_err(dev, "no downstream node\n");
> +		return -EINVAL;
> +	}
> +
> +	up_np = of_get_child_by_name(np, "upstream");
> +	if (!dn_np) {
> +		dev_err(dev, "no upstream node\n");
> +		ret = -EINVAL;
> +		goto err_put_dn_node;
> +	}
> +
> +	ret = ad242x_get_slot_mask(dn_np, "rx-slots", &config->dn_rx_slots);
> +	if (ret < 0 && ret != -ENOENT) {

If you're going to ignore -ENOENT, why not just return 0?

> +		dev_err(dev, "invalid downstream rx-slots property\n");
> +		goto err_put_nodes;
> +	}
> +
> +	of_property_read_u32(dn_np, "#tx-slots", &config->dn_n_tx_slots);
> +	of_property_read_u32(dn_np, "#forward-slots",
> +			     &config->dn_n_forward_slots);
> +	if (config->dn_n_tx_slots + config->dn_n_forward_slots >= 32) {
> +		dev_err(dev, "invalid downstream tx-slots property\n");
> +		goto err_put_nodes;
> +	}
> +
> +

Superfluous '\n'.

> +	ret = ad242x_get_slot_mask(up_np, "rx-slots", &config->up_rx_slots);
> +	if (ret < 0) {
> +		dev_err(dev, "invalid upstream rx-slots property\n");
> +		goto err_put_nodes;
> +	}
> +
> +	of_property_read_u32(up_np, "#tx-slots", &config->up_n_tx_slots);
> +	of_property_read_u32(up_np, "#forward-slots",
> +			     &config->up_n_forward_slots);
> +	if (config->up_n_tx_slots + config->up_n_forward_slots >= 32) {
> +		dev_err(dev, "invalid downstream tx-slots property\n");
> +		goto err_put_nodes;
> +	}
> +
> +err_put_nodes:
> +	of_node_put(up_np);
> +err_put_dn_node:
> +	of_node_put(dn_np);
> +
> +	return ret;
> +}
> diff --git a/drivers/mfd/ad242x-slave.c b/drivers/mfd/ad242x-slave.c
> new file mode 100644
> index 000000000000..ad255d67a5b6
> --- /dev/null
> +++ b/drivers/mfd/ad242x-slave.c
> @@ -0,0 +1,234 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/mfd/ad242x.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mutex.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +struct ad242x_slave {
> +	struct ad242x_node		node;
> +	struct ad242x_node		*master;
> +	struct ad242x_slot_config	slot_config;
> +	unsigned int			sync_offset;
> +};
> +
> +int ad242x_slave_read(struct ad242x_i2c_bus *bus,
> +		      struct regmap *master_regmap,
> +		      uint8_t node_id, uint8_t reg, unsigned int *val)
> +{
> +	int ret;
> +
> +	mutex_lock(&bus->mutex);
> +
> +	ret = regmap_write(master_regmap, AD242X_NODEADR, node_id);
> +	if (ret < 0)
> +		goto err_unlock;
> +
> +	ret = i2c_smbus_read_byte_data(bus->client, reg);
> +	if (ret < 0)
> +		goto err_unlock;
> +
> +	*val = ret;
> +	ret = 0;
> +
> +err_unlock:
> +	mutex_unlock(&bus->mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(ad242x_slave_read);
> +
> +int ad242x_slave_write(struct ad242x_i2c_bus *bus,
> +		       struct regmap *master_regmap,
> +		       uint8_t node_id, uint8_t reg, unsigned int val)
> +{
> +	int ret;
> +
> +	mutex_lock(&bus->mutex);
> +
> +	ret = regmap_write(master_regmap, AD242X_NODEADR, node_id);
> +	if (ret < 0)
> +		goto err_unlock;
> +
> +	ret = i2c_smbus_write_byte_data(bus->client, reg, val);
> +	if (ret < 0)
> +		goto err_unlock;
> +
> +	ret = 0;
> +
> +err_unlock:
> +	mutex_unlock(&bus->mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(ad242x_slave_write);
> +
> +static int ad242x_slave_regmap_read(void *context, unsigned int reg,
> +				    unsigned int *val)
> +{
> +	struct ad242x_slave *slave = context;
> +	struct ad242x_i2c_bus *bus = ad242x_master_get_bus(slave->node.master);
> +	struct ad242x_node *mnode = ad242x_master_get_node(slave->node.master);
> +
> +	if (reg > 0xff)
> +		return -EINVAL;
> +
> +	return ad242x_slave_read(bus, mnode->regmap, slave->node.id, reg, val);
> +}
> +
> +static int ad242x_slave_regmap_write(void *context, unsigned int reg,
> +				     unsigned int val)
> +{
> +	struct ad242x_slave *slave = context;
> +	struct ad242x_i2c_bus *bus = ad242x_master_get_bus(slave->node.master);
> +	struct ad242x_node *mnode = ad242x_master_get_node(slave->node.master);
> +
> +	if (val > 0xff || reg > 0xff)
> +		return -EINVAL;
> +
> +	return ad242x_slave_write(bus, mnode->regmap, slave->node.id, reg, val);
> +}
> +
> +static const struct regmap_config ad242x_regmap_config = {
> +	.reg_bits	= 8,
> +	.val_bits	= 8,
> +	.volatile_reg	= ad242x_is_volatile_reg,
> +	.writeable_reg	= ad242x_is_writeable_reg,
> +	.reg_read	= ad242x_slave_regmap_read,
> +	.reg_write	= ad242x_slave_regmap_write,
> +	.max_register	= AD242X_MAX_REG,
> +	.cache_type	= REGCACHE_RBTREE,
> +};
> +
> +static int ad242x_calc_sync_offset(unsigned int val)
> +{
> +	if (val == 0)
> +		return 0;
> +
> +	if (val > 127)
> +		return -EINVAL;
> +
> +	return 256 - val;

More magic numbers to define.

> +}
> +
> +static int ad242x_slave_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct ad242x_slave *slave;
> +	struct ad242x_node *mnode;
> +	struct regmap *regmap;
> +	unsigned int val;
> +	int i, ret;
> +
> +	slave = devm_kzalloc(dev, sizeof(*slave), GFP_KERNEL);
> +	if (!slave)
> +		return -ENOMEM;
> +
> +	regmap = devm_regmap_init(dev, NULL, slave, &ad242x_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		ret = PTR_ERR(regmap);
> +		dev_err(dev, "regmap init failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	of_property_read_u32(dev->of_node, "reg", &val);
> +	slave->node.id = val;

This looks like an abuse of the 'reg' property.

> +	slave->node.dev = dev;
> +	slave->node.regmap = regmap;
> +
> +	mnode = dev_get_drvdata(dev->parent);

What is the parent of the slave?

(it's not clear without looking at the DT I guess)

> +	slave->node.master = mnode->master;
> +
> +	dev_set_name(dev, "%s-a2b-%d", dev_name(dev->parent), slave->node.id);
> +	dev_set_drvdata(dev, &slave->node);
> +
> +	ret = ad242x_node_probe(&slave->node);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = ad242x_read_slot_config(dev, dev->of_node, &slave->slot_config);
> +	if (ret < 0) {
> +		dev_err(dev, "slot config is invalid: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_write(regmap, AD242X_UPSLOTS,
> +			   slave->slot_config.up_n_forward_slots);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_write(regmap, AD242X_DNSLOTS,
> +			   slave->slot_config.dn_n_forward_slots);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_write(regmap, AD242X_LUPSLOTS,
> +			   slave->slot_config.up_n_tx_slots);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_write(regmap, AD242X_LDNSLOTS,
> +			   slave->slot_config.dn_n_tx_slots |
> +			   AD242X_LDNSLOTS_DNMASKEN);
> +	if (ret < 0)
> +		return ret;
> +
> +	for (i = 0; i < 4; i++) {

Why 4?  Please define it.

> +		ret = regmap_write(regmap, AD242X_UPMASK(i),
> +			(slave->slot_config.up_rx_slots >> (i * 8)) & 0xff);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = regmap_write(regmap, AD242X_DNMASK(i),
> +			(slave->slot_config.dn_rx_slots >> (i * 8)) & 0xff);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	of_property_read_u32(dev->of_node, "adi,sync-offset",
> +			     &slave->sync_offset);
> +
> +	ret = ad242x_calc_sync_offset(slave->sync_offset);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_write(regmap, AD242X_SYNCOFFSET, ret);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = ad242x_node_add_mfd_cells(dev);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to add MFD devices %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ad242x_slave_of_match[] = {
> +	{ .compatible = "adi,ad2428w-slave" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ad242x_slave_of_match);
> +
> +static struct platform_driver ad242x_slave_driver = {
> +	.driver = {
> +		.name = "ad242x-slave",
> +		.of_match_table = ad242x_slave_of_match,
> +	},
> +	.probe = ad242x_slave_probe,
> +};
> +
> +module_platform_driver(ad242x_slave_driver);
> +
> +MODULE_DESCRIPTION("AD242x slave node driver");
> +MODULE_AUTHOR("Daniel Mack <daniel@zonque.org>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/ad242x.h b/include/linux/mfd/ad242x.h
> new file mode 100644
> index 000000000000..02a174824f85
> --- /dev/null
> +++ b/include/linux/mfd/ad242x.h
> @@ -0,0 +1,400 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __LINUX_MFD_AD242X_H
> +#define __LINUX_MFD_AD242X_H
> +
> +#define AD242X_CHIP			0x00
> +#define AD242X_NODEADR			0x01
> +#define AD242X_NODEADR_MASK		0x0f
> +#define AD242X_NODEADR_PERI		BIT(5)
> +#define AD242X_NODEADR_BRCST		BIT(7)
> +
> +#define AD242X_VENDOR			0x02
> +#define AD242X_PRODUCT			0x03
> +#define AD242X_VERSION			0x04
> +
> +#define AD242X_CAPABILITY		0x05
> +#define AD242X_CAPABILITY_I2C		BIT(0)
> +
> +#define AD242X_SWCTL			0x09
> +#define AD242X_SWCTL_ENSW		BIT(0)
> +#define AD242X_SWCTL_DIAGMODE		BIT(3)
> +#define AD242X_SWCTL_MODE(X)		(((X) & 3) << 4)
> +#define AD242X_SWCTL_MODE_MASK		(3 << 4)
> +#define AD242X_SWCTL_DISNXT		BIT(6)
> +
> +#define AD242X_BCDNSLOTS		0x0a
> +#define AD242X_BCDNSLOTS_MASK		0x3f
> +
> +#define AD242X_LDNSLOTS			0x0b
> +#define AD242X_LDNSLOTS_MASK		0x3f
> +#define AD242X_LDNSLOTS_DNMASKEN	BIT(7)
> +
> +#define AD242X_LUPSLOTS			0x0c
> +#define AD242X_LUPSLOTS_MASK		0x3f
> +
> +#define AD242X_DNSLOTS			0x0d
> +#define AD242X_DNSLOTS_MASK		0x3f
> +
> +#define AD242X_UPSLOTS			0x0e
> +#define AD242X_UPSLOTS_MASK		0x3f
> +
> +#define AD242X_RESPCYCS			0x0f
> +
> +#define AD242X_SLOTFMT			0x10
> +#define AD242X_SLOTFMT_DNSIZE(X)	((((X) - 8) >> 2) & 7)
> +#define AD242X_SLOTFMT_DNFMT		BIT(3)
> +#define AD242X_SLOTFMT_UPSIZE(X)	(((((X) - 8) >> 2) & 7) << 4)
> +#define AD242X_SLOTFMT_UPFMT		BIT(7)
> +
> +#define AD242X_DATCTL			0x11
> +#define AD242X_DATCTL_DNS		BIT(0)
> +#define AD242X_DATCTL_UPS		BIT(1)
> +#define AD242X_DATCTL_ENDSNIFF		BIT(5)
> +#define AD242X_DATCTL_STANDBY		BIT(7)
> +
> +#define AD242X_CONTROL			0x12
> +#define AD242X_CONTROL_NEWSTRCT		BIT(0)
> +#define AD242X_CONTROL_ENDDSC		BIT(1)
> +#define AD242X_CONTROL_SOFTRST		BIT(2)
> +#define AD242X_CONTROL_SWBYP		BIT(3)
> +#define AD242X_CONTROL_XCVRBINV		BIT(4)
> +#define AD242X_CONTROL_MSTR		BIT(7)
> +
> +#define AD242X_DISCVRY			0x13
> +
> +#define AD242X_SWSTAT			0x14
> +#define AD242X_SWSTAT_FIN		BIT(0)
> +#define AD242X_SWSTAT_FAULT		BIT(1)
> +#define AD242X_SWSTAT_FAULTCODE(X)	(((X) & 0x7) >> 4)
> +#define AD242X_SWSTAT_FAULT_NLOC	BIT(7)
> +
> +#define AD242X_INTSTAT			0x15
> +#define AD242X_INTSTAT_IRQ		BIT(0)
> +
> +#define AD242X_INTSRC			0x16
> +#define AD242X_INTSRC_INODE		0x0f
> +#define AD242X_INTSRC_SLVINT		BIT(6)
> +#define AD242X_INTSRC_MSTINT		BIT(7)
> +
> +#define AD242X_INTTYPE			0x17
> +
> +#define AD242X_INTTYPE_DSCDONE		24
> +#define AD242X_INTTYPE_MSTR_RUNNING	255
> +
> +#define AD242X_INTPND0			0x18
> +#define AD242X_INTPDN0_HDCNTERR		BIT(0)
> +#define AD242X_INTPDN0_DDERR		BIT(1)
> +#define AD242X_INTPDN0_CRCERR		BIT(2)
> +#define AD242X_INTPDN0_DPERR		BIT(3)
> +#define AD242X_INTPDN0_PWRERR		BIT(4)
> +#define AD242X_INTPDN0_BECOVF		BIT(5)
> +#define AD242X_INTPDN0_SRFERR		BIT(6)
> +#define AD242X_INTPDN0_SRFCRCERR	BIT(7)
> +
> +#define AD242X_INTPND1			0x19
> +#define AD242X_INTPND1_IOPND(X)		BIT(X)
> +
> +#define AD242X_INTPND2			0x1a
> +#define AD242X_INTPND2_DSCDONE		BIT(0)
> +#define AD242X_INTPND2_I2CERR		BIT(1)
> +#define AD242X_INTPND2_ICRCERR		BIT(2)
> +#define AD242X_INTPND2_SLVIRQ		BIT(3)
> +
> +#define AD242X_INTMSK0			0x1b
> +#define AD242X_INTMSK0_HCEIEN		BIT(0)
> +#define AD242X_INTMSK0_DDEIEN		BIT(1)
> +#define AD242X_INTMSK0_CRCEIEN		BIT(2)
> +#define AD242X_INTMSK0_DPEIEN		BIT(3)
> +#define AD242X_INTMSK0_PWREIEN		BIT(4)
> +#define AD242X_INTMSK0_BECIEN		BIT(5)
> +#define AD242X_INTMSK0_SRFEIEN		BIT(6)
> +#define AD242X_INTMSK0_SRFCRCEIEN	BIT(7)
> +
> +#define AD242X_INTMSK1			0x1c
> +#define AD242X_INTMSK1_IOIRQEN(X)	BIT(X)
> +
> +#define AD242X_INTMSK2			0x1d
> +#define AD242X_INTMSK2_DSCDIEN		BIT(0)
> +#define AD242X_INTMSK2_I2CEIEN		BIT(1)
> +#define AD242X_INTMSK2_ICRCEIEN		BIT(2)
> +#define AD242X_INTMSK2_SLVIRQEN		BIT(3)
> +
> +#define AD242X_BECCTL			0x1e
> +#define AD242X_BECCTL_ENHDCNT		BIT(0)
> +#define AD242X_BECCTL_ENDD		BIT(1)
> +#define AD242X_BECCTL_ENCRC		BIT(2)
> +#define AD242X_BECCTL_ENDP		BIT(3)
> +#define AD242X_BECCTL_ENICRC		BIT(4)
> +#define AD242X_BECCTL_THRESHLD(X)	((X) >> 5)
> +
> +#define AD242X_BECNT			0x1f
> +
> +#define AD242X_TESTMODE			0x20
> +#define AD242X_TESTMODE_PRBSUP		BIT(0)
> +#define AD242X_TESTMODE_PRBSDN		BIT(1)
> +#define AD242X_TESTMODE_PRBSN2N		BIT(2)
> +#define AD242X_TESTMODE_RXDPTH(X)	((X) >> 4)
> +
> +#define AD242X_ERRCNT0			0x21
> +#define AD242X_ERRCNT1			0x22
> +#define AD242X_ERRCNT2			0x23
> +#define AD242X_ERRCNT3			0x24
> +
> +#define AD242X_NODE			0x29
> +#define AD242X_NODE_MASK		0xf
> +#define AD242X_NODE_DISCVD		BIT(5)
> +#define AD242X_NODE_NLAST		BIT(6)
> +#define AD242X_NODE_LAST		BIT(7)
> +
> +#define AD242X_DISCSTAT			0x2b
> +#define AD242X_DISCSTAT_DNODE(X)	((X) & 0xf)
> +#define AD242X_DISCSTAT_DSCACT		BIT(7)
> +
> +#define AD242X_TXACTL			0x2e
> +#define AD242X_TXACTL_LEVEL_HIGH	0
> +#define AD242X_TXACTL_LEVEL_MEDIUM	2
> +#define AD242X_TXACTL_LEVEL_LOW		3
> +
> +#define AD242X_TXBCTL			0x30
> +#define AD242X_TXBCTL_LEVEL_HIGH	0
> +#define AD242X_TXBCTL_LEVEL_MEDIUM	2
> +#define AD242X_TXBCTL_LEVEL_LOW		3
> +
> +#define AD242X_LINTTYPE			0x3e
> +
> +#define AD242X_I2CCFG			0x3f
> +#define AD242X_I2CCFG_DATARATE		BIT(0)
> +#define AD242X_I2CCFG_EACK		BIT(1)
> +#define AD242X_I2CCFG_FRAMERATE		BIT(2)
> +
> +#define AD242X_PLLCTL			0x40
> +#define AD242X_PLLCTL_SSFREQ(X)		((X) & 3)
> +#define AD242X_PLLCTL_SSDEPTH		BIT(2)
> +#define AD242X_PLLCTL_SSMODE_AB		(1 << 6)
> +#define AD242X_PLLCTL_SSMODE_AB_I2S	(2 << 6)
> +
> +#define AD242X_I2SGCTL			0x41
> +#define AD242X_I2SGCTL_TDMMODE(X)	((X) & 3)
> +#define AD242X_I2SGCTL_RXONDTX1		BIT(3)
> +#define AD242X_I2SGCTL_TDMSS		BIT(4)
> +#define AD242X_I2SGCTL_ALT		BIT(5)
> +#define AD242X_I2SGCTL_EARLY		BIT(6)
> +#define AD242X_I2SGCTL_INV		BIT(7)
> +
> +#define AD242X_I2SCTL			0x42
> +#define AD242X_I2SCTL_TX0EN		BIT(0)
> +#define AD242X_I2SCTL_TX1EN		BIT(1)
> +#define AD242X_I2SCTL_TX2PINTL		BIT(2)
> +#define AD242X_I2SCTL_TXBCLKINV		BIT(3)
> +#define AD242X_I2SCTL_RX0EN		BIT(4)
> +#define AD242X_I2SCTL_RX1EN		BIT(5)
> +#define AD242X_I2SCTL_RX2PINTL		BIT(6)
> +#define AD242X_I2SCTL_RXBCLKINV		BIT(7)
> +
> +#define AD242X_I2SRATE			0x43
> +#define AD242X_I2SRATE_I2SRATE(X)	((X) & 3)
> +#define AD242X_I2SRATE_BCLKRATE(X)	(((X) << 3) & 3)
> +#define AD242X_I2SRATE_REDUCE		BIT(6)
> +#define AD242X_I2SRATE_SHARE		BIT(7)
> +
> +#define AD242X_I2STXOFFSET		0x44
> +#define AD242X_I2STXOFFSET_VAR(X)	((X) & 0x3f)
> +#define AD242X_I2STXOFFSET_TSAFTER	BIT(6)
> +#define AD242X_I2STXOFFSET_TSBEFORE	BIT(7)
> +
> +#define AD242X_2SRXOFFSET		0x45
> +#define AD242X_I2SRXOFFSET_VAR(X)	((X) & 0x3f)
> +
> +#define AD242X_SYNCOFFSET		0x46
> +
> +#define AD242X_PDMCTL			0x47
> +#define AD242X_PDMCTL_PDM0EN		BIT(0)
> +#define AD242X_PDMCTL_PDM0SLOTS		BIT(1)
> +#define AD242X_PDMCTL_PDM1EN		BIT(2)
> +#define AD242X_PDMCTL_PDM1SLOTS		BIT(3)
> +#define AD242X_PDMCTL_HPFEN		BIT(4)
> +#define AD242X_PDMCTL_PDMRATE(X)	(((X) & 3) << 5)
> +
> +#define AD242X_ERRMGMT			0x48
> +#define AD242X_ERRMGMT_ERRLSB		BIT(0)
> +#define AD242X_ERRMGMT_ERRSIG		BIT(1)
> +#define AD242X_ERRMGMT_ERRSLOT		BIT(2)
> +
> +#define AD242X_GPIODAT			0x4a
> +#define AD242X_GPIODAT_SET		0x4b
> +#define AD242X_GPIODAT_CLR		0x4c
> +#define AD242X_GPIOOEN			0x4d
> +#define AD242X_GPIOIEN			0x4e
> +#define AD242X_GPIODAT_IN		0x4f
> +#define AD242X_PINTEN			0x50
> +#define AD242X_PINTINV			0x51
> +
> +#define AD242X_PINCFG			0x52
> +#define AD242X_PINCFG_DRVSTR		BIT(0)
> +#define AD242X_PINCFG_IRQINV		BIT(4)
> +#define AD242X_PINCFG_IRQTS		BIT(5)
> +
> +#define AD242X_I2STEST			0x53
> +#define AD242X_I2STEST_PATTRN2TX	BIT(0)
> +#define AD242X_I2STEST_LOOPBK2TX	BIT(1)
> +#define AD242X_I2STEST_RX2LOOPBK	BIT(2)
> +#define AD242X_I2STEST_SELRX1		BIT(3)
> +#define AD242X_I2STEST_BUSLOOPBK	BIT(4)
> +
> +#define AD242X_RAISE			0x54
> +
> +#define AD242X_GENERR			0x55
> +#define AD242X_GENERR_GENHCERR		BIT(0)
> +#define AD242X_GENERR_GENDDERR		BIT(1)
> +#define AD242X_GENERR_GENCRCERR		BIT(2)
> +#define AD242X_GENERR_GENDPERR		BIT(3)
> +#define AD242X_GENERR_GENICRCERR	BIT(4)
> +
> +#define AD242X_I2SRRATE			0x56
> +#define AD242X_I2SRRATE_RRDIV(X)	((X) & 0x3f)
> +#define AD242X_I2SRRATE_RBUS		BIT(7)
> +
> +#define AD242X_I2SRRCTL			0x57
> +#define AD242X_I2SRRCTL_ENVLSB		BIT(0)
> +#define AD242X_I2SRRCTL_ENXBIT		BIT(1)
> +#define AD242X_I2SRRCTL_ENSTRB		BIT(4)
> +#define AD242X_I2SRRCTL_STRBDIR		BIT(5)
> +
> +#define AD242X_I2SRRSOFFS		0x58
> +
> +#define AD242X_CLK1CFG			0x59
> +#define AD242X_CLK2CFG			0x5a
> +#define AD242X_CLKCFG_DIV(X)		((X) & 0xf)
> +#define AD242X_CLKCFG_DIVMSK		0xf
> +#define AD242X_CLKCFG_PDIV32		BIT(5)
> +#define AD242X_CLKCFG_INV		BIT(6)
> +#define AD242X_CLKCFG_EN		BIT(7)
> +
> +#define AD242X_BMMCFG			0x5b
> +#define AD242X_BMMCFG_BMMEN		BIT(0)
> +#define AD242X_BMMCFG_BMMRXEN		BIT(1)
> +#define AD242X_BMMCFG_BMMNDSC		BIT(2)
> +
> +#define AD242X_PDMCTL2			0x5d
> +#define AD242X_PDMCTL2_DEST_A2B		0
> +#define AD242X_PDMCTL2_DEST_DTX		1
> +#define AD242X_PDMCTL2_DEST_A2B_DTX	2
> +
> +#define AD242X_UPMASK(X)		(0x60 + ((X) & 3))
> +
> +#define AD242X_UPOFFSET			0x64
> +#define AD242X_UPOFFSET_VAL(X)		((X) & 0x1f)
> +
> +#define AD242X_DNMASK(X)		(0x65 + ((X) % 3))
> +
> +#define AD242X_DNOFFSET			0x69
> +#define AD242X_DNOFFSET_VAL(X)		((X) & 0x1f)
> +
> +#define AD242X_CHIPID(X)		((X) + 0x6a)
> +
> +#define AD242X_GPIODEN			0x80
> +#define AD242X_GPIOD_MSK(X)		((X) + 0x81)
> +
> +#define AD242X_GPIODDAT			0x89
> +#define AD242X_GPIODINV			0x8a
> +
> +#define AD242X_MAX_REG			0x9b
> +
> +static inline bool ad242x_is_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case AD242X_VENDOR:
> +	case AD242X_PRODUCT:
> +	case AD242X_VERSION:
> +	case AD242X_CAPABILITY:
> +	case AD242X_SWSTAT:
> +	case AD242X_INTSTAT:
> +	case AD242X_INTSRC:
> +	case AD242X_INTTYPE:
> +	case AD242X_INTPND0:
> +	case AD242X_INTPND1:
> +	case AD242X_INTPND2:
> +	case AD242X_BECNT:
> +	case AD242X_ERRCNT0:
> +	case AD242X_ERRCNT1:
> +	case AD242X_ERRCNT2:
> +	case AD242X_ERRCNT3:
> +	case AD242X_NODE:
> +	case AD242X_DISCSTAT:
> +	case AD242X_LINTTYPE:
> +	case AD242X_GPIODAT:
> +	case AD242X_GPIODAT_IN:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static inline bool ad242x_is_writeable_reg(struct device *dev, unsigned int reg)
> +{
> +	/* Write-to-clean registers */
> +	switch (reg) {
> +	case AD242X_INTPND0:
> +	case AD242X_INTPND1:
> +	case AD242X_INTPND2:
> +	case AD242X_BECNT:
> +		return true;
> +	default:
> +		return !ad242x_is_volatile_reg(dev, reg);
> +	}
> +}
> +
> +#define AD242X_MASTER_ID 0xff
> +
> +struct ad242x_master;
> +
> +struct ad242x_i2c_bus {
> +	struct i2c_client	*client;
> +	struct mutex		mutex;
> +};
> +
> +struct ad242x_node {
> +	struct device		*dev;
> +	struct regmap		*regmap;
> +	struct ad242x_master	*master;
> +	unsigned int		tdm_mode;
> +	unsigned int		tdm_slot_size;
> +	uint8_t			id;
> +	uint8_t			caps;
> +};
> +
> +struct ad242x_slot_config {
> +	unsigned int dn_rx_slots;
> +	unsigned int dn_n_tx_slots;
> +	unsigned int dn_n_forward_slots;
> +	unsigned int up_rx_slots;
> +	unsigned int up_n_tx_slots;
> +	unsigned int up_n_forward_slots;
> +};
> +
> +int ad242x_read_slot_config(struct device *dev,
> +			    struct device_node *np,
> +			    struct ad242x_slot_config *config);
> +
> +static inline bool ad242x_node_is_master(struct ad242x_node *node)
> +{
> +	return node->id == AD242X_MASTER_ID;
> +}
> +
> +int ad242x_node_probe(struct ad242x_node *node);
> +int ad242x_node_add_mfd_cells(struct device *dev);
> +
> +struct ad242x_node *ad242x_master_get_node(struct ad242x_master *master);
> +struct ad242x_i2c_bus *ad242x_master_get_bus(struct ad242x_master *master);
> +const char *ad242x_master_get_clk_name(struct ad242x_master *master);
> +unsigned int ad242x_master_get_clk_rate(struct ad242x_master *master);
> +
> +int ad242x_slave_read(struct ad242x_i2c_bus *bus,
> +		      struct regmap *master_regmap,
> +		      uint8_t node_id, uint8_t reg, unsigned int *val);
> +int ad242x_slave_write(struct ad242x_i2c_bus *bus,
> +		       struct regmap *master_regmap,
> +		       uint8_t node_id, uint8_t reg, unsigned int val);
> +
> +#endif

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog