[PATCH] i2c: Add support for NXP PCA984x family.

[PATCH] i2c: Add support for NXP PCA984x family.

[Adrian Fiergolski]

This patch exetends the current i2c-mux-pca954x driver and adds suuport for
a newer PCA984x family of the I2C switches and multiplexers from NXP.
In probe function, the patch supports device ID function, introduced in the
new family, and checks it against configuration provided by the
devicetree. Moreover, it also performs software reset which is now
available for the new devices.
The reset of the code remains common with the legacy PCA954x devices.

Signed-off-by: Adrian Fiergolski <adrian.fiergolski@cern.ch>
---
 .../{i2c-mux-pca954x.txt => i2c-mux-pca9x4x.txt}   |   5 +-
 arch/arm/configs/aspeed_g4_defconfig               |   2 +-
 arch/arm/configs/aspeed_g5_defconfig               |   2 +-
 arch/arm/configs/multi_v7_defconfig                |   2 +-
 arch/arm/configs/pxa_defconfig                     |   2 +-
 arch/arm/configs/tegra_defconfig                   |   2 +-
 arch/arm64/configs/defconfig                       |   2 +-
 arch/powerpc/configs/85xx-hw.config                |   2 +-
 drivers/i2c/muxes/Kconfig                          |   6 +-
 drivers/i2c/muxes/Makefile                         |   2 +-
 drivers/i2c/muxes/i2c-mux-pca9541.c                |   4 +-
 .../muxes/{i2c-mux-pca954x.c => i2c-mux-pca9x4x.c} | 295 ++++++++++++++++-----
 .../linux/platform_data/{pca954x.h => pca9x4x.h}   |  15 +-
 13 files changed, 246 insertions(+), 95 deletions(-)
 rename Documentation/devicetree/bindings/i2c/{i2c-mux-pca954x.txt => i2c-mux-pca9x4x.txt} (91%)
 rename drivers/i2c/muxes/{i2c-mux-pca954x.c => i2c-mux-pca9x4x.c} (56%)
 rename include/linux/platform_data/{pca954x.h => pca9x4x.h} (80%)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca9x4x.txt
similarity index 91%
rename from Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
rename to Documentation/devicetree/bindings/i2c/i2c-mux-pca9x4x.txt
index aa097045a10e..cf9a075ca1dd 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca9x4x.txt
@@ -1,10 +1,11 @@
-* NXP PCA954x I2C bus switch
+* NXP PCA9x4x I2C bus switch
 
 Required Properties:
 
   - compatible: Must contain one of the following.
     "nxp,pca9540", "nxp,pca9542", "nxp,pca9543", "nxp,pca9544",
-    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548"
+    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548",
+    "nxp,pca9846", "nxp,pca9847", "nxp,pca9848", "nxp,pca9849"
 
   - reg: The I2C address of the device.
 
diff --git a/arch/arm/configs/aspeed_g4_defconfig b/arch/arm/configs/aspeed_g4_defconfig
index d23b9d56a88b..a461ad3cf63d 100644
--- a/arch/arm/configs/aspeed_g4_defconfig
+++ b/arch/arm/configs/aspeed_g4_defconfig
@@ -116,7 +116,7 @@ CONFIG_I2C=y
 CONFIG_I2C_CHARDEV=y
 CONFIG_I2C_MUX=y
 CONFIG_I2C_MUX_PCA9541=y
-CONFIG_I2C_MUX_PCA954x=y
+CONFIG_I2C_MUX_PCA9x4x=y
 CONFIG_I2C_ASPEED=y
 CONFIG_GPIOLIB=y
 CONFIG_GPIO_SYSFS=y
diff --git a/arch/arm/configs/aspeed_g5_defconfig b/arch/arm/configs/aspeed_g5_defconfig
index c0ad7b82086b..5e71e889aeb1 100644
--- a/arch/arm/configs/aspeed_g5_defconfig
+++ b/arch/arm/configs/aspeed_g5_defconfig
@@ -118,7 +118,7 @@ CONFIG_I2C=y
 CONFIG_I2C_CHARDEV=y
 CONFIG_I2C_MUX=y
 CONFIG_I2C_MUX_PCA9541=y
-CONFIG_I2C_MUX_PCA954x=y
+CONFIG_I2C_MUX_PCA9x4x=y
 CONFIG_I2C_ASPEED=y
 CONFIG_GPIOLIB=y
 CONFIG_GPIO_SYSFS=y
diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
index 61509c4b769f..0a9b3ec54e2f 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -352,7 +352,7 @@ CONFIG_I2C_CHARDEV=y
 CONFIG_I2C_DAVINCI=y
 CONFIG_I2C_MUX=y
 CONFIG_I2C_ARB_GPIO_CHALLENGE=m
-CONFIG_I2C_MUX_PCA954x=y
+CONFIG_I2C_MUX_PCA9x4x=y
 CONFIG_I2C_MUX_PINCTRL=y
 CONFIG_I2C_DEMUX_PINCTRL=y
 CONFIG_I2C_AT91=m
diff --git a/arch/arm/configs/pxa_defconfig b/arch/arm/configs/pxa_defconfig
index 830e817a028a..7b181af815d9 100644
--- a/arch/arm/configs/pxa_defconfig
+++ b/arch/arm/configs/pxa_defconfig
@@ -350,7 +350,7 @@ CONFIG_SERIAL_PXA=y
 CONFIG_SERIAL_PXA_CONSOLE=y
 CONFIG_HW_RANDOM=y
 CONFIG_I2C_CHARDEV=m
-CONFIG_I2C_MUX_PCA954x=m
+CONFIG_I2C_MUX_PCA9x4x=m
 CONFIG_I2C_MUX_PINCTRL=m
 CONFIG_I2C_DESIGNWARE_PLATFORM=m
 CONFIG_I2C_PXA_SLAVE=y
diff --git a/arch/arm/configs/tegra_defconfig b/arch/arm/configs/tegra_defconfig
index 6678f2929356..4c6efffe5659 100644
--- a/arch/arm/configs/tegra_defconfig
+++ b/arch/arm/configs/tegra_defconfig
@@ -128,7 +128,7 @@ CONFIG_SERIAL_TEGRA=y
 # CONFIG_HW_RANDOM is not set
 # CONFIG_I2C_COMPAT is not set
 CONFIG_I2C_CHARDEV=y
-CONFIG_I2C_MUX_PCA954x=y
+CONFIG_I2C_MUX_PCA9x4x=y
 CONFIG_I2C_MUX_PINCTRL=y
 CONFIG_I2C_TEGRA=y
 CONFIG_SPI=y
diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 6356c6da34ea..9685367bd073 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -268,7 +268,7 @@ CONFIG_SERIAL_DEV_CTRL_TTYPORT=y
 CONFIG_VIRTIO_CONSOLE=y
 CONFIG_I2C_CHARDEV=y
 CONFIG_I2C_MUX=y
-CONFIG_I2C_MUX_PCA954x=y
+CONFIG_I2C_MUX_PCA9x4x=y
 CONFIG_I2C_BCM2835=m
 CONFIG_I2C_DESIGNWARE_PLATFORM=y
 CONFIG_I2C_IMX=y
diff --git a/arch/powerpc/configs/85xx-hw.config b/arch/powerpc/configs/85xx-hw.config
index c03d0fb16665..0772c249a287 100644
--- a/arch/powerpc/configs/85xx-hw.config
+++ b/arch/powerpc/configs/85xx-hw.config
@@ -48,7 +48,7 @@ CONFIG_HID_SUNPLUS=y
 CONFIG_I2C_CHARDEV=y
 CONFIG_I2C_CPM=m
 CONFIG_I2C_MPC=y
-CONFIG_I2C_MUX_PCA954x=y
+CONFIG_I2C_MUX_PCA9x4x=y
 CONFIG_I2C_MUX=y
 CONFIG_I2C=y
 CONFIG_IGB=y
diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
index 0f5c8fc36625..0a35869020ea 100644
--- a/drivers/i2c/muxes/Kconfig
+++ b/drivers/i2c/muxes/Kconfig
@@ -63,11 +63,11 @@ config I2C_MUX_PCA9541
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-mux-pca9541.
 
-config I2C_MUX_PCA954x
-	tristate "Philips PCA954x I2C Mux/switches"
+config I2C_MUX_PCA9x4x
+	tristate "Philips PCA9x4x I2C Mux/switches"
 	depends on GPIOLIB || COMPILE_TEST
 	help
-	  If you say yes here you get support for the Philips PCA954x
+	  If you say yes here you get support for the Philips PCA9x4x
 	  I2C mux/switch devices.
 
 	  This driver can also be built as a module.  If so, the module
diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile
index 6d9d865e8518..a87c9adc5671 100644
--- a/drivers/i2c/muxes/Makefile
+++ b/drivers/i2c/muxes/Makefile
@@ -11,7 +11,7 @@ obj-$(CONFIG_I2C_MUX_GPMUX)	+= i2c-mux-gpmux.o
 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_PCA9x4x)	+= i2c-mux-pca9x4x.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-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c
index 6a39adaf433f..0eb84deef566 100644
--- a/drivers/i2c/muxes/i2c-mux-pca9541.c
+++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
@@ -22,7 +22,7 @@
 #include <linux/i2c-mux.h>
 #include <linux/jiffies.h>
 #include <linux/module.h>
-#include <linux/platform_data/pca954x.h>
+#include <linux/platform_data/pca9x4x.h>
 #include <linux/slab.h>
 
 /*
@@ -334,7 +334,7 @@ static int pca9541_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 pca9x4x_platform_data *pdata = dev_get_platdata(&client->dev);
 	struct i2c_mux_core *muxc;
 	struct pca9541 *data;
 	int force;
diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca9x4x.c
similarity index 56%
rename from drivers/i2c/muxes/i2c-mux-pca954x.c
rename to drivers/i2c/muxes/i2c-mux-pca9x4x.c
index 2ca068d8b92d..14ef7cfca751 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca9x4x.c
@@ -1,14 +1,16 @@
 /*
  * I2C multiplexer
  *
+ * Copyright (c) 2017 Adrian Fiergolski <Adrian.Fiergolski@cern.ch>
  * Copyright (c) 2008-2009 Rodolfo Giometti <giometti@linux.it>
  * Copyright (c) 2008-2009 Eurotech S.p.A. <info@eurotech.it>
  *
- * This module supports the PCA954x series of I2C multiplexer/switch chips
+ * This module supports the Pca9x4x series of I2C multiplexer/switch chips
  * made by Philips Semiconductors.
  * This includes the:
  *	 PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547
  *	 and PCA9548.
+ *       PCA9846, PCA9847, PCA9848 and PCA9849
  *
  * These chips are all controlled via the I2C bus itself, and all have a
  * single 8-bit register. The upstream "parent" bus fans out to two,
@@ -45,14 +47,20 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/of_irq.h>
-#include <linux/platform_data/pca954x.h>
+#include <linux/platform_data/pca9x4x.h>
 #include <linux/pm.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 
-#define PCA954X_MAX_NCHANS 8
+#define PCA9X4X_MAX_NCHANS 8
 
-#define PCA954X_IRQ_OFFSET 4
+#define PCA9X4X_IRQ_OFFSET 4
+
+//PCA984x family I2C other addresses
+#define GENERAL_CALL  0x00
+#define SOFTWARE_RESET 0x06
+#define DEVICE_ID_ADDRESS 0x7C
+#define NXP_ID 0x00
 
 enum pca_type {
 	pca_9540,
@@ -63,6 +71,12 @@ enum pca_type {
 	pca_9546,
 	pca_9547,
 	pca_9548,
+
+	pca_9846,
+	pca_9847,
+	pca_9848,
+	pca_9849,
+
 };
 
 struct chip_desc {
@@ -70,12 +84,13 @@ struct chip_desc {
 	u8 enable;	/* used for muxes only */
 	u8 has_irq;
 	enum muxtype {
-		pca954x_ismux = 0,
-		pca954x_isswi
+		pca9x4x_ismux = 0,
+		pca9x4x_isswi
 	} muxtype;
+	u16 deviceID; //used by PCA984x family only
 };
 
