Date:   Mon, 19 Mar 2018 13:45:39 +0100
From:   Peter Rosin <peda@...ntia.se>
To:     Ken Chen <chen.kenyy@...entec.com>, linux@...ck-us.net,
        wsa@...-dreams.de, linux-i2c@...r.kernel.org,
        linux-kernel@...r.kernel.org
Cc:     joel@....id.au
Subject: Re: [PATCH linux dev-4.16] drivers: i2c: master arbiter: create
 driver

Hi Ken,

Thanks for the patch!

I would have liked this subject:
i2c: muxes: pca9641: new driver

On 2018-03-19 12:38, Ken Chen wrote:
> Initial PCA9641 2 chennel I2C bus master arbiter

channel

> with separate implementation. It has probe issue
> and difference device hehavior between PCA9541

behavior

> and PCA9641 in original PCA9641 patch.
> 
> Signed-off-by: Ken Chen <chen.kenyy@...entec.com>
> ---
>  drivers/i2c/muxes/Kconfig           |   9 +
>  drivers/i2c/muxes/Makefile          |   1 +
>  drivers/i2c/muxes/i2c-mux-pca9641.c | 360 ++++++++++++++++++++++++++++++++++++

Given the big similarities between this and the pca9541 driver,
I would very much prefer if you could extend that driver instead
of making an almost-copy like this.

I have added some comments below anyway, but most of them are
irrelevant given that I want this merged with the pca9541 driver.

But maybe Guenter feels differently about that merge?

