[07/10] i2c: Add driver for AD242x bus controller
diff mbox series

Message ID 20191209183511.3576038-9-daniel@zonque.org
State New
Headers show
Series
  • mfd: Add support for Analog Devices A2B transceiver
Related show

Commit Message

Daniel Mack Dec. 9, 2019, 6:35 p.m. UTC
This device must be instantiated as a sub-device of the AD242x MFD
device.

In order to access remote I2C peripherals, the master node is configured
to the slave node number and the remote I2C client address on the remote
side, and then the payload is sent to the BUS client of the master node,
which transparently proxies the traffic through.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 drivers/i2c/busses/Kconfig      |  10 ++
 drivers/i2c/busses/Makefile     |   1 +
 drivers/i2c/busses/i2c-ad242x.c | 178 ++++++++++++++++++++++++++++++++
 3 files changed, 189 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-ad242x.c

Comments

Luca Ceresoli Dec. 12, 2019, 4:11 p.m. UTC | #1
Hi Daniel,

On 09/12/19 19:35, Daniel Mack wrote:
> This device must be instantiated as a sub-device of the AD242x MFD
> device.
> 
> In order to access remote I2C peripherals, the master node is configured
> to the slave node number and the remote I2C client address on the remote
> side, and then the payload is sent to the BUS client of the master node,
> which transparently proxies the traffic through.

This remote I2C feature in these chips is interesting. It looks somewhat
similar to remote I2C in the video serdes chip by TI and Maxim, but it's
different from both of them. So now we have 3 vendors implementing the
same feature in 3 different ways.

Cool.