-struct pca954x {
+struct pca9x4x {
 	const struct chip_desc *chip;
 
 	u8 last_chan;		/* last register value */
@@ -87,51 +102,79 @@ struct pca954x {
 	raw_spinlock_t lock;
 };
 
-/* Provide specs for the PCA954x types we know about */
+/* Provide specs for the PCA9x4x types we know about */
 static const struct chip_desc chips[] = {
+	//954x family
 	[pca_9540] = {
 		.nchans = 2,
 		.enable = 0x4,
-		.muxtype = pca954x_ismux,
+		.muxtype = pca9x4x_ismux,
 	},
 	[pca_9542] = {
 		.nchans = 2,
 		.enable = 0x4,
 		.has_irq = 1,
-		.muxtype = pca954x_ismux,
+		.muxtype = pca9x4x_ismux,
 	},
 	[pca_9543] = {
 		.nchans = 2,
 		.has_irq = 1,
-		.muxtype = pca954x_isswi,
+		.muxtype = pca9x4x_isswi,
 	},
 	[pca_9544] = {
 		.nchans = 4,
 		.enable = 0x4,
 		.has_irq = 1,
-		.muxtype = pca954x_ismux,
+		.muxtype = pca9x4x_ismux,
 	},
 	[pca_9545] = {
 		.nchans = 4,
 		.has_irq = 1,
-		.muxtype = pca954x_isswi,
+		.muxtype = pca9x4x_isswi,
 	},
 	[pca_9546] = {
 		.nchans = 4,
-		.muxtype = pca954x_isswi,
+		.muxtype = pca9x4x_isswi,
 	},
 	[pca_9547] = {
 		.nchans = 8,
 		.enable = 0x8,
-		.muxtype = pca954x_ismux,
+		.muxtype = pca9x4x_ismux,
 	},
 	[pca_9548] = {
 		.nchans = 8,
-		.muxtype = pca954x_isswi,
+		.muxtype = pca9x4x_isswi,
+	},
+
+	//984x family
+	[pca_9846] = {
+		.nchans = 4,
+		.muxtype = pca9x4x_isswi,
+		.deviceID = 0x10B,
+	},
+
+	[pca_9847] = {
+		.nchans = 8,
+		.muxtype = pca9x4x_ismux,
+		.deviceID = 0x108,
 	},
+
+	[pca_9848] = {
+		.nchans = 8,
+		.muxtype = pca9x4x_isswi,
+		.deviceID = 0x10A,
+	},
+
+	[pca_9849] = {
+		.nchans = 4,
+		.muxtype = pca9x4x_ismux,
+		.deviceID = 0x109,
+	},
+
 };
 
-static const struct i2c_device_id pca954x_id[] = {
+static const struct i2c_device_id pca9x4x_id[] = {
+	//954x family
 	{ "pca9540", pca_9540 },
 	{ "pca9542", pca_9542 },
 	{ "pca9543", pca_9543 },
@@ -140,12 +183,20 @@ static const struct i2c_device_id pca954x_id[] = {
 	{ "pca9546", pca_9546 },
 	{ "pca9547", pca_9547 },
 	{ "pca9548", pca_9548 },
+
+	//984x family
+	{ "pca9846", pca_9846 },
+	{ "pca9847", pca_9847 },
+	{ "pca9848", pca_9848 },
+	{ "pca9849", pca_9849 },
+
 	{ }
 };
-MODULE_DEVICE_TABLE(i2c, pca954x_id);
+MODULE_DEVICE_TABLE(i2c, pca9x4x_id);
 
 #ifdef CONFIG_OF
-static const struct of_device_id pca954x_of_match[] = {
+static const struct of_device_id pca9x4x_of_match[] = {
+	//954x family
 	{ .compatible = "nxp,pca9540", .data = &chips[pca_9540] },
 	{ .compatible = "nxp,pca9542", .data = &chips[pca_9542] },
 	{ .compatible = "nxp,pca9543", .data = &chips[pca_9543] },
@@ -154,14 +205,22 @@ static const struct of_device_id pca954x_of_match[] = {
 	{ .compatible = "nxp,pca9546", .data = &chips[pca_9546] },
 	{ .compatible = "nxp,pca9547", .data = &chips[pca_9547] },
 	{ .compatible = "nxp,pca9548", .data = &chips[pca_9548] },
+
+
+	//984x family
+	{ .compatible = "nxp,pca9846", .data = &chips[pca_9846] },
+	{ .compatible = "nxp,pca9847", .data = &chips[pca_9847] },
+	{ .compatible = "nxp,pca9848", .data = &chips[pca_9848] },
+	{ .compatible = "nxp,pca9849", .data = &chips[pca_9849] },
+
 	{}
 };
-MODULE_DEVICE_TABLE(of, pca954x_of_match);
+MODULE_DEVICE_TABLE(of, pca9x4x_of_match);
 #endif
 
 /* Write to mux register. Don't use i2c_transfer()/i2c_smbus_xfer()
    for this as they will try to lock adapter a second time */
-static int pca954x_reg_write(struct i2c_adapter *adap,
+static int pca9x4x_reg_write(struct i2c_adapter *adap,
 			     struct i2c_client *client, u8 val)
 {
 	int ret = -ENODEV;
@@ -190,32 +249,32 @@ static int pca954x_reg_write(struct i2c_adapter *adap,
 	return ret;
 }
 
-static int pca954x_select_chan(struct i2c_mux_core *muxc, u32 chan)
+static int pca9x4x_select_chan(struct i2c_mux_core *muxc, u32 chan)
 {
-	struct pca954x *data = i2c_mux_priv(muxc);
+	struct pca9x4x *data = i2c_mux_priv(muxc);
 	struct i2c_client *client = data->client;
 	const struct chip_desc *chip = data->chip;
 	u8 regval;
 	int ret = 0;
 
 	/* we make switches look like muxes, not sure how to be smarter */
-	if (chip->muxtype == pca954x_ismux)
+	if (chip->muxtype == pca9x4x_ismux)
 		regval = chan | chip->enable;
 	else
 		regval = 1 << chan;
 
 	/* Only select the channel if its different from the last channel */
 	if (data->last_chan != regval) {
-		ret = pca954x_reg_write(muxc->parent, client, regval);
+		ret = pca9x4x_reg_write(muxc->parent, client, regval);
 		data->last_chan = ret < 0 ? 0 : regval;
 	}
 
 	return ret;
 }
 
-static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
+static int pca9x4x_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
 {
-	struct pca954x *data = i2c_mux_priv(muxc);
+	struct pca9x4x *data = i2c_mux_priv(muxc);
 	struct i2c_client *client = data->client;
 
 	if (!(data->deselect & (1 << chan)))
@@ -223,12 +282,12 @@ static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
 
 	/* Deselect active channel */
 	data->last_chan = 0;
-	return pca954x_reg_write(muxc->parent, client, data->last_chan);
+	return pca9x4x_reg_write(muxc->parent, client, data->last_chan);
 }
 
-static irqreturn_t pca954x_irq_handler(int irq, void *dev_id)
+static irqreturn_t pca9x4x_irq_handler(int irq, void *dev_id)
 {
-	struct pca954x *data = dev_id;
+	struct pca9x4x *data = dev_id;
 	unsigned int child_irq;
 	int ret, i, handled = 0;
 
@@ -237,7 +296,7 @@ static irqreturn_t pca954x_irq_handler(int irq, void *dev_id)
 		return IRQ_NONE;
 
 	for (i = 0; i < data->chip->nchans; i++) {
-		if (ret & BIT(PCA954X_IRQ_OFFSET + i)) {
+		if (ret & BIT(PCA9X4X_IRQ_OFFSET + i)) {
 			child_irq = irq_linear_revmap(data->irq, i);
 			handle_nested_irq(child_irq);
 			handled++;
@@ -246,21 +305,21 @@ static irqreturn_t pca954x_irq_handler(int irq, void *dev_id)
 	return handled ? IRQ_HANDLED : IRQ_NONE;
 }
 
-static int pca954x_irq_set_type(struct irq_data *idata, unsigned int type)
+static int pca9x4x_irq_set_type(struct irq_data *idata, unsigned int type)
 {
 	if ((type & IRQ_TYPE_SENSE_MASK) != IRQ_TYPE_LEVEL_LOW)
 		return -EINVAL;
 	return 0;
 }
 
-static struct irq_chip pca954x_irq_chip = {
-	.name = "i2c-mux-pca954x",
-	.irq_set_type = pca954x_irq_set_type,
+static struct irq_chip pca9x4x_irq_chip = {
+	.name = "i2c-mux-pca9x4x",
+	.irq_set_type = pca9x4x_irq_set_type,
 };
 
-static int pca954x_irq_setup(struct i2c_mux_core *muxc)
+static int pca9x4x_irq_setup(struct i2c_mux_core *muxc)
 {
-	struct pca954x *data = i2c_mux_priv(muxc);
+	struct pca9x4x *data = i2c_mux_priv(muxc);
 	struct i2c_client *client = data->client;
 	int c, irq;
 
@@ -282,16 +341,16 @@ static int pca954x_irq_setup(struct i2c_mux_core *muxc)
 			return -EINVAL;
 		}
 		irq_set_chip_data(irq, data);
-		irq_set_chip_and_handler(irq, &pca954x_irq_chip,
-			handle_simple_irq);
+		irq_set_chip_and_handler(irq, &pca9x4x_irq_chip,
+					 handle_simple_irq);
 	}
 
 	return 0;
 }
 
-static void pca954x_cleanup(struct i2c_mux_core *muxc)
+static void pca9x4x_cleanup(struct i2c_mux_core *muxc)
 {
-	struct pca954x *data = i2c_mux_priv(muxc);
+	struct pca9x4x *data = i2c_mux_priv(muxc);
 	int c, irq;
 
 	if (data->irq) {
@@ -304,20 +363,92 @@ static void pca954x_cleanup(struct i2c_mux_core *muxc)
 	i2c_mux_del_adapters(muxc);
 }
 
+/*
+ * Part of probe function specific for pca954x family
+ */
+inline int pca954x_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id){
+
+	/* Write the mux register at addr to verify
+	 * that the mux is in fact present. This also
+	 * initializes the mux to disconnected state.
+	 */
+	if (i2c_smbus_write_byte(client, 0) < 0)
+		return -ENODEV;
+
+	return 0;
+}
+
+/*
+ * Part of probe function specific for pca984x family
+ */
+inline int pca984x_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id){
+
+	struct i2c_adapter *adap = to_i2c_adapter(client->dev.parent);
+	union i2c_smbus_data device_id_raw;
+	u16 manufacturer_id; //12 bits
+
+	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) {
+		dev_warn(&client->dev, "PCA9846 family: I2c adapter doesn't support I2C_FUNC_SMBUS_WRITE_BYTE_DATA");
+		return -ENODEV;
+	}
+
+	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
+		dev_warn(&client->dev, "PCA9846 family: I2c adapter doesn't support I2C_FUNC_SMBUS_READ_I2C_BLOCK");
+		return -ENODEV;
+	}
+
+	//
+	//Software reset
+	//
+	if (i2c_smbus_xfer(client->adapter, GENERAL_CALL, client->flags,
+			   I2C_SMBUS_WRITE, SOFTWARE_RESET, I2C_SMBUS_BYTE, NULL)  < 0) {
+		dev_warn(&client->dev, "PCA9846 family: Sofrware reset failed\n");
+		return -ENODEV;
+	}
+
+	//
+	//Get device ID
+	//
+	device_id_raw.block[0] = 3;  //read 3 bytes
+	if (i2c_smbus_xfer(client->adapter, DEVICE_ID_ADDRESS, client->flags,
+			   I2C_SMBUS_READ,  client->addr << 1,
+			   I2C_SMBUS_I2C_BLOCK_DATA, &device_id_raw)) {
+		dev_warn(&client->dev, "PCA9846 family: Get device ID failed\n");
+		return -ENODEV;
+	}
+
+	//Device ID contains only 3 bytes
+	if (device_id_raw.block[0] != 3) {
+		dev_warn(&client->dev, "PCA9846 family: Get device ID failed\n");
+		return -ENODEV;
+	}
+
+	//Check manufacturer ID (12 bits)
+	manufacturer_id = ((u16) device_id_raw.block[1] << 4) | (device_id_raw.block[2] >> 4);
+	if (manufacturer_id != NXP_ID) {
+		dev_warn(&client->dev, "PCA9846 family: Manufacturer ID does not match NXP\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
 /*
  * I2C init/probing/exit functions
  */
-static int pca954x_probe(struct i2c_client *client,
+static int pca9x4x_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
 	struct i2c_adapter *adap = to_i2c_adapter(client->dev.parent);
-	struct pca954x_platform_data *pdata = dev_get_platdata(&client->dev);
+	struct pca9x4x_platform_data *pdata = dev_get_platdata(&client->dev);
 	struct device_node *of_node = client->dev.of_node;
 	bool idle_disconnect_dt;
 	struct gpio_desc *gpio;
 	int num, force, class;
 	struct i2c_mux_core *muxc;
-	struct pca954x *data;
+	struct pca9x4x *data;
 	const struct of_device_id *match;
 	int ret;
 
@@ -325,8 +456,8 @@ static int pca954x_probe(struct i2c_client *client,
 		return -ENODEV;
 
 	muxc = i2c_mux_alloc(adap, &client->dev,
-			     PCA954X_MAX_NCHANS, sizeof(*data), 0,
-			     pca954x_select_chan, pca954x_deselect_mux);
+			     PCA9X4X_MAX_NCHANS, sizeof(*data), 0,
+			     pca9x4x_select_chan, pca9x4x_deselect_mux);
 	if (!muxc)
 		return -ENOMEM;
 	data = i2c_mux_priv(muxc);
@@ -339,16 +470,34 @@ static int pca954x_probe(struct i2c_client *client,
 	if (IS_ERR(gpio))
 		return PTR_ERR(gpio);
 
-	/* Write the mux register at addr to verify
-	 * that the mux is in fact present. This also
-	 * initializes the mux to disconnected state.
-	 */
-	if (i2c_smbus_write_byte(client, 0) < 0) {
+	switch (id->driver_data) {
+	case pca_9540:
+	case pca_9542:
+	case pca_9543:
+	case pca_9544:
+	case pca_9545:
+	case pca_9546:
+	case pca_9547:
+	case pca_9548:
+		ret = pca954x_probe(client, id);
+		break;
+	case pca_9846:
+	case pca_9847:
+	case pca_9848:
+	case pca_9849:
+		ret = pca984x_probe(client, id);
+		break;
+	default: //unknown device
+		ret = -ENODEV;
+		break;
+	}
+
+	if (ret < 0) {
 		dev_warn(&client->dev, "probe failed\n");
 		return -ENODEV;
 	}
 
-	match = of_match_device(of_match_ptr(pca954x_of_match), &client->dev);
+	match = of_match_device(of_match_ptr(pca9x4x_of_match), &client->dev);
 	if (match)
 		data->chip = of_device_get_match_data(&client->dev);
 	else
@@ -359,7 +508,7 @@ static int pca954x_probe(struct i2c_client *client,
 	idle_disconnect_dt = of_node &&
 		of_property_read_bool(of_node, "i2c-mux-idle-disconnect");
 
-	ret = pca954x_irq_setup(muxc);
+	ret = pca9x4x_irq_setup(muxc);
 	if (ret)
 		goto fail_cleanup;
 
@@ -389,60 +538,60 @@ static int pca954x_probe(struct i2c_client *client,
 
 	if (data->irq) {
 		ret = devm_request_threaded_irq(&client->dev, data->client->irq,
-						NULL, pca954x_irq_handler,
+						NULL, pca9x4x_irq_handler,
 						IRQF_ONESHOT | IRQF_SHARED,
-						"pca954x", data);
+						"pca9x4x", data);
 		if (ret)
 			goto fail_cleanup;
 	}
 
 	dev_info(&client->dev,
 		 "registered %d multiplexed busses for I2C %s %s\n",
-		 num, data->chip->muxtype == pca954x_ismux
-				? "mux" : "switch", client->name);
+		 num, data->chip->muxtype == pca9x4x_ismux
+		 ? "mux" : "switch", client->name);
 
 	return 0;
 
 fail_cleanup:
-	pca954x_cleanup(muxc);
+	pca9x4x_cleanup(muxc);
 	return ret;
 }
 
-static int pca954x_remove(struct i2c_client *client)
+static int pca9x4x_remove(struct i2c_client *client)
 {
 	struct i2c_mux_core *muxc = i2c_get_clientdata(client);
 
-	pca954x_cleanup(muxc);
+	pca9x4x_cleanup(muxc);
 	return 0;
 }
 
 #ifdef CONFIG_PM_SLEEP
-static int pca954x_resume(struct device *dev)
+static int pca9x4x_resume(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct i2c_mux_core *muxc = i2c_get_clientdata(client);
-	struct pca954x *data = i2c_mux_priv(muxc);
+	struct pca9x4x *data = i2c_mux_priv(muxc);
 
 	data->last_chan = 0;
 	return i2c_smbus_write_byte(client, 0);
 }
 #endif
 
-static SIMPLE_DEV_PM_OPS(pca954x_pm, NULL, pca954x_resume);
+static SIMPLE_DEV_PM_OPS(pca9x4x_pm, NULL, pca9x4x_resume);
 
-static struct i2c_driver pca954x_driver = {
+static struct i2c_driver pca9x4x_driver = {
 	.driver		= {
-		.name	= "pca954x",
-		.pm	= &pca954x_pm,
-		.of_match_table = of_match_ptr(pca954x_of_match),
+		.name	= "pca9x4x",
+		.pm	= &pca9x4x_pm,
+		.of_match_table = of_match_ptr(pca9x4x_of_match),
 	},
-	.probe		= pca954x_probe,
-	.remove		= pca954x_remove,
-	.id_table	= pca954x_id,
+	.probe		= pca9x4x_probe,
+	.remove		= pca9x4x_remove,
+	.id_table	= pca9x4x_id,
 };
 
-module_i2c_driver(pca954x_driver);
+module_i2c_driver(pca9x4x_driver);
 
-MODULE_AUTHOR("Rodolfo Giometti <giometti@linux.it>");
-MODULE_DESCRIPTION("PCA954x I2C mux/switch driver");
+MODULE_AUTHOR("Rodolfo Giometti <giometti@linux.it>, Adrian Fiergolski <Adrian.Fiergolski@cern.ch>");
+MODULE_DESCRIPTION("PCA9x4x I2C mux/switch driver");
 MODULE_LICENSE("GPL v2");
diff --git a/include/linux/platform_data/pca954x.h b/include/linux/platform_data/pca9x4x.h
similarity index 80%
rename from include/linux/platform_data/pca954x.h
rename to include/linux/platform_data/pca9x4x.h
index 1712677d5904..a33c5afbfd7f 100644
--- a/include/linux/platform_data/pca954x.h
+++ b/include/linux/platform_data/pca9x4x.h
@@ -2,6 +2,7 @@
  *
  * pca954x.h - I2C multiplexer/switch support
  *
+ * Copyright (c) 2017 Adrian Fiergolski <Adrian.Fiergolski@cern.ch>
  * Copyright (c) 2008-2009 Rodolfo Giometti <giometti@linux.it>
  * Copyright (c) 2008-2009 Eurotech S.p.A. <info@eurotech.it>
  * Michael Lawnick <michael.lawnick.ext@nsn.com>
@@ -22,10 +23,10 @@
  */
 
 
-#ifndef _LINUX_I2C_PCA954X_H
-#define _LINUX_I2C_PCA954X_H
+#ifndef _LINUX_I2C_PCA9X4X_H
+#define _LINUX_I2C_PCA9X4X_H
 
-/* Platform data for the PCA954x I2C multiplexers */
+/* Platform data for the PCA9x4x I2C multiplexers */
 
 /* Per channel initialisation data:
  * @adap_id: bus number for the adapter. 0 = don't care
@@ -33,16 +34,16 @@
  *                    of this channel after transaction.
  *
  */
-struct pca954x_platform_mode {
+struct pca9x4x_platform_mode {
 	int		adap_id;
 	unsigned int	deselect_on_exit:1;
 	unsigned int	class;
 };
 
 /* Per mux/switch data, used with i2c_register_board_info */
-struct pca954x_platform_data {
-	struct pca954x_platform_mode *modes;
+struct pca9x4x_platform_data {
+	struct pca9x4x_platform_mode *modes;
 	int num_modes;
 };
 
-#endif /* _LINUX_I2C_PCA954X_H */
+#endif /* _LINUX_I2C_PCA9X4X_H */
-- 
2.14.1

Re: [PATCH] i2c: Add support for NXP PCA984x family.

[Rodolfo Giometti]

On 11/12/17 12:10, Adrian Fiergolski wrote:
> This patch exetends the current i2c-mux-pca954x driver and adds suuport for
> a newer PCA984x family of the I2C switches and multiplexers from NXP.
> In probe function, the patch supports device ID function, introduced in the
> new family, and checks it against configuration provided by the
> devicetree. Moreover, it also performs software reset which is now
> available for the new devices.
> The reset of the code remains common with the legacy PCA954x devices.
> 
> Signed-off-by: Adrian Fiergolski <adrian.fiergolski@cern.ch>
> ---
>   .../{i2c-mux-pca954x.txt => i2c-mux-pca9x4x.txt}   |   5 +-
>   arch/arm/configs/aspeed_g4_defconfig               |   2 +-
>   arch/arm/configs/aspeed_g5_defconfig               |   2 +-
>   arch/arm/configs/multi_v7_defconfig                |   2 +-
>   arch/arm/configs/pxa_defconfig                     |   2 +-
>   arch/arm/configs/tegra_defconfig                   |   2 +-
>   arch/arm64/configs/defconfig                       |   2 +-
>   arch/powerpc/configs/85xx-hw.config                |   2 +-
>   drivers/i2c/muxes/Kconfig                          |   6 +-
>   drivers/i2c/muxes/Makefile                         |   2 +-
>   drivers/i2c/muxes/i2c-mux-pca9541.c                |   4 +-
>   .../muxes/{i2c-mux-pca954x.c => i2c-mux-pca9x4x.c} | 295 ++++++++++++++++-----
>   .../linux/platform_data/{pca954x.h => pca9x4x.h}   |  15 +-
>   13 files changed, 246 insertions(+), 95 deletions(-)
>   rename Documentation/devicetree/bindings/i2c/{i2c-mux-pca954x.txt => i2c-mux-pca9x4x.txt} (91%)
>   rename drivers/i2c/muxes/{i2c-mux-pca954x.c => i2c-mux-pca9x4x.c} (56%)
>   rename include/linux/platform_data/{pca954x.h => pca9x4x.h} (80%)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca9x4x.txt
> similarity index 91%
> rename from Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> rename to Documentation/devicetree/bindings/i2c/i2c-mux-pca9x4x.txt
> index aa097045a10e..cf9a075ca1dd 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca9x4x.txt
> @@ -1,10 +1,11 @@
> -* NXP PCA954x I2C bus switch
> +* NXP PCA9x4x I2C bus switch
>   
>   Required Properties:
>   
>     - compatible: Must contain one of the following.
>       "nxp,pca9540", "nxp,pca9542", "nxp,pca9543", "nxp,pca9544",
> -    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548"
> +    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548",
> +    "nxp,pca9846", "nxp,pca9847", "nxp,pca9848", "nxp,pca9849"
>   
>     - reg: The I2C address of the device.
>   
> diff --git a/arch/arm/configs/aspeed_g4_defconfig b/arch/arm/configs/aspeed_g4_defconfig
> index d23b9d56a88b..a461ad3cf63d 100644
> --- a/arch/arm/configs/aspeed_g4_defconfig
> +++ b/arch/arm/configs/aspeed_g4_defconfig
> @@ -116,7 +116,7 @@ CONFIG_I2C=y
>   CONFIG_I2C_CHARDEV=y
>   CONFIG_I2C_MUX=y
>   CONFIG_I2C_MUX_PCA9541=y
> -CONFIG_I2C_MUX_PCA954x=y
> +CONFIG_I2C_MUX_PCA9x4x=y

Nak.

I'm not sure you should break backward compatibility. You should keep the 
CONFIG_I2C_MUX_PCA954x configuration setting and the current name convention.

>   CONFIG_I2C_ASPEED=y
>   CONFIG_GPIOLIB=y
>   CONFIG_GPIO_SYSFS=y
> diff --git a/arch/arm/configs/aspeed_g5_defconfig b/arch/arm/configs/aspeed_g5_defconfig
> index c0ad7b82086b..5e71e889aeb1 100644
> --- a/arch/arm/configs/aspeed_g5_defconfig
> +++ b/arch/arm/configs/aspeed_g5_defconfig
> @@ -118,7 +118,7 @@ CONFIG_I2C=y
>   CONFIG_I2C_CHARDEV=y
>   CONFIG_I2C_MUX=y
>   CONFIG_I2C_MUX_PCA9541=y
> -CONFIG_I2C_MUX_PCA954x=y
> +CONFIG_I2C_MUX_PCA9x4x=y
>   CONFIG_I2C_ASPEED=y
>   CONFIG_GPIOLIB=y
>   CONFIG_GPIO_SYSFS=y
> diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
> index 61509c4b769f..0a9b3ec54e2f 100644
> --- a/arch/arm/configs/multi_v7_defconfig
> +++ b/arch/arm/configs/multi_v7_defconfig
> @@ -352,7 +352,7 @@ CONFIG_I2C_CHARDEV=y
>   CONFIG_I2C_DAVINCI=y
>   CONFIG_I2C_MUX=y
>   CONFIG_I2C_ARB_GPIO_CHALLENGE=m
> -CONFIG_I2C_MUX_PCA954x=y
> +CONFIG_I2C_MUX_PCA9x4x=y
>   CONFIG_I2C_MUX_PINCTRL=y
>   CONFIG_I2C_DEMUX_PINCTRL=y
>   CONFIG_I2C_AT91=m
> diff --git a/arch/arm/configs/pxa_defconfig b/arch/arm/configs/pxa_defconfig
> index 830e817a028a..7b181af815d9 100644
> --- a/arch/arm/configs/pxa_defconfig
> +++ b/arch/arm/configs/pxa_defconfig
> @@ -350,7 +350,7 @@ CONFIG_SERIAL_PXA=y
>   CONFIG_SERIAL_PXA_CONSOLE=y
>   CONFIG_HW_RANDOM=y
>   CONFIG_I2C_CHARDEV=m
> -CONFIG_I2C_MUX_PCA954x=m
> +CONFIG_I2C_MUX_PCA9x4x=m
>   CONFIG_I2C_MUX_PINCTRL=m
>   CONFIG_I2C_DESIGNWARE_PLATFORM=m
>   CONFIG_I2C_PXA_SLAVE=y
> diff --git a/arch/arm/configs/tegra_defconfig b/arch/arm/configs/tegra_defconfig
> index 6678f2929356..4c6efffe5659 100644
> --- a/arch/arm/configs/tegra_defconfig
> +++ b/arch/arm/configs/tegra_defconfig
> @@ -128,7 +128,7 @@ CONFIG_SERIAL_TEGRA=y
>   # CONFIG_HW_RANDOM is not set
>   # CONFIG_I2C_COMPAT is not set
>   CONFIG_I2C_CHARDEV=y
> -CONFIG_I2C_MUX_PCA954x=y
> +CONFIG_I2C_MUX_PCA9x4x=y
>   CONFIG_I2C_MUX_PINCTRL=y
>   CONFIG_I2C_TEGRA=y
>   CONFIG_SPI=y
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index 6356c6da34ea..9685367bd073 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -268,7 +268,7 @@ CONFIG_SERIAL_DEV_CTRL_TTYPORT=y
>   CONFIG_VIRTIO_CONSOLE=y
>   CONFIG_I2C_CHARDEV=y
>   CONFIG_I2C_MUX=y
> -CONFIG_I2C_MUX_PCA954x=y
> +CONFIG_I2C_MUX_PCA9x4x=y
>   CONFIG_I2C_BCM2835=m
>   CONFIG_I2C_DESIGNWARE_PLATFORM=y
>   CONFIG_I2C_IMX=y
> diff --git a/arch/powerpc/configs/85xx-hw.config b/arch/powerpc/configs/85xx-hw.config
> index c03d0fb16665..0772c249a287 100644
> --- a/arch/powerpc/configs/85xx-hw.config
> +++ b/arch/powerpc/configs/85xx-hw.config
> @@ -48,7 +48,7 @@ CONFIG_HID_SUNPLUS=y
>   CONFIG_I2C_CHARDEV=y
>   CONFIG_I2C_CPM=m
>   CONFIG_I2C_MPC=y
> -CONFIG_I2C_MUX_PCA954x=y
> +CONFIG_I2C_MUX_PCA9x4x=y
>   CONFIG_I2C_MUX=y
>   CONFIG_I2C=y
>   CONFIG_IGB=y
> diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
> index 0f5c8fc36625..0a35869020ea 100644
> --- a/drivers/i2c/muxes/Kconfig
> +++ b/drivers/i2c/muxes/Kconfig
> @@ -63,11 +63,11 @@ config I2C_MUX_PCA9541
>   	  This driver can also be built as a module.  If so, the module
>   	  will be called i2c-mux-pca9541.
>   
> -config I2C_MUX_PCA954x
> -	tristate "Philips PCA954x I2C Mux/switches"
> +config I2C_MUX_PCA9x4x
> +	tristate "Philips PCA9x4x I2C Mux/switches"
>   	depends on GPIOLIB || COMPILE_TEST
>   	help
> -	  If you say yes here you get support for the Philips PCA954x
> +	  If you say yes here you get support for the Philips PCA9x4x
>   	  I2C mux/switch devices.

Even here you should point out that the driver supports both PCA954x and PCA984x 
families... maybe you can write as follow:

If you say yes here you get support for the Philips PCA954x and  PCA984x I2C 
mux/switch devices.

>   	  This driver can also be built as a module.  If so, the module
> diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile
> index 6d9d865e8518..a87c9adc5671 100644
> --- a/drivers/i2c/muxes/Makefile
> +++ b/drivers/i2c/muxes/Makefile
> @@ -11,7 +11,7 @@ obj-$(CONFIG_I2C_MUX_GPMUX)	+= i2c-mux-gpmux.o
>   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_PCA9x4x)	+= i2c-mux-pca9x4x.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-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c
> index 6a39adaf433f..0eb84deef566 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca9541.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
> @@ -22,7 +22,7 @@
>   #include <linux/i2c-mux.h>
>   #include <linux/jiffies.h>
>   #include <linux/module.h>
> -#include <linux/platform_data/pca954x.h>
> +#include <linux/platform_data/pca9x4x.h>
>   #include <linux/slab.h>
>   
>   /*
> @@ -334,7 +334,7 @@ static int pca9541_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 pca9x4x_platform_data *pdata = dev_get_platdata(&client->dev);
>   	struct i2c_mux_core *muxc;
>   	struct pca9541 *data;
>   	int force;
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca9x4x.c
> similarity index 56%
> rename from drivers/i2c/muxes/i2c-mux-pca954x.c
> rename to drivers/i2c/muxes/i2c-mux-pca9x4x.c
> index 2ca068d8b92d..14ef7cfca751 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca9x4x.c
> @@ -1,14 +1,16 @@
>   /*
>    * I2C multiplexer
>    *
> + * Copyright (c) 2017 Adrian Fiergolski <Adrian.Fiergolski@cern.ch>
>    * Copyright (c) 2008-2009 Rodolfo Giometti <giometti@linux.it>
>    * Copyright (c) 2008-2009 Eurotech S.p.A. <info@eurotech.it>
>    *
> - * This module supports the PCA954x series of I2C multiplexer/switch chips
> + * This module supports the Pca9x4x series of I2C multiplexer/switch chips
>    * made by Philips Semiconductors.
>    * This includes the:
>    *	 PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547
>    *	 and PCA9548.
> + *       PCA9846, PCA9847, PCA9848 and PCA9849
>    *
>    * These chips are all controlled via the I2C bus itself, and all have a
>    * single 8-bit register. The upstream "parent" bus fans out to two,
> @@ -45,14 +47,20 @@
>   #include <linux/of.h>
>   #include <linux/of_device.h>
>   #include <linux/of_irq.h>
> -#include <linux/platform_data/pca954x.h>
> +#include <linux/platform_data/pca9x4x.h>
>   #include <linux/pm.h>
>   #include <linux/slab.h>
>   #include <linux/spinlock.h>
>   
> -#define PCA954X_MAX_NCHANS 8
> +#define PCA9X4X_MAX_NCHANS 8
>   
> -#define PCA954X_IRQ_OFFSET 4
> +#define PCA9X4X_IRQ_OFFSET 4
> +
> +//PCA984x family I2C other addresses
> +#define GENERAL_CALL  0x00
> +#define SOFTWARE_RESET 0x06
> +#define DEVICE_ID_ADDRESS 0x7C
> +#define NXP_ID 0x00
>   
>   enum pca_type {
>   	pca_9540,
> @@ -63,6 +71,12 @@ enum pca_type {
>   	pca_9546,
>   	pca_9547,
>   	pca_9548,
> +

Why is a blank line here?

> +	pca_9846,
> +	pca_9847,
> +	pca_9848,
> +	pca_9849,
> +
>   };
>   
>   struct chip_desc {
> @@ -70,12 +84,13 @@ struct chip_desc {
>   	u8 enable;	/* used for muxes only */
>   	u8 has_irq;
>   	enum muxtype {
> -		pca954x_ismux = 0,
> -		pca954x_isswi
> +		pca9x4x_ismux = 0,
> +		pca9x4x_isswi
>   	} muxtype;
> +	u16 deviceID; //used by PCA984x family only
>   };
>   
> -struct pca954x {
> +struct pca9x4x {
>   	const struct chip_desc *chip;
>   
>   	u8 last_chan;		/* last register value */
> @@ -87,51 +102,79 @@ struct pca954x {
>   	raw_spinlock_t lock;
>   };
>   
> -/* Provide specs for the PCA954x types we know about */
> +/* Provide specs for the PCA9x4x types we know about */
>   static const struct chip_desc chips[] = {
> +	//954x family
>   	[pca_9540] = {
>   		.nchans = 2,
>   		.enable = 0x4,
> -		.muxtype = pca954x_ismux,
> +		.muxtype = pca9x4x_ismux,
>   	},
>   	[pca_9542] = {
>   		.nchans = 2,
>   		.enable = 0x4,
>   		.has_irq = 1,
> -		.muxtype = pca954x_ismux,
> +		.muxtype = pca9x4x_ismux,
>   	},
>   	[pca_9543] = {
>   		.nchans = 2,
>   		.has_irq = 1,
> -		.muxtype = pca954x_isswi,
> +		.muxtype = pca9x4x_isswi,
>   	},
>   	[pca_9544] = {
>   		.nchans = 4,
>   		.enable = 0x4,
>   		.has_irq = 1,
> -		.muxtype = pca954x_ismux,
> +		.muxtype = pca9x4x_ismux,
>   	},
>   	[pca_9545] = {
>   		.nchans = 4,
>   		.has_irq = 1,
> -		.muxtype = pca954x_isswi,
> +		.muxtype = pca9x4x_isswi,
>   	},
>   	[pca_9546] = {
>   		.nchans = 4,
> -		.muxtype = pca954x_isswi,
> +		.muxtype = pca9x4x_isswi,
>   	},
>   	[pca_9547] = {
>   		.nchans = 8,
>   		.enable = 0x8,
> -		.muxtype = pca954x_ismux,
> +		.muxtype = pca9x4x_ismux,
>   	},
>   	[pca_9548] = {
>   		.nchans = 8,
> -		.muxtype = pca954x_isswi,
> +		.muxtype = pca9x4x_isswi,
> +	},
> +
> +	//984x family
> +	[pca_9846] = {
> +		.nchans = 4,
> +		.muxtype = pca9x4x_isswi,
> +		.deviceID = 0x10B,
> +	},
> +
> +	[pca_9847] = {
> +		.nchans = 8,
> +		.muxtype = pca9x4x_ismux,
> +		.deviceID = 0x108,
>   	},
> +
> +	[pca_9848] = {
> +		.nchans = 8,
> +		.muxtype = pca9x4x_isswi,
> +		.deviceID = 0x10A,
> +	},
> +
> +	[pca_9849] = {
> +		.nchans = 4,
> +		.muxtype = pca9x4x_ismux,
> +		.deviceID = 0x109,
> +	},
> +
>   };
>   
> -static const struct i2c_device_id pca954x_id[] = {
> +static const struct i2c_device_id pca9x4x_id[] = {
> +	//954x family
>   	{ "pca9540", pca_9540 },
>   	{ "pca9542", pca_9542 },
>   	{ "pca9543", pca_9543 },
> @@ -140,12 +183,20 @@ static const struct i2c_device_id pca954x_id[] = {
>   	{ "pca9546", pca_9546 },
>   	{ "pca9547", pca_9547 },
>   	{ "pca9548", pca_9548 },
> +
> +	//984x family
> +	{ "pca9846", pca_9846 },
> +	{ "pca9847", pca_9847 },
> +	{ "pca9848", pca_9848 },
> +	{ "pca9849", pca_9849 },
> +
>   	{ }
>   };
> -MODULE_DEVICE_TABLE(i2c, pca954x_id);
> +MODULE_DEVICE_TABLE(i2c, pca9x4x_id);
>   
>   #ifdef CONFIG_OF
> -static const struct of_device_id pca954x_of_match[] = {
> +static const struct of_device_id pca9x4x_of_match[] = {
> +	//954x family
>   	{ .compatible = "nxp,pca9540", .data = &chips[pca_9540] },
>   	{ .compatible = "nxp,pca9542", .data = &chips[pca_9542] },
>   	{ .compatible = "nxp,pca9543", .data = &chips[pca_9543] },
> @@ -154,14 +205,22 @@ static const struct of_device_id pca954x_of_match[] = {
>   	{ .compatible = "nxp,pca9546", .data = &chips[pca_9546] },
>   	{ .compatible = "nxp,pca9547", .data = &chips[pca_9547] },
>   	{ .compatible = "nxp,pca9548", .data = &chips[pca_9548] },
> +
> +
> +	//984x family
> +	{ .compatible = "nxp,pca9846", .data = &chips[pca_9846] },
> +	{ .compatible = "nxp,pca9847", .data = &chips[pca_9847] },
> +	{ .compatible = "nxp,pca9848", .data = &chips[pca_9848] },
> +	{ .compatible = "nxp,pca9849", .data = &chips[pca_9849] },
> +
>   	{}
>   };
> -MODULE_DEVICE_TABLE(of, pca954x_of_match);
> +MODULE_DEVICE_TABLE(of, pca9x4x_of_match);
>   #endif
>   
>   /* Write to mux register. Don't use i2c_transfer()/i2c_smbus_xfer()
>      for this as they will try to lock adapter a second time */
> -static int pca954x_reg_write(struct i2c_adapter *adap,
> +static int pca9x4x_reg_write(struct i2c_adapter *adap,
>   			     struct i2c_client *client, u8 val)
>   {
>   	int ret = -ENODEV;
> @@ -190,32 +249,32 @@ static int pca954x_reg_write(struct i2c_adapter *adap,
>   	return ret;
>   }
>   
> -static int pca954x_select_chan(struct i2c_mux_core *muxc, u32 chan)
> +static int pca9x4x_select_chan(struct i2c_mux_core *muxc, u32 chan)
>   {
> -	struct pca954x *data = i2c_mux_priv(muxc);
> +	struct pca9x4x *data = i2c_mux_priv(muxc);
>   	struct i2c_client *client = data->client;
>   	const struct chip_desc *chip = data->chip;
>   	u8 regval;
>   	int ret = 0;
>   
>   	/* we make switches look like muxes, not sure how to be smarter */
> -	if (chip->muxtype == pca954x_ismux)
> +	if (chip->muxtype == pca9x4x_ismux)
>   		regval = chan | chip->enable;
>   	else
>   		regval = 1 << chan;
>   
>   	/* Only select the channel if its different from the last channel */
>   	if (data->last_chan != regval) {
> -		ret = pca954x_reg_write(muxc->parent, client, regval);
> +		ret = pca9x4x_reg_write(muxc->parent, client, regval);
>   		data->last_chan = ret < 0 ? 0 : regval;
>   	}
>   
>   	return ret;
>   }
>   
> -static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
> +static int pca9x4x_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
>   {
> -	struct pca954x *data = i2c_mux_priv(muxc);
> +	struct pca9x4x *data = i2c_mux_priv(muxc);
>   	struct i2c_client *client = data->client;
>   
>   	if (!(data->deselect & (1 << chan)))
> @@ -223,12 +282,12 @@ static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
>   
>   	/* Deselect active channel */
>   	data->last_chan = 0;
> -	return pca954x_reg_write(muxc->parent, client, data->last_chan);
> +	return pca9x4x_reg_write(muxc->parent, client, data->last_chan);
>   }
>   
> -static irqreturn_t pca954x_irq_handler(int irq, void *dev_id)
> +static irqreturn_t pca9x4x_irq_handler(int irq, void *dev_id)
>   {
> -	struct pca954x *data = dev_id;
> +	struct pca9x4x *data = dev_id;
>   	unsigned int child_irq;
>   	int ret, i, handled = 0;
>   
> @@ -237,7 +296,7 @@ static irqreturn_t pca954x_irq_handler(int irq, void *dev_id)
>   		return IRQ_NONE;
>   
>   	for (i = 0; i < data->chip->nchans; i++) {
> -		if (ret & BIT(PCA954X_IRQ_OFFSET + i)) {
> +		if (ret & BIT(PCA9X4X_IRQ_OFFSET + i)) {
>   			child_irq = irq_linear_revmap(data->irq, i);
>   			handle_nested_irq(child_irq);
>   			handled++;
> @@ -246,21 +305,21 @@ static irqreturn_t pca954x_irq_handler(int irq, void *dev_id)
>   	return handled ? IRQ_HANDLED : IRQ_NONE;
>   }
>   
> -static int pca954x_irq_set_type(struct irq_data *idata, unsigned int type)
> +static int pca9x4x_irq_set_type(struct irq_data *idata, unsigned int type)
>   {
>   	if ((type & IRQ_TYPE_SENSE_MASK) != IRQ_TYPE_LEVEL_LOW)
>   		return -EINVAL;
>   	return 0;
>   }
>   
> -static struct irq_chip pca954x_irq_chip = {
> -	.name = "i2c-mux-pca954x",
> -	.irq_set_type = pca954x_irq_set_type,
> +static struct irq_chip pca9x4x_irq_chip = {
> +	.name = "i2c-mux-pca9x4x",
> +	.irq_set_type = pca9x4x_irq_set_type,
>   };
>   
> -static int pca954x_irq_setup(struct i2c_mux_core *muxc)
> +static int pca9x4x_irq_setup(struct i2c_mux_core *muxc)
>   {
> -	struct pca954x *data = i2c_mux_priv(muxc);
> +	struct pca9x4x *data = i2c_mux_priv(muxc);
>   	struct i2c_client *client = data->client;
>   	int c, irq;
>   
> @@ -282,16 +341,16 @@ static int pca954x_irq_setup(struct i2c_mux_core *muxc)
>   			return -EINVAL;
>   		}
>   		irq_set_chip_data(irq, data);
> -		irq_set_chip_and_handler(irq, &pca954x_irq_chip,
> -			handle_simple_irq);
> +		irq_set_chip_and_handler(irq, &pca9x4x_irq_chip,
> +					 handle_simple_irq);
>   	}
>   
>   	return 0;
>   }
>   
> -static void pca954x_cleanup(struct i2c_mux_core *muxc)
> +static void pca9x4x_cleanup(struct i2c_mux_core *muxc)
>   {
> -	struct pca954x *data = i2c_mux_priv(muxc);
> +	struct pca9x4x *data = i2c_mux_priv(muxc);
>   	int c, irq;
>   
>   	if (data->irq) {
> @@ -304,20 +363,92 @@ static void pca954x_cleanup(struct i2c_mux_core *muxc)
>   	i2c_mux_del_adapters(muxc);
>   }
>   
> +/*
> + * Part of probe function specific for pca954x family
> + */
> +inline int pca954x_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id){
> +
> +	/* Write the mux register at addr to verify
> +	 * that the mux is in fact present. This also
> +	 * initializes the mux to disconnected state.
> +	 */
> +	if (i2c_smbus_write_byte(client, 0) < 0)
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +/*
> + * Part of probe function specific for pca984x family
> + */
> +inline int pca984x_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id){
> +
> +	struct i2c_adapter *adap = to_i2c_adapter(client->dev.parent);
> +	union i2c_smbus_data device_id_raw;
> +	u16 manufacturer_id; //12 bits
> +
> +	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) {
> +		dev_warn(&client->dev, "PCA9846 family: I2c adapter doesn't support I2C_FUNC_SMBUS_WRITE_BYTE_DATA");
> +		return -ENODEV;
> +	}
> +
> +	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
> +		dev_warn(&client->dev, "PCA9846 family: I2c adapter doesn't support I2C_FUNC_SMBUS_READ_I2C_BLOCK");
> +		return -ENODEV;
> +	}
> +
> +	//
> +	//Software reset
> +	//

I'm not sure you can use "//" for comments...

> +	if (i2c_smbus_xfer(client->adapter, GENERAL_CALL, client->flags,
> +			   I2C_SMBUS_WRITE, SOFTWARE_RESET, I2C_SMBUS_BYTE, NULL)  < 0) {
> +		dev_warn(&client->dev, "PCA9846 family: Sofrware reset failed\n");
> +		return -ENODEV;
> +	}
> +
> +	//
> +	//Get device ID
> +	//
> +	device_id_raw.block[0] = 3;  //read 3 bytes
> +	if (i2c_smbus_xfer(client->adapter, DEVICE_ID_ADDRESS, client->flags,
> +			   I2C_SMBUS_READ,  client->addr << 1,
> +			   I2C_SMBUS_I2C_BLOCK_DATA, &device_id_raw)) {
> +		dev_warn(&client->dev, "PCA9846 family: Get device ID failed\n");
> +		return -ENODEV;
> +	}
> +
> +	//Device ID contains only 3 bytes
> +	if (device_id_raw.block[0] != 3) {
> +		dev_warn(&client->dev, "PCA9846 family: Get device ID failed\n");
> +		return -ENODEV;
> +	}
> +
> +	//Check manufacturer ID (12 bits)
> +	manufacturer_id = ((u16) device_id_raw.block[1] << 4) | (device_id_raw.block[2] >> 4);
> +	if (manufacturer_id != NXP_ID) {
> +		dev_warn(&client->dev, "PCA9846 family: Manufacturer ID does not match NXP\n");
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
>   /*
>    * I2C init/probing/exit functions
>    */
> -static int pca954x_probe(struct i2c_client *client,
> +static int pca9x4x_probe(struct i2c_client *client,
>   			 const struct i2c_device_id *id)
>   {
>   	struct i2c_adapter *adap = to_i2c_adapter(client->dev.parent);
> -	struct pca954x_platform_data *pdata = dev_get_platdata(&client->dev);
> +	struct pca9x4x_platform_data *pdata = dev_get_platdata(&client->dev);
>   	struct device_node *of_node = client->dev.of_node;
>   	bool idle_disconnect_dt;
>   	struct gpio_desc *gpio;
>   	int num, force, class;
>   	struct i2c_mux_core *muxc;
> -	struct pca954x *data;
> +	struct pca9x4x *data;
>   	const struct of_device_id *match;
>   	int ret;
>   
> @@ -325,8 +456,8 @@ static int pca954x_probe(struct i2c_client *client,
>   		return -ENODEV;
>   
>   	muxc = i2c_mux_alloc(adap, &client->dev,
> -			     PCA954X_MAX_NCHANS, sizeof(*data), 0,
> -			     pca954x_select_chan, pca954x_deselect_mux);
> +			     PCA9X4X_MAX_NCHANS, sizeof(*data), 0,
> +			     pca9x4x_select_chan, pca9x4x_deselect_mux);
>   	if (!muxc)
>   		return -ENOMEM;
>   	data = i2c_mux_priv(muxc);
> @@ -339,16 +470,34 @@ static int pca954x_probe(struct i2c_client *client,
>   	if (IS_ERR(gpio))
>   		return PTR_ERR(gpio);
>   
> -	/* Write the mux register at addr to verify
> -	 * that the mux is in fact present. This also
> -	 * initializes the mux to disconnected state.
> -	 */
> -	if (i2c_smbus_write_byte(client, 0) < 0) {
> +	switch (id->driver_data) {
> +	case pca_9540:
> +	case pca_9542:
> +	case pca_9543:
> +	case pca_9544:
> +	case pca_9545:
> +	case pca_9546:
> +	case pca_9547:
> +	case pca_9548:
> +		ret = pca954x_probe(client, id);
> +		break;
> +	case pca_9846:
> +	case pca_9847:
> +	case pca_9848:
> +	case pca_9849:
> +		ret = pca984x_probe(client, id);
> +		break;
> +	default: //unknown device
> +		ret = -ENODEV;
> +		break;
> +	}
> +
> +	if (ret < 0) {
>   		dev_warn(&client->dev, "probe failed\n");
>   		return -ENODEV;
>   	}
>   
> -	match = of_match_device(of_match_ptr(pca954x_of_match), &client->dev);
> +	match = of_match_device(of_match_ptr(pca9x4x_of_match), &client->dev);
>   	if (match)
>   		data->chip = of_device_get_match_data(&client->dev);
>   	else
> @@ -359,7 +508,7 @@ static int pca954x_probe(struct i2c_client *client,
>   	idle_disconnect_dt = of_node &&
>   		of_property_read_bool(of_node, "i2c-mux-idle-disconnect");
>   
> -	ret = pca954x_irq_setup(muxc);
> +	ret = pca9x4x_irq_setup(muxc);
>   	if (ret)
>   		goto fail_cleanup;
>   
> @@ -389,60 +538,60 @@ static int pca954x_probe(struct i2c_client *client,
>   
>   	if (data->irq) {
>   		ret = devm_request_threaded_irq(&client->dev, data->client->irq,
> -						NULL, pca954x_irq_handler,
> +						NULL, pca9x4x_irq_handler,
>   						IRQF_ONESHOT | IRQF_SHARED,
> -						"pca954x", data);
> +						"pca9x4x", data);
>   		if (ret)
>   			goto fail_cleanup;
>   	}
>   
>   	dev_info(&client->dev,
>   		 "registered %d multiplexed busses for I2C %s %s\n",
> -		 num, data->chip->muxtype == pca954x_ismux
> -				? "mux" : "switch", client->name);
> +		 num, data->chip->muxtype == pca9x4x_ismux
> +		 ? "mux" : "switch", client->name);
>   
>   	return 0;
>   
>   fail_cleanup:
> -	pca954x_cleanup(muxc);
> +	pca9x4x_cleanup(muxc);
>   	return ret;
>   }
>   
> -static int pca954x_remove(struct i2c_client *client)
> +static int pca9x4x_remove(struct i2c_client *client)
>   {
>   	struct i2c_mux_core *muxc = i2c_get_clientdata(client);
>   
> -	pca954x_cleanup(muxc);
> +	pca9x4x_cleanup(muxc);
>   	return 0;
>   }
>   
>   #ifdef CONFIG_PM_SLEEP
> -static int pca954x_resume(struct device *dev)
> +static int pca9x4x_resume(struct device *dev)
>   {
>   	struct i2c_client *client = to_i2c_client(dev);
>   	struct i2c_mux_core *muxc = i2c_get_clientdata(client);
> -	struct pca954x *data = i2c_mux_priv(muxc);
> +	struct pca9x4x *data = i2c_mux_priv(muxc);
>   
>   	data->last_chan = 0;
>   	return i2c_smbus_write_byte(client, 0);
>   }
>   #endif
>   
> -static SIMPLE_DEV_PM_OPS(pca954x_pm, NULL, pca954x_resume);
> +static SIMPLE_DEV_PM_OPS(pca9x4x_pm, NULL, pca9x4x_resume);
>   
> -static struct i2c_driver pca954x_driver = {
> +static struct i2c_driver pca9x4x_driver = {
>   	.driver		= {
> -		.name	= "pca954x",
> -		.pm	= &pca954x_pm,
> -		.of_match_table = of_match_ptr(pca954x_of_match),
> +		.name	= "pca9x4x",
> +		.pm	= &pca9x4x_pm,
> +		.of_match_table = of_match_ptr(pca9x4x_of_match),
>   	},
> -	.probe		= pca954x_probe,
> -	.remove		= pca954x_remove,
> -	.id_table	= pca954x_id,
> +	.probe		= pca9x4x_probe,
> +	.remove		= pca9x4x_remove,
> +	.id_table	= pca9x4x_id,
>   };
>   
> -module_i2c_driver(pca954x_driver);
> +module_i2c_driver(pca9x4x_driver);
>   
> -MODULE_AUTHOR("Rodolfo Giometti <giometti@linux.it>");
> -MODULE_DESCRIPTION("PCA954x I2C mux/switch driver");
> +MODULE_AUTHOR("Rodolfo Giometti <giometti@linux.it>, Adrian Fiergolski <Adrian.Fiergolski@cern.ch>");
> +MODULE_DESCRIPTION("PCA9x4x I2C mux/switch driver");
>   MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/platform_data/pca954x.h b/include/linux/platform_data/pca9x4x.h
> similarity index 80%
> rename from include/linux/platform_data/pca954x.h
> rename to include/linux/platform_data/pca9x4x.h
> index 1712677d5904..a33c5afbfd7f 100644
> --- a/include/linux/platform_data/pca954x.h
> +++ b/include/linux/platform_data/pca9x4x.h
> @@ -2,6 +2,7 @@
>    *
>    * pca954x.h - I2C multiplexer/switch support
>    *
> + * Copyright (c) 2017 Adrian Fiergolski <Adrian.Fiergolski@cern.ch>
>    * Copyright (c) 2008-2009 Rodolfo Giometti <giometti@linux.it>
>    * Copyright (c) 2008-2009 Eurotech S.p.A. <info@eurotech.it>
>    * Michael Lawnick <michael.lawnick.ext@nsn.com>
> @@ -22,10 +23,10 @@
>    */
>   
>   
> -#ifndef _LINUX_I2C_PCA954X_H
> -#define _LINUX_I2C_PCA954X_H
> +#ifndef _LINUX_I2C_PCA9X4X_H
> +#define _LINUX_I2C_PCA9X4X_H
>   
> -/* Platform data for the PCA954x I2C multiplexers */
> +/* Platform data for the PCA9x4x I2C multiplexers */
>   
>   /* Per channel initialisation data:
>    * @adap_id: bus number for the adapter. 0 = don't care
> @@ -33,16 +34,16 @@
>    *                    of this channel after transaction.
>    *
>    */
> -struct pca954x_platform_mode {
> +struct pca9x4x_platform_mode {
>   	int		adap_id;
>   	unsigned int	deselect_on_exit:1;
>   	unsigned int	class;
>   };
>   
>   /* Per mux/switch data, used with i2c_register_board_info */
> -struct pca954x_platform_data {
> -	struct pca954x_platform_mode *modes;
> +struct pca9x4x_platform_data {
> +	struct pca9x4x_platform_mode *modes;
>   	int num_modes;
>   };
>   
> -#endif /* _LINUX_I2C_PCA954X_H */
> +#endif /* _LINUX_I2C_PCA9X4X_H */
> 


-- 

HCE Engineering                      e-mail: giometti@hce-engineering.it
GNU/Linux Solutions                          giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti
Cosino Project - the quick prototyping embedded system - www.cosino.it
Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it

Re: [PATCH] i2c: Add support for NXP PCA984x family.

[Peter Rosin]

On 2017-12-11 12:25, Rodolfo Giometti wrote:
> On 11/12/17 12:10, Adrian Fiergolski wrote:
>> This patch exetends the current i2c-mux-pca954x driver and adds suuport for
>> a newer PCA984x family of the I2C switches and multiplexers from NXP.
>> In probe function, the patch supports device ID function, introduced in the
>> new family, and checks it against configuration provided by the
>> devicetree. Moreover, it also performs software reset which is now
>> available for the new devices.
>> The reset of the code remains common with the legacy PCA954x devices.
>>
>> Signed-off-by: Adrian Fiergolski <adrian.fiergolski@cern.ch>
>> ---
>>   .../{i2c-mux-pca954x.txt => i2c-mux-pca9x4x.txt}   |   5 +-
>>   arch/arm/configs/aspeed_g4_defconfig               |   2 +-
>>   arch/arm/configs/aspeed_g5_defconfig               |   2 +-
>>   arch/arm/configs/multi_v7_defconfig                |   2 +-
>>   arch/arm/configs/pxa_defconfig                     |   2 +-
>>   arch/arm/configs/tegra_defconfig                   |   2 +-
>>   arch/arm64/configs/defconfig                       |   2 +-
>>   arch/powerpc/configs/85xx-hw.config                |   2 +-
>>   drivers/i2c/muxes/Kconfig                          |   6 +-
>>   drivers/i2c/muxes/Makefile                         |   2 +-
>>   drivers/i2c/muxes/i2c-mux-pca9541.c                |   4 +-
>>   .../muxes/{i2c-mux-pca954x.c => i2c-mux-pca9x4x.c} | 295 ++++++++++++++++-----
>>   .../linux/platform_data/{pca954x.h => pca9x4x.h}   |  15 +-
>>   13 files changed, 246 insertions(+), 95 deletions(-)
>>   rename Documentation/devicetree/bindings/i2c/{i2c-mux-pca954x.txt => i2c-mux-pca9x4x.txt} (91%)
>>   rename drivers/i2c/muxes/{i2c-mux-pca954x.c => i2c-mux-pca9x4x.c} (56%)
>>   rename include/linux/platform_data/{pca954x.h => pca9x4x.h} (80%)
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca9x4x.txt
>> similarity index 91%
>> rename from Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
>> rename to Documentation/devicetree/bindings/i2c/i2c-mux-pca9x4x.txt
>> index aa097045a10e..cf9a075ca1dd 100644
>> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca9x4x.txt
>> @@ -1,10 +1,11 @@
>> -* NXP PCA954x I2C bus switch
>> +* NXP PCA9x4x I2C bus switch
>>   
>>   Required Properties:
>>   
>>     - compatible: Must contain one of the following.
>>       "nxp,pca9540", "nxp,pca9542", "nxp,pca9543", "nxp,pca9544",
>> -    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548"
>> +    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548",
>> +    "nxp,pca9846", "nxp,pca9847", "nxp,pca9848", "nxp,pca9849"
>>   
>>     - reg: The I2C address of the device.
>>   
>> diff --git a/arch/arm/configs/aspeed_g4_defconfig b/arch/arm/configs/aspeed_g4_defconfig
>> index d23b9d56a88b..a461ad3cf63d 100644
>> --- a/arch/arm/configs/aspeed_g4_defconfig
>> +++ b/arch/arm/configs/aspeed_g4_defconfig
>> @@ -116,7 +116,7 @@ CONFIG_I2C=y
>>   CONFIG_I2C_CHARDEV=y
>>   CONFIG_I2C_MUX=y
>>   CONFIG_I2C_MUX_PCA9541=y
>> -CONFIG_I2C_MUX_PCA954x=y
>> +CONFIG_I2C_MUX_PCA9x4x=y
> 
> Nak.
> 
> I'm not sure you should break backward compatibility. You should keep the 
> CONFIG_I2C_MUX_PCA954x configuration setting and the current name convention.

Right, definitely avoid the mass rename. In addition to the pointless
churn, pca9x4x is just plain wrong since it matches e.g. pca9745 which
has nothing whatsoever to do with this driver.

I'll add more comments once the rename noise is gone.

Cheers,
Peter

Re: [PATCH] i2c: Add support for NXP PCA984x family.

[Adrian Fiergolski]

On 11.12.2017 at 13:51, Peter Rosin wrote:
> On 2017-12-11 12:25, Rodolfo Giometti wrote:
>> On 11/12/17 12:10, Adrian Fiergolski wrote:
>>> This patch exetends the current i2c-mux-pca954x driver and adds suuport for
>>> a newer PCA984x family of the I2C switches and multiplexers from NXP.
>>> In probe function, the patch supports device ID function, introduced in the
>>> new family, and checks it against configuration provided by the
>>> devicetree. Moreover, it also performs software reset which is now
>>> available for the new devices.
>>> The reset of the code remains common with the legacy PCA954x devices.
>>>
>>> Signed-off-by: Adrian Fiergolski <adrian.fiergolski@cern.ch>
>>> ---
>>>   .../{i2c-mux-pca954x.txt => i2c-mux-pca9x4x.txt}   |   5 +-
>>>   arch/arm/configs/aspeed_g4_defconfig               |   2 +-
>>>   arch/arm/configs/aspeed_g5_defconfig               |   2 +-
>>>   arch/arm/configs/multi_v7_defconfig                |   2 +-
>>>   arch/arm/configs/pxa_defconfig                     |   2 +-
>>>   arch/arm/configs/tegra_defconfig                   |   2 +-
>>>   arch/arm64/configs/defconfig                       |   2 +-
>>>   arch/powerpc/configs/85xx-hw.config                |   2 +-
>>>   drivers/i2c/muxes/Kconfig                          |   6 +-
>>>   drivers/i2c/muxes/Makefile                         |   2 +-
>>>   drivers/i2c/muxes/i2c-mux-pca9541.c                |   4 +-
>>>   .../muxes/{i2c-mux-pca954x.c => i2c-mux-pca9x4x.c} | 295 ++++++++++++++++-----
>>>   .../linux/platform_data/{pca954x.h => pca9x4x.h}   |  15 +-
>>>   13 files changed, 246 insertions(+), 95 deletions(-)
>>>   rename Documentation/devicetree/bindings/i2c/{i2c-mux-pca954x.txt => i2c-mux-pca9x4x.txt} (91%)
>>>   rename drivers/i2c/muxes/{i2c-mux-pca954x.c => i2c-mux-pca9x4x.c} (56%)
>>>   rename include/linux/platform_data/{pca954x.h => pca9x4x.h} (80%)
>>>
>>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca9x4x.txt
>>> similarity index 91%
>>> rename from Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
>>> rename to Documentation/devicetree/bindings/i2c/i2c-mux-pca9x4x.txt
>>> index aa097045a10e..cf9a075ca1dd 100644
>>> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
>>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca9x4x.txt
>>> @@ -1,10 +1,11 @@
>>> -* NXP PCA954x I2C bus switch
>>> +* NXP PCA9x4x I2C bus switch
>>>   
>>>   Required Properties:
>>>   
>>>     - compatible: Must contain one of the following.
>>>       "nxp,pca9540", "nxp,pca9542", "nxp,pca9543", "nxp,pca9544",
>>> -    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548"
>>> +    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548",
>>> +    "nxp,pca9846", "nxp,pca9847", "nxp,pca9848", "nxp,pca9849"
>>>   
>>>     - reg: The I2C address of the device.
>>>   
>>> diff --git a/arch/arm/configs/aspeed_g4_defconfig b/arch/arm/configs/aspeed_g4_defconfig
>>> index d23b9d56a88b..a461ad3cf63d 100644
>>> --- a/arch/arm/configs/aspeed_g4_defconfig
>>> +++ b/arch/arm/configs/aspeed_g4_defconfig
>>> @@ -116,7 +116,7 @@ CONFIG_I2C=y
>>>   CONFIG_I2C_CHARDEV=y
>>>   CONFIG_I2C_MUX=y
>>>   CONFIG_I2C_MUX_PCA9541=y
>>> -CONFIG_I2C_MUX_PCA954x=y
>>> +CONFIG_I2C_MUX_PCA9x4x=y
>> Nak.
>>
>> I'm not sure you should break backward compatibility. You should keep the 
>> CONFIG_I2C_MUX_PCA954x configuration setting and the current name convention.
> Right, definitely avoid the mass rename. In addition to the pointless
> churn, pca9x4x is just plain wrong since it matches e.g. pca9745 which
> has nothing whatsoever to do with this driver.
>
> I'll add more comments once the rename noise is gone.
>
> Cheers,
> Peter
>
Thanks for comments.
What do you suggest then ? I could keep CONFIG_I2C_MUX_PCA954x, but
then, since there is a common source, if enabled, the PCA985x family
support would be provided as well anyway. I could try to move PCA985x
features to a separate file which would be included by i2c-mux-pca954x.c
if a dedicated CONFIG_I2C_MUX_PCA984x is enabled, but it doesn't seem to
be an elegant solution to me.

I understand, that your main concern regards CONFIG_I2C_MUX_PCA954x
only. The fact that internal data types and files have been renamed to a
more generic and actual pca9x5x style is fine, right ?

Cheers,
Adrian

Re: [PATCH] i2c: Add support for NXP PCA984x family.

[Peter Rosin]

On 2017-12-11 14:15, Adrian Fiergolski wrote:
> On 11.12.2017 at 13:51, Peter Rosin wrote:
>> On 2017-12-11 12:25, Rodolfo Giometti wrote:
>>> On 11/12/17 12:10, Adrian Fiergolski wrote:
>>>> This patch exetends the current i2c-mux-pca954x driver and adds suuport for
>>>> a newer PCA984x family of the I2C switches and multiplexers from NXP.
>>>> In probe function, the patch supports device ID function, introduced in the
>>>> new family, and checks it against configuration provided by the
>>>> devicetree. Moreover, it also performs software reset which is now
>>>> available for the new devices.
>>>> The reset of the code remains common with the legacy PCA954x devices.
>>>>
>>>> Signed-off-by: Adrian Fiergolski <adrian.fiergolski@cern.ch>
>>>> ---
>>>>   .../{i2c-mux-pca954x.txt => i2c-mux-pca9x4x.txt}   |   5 +-
>>>>   arch/arm/configs/aspeed_g4_defconfig               |   2 +-
>>>>   arch/arm/configs/aspeed_g5_defconfig               |   2 +-
>>>>   arch/arm/configs/multi_v7_defconfig                |   2 +-
>>>>   arch/arm/configs/pxa_defconfig                     |   2 +-
>>>>   arch/arm/configs/tegra_defconfig                   |   2 +-
>>>>   arch/arm64/configs/defconfig                       |   2 +-
>>>>   arch/powerpc/configs/85xx-hw.config                |   2 +-
>>>>   drivers/i2c/muxes/Kconfig                          |   6 +-
>>>>   drivers/i2c/muxes/Makefile                         |   2 +-
>>>>   drivers/i2c/muxes/i2c-mux-pca9541.c                |   4 +-
>>>>   .../muxes/{i2c-mux-pca954x.c => i2c-mux-pca9x4x.c} | 295 ++++++++++++++++-----
>>>>   .../linux/platform_data/{pca954x.h => pca9x4x.h}   |  15 +-
>>>>   13 files changed, 246 insertions(+), 95 deletions(-)
>>>>   rename Documentation/devicetree/bindings/i2c/{i2c-mux-pca954x.txt => i2c-mux-pca9x4x.txt} (91%)
>>>>   rename drivers/i2c/muxes/{i2c-mux-pca954x.c => i2c-mux-pca9x4x.c} (56%)
>>>>   rename include/linux/platform_data/{pca954x.h => pca9x4x.h} (80%)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca9x4x.txt
>>>> similarity index 91%
>>>> rename from Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
>>>> rename to Documentation/devicetree/bindings/i2c/i2c-mux-pca9x4x.txt
>>>> index aa097045a10e..cf9a075ca1dd 100644
>>>> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
>>>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca9x4x.txt
>>>> @@ -1,10 +1,11 @@
>>>> -* NXP PCA954x I2C bus switch
>>>> +* NXP PCA9x4x I2C bus switch
>>>>   
>>>>   Required Properties:
>>>>   
>>>>     - compatible: Must contain one of the following.
>>>>       "nxp,pca9540", "nxp,pca9542", "nxp,pca9543", "nxp,pca9544",
>>>> -    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548"
>>>> +    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548",
>>>> +    "nxp,pca9846", "nxp,pca9847", "nxp,pca9848", "nxp,pca9849"
>>>>   
>>>>     - reg: The I2C address of the device.
>>>>   
>>>> diff --git a/arch/arm/configs/aspeed_g4_defconfig b/arch/arm/configs/aspeed_g4_defconfig
>>>> index d23b9d56a88b..a461ad3cf63d 100644
>>>> --- a/arch/arm/configs/aspeed_g4_defconfig
>>>> +++ b/arch/arm/configs/aspeed_g4_defconfig
>>>> @@ -116,7 +116,7 @@ CONFIG_I2C=y
>>>>   CONFIG_I2C_CHARDEV=y
>>>>   CONFIG_I2C_MUX=y
>>>>   CONFIG_I2C_MUX_PCA9541=y
>>>> -CONFIG_I2C_MUX_PCA954x=y
>>>> +CONFIG_I2C_MUX_PCA9x4x=y
>>> Nak.
>>>
>>> I'm not sure you should break backward compatibility. You should keep the 
>>> CONFIG_I2C_MUX_PCA954x configuration setting and the current name convention.
>> Right, definitely avoid the mass rename. In addition to the pointless
>> churn, pca9x4x is just plain wrong since it matches e.g. pca9745 which
>> has nothing whatsoever to do with this driver.
>>
>> I'll add more comments once the rename noise is gone.
>>
>> Cheers,
>> Peter
>>
> Thanks for comments.
> What do you suggest then ? I could keep CONFIG_I2C_MUX_PCA954x, but
> then, since there is a common source, if enabled, the PCA985x family
> support would be provided as well anyway. I could try to move PCA985x
> features to a separate file which would be included by i2c-mux-pca954x.c
> if a dedicated CONFIG_I2C_MUX_PCA984x is enabled, but it doesn't seem to
> be an elegant solution to me.
> 
> I understand, that your main concern regards CONFIG_I2C_MUX_PCA954x
> only. The fact that internal data types and files have been renamed to a
> more generic and actual pca9x5x style is fine, right ?

Keep the name as PCA954x, just add support for PCA9846-9 without touching
either file names, config options or variables prefixes. The only rename
that is appropriate is to PCA9540, but since the x is already there, PCA954x
stays as it is.

The config option CONFIG_I2C_MUX_PCA954x will do nicely for PCA9846-9 too,
all that is needed is to explain what is going on in the help text, as
indicated by Rodolfo.

So, don't split the file and don't get distracted by the driver covering
more hardware than indicated by the driver names.

Cheers,
Peter

Re: [PATCH] i2c: Add support for NXP PCA984x family.

[Rodolfo Giometti]

On 11/12/17 14:15, Adrian Fiergolski wrote:
> On 11.12.2017 at 13:51, Peter Rosin wrote:
>> On 2017-12-11 12:25, Rodolfo Giometti wrote:
>>> On 11/12/17 12:10, Adrian Fiergolski wrote:
>>>> This patch exetends the current i2c-mux-pca954x driver and adds suuport for
>>>> a newer PCA984x family of the I2C switches and multiplexers from NXP.
>>>> In probe function, the patch supports device ID function, introduced in the
>>>> new family, and checks it against configuration provided by the
>>>> devicetree. Moreover, it also performs software reset which is now
>>>> available for the new devices.
>>>> The reset of the code remains common with the legacy PCA954x devices.
>>>>
>>>> Signed-off-by: Adrian Fiergolski <adrian.fiergolski@cern.ch>
>>>> ---
>>>>    .../{i2c-mux-pca954x.txt => i2c-mux-pca9x4x.txt}   |   5 +-
>>>>    arch/arm/configs/aspeed_g4_defconfig               |   2 +-
>>>>    arch/arm/configs/aspeed_g5_defconfig               |   2 +-
>>>>    arch/arm/configs/multi_v7_defconfig                |   2 +-
>>>>    arch/arm/configs/pxa_defconfig                     |   2 +-
>>>>    arch/arm/configs/tegra_defconfig                   |   2 +-
>>>>    arch/arm64/configs/defconfig                       |   2 +-
>>>>    arch/powerpc/configs/85xx-hw.config                |   2 +-
>>>>    drivers/i2c/muxes/Kconfig                          |   6 +-
>>>>    drivers/i2c/muxes/Makefile                         |   2 +-
>>>>    drivers/i2c/muxes/i2c-mux-pca9541.c                |   4 +-
>>>>    .../muxes/{i2c-mux-pca954x.c => i2c-mux-pca9x4x.c} | 295 ++++++++++++++++-----
>>>>    .../linux/platform_data/{pca954x.h => pca9x4x.h}   |  15 +-
>>>>    13 files changed, 246 insertions(+), 95 deletions(-)
>>>>    rename Documentation/devicetree/bindings/i2c/{i2c-mux-pca954x.txt => i2c-mux-pca9x4x.txt} (91%)
>>>>    rename drivers/i2c/muxes/{i2c-mux-pca954x.c => i2c-mux-pca9x4x.c} (56%)
>>>>    rename include/linux/platform_data/{pca954x.h => pca9x4x.h} (80%)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca9x4x.txt
>>>> similarity index 91%
>>>> rename from Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
>>>> rename to Documentation/devicetree/bindings/i2c/i2c-mux-pca9x4x.txt
>>>> index aa097045a10e..cf9a075ca1dd 100644
>>>> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
>>>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca9x4x.txt
>>>> @@ -1,10 +1,11 @@
>>>> -* NXP PCA954x I2C bus switch
>>>> +* NXP PCA9x4x I2C bus switch
>>>>    
>>>>    Required Properties:
>>>>    
>>>>      - compatible: Must contain one of the following.
>>>>        "nxp,pca9540", "nxp,pca9542", "nxp,pca9543", "nxp,pca9544",
>>>> -    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548"
>>>> +    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548",
>>>> +    "nxp,pca9846", "nxp,pca9847", "nxp,pca9848", "nxp,pca9849"
>>>>    
>>>>      - reg: The I2C address of the device.
>>>>    
>>>> diff --git a/arch/arm/configs/aspeed_g4_defconfig b/arch/arm/configs/aspeed_g4_defconfig
>>>> index d23b9d56a88b..a461ad3cf63d 100644
>>>> --- a/arch/arm/configs/aspeed_g4_defconfig
>>>> +++ b/arch/arm/configs/aspeed_g4_defconfig
>>>> @@ -116,7 +116,7 @@ CONFIG_I2C=y
>>>>    CONFIG_I2C_CHARDEV=y
>>>>    CONFIG_I2C_MUX=y
>>>>    CONFIG_I2C_MUX_PCA9541=y
>>>> -CONFIG_I2C_MUX_PCA954x=y
>>>> +CONFIG_I2C_MUX_PCA9x4x=y
>>> Nak.
>>>
>>> I'm not sure you should break backward compatibility. You should keep the
>>> CONFIG_I2C_MUX_PCA954x configuration setting and the current name convention.
>> Right, definitely avoid the mass rename. In addition to the pointless
>> churn, pca9x4x is just plain wrong since it matches e.g. pca9745 which
>> has nothing whatsoever to do with this driver.
>>
>> I'll add more comments once the rename noise is gone.
>>
>> Cheers,
>> Peter
>>
> Thanks for comments.
> What do you suggest then ? I could keep CONFIG_I2C_MUX_PCA954x, but
> then, since there is a common source, if enabled, the PCA985x family
> support would be provided as well anyway. I could try to move PCA985x
> features to a separate file which would be included by i2c-mux-pca954x.c
> if a dedicated CONFIG_I2C_MUX_PCA984x is enabled, but it doesn't seem to
> be an elegant solution to me.
> 
> I understand, that your main concern regards CONFIG_I2C_MUX_PCA954x
> only. The fact that internal data types and files have been renamed to a
> more generic and actual pca9x5x style is fine, right ?

None. Just drop the mass rename and add support for the new family. Also don't 
forget to mention the new support into documentation/configuration files.

Ciao,

Rodolfo

-- 

HCE Engineering                      e-mail: giometti@hce-engineering.it
GNU/Linux Solutions                          giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti
Cosino Project - the quick prototyping embedded system - www.cosino.it
Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it

[PATCH v2] i2c: Add support for NXP PCA984x family.

[Adrian Fiergolski]

This patch exetends the current i2c-mux-pca954x driver and adds suuport for
a newer PCA984x family of the I2C switches and multiplexers from NXP.
In probe function, the patch supports device ID function, introduced in the
new family, and checks it against configuration provided by the
devicetree. Moreover, it also performs software reset which is now
available for the new devices.
The reset of the code remains common with the legacy PCA954x devices.

Signed-off-by: Adrian Fiergolski <adrian.fiergolski@cern.ch>
---
 .../devicetree/bindings/i2c/i2c-mux-pca954x.txt    |   5 +-
 drivers/i2c/muxes/Kconfig                          |   2 +-
 drivers/i2c/muxes/i2c-mux-pca954x.c                | 164 ++++++++++++++++++++-
 3 files changed, 161 insertions(+), 10 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
index aa097045a10e..b428bc0d81b1 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
@@ -1,10 +1,13 @@
 * NXP PCA954x I2C bus switch
 
+The driver supports NXP PCA954x and PCA984x I2C mux/switch devices.
+
 Required Properties:
 
   - compatible: Must contain one of the following.
     "nxp,pca9540", "nxp,pca9542", "nxp,pca9543", "nxp,pca9544",
-    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548"
+    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548",
+    "nxp,pca9846", "nxp,pca9847", "nxp,pca9848", "nxp,pca9849"
 
   - reg: The I2C address of the device.
 
diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
index 0f5c8fc36625..5da2e04585a9 100644
--- a/drivers/i2c/muxes/Kconfig
+++ b/drivers/i2c/muxes/Kconfig
@@ -68,7 +68,7 @@ config I2C_MUX_PCA954x
 	depends on GPIOLIB || COMPILE_TEST
 	help
 	  If you say yes here you get support for the Philips PCA954x
-	  I2C mux/switch devices.
+	  and PCA984x I2C mux/switch devices.
 
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-mux-pca954x.
diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 2ca068d8b92d..88de57ae5b7a 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -1,14 +1,16 @@
 /*
  * I2C multiplexer
  *
+ * Copyright (c) 2017 Adrian Fiergolski <Adrian.Fiergolski@cern.ch>
  * Copyright (c) 2008-2009 Rodolfo Giometti <giometti@linux.it>
  * Copyright (c) 2008-2009 Eurotech S.p.A. <info@eurotech.it>
  *
- * This module supports the PCA954x series of I2C multiplexer/switch chips
- * made by Philips Semiconductors.
+ * This module supports the PCA954x and PCA954x series of I2C multiplexer/switch
+ * chips made by Philips Semiconductors.
  * This includes the:
  *	 PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547
  *	 and PCA9548.
+ *       PCA9846, PCA9847, PCA9848 and PCA9849
  *
  * These chips are all controlled via the I2C bus itself, and all have a
  * single 8-bit register. The upstream "parent" bus fans out to two,
@@ -54,6 +56,12 @@
 
 #define PCA954X_IRQ_OFFSET 4
 
+//PCA984x family I2C other addresses
+#define GENERAL_CALL  0x00
+#define SOFTWARE_RESET 0x06
+#define DEVICE_ID_ADDRESS 0x7C
+#define NXP_ID 0x00
+
 enum pca_type {
 	pca_9540,
 	pca_9542,
@@ -63,6 +71,11 @@ enum pca_type {
 	pca_9546,
 	pca_9547,
 	pca_9548,
+	pca_9846,
+	pca_9847,
+	pca_9848,
+	pca_9849,
+
 };
 
 struct chip_desc {
@@ -73,6 +86,7 @@ struct chip_desc {
 		pca954x_ismux = 0,
 		pca954x_isswi
 	} muxtype;
+	u16 deviceID; //used by PCA984x family only
 };
 
 struct pca954x {
@@ -89,6 +103,7 @@ struct pca954x {
 
 /* Provide specs for the PCA954x types we know about */
 static const struct chip_desc chips[] = {
+	//954x family
 	[pca_9540] = {
 		.nchans = 2,
 		.enable = 0x4,
@@ -129,9 +144,36 @@ static const struct chip_desc chips[] = {
 		.nchans = 8,
 		.muxtype = pca954x_isswi,
 	},
+
+	//984x family
+	[pca_9846] = {
+		.nchans = 4,
+		.muxtype = pca954x_isswi,
+		.deviceID = 0x10B,
+	},
+
+	[pca_9847] = {
+		.nchans = 8,
+		.muxtype = pca954x_ismux,
+		.deviceID = 0x108,
+	},
+
+	[pca_9848] = {
+		.nchans = 8,
+		.muxtype = pca954x_isswi,
+		.deviceID = 0x10A,
+	},
+
+	[pca_9849] = {
+		.nchans = 4,
+		.muxtype = pca954x_ismux,
+		.deviceID = 0x109,
+	},
+
 };
 
 static const struct i2c_device_id pca954x_id[] = {
+	//954x family
 	{ "pca9540", pca_9540 },
 	{ "pca9542", pca_9542 },
 	{ "pca9543", pca_9543 },
@@ -140,12 +182,20 @@ static const struct i2c_device_id pca954x_id[] = {
 	{ "pca9546", pca_9546 },
 	{ "pca9547", pca_9547 },
 	{ "pca9548", pca_9548 },
+
+	//984x family
+	{ "pca9846", pca_9846 },
+	{ "pca9847", pca_9847 },
+	{ "pca9848", pca_9848 },
+	{ "pca9849", pca_9849 },
+
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, pca954x_id);
 
 #ifdef CONFIG_OF
 static const struct of_device_id pca954x_of_match[] = {
+	//954x family
 	{ .compatible = "nxp,pca9540", .data = &chips[pca_9540] },
 	{ .compatible = "nxp,pca9542", .data = &chips[pca_9542] },
 	{ .compatible = "nxp,pca9543", .data = &chips[pca_9543] },
@@ -154,6 +204,14 @@ static const struct of_device_id pca954x_of_match[] = {
 	{ .compatible = "nxp,pca9546", .data = &chips[pca_9546] },
 	{ .compatible = "nxp,pca9547", .data = &chips[pca_9547] },
 	{ .compatible = "nxp,pca9548", .data = &chips[pca_9548] },
+
+
+	//984x family
+	{ .compatible = "nxp,pca9846", .data = &chips[pca_9846] },
+	{ .compatible = "nxp,pca9847", .data = &chips[pca_9847] },
+	{ .compatible = "nxp,pca9848", .data = &chips[pca_9848] },
+	{ .compatible = "nxp,pca9849", .data = &chips[pca_9849] },
+
 	{}
 };
 MODULE_DEVICE_TABLE(of, pca954x_of_match);
@@ -304,6 +362,78 @@ static void pca954x_cleanup(struct i2c_mux_core *muxc)
 	i2c_mux_del_adapters(muxc);
 }
 
+/*
+ * Part of probe function specific for pca954x family
+ */
+inline int _pca954x_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id){
+
+	/* Write the mux register at addr to verify
+	 * that the mux is in fact present. This also
+	 * initializes the mux to disconnected state.
+	 */
+	if (i2c_smbus_write_byte(client, 0) < 0)
+		return -ENODEV;
+
+	return 0;
+}
+
+/*
+ * Part of probe function specific for pca984x family
+ */
+inline int _pca984x_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id){
+
+	struct i2c_adapter *adap = to_i2c_adapter(client->dev.parent);
+	union i2c_smbus_data device_id_raw;
+	u16 manufacturer_id; //12 bits
+
+	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) {
+		dev_warn(&client->dev, "PCA9846 family: I2c adapter doesn't support I2C_FUNC_SMBUS_WRITE_BYTE_DATA");
+		return -ENODEV;
+	}
+
+	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
+		dev_warn(&client->dev, "PCA9846 family: I2c adapter doesn't support I2C_FUNC_SMBUS_READ_I2C_BLOCK");
+		return -ENODEV;
+	}
+
+	/*
+	 * Software reset
+	 */
+	if (i2c_smbus_xfer(client->adapter, GENERAL_CALL, client->flags,
+			   I2C_SMBUS_WRITE, SOFTWARE_RESET, I2C_SMBUS_BYTE, NULL)  < 0) {
+		dev_warn(&client->dev, "PCA9846 family: Sofrware reset failed\n");
+		return -ENODEV;
+	}
+
+	/*
+	 * Get device ID
+	 */
+	device_id_raw.block[0] = 3;  //read 3 bytes
+	if (i2c_smbus_xfer(client->adapter, DEVICE_ID_ADDRESS, client->flags,
+			   I2C_SMBUS_READ,  client->addr << 1,
+			   I2C_SMBUS_I2C_BLOCK_DATA, &device_id_raw)) {
+		dev_warn(&client->dev, "PCA9846 family: Get device ID failed\n");
+		return -ENODEV;
+	}
+
+	//Device ID contains only 3 bytes
+	if (device_id_raw.block[0] != 3) {
+		dev_warn(&client->dev, "PCA9846 family: Get device ID failed\n");
+		return -ENODEV;
+	}
+
+	//Check manufacturer ID (12 bits)
+	manufacturer_id = ((u16) device_id_raw.block[1] << 4) | (device_id_raw.block[2] >> 4);
+	if (manufacturer_id != NXP_ID) {
+		dev_warn(&client->dev, "PCA9846 family: Manufacturer ID does not match NXP\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
 /*
  * I2C init/probing/exit functions
  */
@@ -339,11 +469,29 @@ static int pca954x_probe(struct i2c_client *client,
 	if (IS_ERR(gpio))
 		return PTR_ERR(gpio);
 
-	/* Write the mux register at addr to verify
-	 * that the mux is in fact present. This also
-	 * initializes the mux to disconnected state.
-	 */
-	if (i2c_smbus_write_byte(client, 0) < 0) {
+	switch (id->driver_data) {
+	case pca_9540:
+	case pca_9542:
+	case pca_9543:
+	case pca_9544:
+	case pca_9545:
+	case pca_9546:
+	case pca_9547:
+	case pca_9548:
+		ret = _pca954x_probe(client, id);
+		break;
+	case pca_9846:
+	case pca_9847:
+	case pca_9848:
+	case pca_9849:
+		ret = _pca984x_probe(client, id);
+		break;
+	default: //unknown device
+		ret = -ENODEV;
+		break;
+	}
+
+	if (ret < 0) {
 		dev_warn(&client->dev, "probe failed\n");
 		return -ENODEV;
 	}
@@ -443,6 +591,6 @@ static struct i2c_driver pca954x_driver = {
 
 module_i2c_driver(pca954x_driver);
 
-MODULE_AUTHOR("Rodolfo Giometti <giometti@linux.it>");
+MODULE_AUTHOR("Rodolfo Giometti <giometti@linux.it>, Adrian Fiergolski <Adrian.Fiergolski@cern.ch>");
 MODULE_DESCRIPTION("PCA954x I2C mux/switch driver");
 MODULE_LICENSE("GPL v2");
-- 
2.14.1

Re: [PATCH v2] i2c: Add support for NXP PCA984x family.

[Peter Rosin]

On 2017-12-11 15:27, Adrian Fiergolski wrote:
> This patch exetends the current i2c-mux-pca954x driver and adds suuport for
> a newer PCA984x family of the I2C switches and multiplexers from NXP.
> In probe function, the patch supports device ID function, introduced in the
> new family, and checks it against configuration provided by the
> devicetree. Moreover, it also performs software reset which is now
> available for the new devices.
> The reset of the code remains common with the legacy PCA954x devices.
> 
> Signed-off-by: Adrian Fiergolski <adrian.fiergolski@cern.ch>
> ---

Somewhere below this triple-dash and before the start of the diff is a good
place to keep a list of changes for the patch.

>  .../devicetree/bindings/i2c/i2c-mux-pca954x.txt    |   5 +-
>  drivers/i2c/muxes/Kconfig                          |   2 +-
>  drivers/i2c/muxes/i2c-mux-pca954x.c                | 164 ++++++++++++++++++++-
>  3 files changed, 161 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> index aa097045a10e..b428bc0d81b1 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> @@ -1,10 +1,13 @@
>  * NXP PCA954x I2C bus switch
>  
> +The driver supports NXP PCA954x and PCA984x I2C mux/switch devices.
> +
>  Required Properties:
>  
>    - compatible: Must contain one of the following.
>      "nxp,pca9540", "nxp,pca9542", "nxp,pca9543", "nxp,pca9544",
> -    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548"
> +    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548",
> +    "nxp,pca9846", "nxp,pca9847", "nxp,pca9848", "nxp,pca9849"
>  
>    - reg: The I2C address of the device.
>  
> diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
> index 0f5c8fc36625..5da2e04585a9 100644
> --- a/drivers/i2c/muxes/Kconfig
> +++ b/drivers/i2c/muxes/Kconfig
> @@ -68,7 +68,7 @@ config I2C_MUX_PCA954x
>  	depends on GPIOLIB || COMPILE_TEST
>  	help
>  	  If you say yes here you get support for the Philips PCA954x
> -	  I2C mux/switch devices.
> +	  and PCA984x I2C mux/switch devices.
>  
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called i2c-mux-pca954x.
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 2ca068d8b92d..88de57ae5b7a 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -1,14 +1,16 @@
>  /*
>   * I2C multiplexer
>   *
> + * Copyright (c) 2017 Adrian Fiergolski <Adrian.Fiergolski@cern.ch>
>   * Copyright (c) 2008-2009 Rodolfo Giometti <giometti@linux.it>
>   * Copyright (c) 2008-2009 Eurotech S.p.A. <info@eurotech.it>
>   *
> - * This module supports the PCA954x series of I2C multiplexer/switch chips
> - * made by Philips Semiconductors.
> + * This module supports the PCA954x and PCA954x series of I2C multiplexer/switch
> + * chips made by Philips Semiconductors.
>   * This includes the:
>   *	 PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547
>   *	 and PCA9548.
> + *       PCA9846, PCA9847, PCA9848 and PCA9849

This could be formatted in a nicer way.

>   *
>   * These chips are all controlled via the I2C bus itself, and all have a
>   * single 8-bit register. The upstream "parent" bus fans out to two,
> @@ -54,6 +56,12 @@
>  
>  #define PCA954X_IRQ_OFFSET 4
>  
> +//PCA984x family I2C other addresses
> +#define GENERAL_CALL  0x00
> +#define SOFTWARE_RESET 0x06
> +#define DEVICE_ID_ADDRESS 0x7C
> +#define NXP_ID 0x00
> +
>  enum pca_type {
>  	pca_9540,
>  	pca_9542,
> @@ -63,6 +71,11 @@ enum pca_type {
>  	pca_9546,
>  	pca_9547,
>  	pca_9548,
> +	pca_9846,
> +	pca_9847,
> +	pca_9848,
> +	pca_9849,
> +

Drop this empty line.

>  };
>  
>  struct chip_desc {
> @@ -73,6 +86,7 @@ struct chip_desc {
>  		pca954x_ismux = 0,
>  		pca954x_isswi
>  	} muxtype;
> +	u16 deviceID; //used by PCA984x family only

Except it isn't used at all. Maybe just drop it?

>  };
>  
>  struct pca954x {
> @@ -89,6 +103,7 @@ struct pca954x {
>  
>  /* Provide specs for the PCA954x types we know about */
>  static const struct chip_desc chips[] = {
> +	//954x family

This line adds nothing of interest.

>  	[pca_9540] = {
>  		.nchans = 2,
>  		.enable = 0x4,
> @@ -129,9 +144,36 @@ static const struct chip_desc chips[] = {
>  		.nchans = 8,
>  		.muxtype = pca954x_isswi,
>  	},
> +
> +	//984x family

Drop the empty line here as well as below between the other chip_descs.
Also drop the family comment, I don't feel it adds anything relevant.

> +	[pca_9846] = {
> +		.nchans = 4,
> +		.muxtype = pca954x_isswi,
> +		.deviceID = 0x10B,
> +	},
> +
> +	[pca_9847] = {
> +		.nchans = 8,
> +		.muxtype = pca954x_ismux,
> +		.deviceID = 0x108,
> +	},
> +
> +	[pca_9848] = {
> +		.nchans = 8,
> +		.muxtype = pca954x_isswi,
> +		.deviceID = 0x10A,
> +	},
> +
> +	[pca_9849] = {
> +		.nchans = 4,
> +		.muxtype = pca954x_ismux,
> +		.deviceID = 0x109,
> +	},
> +
>  };
>  
>  static const struct i2c_device_id pca954x_id[] = {
> +	//954x family

Drop this.

>  	{ "pca9540", pca_9540 },
>  	{ "pca9542", pca_9542 },
>  	{ "pca9543", pca_9543 },
> @@ -140,12 +182,20 @@ static const struct i2c_device_id pca954x_id[] = {
>  	{ "pca9546", pca_9546 },
>  	{ "pca9547", pca_9547 },
>  	{ "pca9548", pca_9548 },
> +
> +	//984x family

Drop the empty line and the comment.

> +	{ "pca9846", pca_9846 },
> +	{ "pca9847", pca_9847 },
> +	{ "pca9848", pca_9848 },
> +	{ "pca9849", pca_9849 },
> +

Drop the empty line.

>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, pca954x_id);
>  
>  #ifdef CONFIG_OF
>  static const struct of_device_id pca954x_of_match[] = {
> +	//954x family

Dito.

>  	{ .compatible = "nxp,pca9540", .data = &chips[pca_9540] },
>  	{ .compatible = "nxp,pca9542", .data = &chips[pca_9542] },
>  	{ .compatible = "nxp,pca9543", .data = &chips[pca_9543] },
> @@ -154,6 +204,14 @@ static const struct of_device_id pca954x_of_match[] = {
>  	{ .compatible = "nxp,pca9546", .data = &chips[pca_9546] },
>  	{ .compatible = "nxp,pca9547", .data = &chips[pca_9547] },
>  	{ .compatible = "nxp,pca9548", .data = &chips[pca_9548] },
> +
> +
> +	//984x family

Dito.

> +	{ .compatible = "nxp,pca9846", .data = &chips[pca_9846] },
> +	{ .compatible = "nxp,pca9847", .data = &chips[pca_9847] },
> +	{ .compatible = "nxp,pca9848", .data = &chips[pca_9848] },
> +	{ .compatible = "nxp,pca9849", .data = &chips[pca_9849] },
> +

Dito.

>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, pca954x_of_match);
> @@ -304,6 +362,78 @@ static void pca954x_cleanup(struct i2c_mux_core *muxc)
>  	i2c_mux_del_adapters(muxc);
>  }
>  
> +/*
> + * Part of probe function specific for pca954x family
> + */
> +inline int _pca954x_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id){

The brace should be on a line by itself. No need to manually
say inline, any decent compiler will inline whatever is
appropriate, just use static instead. And lose the leading
underscore.

> +
> +	/* Write the mux register at addr to verify
> +	 * that the mux is in fact present. This also
> +	 * initializes the mux to disconnected state.
> +	 */
> +	if (i2c_smbus_write_byte(client, 0) < 0)
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +/*
> + * Part of probe function specific for pca984x family
> + */
> +inline int _pca984x_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id){

Dito.

> +
> +	struct i2c_adapter *adap = to_i2c_adapter(client->dev.parent);
> +	union i2c_smbus_data device_id_raw;
> +	u16 manufacturer_id; //12 bits
> +
> +	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) {
> +		dev_warn(&client->dev, "PCA9846 family: I2c adapter doesn't support I2C_FUNC_SMBUS_WRITE_BYTE_DATA");
> +		return -ENODEV;
> +	}
> +
> +	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
> +		dev_warn(&client->dev, "PCA9846 family: I2c adapter doesn't support I2C_FUNC_SMBUS_READ_I2C_BLOCK");
> +		return -ENODEV;
> +	}
> +
> +	/*
> +	 * Software reset
> +	 */

This could be a single line comment.

> +	if (i2c_smbus_xfer(client->adapter, GENERAL_CALL, client->flags,
> +			   I2C_SMBUS_WRITE, SOFTWARE_RESET, I2C_SMBUS_BYTE, NULL)  < 0) {
> +		dev_warn(&client->dev, "PCA9846 family: Sofrware reset failed\n");
> +		return -ENODEV;
> +	}

This is extremely invasive, and not appropriate for this driver. Do you
need it? Why?

> +
> +	/*
> +	 * Get device ID
> +	 */

This could be a single line comment.

> +	device_id_raw.block[0] = 3;  //read 3 bytes
> +	if (i2c_smbus_xfer(client->adapter, DEVICE_ID_ADDRESS, client->flags,
> +			   I2C_SMBUS_READ,  client->addr << 1,
> +			   I2C_SMBUS_I2C_BLOCK_DATA, &device_id_raw)) {
> +		dev_warn(&client->dev, "PCA9846 family: Get device ID failed\n");
> +		return -ENODEV;
> +	}
> +
> +	//Device ID contains only 3 bytes

I don't much care for // comments, and the missing space after them is especially
disturbing.

> +	if (device_id_raw.block[0] != 3) {
> +		dev_warn(&client->dev, "PCA9846 family: Get device ID failed\n");
> +		return -ENODEV;
> +	}
> +
> +	//Check manufacturer ID (12 bits)
> +	manufacturer_id = ((u16) device_id_raw.block[1] << 4) | (device_id_raw.block[2] >> 4);

This is just broken, and works by chance only because NXP_ID happens to be
zero.

I get the feeling that you don't need to go digging up the manufacturer. That
way you could avoid the split of the probe for the two families. So, what do
you gain by verifying the manufacturer?

> +	if (manufacturer_id != NXP_ID) {
> +		dev_warn(&client->dev, "PCA9846 family: Manufacturer ID does not match NXP\n");
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * I2C init/probing/exit functions
>   */
> @@ -339,11 +469,29 @@ static int pca954x_probe(struct i2c_client *client,
>  	if (IS_ERR(gpio))
>  		return PTR_ERR(gpio);
>  
> -	/* Write the mux register at addr to verify
> -	 * that the mux is in fact present. This also
> -	 * initializes the mux to disconnected state.
> -	 */
> -	if (i2c_smbus_write_byte(client, 0) < 0) {
> +	switch (id->driver_data) {
> +	case pca_9540:
> +	case pca_9542:
> +	case pca_9543:
> +	case pca_9544:
> +	case pca_9545:
> +	case pca_9546:
> +	case pca_9547:
> +	case pca_9548:
> +		ret = _pca954x_probe(client, id);
> +		break;
> +	case pca_9846:
> +	case pca_9847:
> +	case pca_9848:
> +	case pca_9849:
> +		ret = _pca984x_probe(client, id);
> +		break;
> +	default: //unknown device
> +		ret = -ENODEV;
> +		break;
> +	}
> +
> +	if (ret < 0) {
>  		dev_warn(&client->dev, "probe failed\n");
>  		return -ENODEV;
>  	}
> @@ -443,6 +591,6 @@ static struct i2c_driver pca954x_driver = {
>  
>  module_i2c_driver(pca954x_driver);
>  
> -MODULE_AUTHOR("Rodolfo Giometti <giometti@linux.it>");
> +MODULE_AUTHOR("Rodolfo Giometti <giometti@linux.it>, Adrian Fiergolski <Adrian.Fiergolski@cern.ch>");
>  MODULE_DESCRIPTION("PCA954x I2C mux/switch driver");
>  MODULE_LICENSE("GPL v2");
> 

Re: [PATCH v2] i2c: Add support for NXP PCA984x family.

[Rodolfo Giometti]

On 11/12/17 15:27, Adrian Fiergolski wrote:
> This patch exetends the current i2c-mux-pca954x driver and adds suuport for
> a newer PCA984x family of the I2C switches and multiplexers from NXP.
> In probe function, the patch supports device ID function, introduced in the
> new family, and checks it against configuration provided by the
> devicetree. Moreover, it also performs software reset which is now
> available for the new devices.
> The reset of the code remains common with the legacy PCA954x devices.
> 
> Signed-off-by: Adrian Fiergolski <adrian.fiergolski@cern.ch>
> ---
>   .../devicetree/bindings/i2c/i2c-mux-pca954x.txt    |   5 +-
>   drivers/i2c/muxes/Kconfig                          |   2 +-
>   drivers/i2c/muxes/i2c-mux-pca954x.c                | 164 ++++++++++++++++++++-
>   3 files changed, 161 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> index aa097045a10e..b428bc0d81b1 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> @@ -1,10 +1,13 @@
>   * NXP PCA954x I2C bus switch
>   
> +The driver supports NXP PCA954x and PCA984x I2C mux/switch devices.
> +
>   Required Properties:
>   
>     - compatible: Must contain one of the following.
>       "nxp,pca9540", "nxp,pca9542", "nxp,pca9543", "nxp,pca9544",
> -    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548"
> +    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548",
> +    "nxp,pca9846", "nxp,pca9847", "nxp,pca9848", "nxp,pca9849"
>   
>     - reg: The I2C address of the device.
>   
> diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
> index 0f5c8fc36625..5da2e04585a9 100644
> --- a/drivers/i2c/muxes/Kconfig
> +++ b/drivers/i2c/muxes/Kconfig
> @@ -68,7 +68,7 @@ config I2C_MUX_PCA954x
>   	depends on GPIOLIB || COMPILE_TEST
>   	help
>   	  If you say yes here you get support for the Philips PCA954x
> -	  I2C mux/switch devices.
> +	  and PCA984x I2C mux/switch devices.
>   
>   	  This driver can also be built as a module.  If so, the module
>   	  will be called i2c-mux-pca954x.
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 2ca068d8b92d..88de57ae5b7a 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -1,14 +1,16 @@
>   /*
>    * I2C multiplexer
>    *
> + * Copyright (c) 2017 Adrian Fiergolski <Adrian.Fiergolski@cern.ch>
>    * Copyright (c) 2008-2009 Rodolfo Giometti <giometti@linux.it>
>    * Copyright (c) 2008-2009 Eurotech S.p.A. <info@eurotech.it>
>    *
> - * This module supports the PCA954x series of I2C multiplexer/switch chips
> - * made by Philips Semiconductors.
> + * This module supports the PCA954x and PCA954x series of I2C multiplexer/switch
> + * chips made by Philips Semiconductors.
>    * This includes the:
>    *	 PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547
>    *	 and PCA9548.
> + *       PCA9846, PCA9847, PCA9848 and PCA9849
>    *
>    * These chips are all controlled via the I2C bus itself, and all have a
>    * single 8-bit register. The upstream "parent" bus fans out to two,
> @@ -54,6 +56,12 @@
>   
>   #define PCA954X_IRQ_OFFSET 4
>   
> +//PCA984x family I2C other addresses

Use /* and */ for comments.

> +#define GENERAL_CALL  0x00
> +#define SOFTWARE_RESET 0x06
> +#define DEVICE_ID_ADDRESS 0x7C
> +#define NXP_ID 0x00
> +
>   enum pca_type {
>   	pca_9540,
>   	pca_9542,
> @@ -63,6 +71,11 @@ enum pca_type {
>   	pca_9546,
>   	pca_9547,
>   	pca_9548,
> +	pca_9846,
> +	pca_9847,
> +	pca_9848,
> +	pca_9849,
> +
>   };
>   
>   struct chip_desc {
> @@ -73,6 +86,7 @@ struct chip_desc {
>   		pca954x_ismux = 0,
>   		pca954x_isswi
>   	} muxtype;
> +	u16 deviceID; //used by PCA984x family only
>   };
>   
>   struct pca954x {
> @@ -89,6 +103,7 @@ struct pca954x {
>   
>   /* Provide specs for the PCA954x types we know about */
>   static const struct chip_desc chips[] = {
> +	//954x family
>   	[pca_9540] = {
>   		.nchans = 2,
>   		.enable = 0x4,
> @@ -129,9 +144,36 @@ static const struct chip_desc chips[] = {
>   		.nchans = 8,
>   		.muxtype = pca954x_isswi,
>   	},
> +
> +	//984x family
> +	[pca_9846] = {
> +		.nchans = 4,
> +		.muxtype = pca954x_isswi,
> +		.deviceID = 0x10B,
> +	},
> +
> +	[pca_9847] = {
> +		.nchans = 8,
> +		.muxtype = pca954x_ismux,
> +		.deviceID = 0x108,
> +	},
> +
> +	[pca_9848] = {
> +		.nchans = 8,
> +		.muxtype = pca954x_isswi,
> +		.deviceID = 0x10A,
> +	},
> +
> +	[pca_9849] = {
> +		.nchans = 4,
> +		.muxtype = pca954x_ismux,
> +		.deviceID = 0x109,
> +	},
> +
>   };
>   
>   static const struct i2c_device_id pca954x_id[] = {
> +	//954x family
>   	{ "pca9540", pca_9540 },
>   	{ "pca9542", pca_9542 },
>   	{ "pca9543", pca_9543 },
> @@ -140,12 +182,20 @@ static const struct i2c_device_id pca954x_id[] = {
>   	{ "pca9546", pca_9546 },
>   	{ "pca9547", pca_9547 },
>   	{ "pca9548", pca_9548 },
> +
> +	//984x family
> +	{ "pca9846", pca_9846 },
> +	{ "pca9847", pca_9847 },
> +	{ "pca9848", pca_9848 },
> +	{ "pca9849", pca_9849 },
> +
>   	{ }
>   };
>   MODULE_DEVICE_TABLE(i2c, pca954x_id);
>   
>   #ifdef CONFIG_OF
>   static const struct of_device_id pca954x_of_match[] = {
> +	//954x family
>   	{ .compatible = "nxp,pca9540", .data = &chips[pca_9540] },
>   	{ .compatible = "nxp,pca9542", .data = &chips[pca_9542] },
>   	{ .compatible = "nxp,pca9543", .data = &chips[pca_9543] },
> @@ -154,6 +204,14 @@ static const struct of_device_id pca954x_of_match[] = {
>   	{ .compatible = "nxp,pca9546", .data = &chips[pca_9546] },
>   	{ .compatible = "nxp,pca9547", .data = &chips[pca_9547] },
>   	{ .compatible = "nxp,pca9548", .data = &chips[pca_9548] },
> +
> +
> +	//984x family
> +	{ .compatible = "nxp,pca9846", .data = &chips[pca_9846] },
> +	{ .compatible = "nxp,pca9847", .data = &chips[pca_9847] },
> +	{ .compatible = "nxp,pca9848", .data = &chips[pca_9848] },
> +	{ .compatible = "nxp,pca9849", .data = &chips[pca_9849] },
> +
>   	{}
>   };
>   MODULE_DEVICE_TABLE(of, pca954x_of_match);
> @@ -304,6 +362,78 @@ static void pca954x_cleanup(struct i2c_mux_core *muxc)
>   	i2c_mux_del_adapters(muxc);
>   }
>   
> +/*
> + * Part of probe function specific for pca954x family
> + */
> +inline int _pca954x_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id){
> +
> +	/* Write the mux register at addr to verify
> +	 * that the mux is in fact present. This also
> +	 * initializes the mux to disconnected state.
> +	 */
> +	if (i2c_smbus_write_byte(client, 0) < 0)
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +/*
> + * Part of probe function specific for pca984x family
> + */
> +inline int _pca984x_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id){
> +
> +	struct i2c_adapter *adap = to_i2c_adapter(client->dev.parent);
> +	union i2c_smbus_data device_id_raw;
> +	u16 manufacturer_id; //12 bits
> +
> +	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) {
> +		dev_warn(&client->dev, "PCA9846 family: I2c adapter doesn't support I2C_FUNC_SMBUS_WRITE_BYTE_DATA");
> +		return -ENODEV;
> +	}
> +
> +	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
> +		dev_warn(&client->dev, "PCA9846 family: I2c adapter doesn't support I2C_FUNC_SMBUS_READ_I2C_BLOCK");
> +		return -ENODEV;
> +	}
> +
> +	/*
> +	 * Software reset
> +	 */
> +	if (i2c_smbus_xfer(client->adapter, GENERAL_CALL, client->flags,
> +			   I2C_SMBUS_WRITE, SOFTWARE_RESET, I2C_SMBUS_BYTE, NULL)  < 0) {
> +		dev_warn(&client->dev, "PCA9846 family: Sofrware reset failed\n");
> +		return -ENODEV;
> +	}
> +
> +	/*
> +	 * Get device ID
> +	 */
> +	device_id_raw.block[0] = 3;  //read 3 bytes
> +	if (i2c_smbus_xfer(client->adapter, DEVICE_ID_ADDRESS, client->flags,
> +			   I2C_SMBUS_READ,  client->addr << 1,
> +			   I2C_SMBUS_I2C_BLOCK_DATA, &device_id_raw)) {
> +		dev_warn(&client->dev, "PCA9846 family: Get device ID failed\n");
> +		return -ENODEV;
> +	}
> +
> +	//Device ID contains only 3 bytes
> +	if (device_id_raw.block[0] != 3) {
> +		dev_warn(&client->dev, "PCA9846 family: Get device ID failed\n");
> +		return -ENODEV;
> +	}
> +
> +	//Check manufacturer ID (12 bits)
> +	manufacturer_id = ((u16) device_id_raw.block[1] << 4) | (device_id_raw.block[2] >> 4);
> +	if (manufacturer_id != NXP_ID) {
> +		dev_warn(&client->dev, "PCA9846 family: Manufacturer ID does not match NXP\n");
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
>   /*
>    * I2C init/probing/exit functions
>    */
> @@ -339,11 +469,29 @@ static int pca954x_probe(struct i2c_client *client,
>   	if (IS_ERR(gpio))
>   		return PTR_ERR(gpio);
>   
> -	/* Write the mux register at addr to verify
> -	 * that the mux is in fact present. This also
> -	 * initializes the mux to disconnected state.
> -	 */
> -	if (i2c_smbus_write_byte(client, 0) < 0) {
> +	switch (id->driver_data) {
> +	case pca_9540:
> +	case pca_9542:
> +	case pca_9543:
> +	case pca_9544:
> +	case pca_9545:
> +	case pca_9546:
> +	case pca_9547:
> +	case pca_9548:
> +		ret = _pca954x_probe(client, id);
> +		break;
> +	case pca_9846:
> +	case pca_9847:
> +	case pca_9848:
> +	case pca_9849:
> +		ret = _pca984x_probe(client, id);
> +		break;
> +	default: //unknown device
> +		ret = -ENODEV;
> +		break;
> +	}

I dislike names starting with "_"... maybe you can use pca954x_family_probe() 
and pca984x_family_probe()? :-)

> +
> +	if (ret < 0) {
>   		dev_warn(&client->dev, "probe failed\n");
>   		return -ENODEV;
>   	}
> @@ -443,6 +591,6 @@ static struct i2c_driver pca954x_driver = {
>   
>   module_i2c_driver(pca954x_driver);
>   
> -MODULE_AUTHOR("Rodolfo Giometti <giometti@linux.it>");
> +MODULE_AUTHOR("Rodolfo Giometti <giometti@linux.it>, Adrian Fiergolski <Adrian.Fiergolski@cern.ch>");

Use multiple MODULE_AUTHOR() lines.

>   MODULE_DESCRIPTION("PCA954x I2C mux/switch driver");
>   MODULE_LICENSE("GPL v2");

Ciao,

Rodolfo

-- 

HCE Engineering                      e-mail: giometti@hce-engineering.it
GNU/Linux Solutions                          giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti
Cosino Project - the quick prototyping embedded system - www.cosino.it
Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it

[PATCH v3] i2c: Add support for NXP PCA984x family.

[Adrian Fiergolski]

This patch exetends the current i2c-mux-pca954x driver and adds suuport for
a newer PCA984x family of the I2C switches and multiplexers from NXP.
In probe function, the patch supports device ID function, introduced in the
new family, and checks it against configuration provided by the
devicetree. Moreover, it also performs software reset which is now
available for the new devices.
The reset of the code remains common with the legacy PCA954x devices.

Signed-off-by: Adrian Fiergolski <adrian.fiergolski@cern.ch>
---
Apply Peter's and Rodolfo's comments.
Add missing part in pca984x_family_probe which checks deviceID.

Peter addressing your main comments:

 Quoting the PCA9848 manual: "The Software Reset Call allows all the devices in
 the I2C-bus to be reset to the power-up state value through a specific formatted
 I2C-bus command. To be performed correctly, it implies that the I2C-bus is
 functional and that there is no device hanging the bus."
 This call should reset the multiplexer and all devices which are below it in the
 I2C address space tree to the default state.

 The manufacturer ID and device ID checks in pca984x_family_probe ensures that
 we actually communicate with a device we intended to (according to the
 devicetree) and that communication with this device works properly (this device
 ID and manufacturer ID can be seen as a fixed pattern).

 .../devicetree/bindings/i2c/i2c-mux-pca954x.txt    |   5 +-
 drivers/i2c/muxes/Kconfig                          |   2 +-
 drivers/i2c/muxes/i2c-mux-pca954x.c                | 157 +++++++++++++++++++--
 3 files changed, 153 insertions(+), 11 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
index aa097045a10e..b428bc0d81b1 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
@@ -1,10 +1,13 @@
 * NXP PCA954x I2C bus switch
 
+The driver supports NXP PCA954x and PCA984x I2C mux/switch devices.
+
 Required Properties:
 
   - compatible: Must contain one of the following.
     "nxp,pca9540", "nxp,pca9542", "nxp,pca9543", "nxp,pca9544",
-    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548"
+    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548",
+    "nxp,pca9846", "nxp,pca9847", "nxp,pca9848", "nxp,pca9849"
 
   - reg: The I2C address of the device.
 
diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
index 0f5c8fc36625..5da2e04585a9 100644
--- a/drivers/i2c/muxes/Kconfig
+++ b/drivers/i2c/muxes/Kconfig
@@ -68,7 +68,7 @@ config I2C_MUX_PCA954x
 	depends on GPIOLIB || COMPILE_TEST
 	help
 	  If you say yes here you get support for the Philips PCA954x
-	  I2C mux/switch devices.
+	  and PCA984x I2C mux/switch devices.
 
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-mux-pca954x.
diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 2ca068d8b92d..6205ae52aa4d 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -1,14 +1,15 @@
 /*
  * I2C multiplexer
  *
+ * Copyright (c) 2017 Adrian Fiergolski <Adrian.Fiergolski@cern.ch>
  * Copyright (c) 2008-2009 Rodolfo Giometti <giometti@linux.it>
  * Copyright (c) 2008-2009 Eurotech S.p.A. <info@eurotech.it>
  *
- * This module supports the PCA954x series of I2C multiplexer/switch chips
- * made by Philips Semiconductors.
+ * This module supports the PCA954x and PCA954x series of I2C multiplexer/switch
+ * chips made by Philips Semiconductors.
  * This includes the:
- *	 PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547
- *	 and PCA9548.
+ *	 PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547,
+ *	 PCA9548, PCA9846, PCA9847, PCA9848 and PCA9849
  *
  * These chips are all controlled via the I2C bus itself, and all have a
  * single 8-bit register. The upstream "parent" bus fans out to two,
@@ -54,6 +55,12 @@
 
 #define PCA954X_IRQ_OFFSET 4
 
+/* PCA984x family I2C other addresses */
+#define GENERAL_CALL  0x00
+#define SOFTWARE_RESET 0x06
+#define DEVICE_ID_ADDRESS 0x7C
+#define NXP_ID 0x00
+
 enum pca_type {
 	pca_9540,
 	pca_9542,
@@ -63,6 +70,10 @@ enum pca_type {
 	pca_9546,
 	pca_9547,
 	pca_9548,
+	pca_9846,
+	pca_9847,
+	pca_9848,
+	pca_9849,
 };
 
 struct chip_desc {
@@ -73,6 +84,7 @@ struct chip_desc {
 		pca954x_ismux = 0,
 		pca954x_isswi
 	} muxtype;
+	u16 deviceID; /* used by PCA984x family only */
 };
 
 struct pca954x {
@@ -129,6 +141,29 @@ static const struct chip_desc chips[] = {
 		.nchans = 8,
 		.muxtype = pca954x_isswi,
 	},
+	[pca_9846] = {
+		.nchans = 4,
+		.muxtype = pca954x_isswi,
+		.deviceID = 0x10B,
+	},
+
+	[pca_9847] = {
+		.nchans = 8,
+		.muxtype = pca954x_ismux,
+		.deviceID = 0x108,
+	},
+
+	[pca_9848] = {
+		.nchans = 8,
+		.muxtype = pca954x_isswi,
+		.deviceID = 0x10A,
+	},
+
+	[pca_9849] = {
+		.nchans = 4,
+		.muxtype = pca954x_ismux,
+		.deviceID = 0x109,
+	},
 };
 
 static const struct i2c_device_id pca954x_id[] = {
@@ -140,6 +175,10 @@ static const struct i2c_device_id pca954x_id[] = {
 	{ "pca9546", pca_9546 },
 	{ "pca9547", pca_9547 },
 	{ "pca9548", pca_9548 },
+	{ "pca9846", pca_9846 },
+	{ "pca9847", pca_9847 },
+	{ "pca9848", pca_9848 },
+	{ "pca9849", pca_9849 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, pca954x_id);
@@ -154,6 +193,10 @@ static const struct of_device_id pca954x_of_match[] = {
 	{ .compatible = "nxp,pca9546", .data = &chips[pca_9546] },
 	{ .compatible = "nxp,pca9547", .data = &chips[pca_9547] },
 	{ .compatible = "nxp,pca9548", .data = &chips[pca_9548] },
+	{ .compatible = "nxp,pca9846", .data = &chips[pca_9846] },
+	{ .compatible = "nxp,pca9847", .data = &chips[pca_9847] },
+	{ .compatible = "nxp,pca9848", .data = &chips[pca_9848] },
+	{ .compatible = "nxp,pca9849", .data = &chips[pca_9849] },
 	{}
 };
 MODULE_DEVICE_TABLE(of, pca954x_of_match);
@@ -304,6 +347,83 @@ static void pca954x_cleanup(struct i2c_mux_core *muxc)
 	i2c_mux_del_adapters(muxc);
 }
 
+/*
+ * Part of probe function specific for pca954x family
+ */
+static int pca954x_family_probe(struct i2c_client *client,
+				const struct i2c_device_id *id)
+{
+
+	/* Write the mux register at addr to verify
+	 * that the mux is in fact present. This also
+	 * initializes the mux to disconnected state.
+	 */
+	if (i2c_smbus_write_byte(client, 0) < 0)
+		return -ENODEV;
+
+	return 0;
+}
+
+/*
+ * Part of probe function specific for pca984x family
+ */
+static int pca984x_family_probe(struct i2c_client *client,
+				const struct i2c_device_id *id)
+{
+	struct i2c_adapter *adap = to_i2c_adapter(client->dev.parent);
+	union i2c_smbus_data device_id_raw;
+	u16 manufacturer_id; /* 12 bits */
+	u16 device_id;       /* 9 bits */
+
+	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) {
+		dev_warn(&client->dev, "PCA9846 family: I2c adapter doesn't support I2C_FUNC_SMBUS_WRITE_BYTE_DATA");
+		return -ENODEV;
+	}
+
+	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
+		dev_warn(&client->dev, "PCA9846 family: I2c adapter doesn't support I2C_FUNC_SMBUS_READ_I2C_BLOCK");
+		return -ENODEV;
+	}
+
+	/* Software reset */
+	if (i2c_smbus_xfer(client->adapter, GENERAL_CALL, client->flags,
+			   I2C_SMBUS_WRITE, SOFTWARE_RESET, I2C_SMBUS_BYTE, NULL)  < 0) {
+		dev_warn(&client->dev, "PCA9846 family: Sofrware reset failed\n");
+		return -ENODEV;
+	}
+
+	/* Get device ID */
+	device_id_raw.block[0] = 3;  /* read 3 bytes */
+	if (i2c_smbus_xfer(client->adapter, DEVICE_ID_ADDRESS, client->flags,
+			   I2C_SMBUS_READ,  client->addr << 1,
+			   I2C_SMBUS_I2C_BLOCK_DATA, &device_id_raw)) {
+		dev_warn(&client->dev, "PCA9846 family: Get device ID failed\n");
+		return -ENODEV;
+	}
+
+	/* Device ID contains only 3 bytes */
+	if (device_id_raw.block[0] != 3) {
+		dev_warn(&client->dev, "PCA9846 family: Get device ID failed\n");
+		return -ENODEV;
+	}
+
+	/* Check manufacturer ID (12 bits) */
+	manufacturer_id = ((u16) device_id_raw.block[1] << 4) | (device_id_raw.block[2] >> 4);
+	if (manufacturer_id != NXP_ID) {
+		dev_warn(&client->dev, "PCA9846 family: Manufacturer ID does not match NXP\n");
+		return -ENODEV;
+	}
+
+	/* Check device ID (9 bits) */
+	device_id = ((u16) device_id_raw.block[2] << 5) | (device_id_raw.block[3] >> 3);
+	if (device_id != chips[id->driver_data].deviceID) {
+		dev_warn(&client->dev, "PCA9846 family: Device ID does not match %s\n", id->name);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
 /*
  * I2C init/probing/exit functions
  */
@@ -339,11 +459,29 @@ static int pca954x_probe(struct i2c_client *client,
 	if (IS_ERR(gpio))
 		return PTR_ERR(gpio);
 
-	/* Write the mux register at addr to verify
-	 * that the mux is in fact present. This also
-	 * initializes the mux to disconnected state.
-	 */
-	if (i2c_smbus_write_byte(client, 0) < 0) {
+	switch (id->driver_data) {
+	case pca_9540:
+	case pca_9542:
+	case pca_9543:
+	case pca_9544:
+	case pca_9545:
+	case pca_9546:
+	case pca_9547:
+	case pca_9548:
+		ret = pca954x_family_probe(client, id);
+		break;
+	case pca_9846:
+	case pca_9847:
+	case pca_9848:
+	case pca_9849:
+		ret = pca984x_family_probe(client, id);
+		break;
+	default: /* unknown device */
+		ret = -ENODEV;
+		break;
+	}
+
+	if (ret < 0) {
 		dev_warn(&client->dev, "probe failed\n");
 		return -ENODEV;
 	}
@@ -443,6 +581,7 @@ static struct i2c_driver pca954x_driver = {
 
 module_i2c_driver(pca954x_driver);
 
+MODULE_AUTHOR("Adrian Fiergolski <Adrian.Fiergolski@cern.ch>");
 MODULE_AUTHOR("Rodolfo Giometti <giometti@linux.it>");
 MODULE_DESCRIPTION("PCA954x I2C mux/switch driver");
 MODULE_LICENSE("GPL v2");
-- 
2.14.1

Re: [PATCH v3] i2c: Add support for NXP PCA984x family.

[Peter Rosin]

On 2017-12-11 17:58, Adrian Fiergolski wrote:
> This patch exetends the current i2c-mux-pca954x driver and adds suuport for

support

> a newer PCA984x family of the I2C switches and multiplexers from NXP.
> In probe function, the patch supports device ID function, introduced in the
> new family, and checks it against configuration provided by the
> devicetree. Moreover, it also performs software reset which is now
> available for the new devices.
> The reset of the code remains common with the legacy PCA954x devices.
> 
> Signed-off-by: Adrian Fiergolski <adrian.fiergolski@cern.ch>
> ---
> Apply Peter's and Rodolfo's comments.
> Add missing part in pca984x_family_probe which checks deviceID.
> 
> Peter addressing your main comments:
> 
>  Quoting the PCA9848 manual: "The Software Reset Call allows all the devices in
>  the I2C-bus to be reset to the power-up state value through a specific formatted
>  I2C-bus command. To be performed correctly, it implies that the I2C-bus is
>  functional and that there is no device hanging the bus."
>  This call should reset the multiplexer and all devices which are below it in the
>  I2C address space tree to the default state.

Yes, I read all that in the datasheet, but why do *you* need it? Because
it is wrong to do it in this driver. It is doubly wrong to do it
unconditionally. I was asking so that I could help you find a solution
for your problem, because it is simply not acceptable to add a bus-wide
reset to a random driver.

> 
>  The manufacturer ID and device ID checks in pca984x_family_probe ensures that
>  we actually communicate with a device we intended to (according to the
>  devicetree) and that communication with this device works properly (this device
>  ID and manufacturer ID can be seen as a fixed pattern).

Yes yes yes, but do you actually need it? Because it complicates the driver
for little gain (in my opinion).

>  .../devicetree/bindings/i2c/i2c-mux-pca954x.txt    |   5 +-
>  drivers/i2c/muxes/Kconfig                          |   2 +-
>  drivers/i2c/muxes/i2c-mux-pca954x.c                | 157 +++++++++++++++++++--
>  3 files changed, 153 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> index aa097045a10e..b428bc0d81b1 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> @@ -1,10 +1,13 @@
>  * NXP PCA954x I2C bus switch
>  
> +The driver supports NXP PCA954x and PCA984x I2C mux/switch devices.
> +
>  Required Properties:
>  
>    - compatible: Must contain one of the following.
>      "nxp,pca9540", "nxp,pca9542", "nxp,pca9543", "nxp,pca9544",
> -    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548"
> +    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548",
> +    "nxp,pca9846", "nxp,pca9847", "nxp,pca9848", "nxp,pca9849"
>  
>    - reg: The I2C address of the device.
>  
> diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
> index 0f5c8fc36625..5da2e04585a9 100644
> --- a/drivers/i2c/muxes/Kconfig
> +++ b/drivers/i2c/muxes/Kconfig
> @@ -68,7 +68,7 @@ config I2C_MUX_PCA954x
>  	depends on GPIOLIB || COMPILE_TEST
>  	help
>  	  If you say yes here you get support for the Philips PCA954x
> -	  I2C mux/switch devices.
> +	  and PCA984x I2C mux/switch devices.

lets update Philips to NXP while at it.

>  
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called i2c-mux-pca954x.
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 2ca068d8b92d..6205ae52aa4d 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -1,14 +1,15 @@
>  /*
>   * I2C multiplexer
>   *
> + * Copyright (c) 2017 Adrian Fiergolski <Adrian.Fiergolski@cern.ch>
>   * Copyright (c) 2008-2009 Rodolfo Giometti <giometti@linux.it>
>   * Copyright (c) 2008-2009 Eurotech S.p.A. <info@eurotech.it>
>   *
> - * This module supports the PCA954x series of I2C multiplexer/switch chips
> - * made by Philips Semiconductors.
> + * This module supports the PCA954x and PCA954x series of I2C multiplexer/switch
> + * chips made by Philips Semiconductors.

NXP.

>   * This includes the:
> - *	 PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547
> - *	 and PCA9548.
> + *	 PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547,
> + *	 PCA9548, PCA9846, PCA9847, PCA9848 and PCA9849
>   *
>   * These chips are all controlled via the I2C bus itself, and all have a
>   * single 8-bit register. The upstream "parent" bus fans out to two,
> @@ -54,6 +55,12 @@
>  
>  #define PCA954X_IRQ_OFFSET 4
>  
> +/* PCA984x family I2C other addresses */
> +#define GENERAL_CALL  0x00
> +#define SOFTWARE_RESET 0x06
> +#define DEVICE_ID_ADDRESS 0x7C
> +#define NXP_ID 0x00
> +
>  enum pca_type {
>  	pca_9540,
>  	pca_9542,
> @@ -63,6 +70,10 @@ enum pca_type {
>  	pca_9546,
>  	pca_9547,
>  	pca_9548,
> +	pca_9846,
> +	pca_9847,
> +	pca_9848,
> +	pca_9849,
>  };
>  
>  struct chip_desc {
> @@ -73,6 +84,7 @@ struct chip_desc {
>  		pca954x_ismux = 0,
>  		pca954x_isswi
>  	} muxtype;
> +	u16 deviceID; /* used by PCA984x family only */

Please name it device_id, if you insist on keeping it.

>  };
>  
>  struct pca954x {
> @@ -129,6 +141,29 @@ static const struct chip_desc chips[] = {
>  		.nchans = 8,
>  		.muxtype = pca954x_isswi,
>  	},
> +	[pca_9846] = {
> +		.nchans = 4,
> +		.muxtype = pca954x_isswi,
> +		.deviceID = 0x10B,
> +	},
> +

As requested for v2, please drop these empty lines, the blocks above the
hunk don't have them. Let's keep things consistent.

> +	[pca_9847] = {
> +		.nchans = 8,
> +		.muxtype = pca954x_ismux,
> +		.deviceID = 0x108,
> +	},
> +
> +	[pca_9848] = {
> +		.nchans = 8,
> +		.muxtype = pca954x_isswi,
> +		.deviceID = 0x10A,
> +	},
> +
> +	[pca_9849] = {
> +		.nchans = 4,
> +		.muxtype = pca954x_ismux,
> +		.deviceID = 0x109,
> +	},
>  };
>  
>  static const struct i2c_device_id pca954x_id[] = {
> @@ -140,6 +175,10 @@ static const struct i2c_device_id pca954x_id[] = {
>  	{ "pca9546", pca_9546 },
>  	{ "pca9547", pca_9547 },
>  	{ "pca9548", pca_9548 },
> +	{ "pca9846", pca_9846 },
> +	{ "pca9847", pca_9847 },
> +	{ "pca9848", pca_9848 },
> +	{ "pca9849", pca_9849 },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, pca954x_id);
> @@ -154,6 +193,10 @@ static const struct of_device_id pca954x_of_match[] = {
>  	{ .compatible = "nxp,pca9546", .data = &chips[pca_9546] },
>  	{ .compatible = "nxp,pca9547", .data = &chips[pca_9547] },
>  	{ .compatible = "nxp,pca9548", .data = &chips[pca_9548] },
> +	{ .compatible = "nxp,pca9846", .data = &chips[pca_9846] },
> +	{ .compatible = "nxp,pca9847", .data = &chips[pca_9847] },
> +	{ .compatible = "nxp,pca9848", .data = &chips[pca_9848] },
> +	{ .compatible = "nxp,pca9849", .data = &chips[pca_9849] },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, pca954x_of_match);
> @@ -304,6 +347,83 @@ static void pca954x_cleanup(struct i2c_mux_core *muxc)
>  	i2c_mux_del_adapters(muxc);
>  }
>  
> +/*
> + * Part of probe function specific for pca954x family
> + */
> +static int pca954x_family_probe(struct i2c_client *client,
> +				const struct i2c_device_id *id)
> +{
> +

Drop the blank line.

> +	/* Write the mux register at addr to verify
> +	 * that the mux is in fact present. This also
> +	 * initializes the mux to disconnected state.
> +	 */

/*
 * Use this comment style for all multiline comments that you write
 * in the kernel source.
 */

I realize that you just copied this one, but let's clean it up while
at it.

> +	if (i2c_smbus_write_byte(client, 0) < 0)
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +/*
> + * Part of probe function specific for pca984x family
> + */
> +static int pca984x_family_probe(struct i2c_client *client,
> +				const struct i2c_device_id *id)

Ok, you seem determined to keep the device/manufacturer check. In
that case, it is odd to have functions that does not follow the
driver prefix rule.

So, please rename this one to e.g. pca954x_pca984x_probe and the
one above to pca954x_default_probe. Or something such.

> +{
> +	struct i2c_adapter *adap = to_i2c_adapter(client->dev.parent);
> +	union i2c_smbus_data device_id_raw;
> +	u16 manufacturer_id; /* 12 bits */
> +	u16 device_id;       /* 9 bits */
> +
> +	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) {
> +		dev_warn(&client->dev, "PCA9846 family: I2c adapter doesn't support I2C_FUNC_SMBUS_WRITE_BYTE_DATA");
> +		return -ENODEV;
> +	}
> +
> +	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
> +		dev_warn(&client->dev, "PCA9846 family: I2c adapter doesn't support I2C_FUNC_SMBUS_READ_I2C_BLOCK");
> +		return -ENODEV;
> +	}
> +
> +	/* Software reset */
> +	if (i2c_smbus_xfer(client->adapter, GENERAL_CALL, client->flags,
> +			   I2C_SMBUS_WRITE, SOFTWARE_RESET, I2C_SMBUS_BYTE, NULL)  < 0) {
> +		dev_warn(&client->dev, "PCA9846 family: Sofrware reset failed\n");
> +		return -ENODEV;
> +	}

As stated above, this just has to go.

> +
> +	/* Get device ID */
> +	device_id_raw.block[0] = 3;  /* read 3 bytes */
> +	if (i2c_smbus_xfer(client->adapter, DEVICE_ID_ADDRESS, client->flags,
> +			   I2C_SMBUS_READ,  client->addr << 1,
> +			   I2C_SMBUS_I2C_BLOCK_DATA, &device_id_raw)) {

Whooa, I managed to miss this last time... Why are you not using the
i2c_smbus_read_i2c_block_data helper? (and even if you have some good
reason to open-code this, you seem to have some seriously confused
arguments, so if those are indeed correct you need to document what
the hell is going on)

> +		dev_warn(&client->dev, "PCA9846 family: Get device ID failed\n");
> +		return -ENODEV;
> +	}
> +
> +	/* Device ID contains only 3 bytes */
> +	if (device_id_raw.block[0] != 3) {
> +		dev_warn(&client->dev, "PCA9846 family: Get device ID failed\n");
> +		return -ENODEV;
> +	}
> +
> +	/* Check manufacturer ID (12 bits) */
> +	manufacturer_id = ((u16) device_id_raw.block[1] << 4) | (device_id_raw.block[2] >> 4);

This assignment is still broken...

> +	if (manufacturer_id != NXP_ID) {
> +		dev_warn(&client->dev, "PCA9846 family: Manufacturer ID does not match NXP\n");
> +		return -ENODEV;
> +	}
> +
> +	/* Check device ID (9 bits) */
> +	device_id = ((u16) device_id_raw.block[2] << 5) | (device_id_raw.block[3] >> 3);

...and this is also broken.

> +	if (device_id != chips[id->driver_data].deviceID) {
> +		dev_warn(&client->dev, "PCA9846 family: Device ID does not match %s\n", id->name);
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * I2C init/probing/exit functions
>   */
> @@ -339,11 +459,29 @@ static int pca954x_probe(struct i2c_client *client,
>  	if (IS_ERR(gpio))
>  		return PTR_ERR(gpio);
>  
> -	/* Write the mux register at addr to verify
> -	 * that the mux is in fact present. This also
> -	 * initializes the mux to disconnected state.
> -	 */
> -	if (i2c_smbus_write_byte(client, 0) < 0) {
> +	switch (id->driver_data) {
> +	case pca_9540:
> +	case pca_9542:
> +	case pca_9543:
> +	case pca_9544:
> +	case pca_9545:
> +	case pca_9546:
> +	case pca_9547:
> +	case pca_9548:

Please drop these and do the 954x probe in default.

> +		ret = pca954x_family_probe(client, id);
> +		break;
> +	case pca_9846:
> +	case pca_9847:
> +	case pca_9848:
> +	case pca_9849:
> +		ret = pca984x_family_probe(client, id);
> +		break;
> +	default: /* unknown device */
> +		ret = -ENODEV;
> +		break;
> +	}
> +
> +	if (ret < 0) {
>  		dev_warn(&client->dev, "probe failed\n");
>  		return -ENODEV;

return ret;

Cheers,
Peter

>  	}
> @@ -443,6 +581,7 @@ static struct i2c_driver pca954x_driver = {
>  
>  module_i2c_driver(pca954x_driver);
>  
> +MODULE_AUTHOR("Adrian Fiergolski <Adrian.Fiergolski@cern.ch>");
>  MODULE_AUTHOR("Rodolfo Giometti <giometti@linux.it>");
>  MODULE_DESCRIPTION("PCA954x I2C mux/switch driver");
>  MODULE_LICENSE("GPL v2");
> 

Re: [PATCH v3] i2c: Add support for NXP PCA984x family.

[Adrian Fiergolski]

On 11.12.2017 at 20:14, Peter Rosin wrote:
>
>> a newer PCA984x family of the I2C switches and multiplexers from NXP.
>> In probe function, the patch supports device ID function, introduced in the
>> new family, and checks it against configuration provided by the
>> devicetree. Moreover, it also performs software reset which is now
>> available for the new devices.
>> The reset of the code remains common with the legacy PCA954x devices.
>>
>> Signed-off-by: Adrian Fiergolski <adrian.fiergolski@cern.ch>
>> ---
>> Apply Peter's and Rodolfo's comments.
>> Add missing part in pca984x_family_probe which checks deviceID.
>>
>> Peter addressing your main comments:
>>
>>  Quoting the PCA9848 manual: "The Software Reset Call allows all the devices in
>>  the I2C-bus to be reset to the power-up state value through a specific formatted
>>  I2C-bus command. To be performed correctly, it implies that the I2C-bus is
>>  functional and that there is no device hanging the bus."
>>  This call should reset the multiplexer and all devices which are below it in the
>>  I2C address space tree to the default state.
> Yes, I read all that in the datasheet, but why do *you* need it? Because
> it is wrong to do it in this driver. It is doubly wrong to do it
> unconditionally. I was asking so that I could help you find a solution
> for your problem, because it is simply not acceptable to add a bus-wide
> reset to a random driver.
In a normal run scenario, I don't need it. I can imagine it may be a
problem, if the module
is suddenly unloaded and loaded: the mux remains set to some not default
bus and
the module is not aware of it. However, I agree then the bus could be
reset somewhere
at higher level.
>>  The manufacturer ID and device ID checks in pca984x_family_probe ensures that
>>  we actually communicate with a device we intended to (according to the
>>  devicetree) and that communication with this device works properly (this device
>>  ID and manufacturer ID can be seen as a fixed pattern).
> Yes yes yes, but do you actually need it? Because it complicates the driver
> for little gain (in my opinion).
Well, yes... I want to be sure that the driver is actually able to serve
the device which is going
to be assigned to it. The best, of course, would be if I2C could
enumerate itself (like PCIe), but
as it's not a case and we need to rely on devicetree, I understand the
driver should do "maximum"
to cross-check the static configuration with the actual hardware.
Moreover, in case of a configuration problem, one will be able to trace it
immediately.
By default, the driver performs a dummy access anyway. Since new chips
come with those ID
registers, I think it's reasonable to do something useful.

>>  +
>> +	/* Get device ID */
>> +	device_id_raw.block[0] = 3;  /* read 3 bytes */
>> +	if (i2c_smbus_xfer(client->adapter, DEVICE_ID_ADDRESS, client->flags,
>> +			   I2C_SMBUS_READ,  client->addr << 1,
>> +			   I2C_SMBUS_I2C_BLOCK_DATA, &device_id_raw)) {
>>
> Whooa, I managed to miss this last time... Why are you not using the
> i2c_smbus_read_i2c_block_data helper? (and even if you have some good
> reason to open-code this, you seem to have some seriously confused
> arguments, so if those are indeed correct you need to document what
> the hell is going on)
>
The deviceID access is described in paragraph 6.2.2 of the manual
(https://www.nxp.com/docs/en/data-sheet/PCA9846.pdf).
The fixed DEVICE_ID_ADDRESS (0x7C) is used as address. The
i2c_smbus_read_i2c_block_data function uses
address of a client (client->addr).

Cheers,
Adrian

Re: [PATCH v3] i2c: Add support for NXP PCA984x family.

[Peter Rosin]

On 2017-12-12 13:06, Adrian Fiergolski wrote:
> On 11.12.2017 at 20:14, Peter Rosin wrote:
>>
>>> a newer PCA984x family of the I2C switches and multiplexers from NXP.
>>> In probe function, the patch supports device ID function, introduced in the
>>> new family, and checks it against configuration provided by the
>>> devicetree. Moreover, it also performs software reset which is now
>>> available for the new devices.
>>> The reset of the code remains common with the legacy PCA954x devices.
>>>
>>> Signed-off-by: Adrian Fiergolski <adrian.fiergolski@cern.ch>
>>> ---
>>> Apply Peter's and Rodolfo's comments.
>>> Add missing part in pca984x_family_probe which checks deviceID.
>>>
>>> Peter addressing your main comments:
>>>
>>>  Quoting the PCA9848 manual: "The Software Reset Call allows all the devices in
>>>  the I2C-bus to be reset to the power-up state value through a specific formatted
>>>  I2C-bus command. To be performed correctly, it implies that the I2C-bus is
>>>  functional and that there is no device hanging the bus."
>>>  This call should reset the multiplexer and all devices which are below it in the
>>>  I2C address space tree to the default state.
>> Yes, I read all that in the datasheet, but why do *you* need it? Because
>> it is wrong to do it in this driver. It is doubly wrong to do it
>> unconditionally. I was asking so that I could help you find a solution
>> for your problem, because it is simply not acceptable to add a bus-wide
>> reset to a random driver.
> In a normal run scenario, I don't need it. I can imagine it may be a
> problem, if the module
> is suddenly unloaded and loaded: the mux remains set to some not default
> bus and
> the module is not aware of it. However, I agree then the bus could be
> reset somewhere
> at higher level.

Think about what happens if you have more than one chip on some i2c bus
that uses this reset mechanism. First you instantiate one of the drivers
and both chips are reset. Then you carry on with careful configuration
of that first chip. Then you instantiate the driver for the other chip
and all you careful configuration of the first chip is lost as it is
reset a second time. So, you just can't reset everything on the bus in
any random driver, that has to be done on some other level.

>>>  The manufacturer ID and device ID checks in pca984x_family_probe ensures that
>>>  we actually communicate with a device we intended to (according to the
>>>  devicetree) and that communication with this device works properly (this device
>>>  ID and manufacturer ID can be seen as a fixed pattern).
>> Yes yes yes, but do you actually need it? Because it complicates the driver
>> for little gain (in my opinion).
> Well, yes... I want to be sure that the driver is actually able to serve
> the device which is going
> to be assigned to it. The best, of course, would be if I2C could
> enumerate itself (like PCIe), but
> as it's not a case and we need to rely on devicetree, I understand the
> driver should do "maximum"
> to cross-check the static configuration with the actual hardware.
> Moreover, in case of a configuration problem, one will be able to trace it
> immediately.
> By default, the driver performs a dummy access anyway. Since new chips
> come with those ID
> registers, I think it's reasonable to do something useful.

Sure, there are benefits, but is it worth it?

>>>  +
>>> +	/* Get device ID */
>>> +	device_id_raw.block[0] = 3;  /* read 3 bytes */
>>> +	if (i2c_smbus_xfer(client->adapter, DEVICE_ID_ADDRESS, client->flags,
>>> +			   I2C_SMBUS_READ,  client->addr << 1,
>>> +			   I2C_SMBUS_I2C_BLOCK_DATA, &device_id_raw)) {
>>>
>> Whooa, I managed to miss this last time... Why are you not using the
>> i2c_smbus_read_i2c_block_data helper? (and even if you have some good
>> reason to open-code this, you seem to have some seriously confused
>> arguments, so if those are indeed correct you need to document what
>> the hell is going on)
>>
> The deviceID access is described in paragraph 6.2.2 of the manual
> (https://www.nxp.com/docs/en/data-sheet/PCA9846.pdf).
> The fixed DEVICE_ID_ADDRESS (0x7C) is used as address. The
> i2c_smbus_read_i2c_block_data function uses
> address of a client (client->addr).

Ahh, ok, I see what's going on. I think this should be a helper
function in the i2c core that returns the manufacturer, the device id
and the revision fields, thus eliminating the need to do bit-fiddling
in every driver that needs this info.

And there should be a common "database" of manufacturers. It would be
a disservice to everybody to have that info scattered all over the
place.

Cheers,
Peter

Re: [PATCH v3] i2c: Add support for NXP PCA984x family.

[Adrian Fiergolski]

On 12.12.2017 at 16:25, Peter Rosin wrote:
> On 2017-12-12 13:06, Adrian Fiergolski wrote:
>> On 11.12.2017 at 20:14, Peter Rosin wrote:
>>>> a newer PCA984x family of the I2C switches and multiplexers from NXP.
>>>> In probe function, the patch supports device ID function, introduced in the
>>>> new family, and checks it against configuration provided by the
>>>> devicetree. Moreover, it also performs software reset which is now
>>>> available for the new devices.
>>>> The reset of the code remains common with the legacy PCA954x devices.
>>>>
>>>> Signed-off-by: Adrian Fiergolski <adrian.fiergolski@cern.ch>
>>>> ---
>>>> Apply Peter's and Rodolfo's comments.
>>>> Add missing part in pca984x_family_probe which checks deviceID.
>>>>
>>>> Peter addressing your main comments:
>>>>
>>>>  Quoting the PCA9848 manual: "The Software Reset Call allows all the devices in
>>>>  the I2C-bus to be reset to the power-up state value through a specific formatted
>>>>  I2C-bus command. To be performed correctly, it implies that the I2C-bus is
>>>>  functional and that there is no device hanging the bus."
>>>>  This call should reset the multiplexer and all devices which are below it in the
>>>>  I2C address space tree to the default state.
>>> Yes, I read all that in the datasheet, but why do *you* need it? Because
>>> it is wrong to do it in this driver. It is doubly wrong to do it
>>> unconditionally. I was asking so that I could help you find a solution
>>> for your problem, because it is simply not acceptable to add a bus-wide
>>> reset to a random driver.
>> In a normal run scenario, I don't need it. I can imagine it may be a
>> problem, if the module
>> is suddenly unloaded and loaded: the mux remains set to some not default
>> bus and
>> the module is not aware of it. However, I agree then the bus could be
>> reset somewhere
>> at higher level.
> Think about what happens if you have more than one chip on some i2c bus
> that uses this reset mechanism. First you instantiate one of the drivers
> and both chips are reset. Then you carry on with careful configuration
> of that first chip. Then you instantiate the driver for the other chip
> and all you careful configuration of the first chip is lost as it is
> reset a second time. So, you just can't reset everything on the bus in
> any random driver, that has to be done on some other level.
That's all true. However, we are discussing an I2C mux/switch which is a
root
of an I2C sub-tree. It is initialized first, so the SOFTWARE_RESET command
would reset only mux/switch and all its nodes. The command would be followed
by the configuration of all its nodes, which anyway can't be performed
earlier.
>
>>>>  The manufacturer ID and device ID checks in pca984x_family_probe ensures that
>>>>  we actually communicate with a device we intended to (according to the
>>>>  devicetree) and that communication with this device works properly (this device
>>>>  ID and manufacturer ID can be seen as a fixed pattern).
>>> Yes yes yes, but do you actually need it? Because it complicates the driver
>>> for little gain (in my opinion).
>> Well, yes... I want to be sure that the driver is actually able to serve
>> the device which is going
>> to be assigned to it. The best, of course, would be if I2C could
>> enumerate itself (like PCIe), but
>> as it's not a case and we need to rely on devicetree, I understand the
>> driver should do "maximum"
>> to cross-check the static configuration with the actual hardware.
>> Moreover, in case of a configuration problem, one will be able to trace it
>> immediately.
>> By default, the driver performs a dummy access anyway. Since new chips
>> come with those ID
>> registers, I think it's reasonable to do something useful.
> Sure, there are benefits, but is it worth it?
As we perform anyway a dummy access, we suffer anyway I2C bus delay
which will be, in case of PCA984x family, in the same order of magnitude
(1 vs 3 words access). On another hand, one extra call doesn't change
significantly, in my opinion, the code complexity.
>>>>  +
>>>> +	/* Get device ID */
>>>> +	device_id_raw.block[0] = 3;  /* read 3 bytes */
>>>> +	if (i2c_smbus_xfer(client->adapter, DEVICE_ID_ADDRESS, client->flags,
>>>> +			   I2C_SMBUS_READ,  client->addr << 1,
>>>> +			   I2C_SMBUS_I2C_BLOCK_DATA, &device_id_raw)) {
>>>>
>>> Whooa, I managed to miss this last time... Why are you not using the
>>> i2c_smbus_read_i2c_block_data helper? (and even if you have some good
>>> reason to open-code this, you seem to have some seriously confused
>>> arguments, so if those are indeed correct you need to document what
>>> the hell is going on)
>>>
>> The deviceID access is described in paragraph 6.2.2 of the manual
>> (https://www.nxp.com/docs/en/data-sheet/PCA9846.pdf).
>> The fixed DEVICE_ID_ADDRESS (0x7C) is used as address. The
>> i2c_smbus_read_i2c_block_data function uses
>> address of a client (client->addr).
> Ahh, ok, I see what's going on. I think this should be a helper
> function in the i2c core that returns the manufacturer, the device id
> and the revision fields, thus eliminating the need to do bit-fiddling
> in every driver that needs this info.
>
> And there should be a common "database" of manufacturers. It would be
> a disservice to everybody to have that info scattered all over the
> place.
So how would we like to proceed? We keep for a time being
an i2c_smbus_xfer call and in parallel start implementation of
a new helper function in I2C core which would substitute this
call in the future?

Cheers,
Adrian

Re: [PATCH v3] i2c: Add support for NXP PCA984x family.

[Peter Rosin]

[Adding Wolfram]

On 2017-12-12 18:14, Adrian Fiergolski wrote:
> On 12.12.2017 at 16:25, Peter Rosin wrote:
>> On 2017-12-12 13:06, Adrian Fiergolski wrote:
>>> In a normal run scenario, I don't need it. I can imagine it may be a
>>> problem, if the module
>>> is suddenly unloaded and loaded: the mux remains set to some not default
>>> bus and
>>> the module is not aware of it. However, I agree then the bus could be
>>> reset somewhere
>>> at higher level.
>> Think about what happens if you have more than one chip on some i2c bus
>> that uses this reset mechanism. First you instantiate one of the drivers
>> and both chips are reset. Then you carry on with careful configuration
>> of that first chip. Then you instantiate the driver for the other chip
>> and all you careful configuration of the first chip is lost as it is
>> reset a second time. So, you just can't reset everything on the bus in
>> any random driver, that has to be done on some other level.
> That's all true. However, we are discussing an I2C mux/switch which is a
> root
> of an I2C sub-tree. It is initialized first, so the SOFTWARE_RESET command
> would reset only mux/switch and all its nodes. The command would be followed
> by the configuration of all its nodes, which anyway can't be performed
> earlier.

Have a look at Documentation/i2c/i2c-topology and think again.

>>> Well, yes... I want to be sure that the driver is actually able to serve
>>> the device which is going
>>> to be assigned to it. The best, of course, would be if I2C could
>>> enumerate itself (like PCIe), but
>>> as it's not a case and we need to rely on devicetree, I understand the
>>> driver should do "maximum"
>>> to cross-check the static configuration with the actual hardware.
>>> Moreover, in case of a configuration problem, one will be able to trace it
>>> immediately.
>>> By default, the driver performs a dummy access anyway. Since new chips
>>> come with those ID
>>> registers, I think it's reasonable to do something useful.
>> Sure, there are benefits, but is it worth it?
> As we perform anyway a dummy access, we suffer anyway I2C bus delay
> which will be, in case of PCA984x family, in the same order of magnitude
> (1 vs 3 words access). On another hand, one extra call doesn't change
> significantly, in my opinion, the code complexity.

The cost I'm concerned about isn't the time it takes to probe, the
concern is code. Given that I don't believe the code to check the
manufacturer/device using the special 0x7c address belongs in this
file at all, it seems like the best plan is to...

>>> The deviceID access is described in paragraph 6.2.2 of the manual
>>> (https://www.nxp.com/docs/en/data-sheet/PCA9846.pdf).
>>> The fixed DEVICE_ID_ADDRESS (0x7C) is used as address. The
>>> i2c_smbus_read_i2c_block_data function uses
>>> address of a client (client->addr).
>> Ahh, ok, I see what's going on. I think this should be a helper
>> function in the i2c core that returns the manufacturer, the device id
>> and the revision fields, thus eliminating the need to do bit-fiddling
>> in every driver that needs this info.
>>
>> And there should be a common "database" of manufacturers. It would be
>> a disservice to everybody to have that info scattered all over the
>> place.
> So how would we like to proceed? We keep for a time being
> an i2c_smbus_xfer call and in parallel start implementation of
> a new helper function in I2C core which would substitute this
> call in the future?

...simply skip it until someone has implemented support for it in the
I2C core. You could be the one to do that. Or not, if you don't feel
that you want to.

Wolfram, do you have an opinion on the device id check using the
0x7c address?

Cheers,
Peter

Re: [PATCH v3] i2c: Add support for NXP PCA984x family.

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 297 bytes --]


> Wolfram, do you have an opinion on the device id check using the
> 0x7c address?

I always thought there should be some core support for that feature, but
IIRC I never knew of any device which had this implemented. But since it
is described in the I2C Specification, it should be in core IMO.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3] i2c: Add support for NXP PCA984x family.

[Adrian Fiergolski]

On 12.12.2017 at 23:05, Wolfram Sang wrote:
>
>> Wolfram, do you have an opinion on the device id check using the
>> 0x7c address?
> I always thought there should be some core support for that feature, but
> IIRC I never knew of any device which had this implemented. But since it
> is described in the I2C Specification, it should be in core IMO.
>
I think one could simply use the code from my patch v3. However,
I haven't found any example of such dedicated function (i.e GENERAL_CALL)
in the core.
Do you have in mind sth like i2c_smbus_write_i2c_device_device_id function ?

Cheers,
Adrian

Re: [PATCH v3] i2c: Add support for NXP PCA984x family.

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 265 bytes --]


> Do you have in mind sth like i2c_smbus_write_i2c_device_device_id function ?

SMBus? It's I2C. And write? I'd call it:

u32 i2c_get_device_id(struct client* client)

Instead of returning an u32, we could return a struct where the bits are
already decoded, too.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3] i2c: Add support for NXP PCA984x family.

[Adrian Fiergolski]

On 12.12.2017 at 20:03, Peter Rosin wrote:
> [Adding Wolfram]
>
> On 2017-12-12 18:14, Adrian Fiergolski wrote:
>> On 12.12.2017 at 16:25, Peter Rosin wrote:
>>> On 2017-12-12 13:06, Adrian Fiergolski wrote:
>>>> In a normal run scenario, I don't need it. I can imagine it may be a
>>>> problem, if the module
>>>> is suddenly unloaded and loaded: the mux remains set to some not default
>>>> bus and
>>>> the module is not aware of it. However, I agree then the bus could be
>>>> reset somewhere
>>>> at higher level.
>>> Think about what happens if you have more than one chip on some i2c bus
>>> that uses this reset mechanism. First you instantiate one of the drivers
>>> and both chips are reset. Then you carry on with careful configuration
>>> of that first chip. Then you instantiate the driver for the other chip
>>> and all you careful configuration of the first chip is lost as it is
>>> reset a second time. So, you just can't reset everything on the bus in
>>> any random driver, that has to be done on some other level.
>> That's all true. However, we are discussing an I2C mux/switch which is a
>> root
>> of an I2C sub-tree. It is initialized first, so the SOFTWARE_RESET command
>> would reset only mux/switch and all its nodes. The command would be followed
>> by the configuration of all its nodes, which anyway can't be performed
>> earlier.
> Have a look at Documentation/i2c/i2c-topology and think again.
And... ?

Cheers,
Adrian

Re: [PATCH v3] i2c: Add support for NXP PCA984x family.

[Peter Rosin]

On 2017-12-13 09:47, Adrian Fiergolski wrote:
> On 12.12.2017 at 20:03, Peter Rosin wrote:
>> [Adding Wolfram]
>>
>> On 2017-12-12 18:14, Adrian Fiergolski wrote:
>>> On 12.12.2017 at 16:25, Peter Rosin wrote:
>>>> On 2017-12-12 13:06, Adrian Fiergolski wrote:
>>>>> In a normal run scenario, I don't need it. I can imagine it may be a
>>>>> problem, if the module
>>>>> is suddenly unloaded and loaded: the mux remains set to some not default
>>>>> bus and
>>>>> the module is not aware of it. However, I agree then the bus could be
>>>>> reset somewhere
>>>>> at higher level.
>>>> Think about what happens if you have more than one chip on some i2c bus
>>>> that uses this reset mechanism. First you instantiate one of the drivers
>>>> and both chips are reset. Then you carry on with careful configuration
>>>> of that first chip. Then you instantiate the driver for the other chip
>>>> and all you careful configuration of the first chip is lost as it is
>>>> reset a second time. So, you just can't reset everything on the bus in
>>>> any random driver, that has to be done on some other level.
>>> That's all true. However, we are discussing an I2C mux/switch which is a
>>> root
>>> of an I2C sub-tree. It is initialized first, so the SOFTWARE_RESET command
>>> would reset only mux/switch and all its nodes. The command would be followed
>>> by the configuration of all its nodes, which anyway can't be performed
>>> earlier.
>> Have a look at Documentation/i2c/i2c-topology and think again.
> And... ?

If you have a topology like this (use a fixed-width font):

          .-- deviceA
          |
i2c-root--+          deviceB
          |         /
          '--pca984x
                    \
                     deviceC

and linux for some reason starts with instantiating the driver for deviceA,
and after that instantiates the pca984x driver, then you obviously have a
problem if deviceA reacts to a general call to reset done by the probe in
the pca984x driver.

Or if you have this:

                            deviceA
                           /
                    pca984x
                   /       \
i2c-root----pca984x         deviceB
                   \
                    deviceC

then the driver for the first level pca984x device is instantiated first,
but the general call to reset when the second level pca984x device is
instantiated will trash the already configured first level pca984x
device.

Cheers,
Peter

Re: [PATCH v3] i2c: Add support for NXP PCA984x family.

[Adrian Fiergolski]

On 13.12.2017 at 10:39, Peter Rosin wrote:
> On 2017-12-13 09:47, Adrian Fiergolski wrote:
>> On 12.12.2017 at 20:03, Peter Rosin wrote:
>>> [Adding Wolfram]
>>>
>>> On 2017-12-12 18:14, Adrian Fiergolski wrote:
>>>> On 12.12.2017 at 16:25, Peter Rosin wrote:
>>>>> On 2017-12-12 13:06, Adrian Fiergolski wrote:
>>>>>> In a normal run scenario, I don't need it. I can imagine it may be a
>>>>>> problem, if the module
>>>>>> is suddenly unloaded and loaded: the mux remains set to some not default
>>>>>> bus and
>>>>>> the module is not aware of it. However, I agree then the bus could be
>>>>>> reset somewhere
>>>>>> at higher level.
>>>>> Think about what happens if you have more than one chip on some i2c bus
>>>>> that uses this reset mechanism. First you instantiate one of the drivers
>>>>> and both chips are reset. Then you carry on with careful configuration
>>>>> of that first chip. Then you instantiate the driver for the other chip
>>>>> and all you careful configuration of the first chip is lost as it is
>>>>> reset a second time. So, you just can't reset everything on the bus in
>>>>> any random driver, that has to be done on some other level.
>>>> That's all true. However, we are discussing an I2C mux/switch which is a
>>>> root
>>>> of an I2C sub-tree. It is initialized first, so the SOFTWARE_RESET command
>>>> would reset only mux/switch and all its nodes. The command would be followed
>>>> by the configuration of all its nodes, which anyway can't be performed
>>>> earlier.
>>> Have a look at Documentation/i2c/i2c-topology and think again.
>> And... ?
> If you have a topology like this (use a fixed-width font):
>
>           .-- deviceA
>           |
> i2c-root--+          deviceB
>           |         /
>           '--pca984x
>                     \
>                      deviceC
>
> and linux for some reason starts with instantiating the driver for deviceA,
> and after that instantiates the pca984x driver, then you obviously have a
> problem if deviceA reacts to a general call to reset done by the probe in
> the pca984x driver.
>
> Or if you have this:
>
>                             deviceA
>                            /
>                     pca984x
>                    /       \
> i2c-root----pca984x         deviceB
>                    \
>                     deviceC
>
> then the driver for the first level pca984x device is instantiated first,
> but the general call to reset when the second level pca984x device is
> instantiated will trash the already configured first level pca984x
> device.
>
You are right. I checked addressing in this RESET call again. For some
reasons,
I was sure that this GENERAL_CALL is performed on the I2C buses mastered by
the pca984x device. Of course, this call is performed on its slave
interface. It
couldn't be different.

Thanks,
Adrian

[PATCH v4] i2c: Add support for NXP PCA984x family.

[Adrian Fiergolski]

This patch exetends the current i2c-mux-pca954x driver and adds support for
a newer PCA984x family of the I2C switches and multiplexers from NXP.

Signed-off-by: Adrian Fiergolski <adrian.fiergolski@cern.ch>
---
As suggested by Peter, for a moment the device_id checks have been removed
and need to wait for a support in the I2C core.
 .../devicetree/bindings/i2c/i2c-mux-pca954x.txt    |  5 ++-
 drivers/i2c/muxes/Kconfig                          |  6 ++--
 drivers/i2c/muxes/i2c-mux-pca954x.c                | 41 +++++++++++++++++++---
 3 files changed, 43 insertions(+), 9 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
index aa097045a10e..b428bc0d81b1 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
@@ -1,10 +1,13 @@
 * NXP PCA954x I2C bus switch
 
+The driver supports NXP PCA954x and PCA984x I2C mux/switch devices.
+
 Required Properties:
 
   - compatible: Must contain one of the following.
     "nxp,pca9540", "nxp,pca9542", "nxp,pca9543", "nxp,pca9544",
-    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548"
+    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548",
+    "nxp,pca9846", "nxp,pca9847", "nxp,pca9848", "nxp,pca9849"
 
   - reg: The I2C address of the device.
 
diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
index 0f5c8fc36625..23cc41866a91 100644
--- a/drivers/i2c/muxes/Kconfig
+++ b/drivers/i2c/muxes/Kconfig
@@ -64,11 +64,11 @@ config I2C_MUX_PCA9541
 	  will be called i2c-mux-pca9541.
 
 config I2C_MUX_PCA954x
-	tristate "Philips PCA954x I2C Mux/switches"
+	tristate "NXP PCA954x I2C Mux/switches"
 	depends on GPIOLIB || COMPILE_TEST
 	help
-	  If you say yes here you get support for the Philips PCA954x
-	  I2C mux/switch devices.
+	  If you say yes here you get support for the NXP PCA954x
+	  and PCA984x I2C mux/switch devices.
 
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-mux-pca954x.
diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 2ca068d8b92d..b4a41d013538 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -1,14 +1,15 @@
 /*
  * I2C multiplexer
  *
+ * Copyright (c) 2017 Adrian Fiergolski <Adrian.Fiergolski@cern.ch>
  * Copyright (c) 2008-2009 Rodolfo Giometti <giometti@linux.it>
  * Copyright (c) 2008-2009 Eurotech S.p.A. <info@eurotech.it>
  *
- * This module supports the PCA954x series of I2C multiplexer/switch chips
- * made by Philips Semiconductors.
+ * This module supports the PCA954x and PCA954x series of I2C multiplexer/switch
+ * chips made by NXP Semiconductors.
  * This includes the:
- *	 PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547
- *	 and PCA9548.
+ *	 PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547,
+ *	 PCA9548, PCA9846, PCA9847, PCA9848 and PCA9849
  *
  * These chips are all controlled via the I2C bus itself, and all have a
  * single 8-bit register. The upstream "parent" bus fans out to two,
@@ -63,6 +64,10 @@ enum pca_type {
 	pca_9546,
 	pca_9547,
 	pca_9548,
+	pca_9846,
+	pca_9847,
+	pca_9848,
+	pca_9849,
 };
 
 struct chip_desc {
@@ -129,6 +134,22 @@ static const struct chip_desc chips[] = {
 		.nchans = 8,
 		.muxtype = pca954x_isswi,
 	},
+	[pca_9846] = {
+		.nchans = 4,
+		.muxtype = pca954x_isswi,
+	},
+	[pca_9847] = {
+		.nchans = 8,
+		.muxtype = pca954x_ismux,
+	},
+	[pca_9848] = {
+		.nchans = 8,
+		.muxtype = pca954x_isswi,
+	},
+	[pca_9849] = {
+		.nchans = 4,
+		.muxtype = pca954x_ismux,
+	},
 };
 
 static const struct i2c_device_id pca954x_id[] = {
@@ -140,6 +161,10 @@ static const struct i2c_device_id pca954x_id[] = {
 	{ "pca9546", pca_9546 },
 	{ "pca9547", pca_9547 },
 	{ "pca9548", pca_9548 },
+	{ "pca9846", pca_9846 },
+	{ "pca9847", pca_9847 },
+	{ "pca9848", pca_9848 },
+	{ "pca9849", pca_9849 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, pca954x_id);
@@ -154,6 +179,10 @@ static const struct of_device_id pca954x_of_match[] = {
 	{ .compatible = "nxp,pca9546", .data = &chips[pca_9546] },
 	{ .compatible = "nxp,pca9547", .data = &chips[pca_9547] },
 	{ .compatible = "nxp,pca9548", .data = &chips[pca_9548] },
+	{ .compatible = "nxp,pca9846", .data = &chips[pca_9846] },
+	{ .compatible = "nxp,pca9847", .data = &chips[pca_9847] },
+	{ .compatible = "nxp,pca9848", .data = &chips[pca_9848] },
+	{ .compatible = "nxp,pca9849", .data = &chips[pca_9849] },
 	{}
 };
 MODULE_DEVICE_TABLE(of, pca954x_of_match);
@@ -339,7 +368,8 @@ static int pca954x_probe(struct i2c_client *client,
 	if (IS_ERR(gpio))
 		return PTR_ERR(gpio);
 
-	/* Write the mux register at addr to verify
+	/*
+	 * Write the mux register at addr to verify
 	 * that the mux is in fact present. This also
 	 * initializes the mux to disconnected state.
 	 */
@@ -443,6 +473,7 @@ static struct i2c_driver pca954x_driver = {
 
 module_i2c_driver(pca954x_driver);
 
+MODULE_AUTHOR("Adrian Fiergolski <Adrian.Fiergolski@cern.ch>");
 MODULE_AUTHOR("Rodolfo Giometti <giometti@linux.it>");
 MODULE_DESCRIPTION("PCA954x I2C mux/switch driver");
 MODULE_LICENSE("GPL v2");
-- 
2.14.1

Re: [PATCH v4] i2c: Add support for NXP PCA984x family.

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 211 bytes --]


> +MODULE_AUTHOR("Adrian Fiergolski <Adrian.Fiergolski@cern.ch>");

Authorship for this module because of adding IDs? I'll leave the final
decision to Rodolfo and Peter, but I'd think you are not there yet ;)


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4] i2c: Add support for NXP PCA984x family.

[Rodolfo Giometti]

On 13/12/17 17:56, Wolfram Sang wrote:
> 
>> +MODULE_AUTHOR("Adrian Fiergolski <Adrian.Fiergolski@cern.ch>");
> 
> Authorship for this module because of adding IDs? I'll leave the final
> decision to Rodolfo and Peter, but I'd think you are not there yet ;)

In this case I agree with Wolfram.

Ciao,

Rodolfo

-- 

HCE Engineering                      e-mail: giometti@hce-engineering.it
GNU/Linux Solutions                          giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti
Cosino Project - the quick prototyping embedded system - www.cosino.it
Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it

Re: [PATCH v4] i2c: Add support for NXP PCA984x family.

[Peter Rosin]

On 2017-12-13 17:12, Adrian Fiergolski wrote:
> This patch exetends the current i2c-mux-pca954x driver and adds support for

extends

> a newer PCA984x family of the I2C switches and multiplexers from NXP.
> 
> Signed-off-by: Adrian Fiergolski <adrian.fiergolski@cern.ch>
> ---
> As suggested by Peter, for a moment the device_id checks have been removed
> and need to wait for a support in the I2C core.
>  .../devicetree/bindings/i2c/i2c-mux-pca954x.txt    |  5 ++-
>  drivers/i2c/muxes/Kconfig                          |  6 ++--
>  drivers/i2c/muxes/i2c-mux-pca954x.c                | 41 +++++++++++++++++++---
>  3 files changed, 43 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> index aa097045a10e..b428bc0d81b1 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> @@ -1,10 +1,13 @@
>  * NXP PCA954x I2C bus switch
>  
> +The driver supports NXP PCA954x and PCA984x I2C mux/switch devices.
> +
>  Required Properties:
>  
>    - compatible: Must contain one of the following.
>      "nxp,pca9540", "nxp,pca9542", "nxp,pca9543", "nxp,pca9544",
> -    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548"
> +    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548",
> +    "nxp,pca9846", "nxp,pca9847", "nxp,pca9848", "nxp,pca9849"
>  
>    - reg: The I2C address of the device.
>  
> diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
> index 0f5c8fc36625..23cc41866a91 100644
> --- a/drivers/i2c/muxes/Kconfig
> +++ b/drivers/i2c/muxes/Kconfig
> @@ -64,11 +64,11 @@ config I2C_MUX_PCA9541
>  	  will be called i2c-mux-pca9541.
>  
>  config I2C_MUX_PCA954x
> -	tristate "Philips PCA954x I2C Mux/switches"
> +	tristate "NXP PCA954x I2C Mux/switches"

I think you should perhaps mention PCA984x here in the headline.

>  	depends on GPIOLIB || COMPILE_TEST
>  	help
> -	  If you say yes here you get support for the Philips PCA954x
> -	  I2C mux/switch devices.
> +	  If you say yes here you get support for the NXP PCA954x
> +	  and PCA984x I2C mux/switch devices.
>  
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called i2c-mux-pca954x.
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 2ca068d8b92d..b4a41d013538 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -1,14 +1,15 @@
>  /*
>   * I2C multiplexer
>   *
> + * Copyright (c) 2017 Adrian Fiergolski <Adrian.Fiergolski@cern.ch>

This is a bit over the top when you only add data with questionable
copyright value.

>   * Copyright (c) 2008-2009 Rodolfo Giometti <giometti@linux.it>
>   * Copyright (c) 2008-2009 Eurotech S.p.A. <info@eurotech.it>
>   *
> - * This module supports the PCA954x series of I2C multiplexer/switch chips
> - * made by Philips Semiconductors.
> + * This module supports the PCA954x and PCA954x series of I2C multiplexer/switch
> + * chips made by NXP Semiconductors.
>   * This includes the:
> - *	 PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547
> - *	 and PCA9548.
> + *	 PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547,
> + *	 PCA9548, PCA9846, PCA9847, PCA9848 and PCA9849

The trailing . is missing.

>   *
>   * These chips are all controlled via the I2C bus itself, and all have a
>   * single 8-bit register. The upstream "parent" bus fans out to two,
> @@ -63,6 +64,10 @@ enum pca_type {
>  	pca_9546,
>  	pca_9547,
>  	pca_9548,
> +	pca_9846,
> +	pca_9847,
> +	pca_9848,
> +	pca_9849,
>  };
>  
>  struct chip_desc {
> @@ -129,6 +134,22 @@ static const struct chip_desc chips[] = {
>  		.nchans = 8,
>  		.muxtype = pca954x_isswi,
>  	},
> +	[pca_9846] = {
> +		.nchans = 4,
> +		.muxtype = pca954x_isswi,
> +	},
> +	[pca_9847] = {
> +		.nchans = 8,
> +		.muxtype = pca954x_ismux,
> +	},
> +	[pca_9848] = {
> +		.nchans = 8,
> +		.muxtype = pca954x_isswi,
> +	},
> +	[pca_9849] = {
> +		.nchans = 4,
> +		.muxtype = pca954x_ismux,
> +	},
>  };
>  
>  static const struct i2c_device_id pca954x_id[] = {
> @@ -140,6 +161,10 @@ static const struct i2c_device_id pca954x_id[] = {
>  	{ "pca9546", pca_9546 },
>  	{ "pca9547", pca_9547 },
>  	{ "pca9548", pca_9548 },
> +	{ "pca9846", pca_9846 },
> +	{ "pca9847", pca_9847 },
> +	{ "pca9848", pca_9848 },
> +	{ "pca9849", pca_9849 },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, pca954x_id);
> @@ -154,6 +179,10 @@ static const struct of_device_id pca954x_of_match[] = {
>  	{ .compatible = "nxp,pca9546", .data = &chips[pca_9546] },
>  	{ .compatible = "nxp,pca9547", .data = &chips[pca_9547] },
>  	{ .compatible = "nxp,pca9548", .data = &chips[pca_9548] },
> +	{ .compatible = "nxp,pca9846", .data = &chips[pca_9846] },
> +	{ .compatible = "nxp,pca9847", .data = &chips[pca_9847] },
> +	{ .compatible = "nxp,pca9848", .data = &chips[pca_9848] },
> +	{ .compatible = "nxp,pca9849", .data = &chips[pca_9849] },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, pca954x_of_match);
> @@ -339,7 +368,8 @@ static int pca954x_probe(struct i2c_client *client,
>  	if (IS_ERR(gpio))
>  		return PTR_ERR(gpio);
>  
> -	/* Write the mux register at addr to verify
> +	/*
> +	 * Write the mux register at addr to verify

This is now unrelated and belongs in a separate patch.

>  	 * that the mux is in fact present. This also
>  	 * initializes the mux to disconnected state.
>  	 */
> @@ -443,6 +473,7 @@ static struct i2c_driver pca954x_driver = {
>  
>  module_i2c_driver(pca954x_driver);
>  
> +MODULE_AUTHOR("Adrian Fiergolski <Adrian.Fiergolski@cern.ch>");

And, just as Wolfram said, this is not appropriate anymore. Next
time you should probably ask before adding authorship to an existing
file, that makes you look better. Unless you do some major addition
of course... But when you do add yourself, add yourself last so that
it doesn't look like you are the principal author. Unless you did
some really serious surgery of course...

Anyway, with those changes things are getting in shape.

Thanks for your patience!

Cheers,
Peter

>  MODULE_AUTHOR("Rodolfo Giometti <giometti@linux.it>");
>  MODULE_DESCRIPTION("PCA954x I2C mux/switch driver");
>  MODULE_LICENSE("GPL v2");
> 

Re: [PATCH v4] i2c: Add support for NXP PCA984x family.

[Peter Rosin]

[Adding DT people]

On 2017-12-13 19:26, Peter Rosin wrote:
> On 2017-12-13 17:12, Adrian Fiergolski wrote:
>> This patch exetends the current i2c-mux-pca954x driver and adds support for
> 
> extends
> 
>> a newer PCA984x family of the I2C switches and multiplexers from NXP.
>>
>> Signed-off-by: Adrian Fiergolski <adrian.fiergolski@cern.ch>
>> ---
>> As suggested by Peter, for a moment the device_id checks have been removed
>> and need to wait for a support in the I2C core.
>>  .../devicetree/bindings/i2c/i2c-mux-pca954x.txt    |  5 ++-
>>  drivers/i2c/muxes/Kconfig                          |  6 ++--
>>  drivers/i2c/muxes/i2c-mux-pca954x.c                | 41 +++++++++++++++++++---
>>  3 files changed, 43 insertions(+), 9 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
>> index aa097045a10e..b428bc0d81b1 100644
>> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
>> @@ -1,10 +1,13 @@
>>  * NXP PCA954x I2C bus switch
>>  
>> +The driver supports NXP PCA954x and PCA984x I2C mux/switch devices.
>> +
>>  Required Properties:
>>  
>>    - compatible: Must contain one of the following.
>>      "nxp,pca9540", "nxp,pca9542", "nxp,pca9543", "nxp,pca9544",
>> -    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548"
>> +    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548",
>> +    "nxp,pca9846", "nxp,pca9847", "nxp,pca9848", "nxp,pca9849"

Sorry for the incremental review, but I thought about a couple of
more issues with this patch...

First, I think that perhaps the new chips should have compatibles
like:

	compatible = "nxp,pca9846", "nxp,pca9546";
	compatible = "nxp,pca9847", "nxp,pca9547";
	compatible = "nxp,pca9848", "nxp,pca9548";

since they are extremely similar to the older chips (the only
difference is the device id support and other esoteric stuff that
you don't need to use). That way the device-tree will work even
with an older OS that only supports pca954x chips. And when you
add the device id check, you can differentiate.

(pca9849 isn't really compatible with any of the pca954x chips since
it lacks interrupt handling)

So, I'm adding the device-tree list to get input on how this is
normally handled. You're going to need their ack anyway on this hunk.
Hint, they also like it when the DT changes are in a separate patch.
You can run get_maintainer.pl on patches to find out where you should
send them.

Second, totally unrelated issues below, ...

>>  
>>    - reg: The I2C address of the device.
>>  
>> diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
>> index 0f5c8fc36625..23cc41866a91 100644
>> --- a/drivers/i2c/muxes/Kconfig
>> +++ b/drivers/i2c/muxes/Kconfig
>> @@ -64,11 +64,11 @@ config I2C_MUX_PCA9541
>>  	  will be called i2c-mux-pca9541.
>>  
>>  config I2C_MUX_PCA954x
>> -	tristate "Philips PCA954x I2C Mux/switches"
>> +	tristate "NXP PCA954x I2C Mux/switches"
> 
> I think you should perhaps mention PCA984x here in the headline.
> 
>>  	depends on GPIOLIB || COMPILE_TEST
>>  	help
>> -	  If you say yes here you get support for the Philips PCA954x
>> -	  I2C mux/switch devices.
>> +	  If you say yes here you get support for the NXP PCA954x
>> +	  and PCA984x I2C mux/switch devices.
>>  
>>  	  This driver can also be built as a module.  If so, the module
>>  	  will be called i2c-mux-pca954x.
>> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
>> index 2ca068d8b92d..b4a41d013538 100644
>> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
>> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
>> @@ -1,14 +1,15 @@
>>  /*
>>   * I2C multiplexer
>>   *
>> + * Copyright (c) 2017 Adrian Fiergolski <Adrian.Fiergolski@cern.ch>
> 
> This is a bit over the top when you only add data with questionable
> copyright value.
> 
>>   * Copyright (c) 2008-2009 Rodolfo Giometti <giometti@linux.it>
>>   * Copyright (c) 2008-2009 Eurotech S.p.A. <info@eurotech.it>
>>   *
>> - * This module supports the PCA954x series of I2C multiplexer/switch chips
>> - * made by Philips Semiconductors.
>> + * This module supports the PCA954x and PCA954x series of I2C multiplexer/switch
>> + * chips made by NXP Semiconductors.
>>   * This includes the:
>> - *	 PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547
>> - *	 and PCA9548.
>> + *	 PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547,
>> + *	 PCA9548, PCA9846, PCA9847, PCA9848 and PCA9849
> 
> The trailing . is missing.
> 
>>   *
>>   * These chips are all controlled via the I2C bus itself, and all have a
>>   * single 8-bit register. The upstream "parent" bus fans out to two,
>> @@ -63,6 +64,10 @@ enum pca_type {
>>  	pca_9546,
>>  	pca_9547,
>>  	pca_9548,
>> +	pca_9846,
>> +	pca_9847,
>> +	pca_9848,
>> +	pca_9849,
>>  };
>>  
>>  struct chip_desc {
>> @@ -129,6 +134,22 @@ static const struct chip_desc chips[] = {
>>  		.nchans = 8,
>>  		.muxtype = pca954x_isswi,
>>  	},
>> +	[pca_9846] = {
>> +		.nchans = 4,
>> +		.muxtype = pca954x_isswi,
>> +	},
>> +	[pca_9847] = {
>> +		.nchans = 8,

...you most likely need ".enable = 0x8," here.

>> +		.muxtype = pca954x_ismux,
>> +	},
>> +	[pca_9848] = {
>> +		.nchans = 8,
>> +		.muxtype = pca954x_isswi,
>> +	},
>> +	[pca_9849] = {
>> +		.nchans = 4,

Likewise, you need ".enable = 0x4," here.

Cheers,
Peter

>> +		.muxtype = pca954x_ismux,
>> +	},
>>  };
>>  
>>  static const struct i2c_device_id pca954x_id[] = {
>> @@ -140,6 +161,10 @@ static const struct i2c_device_id pca954x_id[] = {
>>  	{ "pca9546", pca_9546 },
>>  	{ "pca9547", pca_9547 },
>>  	{ "pca9548", pca_9548 },
>> +	{ "pca9846", pca_9846 },
>> +	{ "pca9847", pca_9847 },
>> +	{ "pca9848", pca_9848 },
>> +	{ "pca9849", pca_9849 },
>>  	{ }
>>  };
>>  MODULE_DEVICE_TABLE(i2c, pca954x_id);
>> @@ -154,6 +179,10 @@ static const struct of_device_id pca954x_of_match[] = {
>>  	{ .compatible = "nxp,pca9546", .data = &chips[pca_9546] },
>>  	{ .compatible = "nxp,pca9547", .data = &chips[pca_9547] },
>>  	{ .compatible = "nxp,pca9548", .data = &chips[pca_9548] },
>> +	{ .compatible = "nxp,pca9846", .data = &chips[pca_9846] },
>> +	{ .compatible = "nxp,pca9847", .data = &chips[pca_9847] },
>> +	{ .compatible = "nxp,pca9848", .data = &chips[pca_9848] },
>> +	{ .compatible = "nxp,pca9849", .data = &chips[pca_9849] },
>>  	{}
>>  };
>>  MODULE_DEVICE_TABLE(of, pca954x_of_match);
>> @@ -339,7 +368,8 @@ static int pca954x_probe(struct i2c_client *client,
>>  	if (IS_ERR(gpio))
>>  		return PTR_ERR(gpio);
>>  
>> -	/* Write the mux register at addr to verify
>> +	/*
>> +	 * Write the mux register at addr to verify
> 
> This is now unrelated and belongs in a separate patch.
> 
>>  	 * that the mux is in fact present. This also
>>  	 * initializes the mux to disconnected state.
>>  	 */
>> @@ -443,6 +473,7 @@ static struct i2c_driver pca954x_driver = {
>>  
>>  module_i2c_driver(pca954x_driver);
>>  
>> +MODULE_AUTHOR("Adrian Fiergolski <Adrian.Fiergolski@cern.ch>");
> 
> And, just as Wolfram said, this is not appropriate anymore. Next
> time you should probably ask before adding authorship to an existing
> file, that makes you look better. Unless you do some major addition
> of course... But when you do add yourself, add yourself last so that
> it doesn't look like you are the principal author. Unless you did
> some really serious surgery of course...
> 
> Anyway, with those changes things are getting in shape.
> 
> Thanks for your patience!
> 
> Cheers,
> Peter
> 
>>  MODULE_AUTHOR("Rodolfo Giometti <giometti@linux.it>");
>>  MODULE_DESCRIPTION("PCA954x I2C mux/switch driver");
>>  MODULE_LICENSE("GPL v2");
>>
> 

[PATCH v5] i2c: Add support for NXP PCA984x family.

[Adrian Fiergolski]

This patch extends the current i2c-mux-pca954x driver and adds support for
a newer PCA984x family of the I2C switches and multiplexers from NXP.

Signed-off-by: Adrian Fiergolski <adrian.fiergolski-vJEk5272eHo@public.gmane.org>
---
I have applied recent Peter's comments.
I am waiting for comments from device-tree folks regarding compatibles.

.../devicetree/bindings/i2c/i2c-mux-pca954x.txt    |  5 ++-
 drivers/i2c/muxes/Kconfig                          |  6 ++--
 drivers/i2c/muxes/i2c-mux-pca954x.c                | 36 +++++++++++++++++++---
 3 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
index aa097045a10e..b428bc0d81b1 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
@@ -1,10 +1,13 @@
 * NXP PCA954x I2C bus switch
 
+The driver supports NXP PCA954x and PCA984x I2C mux/switch devices.
+
 Required Properties:
 
   - compatible: Must contain one of the following.
     "nxp,pca9540", "nxp,pca9542", "nxp,pca9543", "nxp,pca9544",
-    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548"
+    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548",
+    "nxp,pca9846", "nxp,pca9847", "nxp,pca9848", "nxp,pca9849"
 
   - reg: The I2C address of the device.
 
diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
index 0f5c8fc36625..52a4a922e7e6 100644
--- a/drivers/i2c/muxes/Kconfig
+++ b/drivers/i2c/muxes/Kconfig
@@ -64,11 +64,11 @@ config I2C_MUX_PCA9541
 	  will be called i2c-mux-pca9541.
 
 config I2C_MUX_PCA954x
-	tristate "Philips PCA954x I2C Mux/switches"
+	tristate "NXP PCA954x and PCA984x I2C Mux/switches"
 	depends on GPIOLIB || COMPILE_TEST
 	help
-	  If you say yes here you get support for the Philips PCA954x
-	  I2C mux/switch devices.
+	  If you say yes here you get support for the NXP PCA954x
+	  and PCA984x I2C mux/switch devices.
 
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-mux-pca954x.
diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 2ca068d8b92d..9796bd7d61d5 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -4,11 +4,11 @@
  * Copyright (c) 2008-2009 Rodolfo Giometti <giometti-k2GhghHVRtY@public.gmane.org>
  * Copyright (c) 2008-2009 Eurotech S.p.A. <info-ymFC7zkAqBI1GQ1Ptb7lUw@public.gmane.org>
  *
- * This module supports the PCA954x series of I2C multiplexer/switch chips
- * made by Philips Semiconductors.
+ * This module supports the PCA954x and PCA954x series of I2C multiplexer/switch
+ * chips made by NXP Semiconductors.
  * This includes the:
- *	 PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547
- *	 and PCA9548.
+ *	 PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547,
+ *	 PCA9548, PCA9846, PCA9847, PCA9848 and PCA9849.
  *
  * These chips are all controlled via the I2C bus itself, and all have a
  * single 8-bit register. The upstream "parent" bus fans out to two,
@@ -63,6 +63,10 @@ enum pca_type {
 	pca_9546,
 	pca_9547,
 	pca_9548,
+	pca_9846,
+	pca_9847,
+	pca_9848,
+	pca_9849,
 };
 
 struct chip_desc {
@@ -129,6 +133,22 @@ static const struct chip_desc chips[] = {
 		.nchans = 8,
 		.muxtype = pca954x_isswi,
 	},
+	[pca_9846] = {
+		.nchans = 4,
+		.muxtype = pca954x_isswi,
+	},
+	[pca_9847] = {
+		.nchans = 8,
+		.muxtype = pca954x_ismux,
+	},
+	[pca_9848] = {
+		.nchans = 8,
+		.muxtype = pca954x_isswi,
+	},
+	[pca_9849] = {
+		.nchans = 4,
+		.muxtype = pca954x_ismux,
+	},
 };
 
 static const struct i2c_device_id pca954x_id[] = {
@@ -140,6 +160,10 @@ static const struct i2c_device_id pca954x_id[] = {
 	{ "pca9546", pca_9546 },
 	{ "pca9547", pca_9547 },
 	{ "pca9548", pca_9548 },
+	{ "pca9846", pca_9846 },
+	{ "pca9847", pca_9847 },
+	{ "pca9848", pca_9848 },
+	{ "pca9849", pca_9849 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, pca954x_id);
@@ -154,6 +178,10 @@ static const struct of_device_id pca954x_of_match[] = {
 	{ .compatible = "nxp,pca9546", .data = &chips[pca_9546] },
 	{ .compatible = "nxp,pca9547", .data = &chips[pca_9547] },
 	{ .compatible = "nxp,pca9548", .data = &chips[pca_9548] },
+	{ .compatible = "nxp,pca9846", .data = &chips[pca_9846] },
+	{ .compatible = "nxp,pca9847", .data = &chips[pca_9847] },
+	{ .compatible = "nxp,pca9848", .data = &chips[pca_9848] },
+	{ .compatible = "nxp,pca9849", .data = &chips[pca_9849] },
 	{}
 };
 MODULE_DEVICE_TABLE(of, pca954x_of_match);
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] i2c: Add support for NXP PCA984x family.

[Peter Rosin]

On 2017-12-14 12:20, Adrian Fiergolski wrote:
> This patch extends the current i2c-mux-pca954x driver and adds support for
> a newer PCA984x family of the I2C switches and multiplexers from NXP.
> 
> Signed-off-by: Adrian Fiergolski <adrian.fiergolski-vJEk5272eHo@public.gmane.org>
> ---
> I have applied recent Peter's comments.

Look like you missed the pair about .enable assignments for the [pca_9847]
and [pca_9849] entries of chips[].

Cheers,
Peter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6] i2c: Add support for NXP PCA984x family.

[Adrian Fiergolski]

This patch extends the current i2c-mux-pca954x driver and adds support for
a newer PCA984x family of the I2C switches and multiplexers from NXP.

Signed-off-by: Adrian Fiergolski <adrian.fiergolski@cern.ch>
---
 .../devicetree/bindings/i2c/i2c-mux-pca954x.txt    |  5 ++-
 drivers/i2c/muxes/Kconfig                          |  6 ++--
 drivers/i2c/muxes/i2c-mux-pca954x.c                | 38 +++++++++++++++++++---
 3 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
index aa097045a10e..b428bc0d81b1 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
@@ -1,10 +1,13 @@
 * NXP PCA954x I2C bus switch
 
+The driver supports NXP PCA954x and PCA984x I2C mux/switch devices.
+
 Required Properties:
 
   - compatible: Must contain one of the following.
     "nxp,pca9540", "nxp,pca9542", "nxp,pca9543", "nxp,pca9544",
-    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548"
+    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548",
+    "nxp,pca9846", "nxp,pca9847", "nxp,pca9848", "nxp,pca9849"
 
   - reg: The I2C address of the device.
 
diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
index 0f5c8fc36625..52a4a922e7e6 100644
--- a/drivers/i2c/muxes/Kconfig
+++ b/drivers/i2c/muxes/Kconfig
@@ -64,11 +64,11 @@ config I2C_MUX_PCA9541
 	  will be called i2c-mux-pca9541.
 
 config I2C_MUX_PCA954x
-	tristate "Philips PCA954x I2C Mux/switches"
+	tristate "NXP PCA954x and PCA984x I2C Mux/switches"
 	depends on GPIOLIB || COMPILE_TEST
 	help
-	  If you say yes here you get support for the Philips PCA954x
-	  I2C mux/switch devices.
+	  If you say yes here you get support for the NXP PCA954x
+	  and PCA984x I2C mux/switch devices.
 
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-mux-pca954x.
diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 2ca068d8b92d..fbb84c7ef282 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -4,11 +4,11 @@
  * Copyright (c) 2008-2009 Rodolfo Giometti <giometti@linux.it>
  * Copyright (c) 2008-2009 Eurotech S.p.A. <info@eurotech.it>
  *
- * This module supports the PCA954x series of I2C multiplexer/switch chips
- * made by Philips Semiconductors.
+ * This module supports the PCA954x and PCA954x series of I2C multiplexer/switch
+ * chips made by NXP Semiconductors.
  * This includes the:
- *	 PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547
- *	 and PCA9548.
+ *	 PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547,
+ *	 PCA9548, PCA9846, PCA9847, PCA9848 and PCA9849.
  *
  * These chips are all controlled via the I2C bus itself, and all have a
  * single 8-bit register. The upstream "parent" bus fans out to two,
@@ -63,6 +63,10 @@ enum pca_type {
 	pca_9546,
 	pca_9547,
 	pca_9548,
+	pca_9846,
+	pca_9847,
+	pca_9848,
+	pca_9849,
 };
 
 struct chip_desc {
@@ -129,6 +133,24 @@ static const struct chip_desc chips[] = {
 		.nchans = 8,
 		.muxtype = pca954x_isswi,
 	},
+	[pca_9846] = {
+		.nchans = 4,
+		.muxtype = pca954x_isswi,
+	},
+	[pca_9847] = {
+		.nchans = 8,
+		.enable = 0x8,
+		.muxtype = pca954x_ismux,
+	},
+	[pca_9848] = {
+		.nchans = 8,
+		.muxtype = pca954x_isswi,
+	},
+	[pca_9849] = {
+		.nchans = 4,
+		.enable = 0x4,
+		.muxtype = pca954x_ismux,
+	},
 };
 
 static const struct i2c_device_id pca954x_id[] = {
@@ -140,6 +162,10 @@ static const struct i2c_device_id pca954x_id[] = {
 	{ "pca9546", pca_9546 },
 	{ "pca9547", pca_9547 },
 	{ "pca9548", pca_9548 },
+	{ "pca9846", pca_9846 },
+	{ "pca9847", pca_9847 },
+	{ "pca9848", pca_9848 },
+	{ "pca9849", pca_9849 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, pca954x_id);
@@ -154,6 +180,10 @@ static const struct of_device_id pca954x_of_match[] = {
 	{ .compatible = "nxp,pca9546", .data = &chips[pca_9546] },
 	{ .compatible = "nxp,pca9547", .data = &chips[pca_9547] },
 	{ .compatible = "nxp,pca9548", .data = &chips[pca_9548] },
+	{ .compatible = "nxp,pca9846", .data = &chips[pca_9846] },
+	{ .compatible = "nxp,pca9847", .data = &chips[pca_9847] },
+	{ .compatible = "nxp,pca9848", .data = &chips[pca_9848] },
+	{ .compatible = "nxp,pca9849", .data = &chips[pca_9849] },
 	{}
 };
 MODULE_DEVICE_TABLE(of, pca954x_of_match);
-- 
2.14.1

Re: [PATCH v6] i2c: Add support for NXP PCA984x family.

[Rob Herring]

On Mon, Dec 18, 2017 at 06:45:06PM +0100, Adrian Fiergolski wrote:
> This patch extends the current i2c-mux-pca954x driver and adds support for
> a newer PCA984x family of the I2C switches and multiplexers from NXP.
> 
> Signed-off-by: Adrian Fiergolski <adrian.fiergolski@cern.ch>
> ---
>  .../devicetree/bindings/i2c/i2c-mux-pca954x.txt    |  5 ++-
>  drivers/i2c/muxes/Kconfig                          |  6 ++--
>  drivers/i2c/muxes/i2c-mux-pca954x.c                | 38 +++++++++++++++++++---
>  3 files changed, 41 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> index aa097045a10e..b428bc0d81b1 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> @@ -1,10 +1,13 @@
>  * NXP PCA954x I2C bus switch
>  
> +The driver supports NXP PCA954x and PCA984x I2C mux/switch devices.
> +
>  Required Properties:
>  
>    - compatible: Must contain one of the following.
>      "nxp,pca9540", "nxp,pca9542", "nxp,pca9543", "nxp,pca9544",
> -    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548"
> +    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548",
> +    "nxp,pca9846", "nxp,pca9847", "nxp,pca9848", "nxp,pca9849"

The format here should be 1 valid combination of compatibles per line. 
So it should be clear what are valid fallbacks as Peter suggested.

>  
>    - reg: The I2C address of the device.
>  

[PATCH v7] i2c: Add support for NXP PCA984x family.

[Adrian Fiergolski]

This patch extends the current i2c-mux-pca954x driver and adds support for
a newer PCA984x family of the I2C switches and multiplexers from NXP.

Signed-off-by: Adrian Fiergolski <adrian.fiergolski-vJEk5272eHo@public.gmane.org>
---
Following Rob's and Peter's remarks, bindings contain now one valid
combination of compatibles per line.

 .../devicetree/bindings/i2c/i2c-mux-pca954x.txt    | 13 ++++++--
 drivers/i2c/muxes/Kconfig                          |  6 ++--
 drivers/i2c/muxes/i2c-mux-pca954x.c                | 38 +++++++++++++++++++---
 3 files changed, 48 insertions(+), 9 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
index aa097045a10e..34d91501342e 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
@@ -1,10 +1,19 @@
 * NXP PCA954x I2C bus switch
 
+The driver supports NXP PCA954x and PCA984x I2C mux/switch devices.
+
 Required Properties:
 
   - compatible: Must contain one of the following.
-    "nxp,pca9540", "nxp,pca9542", "nxp,pca9543", "nxp,pca9544",
-    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548"
+    "nxp,pca9540",
+    "nxp,pca9542",
+    "nxp,pca9543",
+    "nxp,pca9544",
+    "nxp,pca9545",
+    "nxp,pca9546", "nxp,pca9846",
+    "nxp,pca9547", "nxp,pca9847",
+    "nxp,pca9548", "nxp,pca9848",
+    "nxp,pca9849"
 
   - reg: The I2C address of the device.
 
diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
index 0f5c8fc36625..52a4a922e7e6 100644
--- a/drivers/i2c/muxes/Kconfig
+++ b/drivers/i2c/muxes/Kconfig
@@ -64,11 +64,11 @@ config I2C_MUX_PCA9541
 	  will be called i2c-mux-pca9541.
 
 config I2C_MUX_PCA954x
-	tristate "Philips PCA954x I2C Mux/switches"
+	tristate "NXP PCA954x and PCA984x I2C Mux/switches"
 	depends on GPIOLIB || COMPILE_TEST
 	help
-	  If you say yes here you get support for the Philips PCA954x
-	  I2C mux/switch devices.
+	  If you say yes here you get support for the NXP PCA954x
+	  and PCA984x I2C mux/switch devices.
 
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-mux-pca954x.
diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 2ca068d8b92d..fbb84c7ef282 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -4,11 +4,11 @@
  * Copyright (c) 2008-2009 Rodolfo Giometti <giometti-k2GhghHVRtY@public.gmane.org>
  * Copyright (c) 2008-2009 Eurotech S.p.A. <info-ymFC7zkAqBI1GQ1Ptb7lUw@public.gmane.org>
  *
- * This module supports the PCA954x series of I2C multiplexer/switch chips
- * made by Philips Semiconductors.
+ * This module supports the PCA954x and PCA954x series of I2C multiplexer/switch
+ * chips made by NXP Semiconductors.
  * This includes the:
- *	 PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547
- *	 and PCA9548.
+ *	 PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547,
+ *	 PCA9548, PCA9846, PCA9847, PCA9848 and PCA9849.
  *
  * These chips are all controlled via the I2C bus itself, and all have a
  * single 8-bit register. The upstream "parent" bus fans out to two,
@@ -63,6 +63,10 @@ enum pca_type {
 	pca_9546,
 	pca_9547,
 	pca_9548,
+	pca_9846,
+	pca_9847,
+	pca_9848,
+	pca_9849,
 };
 
 struct chip_desc {
@@ -129,6 +133,24 @@ static const struct chip_desc chips[] = {
 		.nchans = 8,
 		.muxtype = pca954x_isswi,
 	},
+	[pca_9846] = {
+		.nchans = 4,
+		.muxtype = pca954x_isswi,
+	},
+	[pca_9847] = {
+		.nchans = 8,
+		.enable = 0x8,
+		.muxtype = pca954x_ismux,
+	},
+	[pca_9848] = {
+		.nchans = 8,
+		.muxtype = pca954x_isswi,
+	},
+	[pca_9849] = {
+		.nchans = 4,
+		.enable = 0x4,
+		.muxtype = pca954x_ismux,
+	},
 };
 
 static const struct i2c_device_id pca954x_id[] = {
@@ -140,6 +162,10 @@ static const struct i2c_device_id pca954x_id[] = {
 	{ "pca9546", pca_9546 },
 	{ "pca9547", pca_9547 },
 	{ "pca9548", pca_9548 },
+	{ "pca9846", pca_9846 },
+	{ "pca9847", pca_9847 },
+	{ "pca9848", pca_9848 },
+	{ "pca9849", pca_9849 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, pca954x_id);
@@ -154,6 +180,10 @@ static const struct of_device_id pca954x_of_match[] = {
 	{ .compatible = "nxp,pca9546", .data = &chips[pca_9546] },
 	{ .compatible = "nxp,pca9547", .data = &chips[pca_9547] },
 	{ .compatible = "nxp,pca9548", .data = &chips[pca_9548] },
+	{ .compatible = "nxp,pca9846", .data = &chips[pca_9846] },
+	{ .compatible = "nxp,pca9847", .data = &chips[pca_9847] },
+	{ .compatible = "nxp,pca9848", .data = &chips[pca_9848] },
+	{ .compatible = "nxp,pca9849", .data = &chips[pca_9849] },
 	{}
 };
 MODULE_DEVICE_TABLE(of, pca954x_of_match);
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7] i2c: Add support for NXP PCA984x family.

[Rob Herring]

On Mon, Dec 25, 2017 at 3:26 PM, Adrian Fiergolski
<adrian.fiergolski-vJEk5272eHo@public.gmane.org> wrote:
> This patch extends the current i2c-mux-pca954x driver and adds support for
> a newer PCA984x family of the I2C switches and multiplexers from NXP.
>
> Signed-off-by: Adrian Fiergolski <adrian.fiergolski-vJEk5272eHo@public.gmane.org>
> ---
> Following Rob's and Peter's remarks, bindings contain now one valid
> combination of compatibles per line.
>
>  .../devicetree/bindings/i2c/i2c-mux-pca954x.txt    | 13 ++++++--
>  drivers/i2c/muxes/Kconfig                          |  6 ++--
>  drivers/i2c/muxes/i2c-mux-pca954x.c                | 38 +++++++++++++++++++---
>  3 files changed, 48 insertions(+), 9 deletions(-)

Reviewed-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7] i2c: Add support for NXP PCA984x family.

[Peter Rosin]

On 2017-12-25 22:26, Adrian Fiergolski wrote:
> This patch extends the current i2c-mux-pca954x driver and adds support for
> a newer PCA984x family of the I2C switches and multiplexers from NXP.
> 
> Signed-off-by: Adrian Fiergolski <adrian.fiergolski-vJEk5272eHo@public.gmane.org>
> ---
> Following Rob's and Peter's remarks, bindings contain now one valid
> combination of compatibles per line.

Patch applied, thanks!

Any idea if you are going to tackle that device-id functionality for the
i2c core?

Cheers,
Peter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] i2c: Add support for NXP PCA984x family.

[Peter Rosin]

[It occurred to me that the DT people might skip the v4 discussion now
that there is a v5. So, in order to spare someone the effort of digging
that up, I'm re-sending the comments]

On 2017-12-14 12:20, Adrian Fiergolski wrote:
> This patch extends the current i2c-mux-pca954x driver and adds support for
> a newer PCA984x family of the I2C switches and multiplexers from NXP.
> 
> Signed-off-by: Adrian Fiergolski <adrian.fiergolski-vJEk5272eHo@public.gmane.org>
> ---
> I have applied recent Peter's comments.
> I am waiting for comments from device-tree folks regarding compatibles.
> 
> .../devicetree/bindings/i2c/i2c-mux-pca954x.txt    |  5 ++-
>  drivers/i2c/muxes/Kconfig                          |  6 ++--
>  drivers/i2c/muxes/i2c-mux-pca954x.c                | 36 +++++++++++++++++++---
>  3 files changed, 39 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> index aa097045a10e..b428bc0d81b1 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> @@ -1,10 +1,13 @@
>  * NXP PCA954x I2C bus switch
>  
> +The driver supports NXP PCA954x and PCA984x I2C mux/switch devices.
> +
>  Required Properties:
>  
>    - compatible: Must contain one of the following.
>      "nxp,pca9540", "nxp,pca9542", "nxp,pca9543", "nxp,pca9544",
> -    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548"
> +    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548",
> +    "nxp,pca9846", "nxp,pca9847", "nxp,pca9848", "nxp,pca9849"

I think that perhaps the new chips should have compatibles like:

	compatible = "nxp,pca9846", "nxp,pca9546";
	compatible = "nxp,pca9847", "nxp,pca9547";
	compatible = "nxp,pca9848", "nxp,pca9548";

since they are extremely similar to the older chips (the only
difference is the device id support and other esoteric stuff that
you don't need to use). That way the device-tree will work even
with an older OS that only supports pca954x chips. And when you
add the device id check for the pca984x family, you are set up
to differentiate.

(pca9849 isn't really compatible with any of the pca954x chips since
it lacks interrupt handling)

So, what is best practice in this situation?

Come to think of it, this is closely related to
	https://lkml.org/lkml/2017/8/3/226
which did not get a satisfactory answer IMHO.

>    - reg: The I2C address of the device.
>  

*snip*

Cheers,
Peter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] i2c: Add support for NXP PCA984x family.

[Peter Rosin]

On 2017-12-11 20:14, Peter Rosin wrote:
> On 2017-12-11 17:58, Adrian Fiergolski wrote:
>> +	/* Check manufacturer ID (12 bits) */
>> +	manufacturer_id = ((u16) device_id_raw.block[1] << 4) | (device_id_raw.block[2] >> 4);
> 
> This assignment is still broken...

Ooops, no, this one is fine. Sorry about that.

>> +	if (manufacturer_id != NXP_ID) {
>> +		dev_warn(&client->dev, "PCA9846 family: Manufacturer ID does not match NXP\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	/* Check device ID (9 bits) */
>> +	device_id = ((u16) device_id_raw.block[2] << 5) | (device_id_raw.block[3] >> 3);
> 
> ...and this is also broken.

But this one is broken. You need to mask out the upper half of
...block[2] so that the lower bits of the manufacturer don't
end up above the device id bits.

Cheers,
Peter

>> +	if (device_id != chips[id->driver_data].deviceID) {
>> +		dev_warn(&client->dev, "PCA9846 family: Device ID does not match %s\n", id->name);
>> +		return -ENODEV;
>> +	}