>  3 files changed, 370 insertions(+)
>  create mode 100644 drivers/i2c/muxes/i2c-mux-pca9641.c
> 
> diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
> index 52a4a92..f9c51b8 100644
> --- a/drivers/i2c/muxes/Kconfig
> +++ b/drivers/i2c/muxes/Kconfig
> @@ -73,6 +73,15 @@ config I2C_MUX_PCA954x
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called i2c-mux-pca954x.
>  
> +config I2C_MUX_PCA9641
> +	tristate "NXP PCA9641 I2C Master demultiplexer"
> +	help
> +	  If you say yes here you get support for the NXP PCA9641
> +	  I2C Master demultiplexer with an arbiter function.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called i2c-mux-pca9641.
> +
>  config I2C_MUX_PINCTRL
>  	tristate "pinctrl-based I2C multiplexer"
>  	depends on PINCTRL
> diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile
> index 6d9d865..a229a1a 100644
> --- a/drivers/i2c/muxes/Makefile
> +++ b/drivers/i2c/muxes/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_I2C_MUX_LTC4306)	+= i2c-mux-ltc4306.o
>  obj-$(CONFIG_I2C_MUX_MLXCPLD)	+= i2c-mux-mlxcpld.o
>  obj-$(CONFIG_I2C_MUX_PCA9541)	+= i2c-mux-pca9541.o
>  obj-$(CONFIG_I2C_MUX_PCA954x)	+= i2c-mux-pca954x.o
> +obj-$(CONFIG_I2C_MUX_PCA9641)	+= i2c-mux-pca9641.o
>  obj-$(CONFIG_I2C_MUX_PINCTRL)	+= i2c-mux-pinctrl.o
>  obj-$(CONFIG_I2C_MUX_REG)	+= i2c-mux-reg.o
>  
> diff --git a/drivers/i2c/muxes/i2c-mux-pca9641.c b/drivers/i2c/muxes/i2c-mux-pca9641.c
> new file mode 100644
> index 0000000..bcf45c8
> --- /dev/null
> +++ b/drivers/i2c/muxes/i2c-mux-pca9641.c
> @@ -0,0 +1,360 @@
> +/*
> + * I2C demultiplexer driver for PCA9641 bus master demultiplexer
> + *
> + * Copyright (c) 2010 Ericsson AB.
> + *
> + * Author: Ken Chen <chen.kenyy@...entec.com>

A bit rich to claim to be the sole author, when you clearly "just"
modified the pca9541 driver. Please state the history!

> + *
> + * Derived from:

Maybe you intended to add something here, but forgot?

> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/jiffies.h>
> +#include <linux/delay.h>
> +#include <linux/slab.h>
> +#include <linux/device.h>
> +#include <linux/i2c.h>
> +#include <linux/i2c-mux.h>
> +
> +#include <linux/i2c/pca954x.h>

What is this last include for? We have moved away from specifying platform
data in (new) code.

> +
> +/*
> + * The PCA9641 is two I2C bus masters demultiplexer. It supports two I2C masters

"is two I2C bus masters" -> "is a two I2C bus master"

> + * connected to a single slave bus.
> + *
> + * It is designed for high reliability dual master I2C bus applications where
> + * correct system operation is required, even when two I2C master issue their
> + * commands at the same time. The arbiter will select a winner and let it work
> + * uninterrupted, and the losing master will take control of the I2C bus after
> + * the winnter has finished. The arbiter also allows for queued requests where

winner

> + * a master requests the downstream bus while the other master has control.
> + *
> + */
> +
> +#define PCA9641_ID		0x01
> +#define PCA9641_ID_MAGIC	0x38
> +
> +#define PCA9641_CONTROL		0x01
> +#define PCA9641_STATUS		0x02
> +#define PCA9641_TIME		0x03
> +
> +#define PCA9641_CTL_LOCK_REQ		BIT(0)
> +#define PCA9641_CTL_LOCK_GRANT		BIT(1)
> +#define PCA9641_CTL_BUS_CONNECT		BIT(2)
> +#define PCA9641_CTL_BUS_INIT		BIT(3)
> +#define PCA9641_CTL_SMBUS_SWRST		BIT(4)
> +#define PCA9641_CTL_IDLE_TIMER_DIS	BIT(5)
> +#define PCA9641_CTL_SMBUS_DIS		BIT(6)
> +#define PCA9641_CTL_PRIORITY		BIT(7)
> +
> +#define PCA9641_STS_OTHER_LOCK		BIT(0)
> +#define PCA9641_STS_BUS_INIT_FAIL	BIT(1)
> +#define PCA9641_STS_BUS_HUNG		BIT(2)
> +#define PCA9641_STS_MBOX_EMPTY		BIT(3)
> +#define PCA9641_STS_MBOX_FULL		BIT(4)
> +#define PCA9641_STS_TEST_INT		BIT(5)
> +#define PCA9641_STS_SCL_IO		BIT(6)
> +#define PCA9641_STS_SDA_IO		BIT(7)
> +
> +#define PCA9641_RES_TIME	0x03
> +
> +#define busoff(x, y)	(!((x) & PCA9641_CTL_LOCK_GRANT) && \
> +			!((y) & PCA9641_STS_OTHER_LOCK))
> +#define other_lock(x)	((x) & PCA9641_STS_OTHER_LOCK)
> +#define lock_grant(x)	((x) & PCA9641_CTL_LOCK_GRANT)
> +
> +/* arbitration timeouts, in jiffies */
> +#define ARB_TIMEOUT	(HZ / 8)	/* 125 ms until forcing bus ownership */
> +#define ARB2_TIMEOUT	(HZ / 4)	/* 250 ms until acquisition failure */
> +
> +/* arbitration retry delays, in us */
> +#define SELECT_DELAY_SHORT	50
> +#define SELECT_DELAY_LONG	1000
> +
> +struct pca9641 {
> +	struct i2c_client *client;
> +	unsigned long select_timeout;
> +	unsigned long arb_timeout;
> +};
> +
> +static const struct i2c_device_id pca9641_id[] = {
> +	{"pca9641", 0},
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, pca9641_id);
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id pca9641_of_match[] = {
> +	{ .compatible = "nxp,pca9641" },

You need to describe the DT binding in Documentation/devicetree/bindings/i2c
See nxp,pca9541.txt for inspiration.

> +	{}
> +};
> +#endif
> +
> +/*
> + * Write to chip register. Don't use i2c_transfer()/i2c_smbus_xfer()
> + * as they will try to lock the adapter a second time.
> + */
> +static int pca9641_reg_write(struct i2c_client *client, u8 command, u8 val)
> +{
> +	struct i2c_adapter *adap = client->adapter;
> +	int ret;
> +
> +	if (adap->algo->master_xfer) {
> +		struct i2c_msg msg;
> +		char buf[2];
> +
> +		msg.addr = client->addr;
> +		msg.flags = 0;
> +		msg.len = 2;
> +		buf[0] = command;
> +		buf[1] = val;
> +		msg.buf = buf;
> +		ret = __i2c_transfer(adap, &msg, 1);
> +	} else {
> +		union i2c_smbus_data data;
> +
> +		data.byte = val;
> +		ret = adap->algo->smbus_xfer(adap, client->addr,
> +					     client->flags,
> +					     I2C_SMBUS_WRITE,
> +					     command,
> +					     I2C_SMBUS_BYTE_DATA, &data);
> +	}
> +
> +	return ret;
> +}
> +
> +/*
> + * Read from chip register. Don't use i2c_transfer()/i2c_smbus_xfer()
> + * as they will try to lock adapter a second time.
> + */
> +static int pca9641_reg_read(struct i2c_client *client, u8 command)
> +{
> +	struct i2c_adapter *adap = client->adapter;
> +	int ret;
> +	u8 val;
> +
> +	if (adap->algo->master_xfer) {
> +		struct i2c_msg msg[2] = {
> +			{
> +				.addr = client->addr,
> +				.flags = 0,
> +				.len = 1,
> +				.buf = &command
> +			},
> +			{
> +				.addr = client->addr,
> +				.flags = I2C_M_RD,
> +				.len = 1,
> +				.buf = &val
> +			}
> +		};
> +		ret = __i2c_transfer(adap, msg, 2);
> +		if (ret == 2)
> +			ret = val;
> +		else if (ret >= 0)
> +			ret = -EIO;
> +	} else {
> +		union i2c_smbus_data data;
> +
> +		ret = adap->algo->smbus_xfer(adap, client->addr,
> +					     client->flags,
> +					     I2C_SMBUS_READ,
> +					     command,
> +					     I2C_SMBUS_BYTE_DATA, &data);
> +		if (!ret)
> +			ret = data.byte;
> +	}
> +	return ret;
> +}
> +
> +/*
> + * Arbitration management functions
> + */
> +static void pca9641_release_bus(struct i2c_client *client)
> +{
> +	pca9641_reg_write(client, PCA9641_CONTROL, 0);
> +}
> +
> +/*
> + * Channel arbitration
> + *
> + * Return values:
> + *  <0: error
> + *  0 : bus not acquired
> + *  1 : bus acquired
> + */
> +static int pca9641_arbitrate(struct i2c_client *client)
> +{
> +	struct i2c_mux_core *muxc = i2c_get_clientdata(client);
> +	struct pca9641 *data = i2c_mux_priv(muxc);
> +	int reg_ctl, reg_sts;
> +
> +	reg_ctl = pca9641_reg_read(client, PCA9641_CONTROL);
> +	if (reg_ctl < 0)
> +		return reg_ctl;
> +	reg_sts = pca9641_reg_read(client, PCA9641_STATUS);
> +
> +	if (busoff(reg_ctl, reg_sts)) {
> +		/*
> +		 * Bus is off. Request ownership or turn it on unless
> +		 * other master requested ownership.
> +		 */
> +		reg_ctl |= PCA9641_CTL_LOCK_REQ;
> +		pca9641_reg_write(client, PCA9641_CONTROL, reg_ctl);
> +		reg_ctl = pca9641_reg_read(client, PCA9641_CONTROL);
> +
> +		if (lock_grant(reg_ctl)) {
> +			/*
> +			 * Other master did not request ownership,
> +			 * or arbitration timeout expired. Take the bus.
> +			 */
> +			reg_ctl |= PCA9641_CTL_BUS_CONNECT \
> +					| PCA9641_CTL_LOCK_REQ;
> +			pca9641_reg_write(client, PCA9641_CONTROL, reg_ctl);
> +			data->select_timeout = SELECT_DELAY_SHORT;
> +
> +			return 1;
> +		} else {
> +			/*
> +			 * Other master requested ownership.
> +			 * Set extra long timeout to give it time to acquire it.
> +			 */
> +			data->select_timeout = SELECT_DELAY_LONG * 2;
> +		}
> +	} else if (lock_grant(reg_ctl)) {
> +		/*
> +		 * Bus is on, and we own it. We are done with acquisition.
> +		 */
> +		reg_ctl |= PCA9641_CTL_BUS_CONNECT | PCA9641_CTL_LOCK_REQ;
> +		pca9641_reg_write(client, PCA9641_CONTROL, reg_ctl);
> +
> +		return 1;
> +	} else if (other_lock(reg_sts)) {
> +		/*
> +		 * Other master owns the bus.
> +		 * If arbitration timeout has expired, force ownership.
> +		 * Otherwise request it.
> +		 */
> +		data->select_timeout = SELECT_DELAY_LONG;
> +		reg_ctl |= PCA9641_CTL_LOCK_REQ;
> +		pca9641_reg_write(client, PCA9641_CONTROL, reg_ctl);
> +	}
> +	return 0;
> +}
> +
> +static int pca9641_select_chan(struct i2c_mux_core *muxc, u32 chan)
> +{
> +	struct pca9641 *data = i2c_mux_priv(muxc);
> +	struct i2c_client *client = data->client;
> +	int ret;
> +	unsigned long timeout = jiffies + ARB2_TIMEOUT;
> +		/* give up after this time */
> +
> +	data->arb_timeout = jiffies + ARB_TIMEOUT;
> +		/* force bus ownership after this time */
> +
> +	do {
> +		ret = pca9641_arbitrate(client);
> +		if (ret)
> +			return ret < 0 ? ret : 0;
> +
> +		if (data->select_timeout == SELECT_DELAY_SHORT)
> +			udelay(data->select_timeout);
> +		else
> +			msleep(data->select_timeout / 1000);
> +	} while (time_is_after_eq_jiffies(timeout));
> +
> +	return -ETIMEDOUT;
> +}
> +
> +static int pca9641_release_chan(struct i2c_mux_core *muxc, u32 chan)
> +{
> +	struct pca9641 *data = i2c_mux_priv(muxc);
> +	struct i2c_client *client = data->client;
> +
> +	pca9641_release_bus(client);
> +	return 0;
> +}
> +
> +/*
> + * I2C init/probing/exit functions
> + */
> +static int pca9641_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	struct i2c_adapter *adap = client->adapter;
> +	struct pca954x_platform_data *pdata = dev_get_platdata(&client->dev);
> +	struct i2c_mux_core *muxc;
> +	struct pca9641 *data;
> +	int force;
> +	int ret;
> +
> +	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE_DATA))
> +		return -ENODEV;