> Signed-off-by: Daniel Mack <daniel@zonque.org>
> ---
>  drivers/i2c/busses/Kconfig      |  10 ++
>  drivers/i2c/busses/Makefile     |   1 +
>  drivers/i2c/busses/i2c-ad242x.c | 178 ++++++++++++++++++++++++++++++++
>  3 files changed, 189 insertions(+)
>  create mode 100644 drivers/i2c/busses/i2c-ad242x.c
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 6a0aa76859f3..b9cf049bedb0 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -365,6 +365,16 @@ config I2C_POWERMAC
>  
>  comment "I2C system bus drivers (mostly embedded / system-on-chip)"
>  
> +config I2C_AD242X
> +	tristate "Analog Devices AD242x"
> +	depends on MFD_AD242X
> +	help
> +	  If you say yes to this option, support will be included for the
> +	  I2C bus controller function of AD242x slave nodes.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called i2c-ad242x.
> +
>  config I2C_ALTERA
>  	tristate "Altera Soft IP I2C"
>  	depends on (ARCH_SOCFPGA || NIOS2) && OF
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 3ab8aebc39c9..57c31ea8a477 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -32,6 +32,7 @@ obj-$(CONFIG_I2C_HYDRA)		+= i2c-hydra.o
>  obj-$(CONFIG_I2C_POWERMAC)	+= i2c-powermac.o
>  
>  # Embedded system I2C/SMBus host controller drivers
> +obj-$(CONFIG_I2C_AD242X)	+= i2c-ad242x.o
>  obj-$(CONFIG_I2C_ALTERA)	+= i2c-altera.o
>  obj-$(CONFIG_I2C_AMD_MP2)	+= i2c-amd-mp2-pci.o i2c-amd-mp2-plat.o
>  obj-$(CONFIG_I2C_ASPEED)	+= i2c-aspeed.o
> diff --git a/drivers/i2c/busses/i2c-ad242x.c b/drivers/i2c/busses/i2c-ad242x.c
> new file mode 100644
> index 000000000000..b94056653898
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-ad242x.c
> @@ -0,0 +1,178 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mfd/ad242x.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +struct ad242x_i2c {
> +	struct device		*dev;
> +	struct ad242x_node	*node;
> +	struct i2c_adapter	adap;
> +	u32			node_index;
> +};
> +
> +static int ad242x_set_addr(struct ad242x_node *mnode,
> +			   struct ad242x_i2c_bus *bus,
> +			   uint8_t node_id, uint8_t addr)
> +{
> +	int ret;
> +	uint8_t buf[2] = { AD242X_CHIP, addr };
> +
> +	ret = regmap_update_bits(mnode->regmap, AD242X_NODEADR,
> +				 AD242X_NODEADR_PERI | AD242X_NODEADR_MASK,
> +				 node_id);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * We can't use the slave's regmap here as it holds the same
> +	 * lock we also need to guard this context.
> +	 */
> +	ret = i2c_transfer_buffer_flags(bus->client,
> +					buf, sizeof(buf), 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	return regmap_update_bits(mnode->regmap, AD242X_NODEADR,
> +				  AD242X_NODEADR_PERI, AD242X_NODEADR_PERI);
> +}
> +
> +static int ad242x_i2c_xfer(struct i2c_adapter *adap,
> +			   struct i2c_msg msgs[], int num)
> +{
> +	struct ad242x_i2c *i2c = adap->algo_data;
> +	struct ad242x_i2c_bus *bus = ad242x_master_get_bus(i2c->node->master);
> +	struct ad242x_node *mnode = ad242x_master_get_node(i2c->node->master);
> +	int ret, i, current_addr = -1;
> +
> +	mutex_lock(&bus->mutex);
> +
> +	for (i = 0; i < num; i++) {
> +		struct i2c_msg *msg = msgs + i;
> +
> +		if (msg->addr != current_addr) {
> +			ret = ad242x_set_addr(mnode, bus,
> +					      i2c->node->id, msg->addr);
> +			if (ret < 0) {
> +				dev_err(i2c->node->dev,
> +					"Cannot set address: %d\n", ret);
> +				break;
> +			}
> +
> +			current_addr = msg->addr;
> +		}
> +
> +		ret = i2c_transfer_buffer_flags(bus->client,
> +						msg->buf, msg->len, msg->flags);
> +		if (ret < 0)
> +			break;
> +	}
> +
> +	mutex_unlock(&bus->mutex);
> +
> +	return ret < 0 ? ret : num;
> +}

Your implementation here looks quite clean and simple, and simple is
good, but I think there's a problem in this function. A "normal"
master_xfer function issues a repeated start between one msg and the
next one, at least in the typical case where all msgs have the same
slave address. Your implementation breaks repeated start. At first sight
we might need more complex code here to coalesce all consecutive msgs
with the same address into a single i2c_transfer() call.
Wolfram Sang Dec. 12, 2019, 4:33 p.m. UTC | #2
Hi Luca,

thanks for the review!

> good, but I think there's a problem in this function. A "normal"
> master_xfer function issues a repeated start between one msg and the
> next one, at least in the typical case where all msgs have the same
> slave address. Your implementation breaks repeated start. At first sight
> we might need more complex code here to coalesce all consecutive msgs
> with the same address into a single i2c_transfer() call.

Note that it is by far the standard case that all messages in a transfer
have the same client address (99,999%?). But technically, this is not a
requirement and the repeated start on the bus is totally independent of
the addresses used. It is just a master wanting to send without being
interrupted by another master.

   Wolfram
Daniel Mack Dec. 15, 2019, 8:27 p.m. UTC | #3
Hi,

Thanks for the review!

On 12/12/2019 5:33 pm, Wolfram Sang wrote:
> Hi Luca,
> 
> thanks for the review!
> 
>> good, but I think there's a problem in this function. A "normal"
>> master_xfer function issues a repeated start between one msg and the
>> next one, at least in the typical case where all msgs have the same
>> slave address. Your implementation breaks repeated start. At first sight
>> we might need more complex code here to coalesce all consecutive msgs
>> with the same address into a single i2c_transfer() call.
> 
> Note that it is by far the standard case that all messages in a transfer
> have the same client address (99,999%?). But technically, this is not a
> requirement and the repeated start on the bus is totally independent of
> the addresses used. It is just a master wanting to send without being
> interrupted by another master.

I'm not quite sure I understand.

Let's assume the following setup. An i2c client (some driver code) is
sending a list of messages to the a2b xfer function, which in turn is
logically connected to a 'real' i2c bus master that'll put the data on
the wire.

The a2b code has to tell the 'master node' the final destination of the
payload by programming registers on its primary i2c address, and then
forwards the messages to its secondary i2c address. The layout of the
messages don't change, and neither do the flags; i2c messages are being
sent as i2c messages, except their addresses are changed, a bit like NAT
in networking. That procedure is described on page 3-4 of the TRM,
"Remote Peripheral I2C Accesses".

The 'real' i2c master that handles the hardware bus is responsible for
adding start conditions, and as the messages as such are untouched, I
believe it should do the right thing. The code in my xfer functions
merely suppresses reprogramming remote addresses by remembering the last
one that was used, but that is independent of the start conditions on
the wire.

Maybe I'm missing anything. Could you provide an example that explains
in which case this approach would leads to issues?


Thanks,
Daniel
Luca Ceresoli Dec. 17, 2019, 8:35 a.m. UTC | #4
Hi Daniel,

On 15/12/19 21:27, Daniel Mack wrote:
> Hi,
> 
> Thanks for the review!
> 
> On 12/12/2019 5:33 pm, Wolfram Sang wrote:
>> Hi Luca,
>>
>> thanks for the review!
>>
>>> good, but I think there's a problem in this function. A "normal"
>>> master_xfer function issues a repeated start between one msg and the
>>> next one, at least in the typical case where all msgs have the same
>>> slave address. Your implementation breaks repeated start. At first sight
>>> we might need more complex code here to coalesce all consecutive msgs
>>> with the same address into a single i2c_transfer() call.
>>
>> Note that it is by far the standard case that all messages in a transfer
>> have the same client address (99,999%?). But technically, this is not a
>> requirement and the repeated start on the bus is totally independent of
>> the addresses used. It is just a master wanting to send without being
>> interrupted by another master.
> 
> I'm not quite sure I understand.
> 
> Let's assume the following setup. An i2c client (some driver code) is
> sending a list of messages to the a2b xfer function, which in turn is
> logically connected to a 'real' i2c bus master that'll put the data on
> the wire.
> 
> The a2b code has to tell the 'master node' the final destination of the
> payload by programming registers on its primary i2c address, and then
> forwards the messages to its secondary i2c address. The layout of the
> messages don't change, and neither do the flags; i2c messages are being
> sent as i2c messages, except their addresses are changed, a bit like NAT
> in networking. That procedure is described on page 3-4 of the TRM,
> "Remote Peripheral I2C Accesses".
> 
> The 'real' i2c master that handles the hardware bus is responsible for
> adding start conditions, and as the messages as such are untouched, I
> believe it should do the right thing. The code in my xfer functions
> merely suppresses reprogramming remote addresses by remembering the last
> one that was used, but that is independent of the start conditions on
> the wire.

My concern is not about the start condition, it's about the *repeated*
start condition.

The first question is whether the A2B chips can do it. What if the host
processor sets a slave chip address and then issues two messages
separated by a repeated start condition? Will the slave transceiver emit
a repeated start condition too?

If the answer is "yes", then the issue moves to the driver code. A
master xfer function receives a set of messages that are normally
emitted with a repeated start between each other. But ad242x_i2c_xfer()
splits the msgs and calls i2c_transfer_buffer_flags() with one msg at a
time. i2c_transfer_buffer_flags() then will emit a stop condition.

This is not necessarily a problem, unless multi-master is used, but if
there are limitations or deviations from the standard they should at
least be well known and documented.
Daniel Mack Dec. 17, 2019, 6:17 p.m. UTC | #5
Hi Luca,

On 12/17/19 9:35 AM, Luca Ceresoli wrote:
> On 15/12/19 21:27, Daniel Mack wrote:

>> The a2b code has to tell the 'master node' the final destination of the
>> payload by programming registers on its primary i2c address, and then
>> forwards the messages to its secondary i2c address. The layout of the
>> messages don't change, and neither do the flags; i2c messages are being
>> sent as i2c messages, except their addresses are changed, a bit like NAT
>> in networking. That procedure is described on page 3-4 of the TRM,
>> "Remote Peripheral I2C Accesses".
>>
>> The 'real' i2c master that handles the hardware bus is responsible for
>> adding start conditions, and as the messages as such are untouched, I
>> believe it should do the right thing. The code in my xfer functions
>> merely suppresses reprogramming remote addresses by remembering the last
>> one that was used, but that is independent of the start conditions on
>> the wire.
> 
> My concern is not about the start condition, it's about the *repeated*
> start condition.
> 
> The first question is whether the A2B chips can do it. What if the host
> processor sets a slave chip address and then issues two messages
> separated by a repeated start condition? Will the slave transceiver emit
> a repeated start condition too?

Ah, alright. Thanks for taking the time to explain. I'll have to do some 
measurements with a hardware analyzer. Will revisit this then, and 
either provide an implementation that handles such cases correctly, or a 
comment to explain that the hardware can't do it.


Best regards,
Daniel

Patch
diff mbox series

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 6a0aa76859f3..b9cf049bedb0 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -365,6 +365,16 @@  config I2C_POWERMAC
 
 comment "I2C system bus drivers (mostly embedded / system-on-chip)"
 
+config I2C_AD242X
+	tristate "Analog Devices AD242x"
+	depends on MFD_AD242X
+	help
+	  If you say yes to this option, support will be included for the
+	  I2C bus controller function of AD242x slave nodes.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-ad242x.
+
 config I2C_ALTERA
 	tristate "Altera Soft IP I2C"
 	depends on (ARCH_SOCFPGA || NIOS2) && OF
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 3ab8aebc39c9..57c31ea8a477 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -32,6 +32,7 @@  obj-$(CONFIG_I2C_HYDRA)		+= i2c-hydra.o
 obj-$(CONFIG_I2C_POWERMAC)	+= i2c-powermac.o
 
 # Embedded system I2C/SMBus host controller drivers
+obj-$(CONFIG_I2C_AD242X)	+= i2c-ad242x.o
 obj-$(CONFIG_I2C_ALTERA)	+= i2c-altera.o
 obj-$(CONFIG_I2C_AMD_MP2)	+= i2c-amd-mp2-pci.o i2c-amd-mp2-plat.o
 obj-$(CONFIG_I2C_ASPEED)	+= i2c-aspeed.o
diff --git a/drivers/i2c/busses/i2c-ad242x.c b/drivers/i2c/busses/i2c-ad242x.c
new file mode 100644
index 000000000000..b94056653898
--- /dev/null
+++ b/drivers/i2c/busses/i2c-ad242x.c
@@ -0,0 +1,178 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mfd/ad242x.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+struct ad242x_i2c {
+	struct device		*dev;
+	struct ad242x_node	*node;
+	struct i2c_adapter	adap;
+	u32			node_index;
+};
+
+static int ad242x_set_addr(struct ad242x_node *mnode,
+			   struct ad242x_i2c_bus *bus,
+			   uint8_t node_id, uint8_t addr)
+{
+	int ret;
+	uint8_t buf[2] = { AD242X_CHIP, addr };
+
+	ret = regmap_update_bits(mnode->regmap, AD242X_NODEADR,
+				 AD242X_NODEADR_PERI | AD242X_NODEADR_MASK,
+				 node_id);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * We can't use the slave's regmap here as it holds the same
+	 * lock we also need to guard this context.
+	 */
+	ret = i2c_transfer_buffer_flags(bus->client,
+					buf, sizeof(buf), 0);
+	if (ret < 0)
+		return ret;
+
+	return regmap_update_bits(mnode->regmap, AD242X_NODEADR,
+				  AD242X_NODEADR_PERI, AD242X_NODEADR_PERI);
+}
+
+static int ad242x_i2c_xfer(struct i2c_adapter *adap,
+			   struct i2c_msg msgs[], int num)
+{
+	struct ad242x_i2c *i2c = adap->algo_data;
+	struct ad242x_i2c_bus *bus = ad242x_master_get_bus(i2c->node->master);
+	struct ad242x_node *mnode = ad242x_master_get_node(i2c->node->master);
+	int ret, i, current_addr = -1;
+
+	mutex_lock(&bus->mutex);
+
+	for (i = 0; i < num; i++) {
+		struct i2c_msg *msg = msgs + i;
+
+		if (msg->addr != current_addr) {
+			ret = ad242x_set_addr(mnode, bus,
+					      i2c->node->id, msg->addr);
+			if (ret < 0) {
+				dev_err(i2c->node->dev,
+					"Cannot set address: %d\n", ret);
+				break;
+			}
+
+			current_addr = msg->addr;
+		}
+
+		ret = i2c_transfer_buffer_flags(bus->client,
+						msg->buf, msg->len, msg->flags);
+		if (ret < 0)
+			break;
+	}
+
+	mutex_unlock(&bus->mutex);
+
+	return ret < 0 ? ret : num;
+}
+
+static u32 ad242x_i2c_functionality(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm ad242x_i2c_algorithm = {
+	.master_xfer	= ad242x_i2c_xfer,
+	.functionality	= ad242x_i2c_functionality,
+};
+
+static int ad242x_i2c_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct ad242x_node *node;
+	struct ad242x_i2c *i2c;
+	u32 freq, val = 0;
+	int ret;
+
+	if (!dev->of_node)
+		return -ENODEV;
+
+	node = dev_get_drvdata(dev->parent);
+	if ((node->caps & AD242X_CAPABILITY_I2C) == 0) {
+		dev_err(dev, "Node %d has no I2C capability", node->id);
+		return -ENOTSUPP;
+	}
+
+	if (ad242x_node_is_master(node))
+		return -EINVAL;
+
+	freq = ad242x_master_get_clk_rate(node->master);
+	if (freq == 44100)
+		val |= AD242X_I2CCFG_FRAMERATE;
+
+	if (!of_property_read_u32(dev->of_node, "clock-frequency", &freq)) {
+		if (freq == 400000)
+			val |= AD242X_I2CCFG_DATARATE;
+		else if (freq != 100000)
+			dev_warn(dev, "Unsupported frequency %d\n", freq);
+	}
+
+	ret = regmap_write(node->regmap, AD242X_I2CCFG, val);
+	if (ret < 0)
+		return ret;
+
+	i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
+	if (!i2c)
+		return -ENOMEM;
+
+	i2c->node = node;
+	i2c->adap.algo = &ad242x_i2c_algorithm;
+	i2c->adap.algo_data = i2c;
+	i2c->adap.dev.parent = dev;
+	i2c->adap.dev.of_node = dev->of_node;
+	i2c_set_adapdata(&i2c->adap, i2c);
+	strlcpy(i2c->adap.name, "ad242x remote I2C bus",
+		sizeof(i2c->adap.name));
+
+	ret = i2c_add_adapter(&i2c->adap);
+	if (ret < 0) {
+		dev_err(dev, "error registering adapter: %d\n", ret);
+		return ret;
+	}
+
+	dev_info(dev, "ad242x i2c driver, node ID %d\n", node->id);
+	platform_set_drvdata(pdev, i2c);
+
+	return 0;
+}
+
+static int ad242x_i2c_remove(struct platform_device *dev)
+{
+	struct ad242x_i2c *i2c = platform_get_drvdata(dev);
+
+	i2c_del_adapter(&i2c->adap);
+
+	return 0;
+}
+
+static const struct of_device_id ad242x_i2c_of_match[] = {
+	{ .compatible = "adi,ad2428w-i2c" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ad242x_i2c_of_match);
+
+static struct platform_driver ad242x_i2c_driver = {
+	.driver = {
+		.name = "ad242x-i2c",
+		.of_match_table = ad242x_i2c_of_match,
+	},
+	.probe = ad242x_i2c_probe,
+	.remove = ad242x_i2c_remove,
+};
+
+module_platform_driver(ad242x_i2c_driver);
+MODULE_LICENSE("GPL");