Reading the data sheet, I noticed that the chip supports I2C device id.
There's a brand new function available in -next [1] that allows you to
check this. See the pca954x driver in -next for an example [2]. It checks
the I2C device id for the newer pca984x chips.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/i2c/i2c-core-base.c
i2c_get_device_id(), currently circa line 2000

[2] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/i2c/muxes/i2c-mux-pca954x.c

> +	/*
> +	 * I2C accesses are unprotected here.
> +	 * We have to lock the adapter before releasing the bus.
> +	 */
> +	i2c_lock_adapter(adap);
> +	pca9641_release_bus(client);
> +	i2c_unlock_adapter(adap);
> +
> +	/* Create mux adapter */
> +
> +	force = 0;
> +	if (pdata)
> +		force = pdata->modes[0].adap_id;
> +	muxc = i2c_mux_alloc(adap, &client->dev, 8, sizeof(*data),

Why 8? Should be 1, no?

> +			     I2C_MUX_ARBITRATOR,
> +			     pca9641_select_chan, pca9641_release_chan);
> +	if (!muxc)
> +		return -ENOMEM;
> +
> +	data = i2c_mux_priv(muxc);
> +	data->client = client;
> +
> +	i2c_set_clientdata(client, muxc);
> +
> +

Lose one of the empty lines here.

> +	ret = i2c_mux_add_adapter(muxc, force, 0, 0);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to register master demultiplexer\n");

This dev_err should go. i2c_mux_add_adapter provides a more
detailed error message on failure.

Cheers,
Peter

> +		return ret;
> +	}
> +
> +	dev_info(&client->dev, "registered master demultiplexer for I2C %s\n",
> +		 client->name);
> +
> +	return 0;
> +}
> +
> +static int pca9641_remove(struct i2c_client *client)
> +{
> +	struct i2c_mux_core *muxc = i2c_get_clientdata(client);
> +
> +	i2c_mux_del_adapters(muxc);
> +	return 0;
> +}
> +
> +static struct i2c_driver pca9641_driver = {
> +	.driver = {
> +		   .name = "pca9641",
> +		   .of_match_table = of_match_ptr(pca9641_of_match),
> +		   },
> +	.probe = pca9641_probe,
> +	.remove = pca9641_remove,
> +	.id_table = pca9641_id,
> +};
> +
> +module_i2c_driver(pca9641_driver);
> +
> +MODULE_AUTHOR("Ken Chen <chen.kenyy@...entec.com>");
> +MODULE_DESCRIPTION("PCA9641 I2C master demultiplexer driver");
> +MODULE_LICENSE("GPL v2");
> 

Powered by blists - more mailing lists