[v6 0/3] Add support for Maxim MAX735x/MAX736x variants

[v6 0/3] Add support for Maxim MAX735x/MAX736x variants

[Patrick Rudolph]

v6:
- Fix typo in dt-bindings

v5:
- Remove optional and make vdd-supply mandatory

v4:
- Add missing maxitems dt-bindings property

v3:
- Merge dt-bindings into i2c-mux-pca954x.yaml

v2:
- Move dt-bindings to separate file
- Added support for MAX736x as they are very similar
- Fixed an issue found by kernel test robot
- Dropped max735x property and custom IRQ check
- Added MAX7357 config register defines instead of magic values
- Renamed vcc-supply to vdd-supply

Patrick Rudolph (3):
  dt-bindings: i2c: Add Maxim MAX735x/MAX736x variants
  i2c: muxes: pca954x: Add MAX735x/MAX736x support
  i2c: muxes: pca954x: Add regulator support

 .../bindings/i2c/i2c-mux-pca954x.yaml         |  44 ++++--
 drivers/i2c/muxes/Kconfig                     |   4 +-
 drivers/i2c/muxes/i2c-mux-pca954x.c           | 126 ++++++++++++++++--
 3 files changed, 153 insertions(+), 21 deletions(-)

-- 
2.34.1

[v6 1/3] dt-bindings: i2c: Add Maxim MAX735x/MAX736x variants

[Patrick Rudolph]

Update the pca954x bindings to add support for the Maxim MAX735x/MAX736x
chips. The functionality will be provided by the exisintg pca954x driver.

While on it make the interrupts support conditionally as not all of the
existing chips have interrupts.

For chips that are powered off by default add an optional regulator
called vdd-supply.

Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
---
 .../bindings/i2c/i2c-mux-pca954x.yaml         | 44 ++++++++++++++-----
 1 file changed, 34 insertions(+), 10 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml
index 9f1726d0356b..132c3e54e7ab 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml
@@ -4,21 +4,48 @@
 $id: http://devicetree.org/schemas/i2c/i2c-mux-pca954x.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: NXP PCA954x I2C bus switch
+title: NXP PCA954x I2C and compatible bus switches
 
 maintainers:
   - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
 
 description:
-  The binding supports NXP PCA954x and PCA984x I2C mux/switch devices.
+  The binding supports NXP PCA954x and PCA984x I2C mux/switch devices,
+  and the Maxim MAX735x and MAX736x I2C mux/switch devices.
 
 allOf:
   - $ref: /schemas/i2c/i2c-mux.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - maxim,max7367
+              - maxim,max7369
+              - nxp,pca9542
+              - nxp,pca9543
+              - nxp,pca9544
+              - nxp,pca9545
+    then:
+      properties:
+        interrupts:
+          maxItems: 1
+
+        "#interrupt-cells":
+          const: 2
+
+        interrupt-controller: true
 
 properties:
   compatible:
     oneOf:
       - enum:
+          - maxim,max7356
+          - maxim,max7357
+          - maxim,max7358
+          - maxim,max7367
+          - maxim,max7368
+          - maxim,max7369
           - nxp,pca9540
           - nxp,pca9542
           - nxp,pca9543
@@ -38,14 +65,6 @@ properties:
   reg:
     maxItems: 1
 
-  interrupts:
-    maxItems: 1
-
-  "#interrupt-cells":
-    const: 2
-
-  interrupt-controller: true
-
   reset-gpios:
     maxItems: 1
 
@@ -59,6 +78,9 @@ properties:
     description: if present, overrides i2c-mux-idle-disconnect
     $ref: /schemas/mux/mux-controller.yaml#/properties/idle-state
 
+  vdd-supply:
+    description: A voltage regulator supplying power to the chip.
+
 required:
   - compatible
   - reg
@@ -79,6 +101,8 @@ examples:
             #size-cells = <0>;
             reg = <0x74>;
 
+            vdd-supply = <&p3v3>;
+
             interrupt-parent = <&ipic>;
             interrupts = <17 IRQ_TYPE_LEVEL_LOW>;
             interrupt-controller;
-- 
2.34.1

[v6 2/3] i2c: muxes: pca954x: Add MAX735x/MAX736x support

[Patrick Rudolph]

Add support for the following Maxim chips using the existing PCA954x
driver:
- MAX7356
- MAX7357
- MAX7358
- MAX7367
- MAX7368
- MAX7369

All added Maxim chips behave like the PCA954x, where a single SMBUS byte
write selects up to 8 channels to be bridged to the primary bus.

The MAX7357 exposes 6 additional registers at Power-On-Reset and is
configured to:
 - Disabled interrupts on bus locked up detection
 - Enable bus locked-up clearing
 - Disconnect only locked bus instead of all channels

While the MAX7357/MAX7358 have interrupt support, they don't act as
interrupt controller like the PCA9545 does. Thus don't enable IRQ support
and handle them like the PCA9548.

Tested using the MAX7357 and verified that the stalled bus is disconnected
while the other channels remain operational.

Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
---
 drivers/i2c/muxes/Kconfig           |  4 +-
 drivers/i2c/muxes/i2c-mux-pca954x.c | 92 +++++++++++++++++++++++++++--
 2 files changed, 90 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
index 1708b1a82da2..2ac99d044199 100644
--- a/drivers/i2c/muxes/Kconfig
+++ b/drivers/i2c/muxes/Kconfig
@@ -65,11 +65,11 @@ config I2C_MUX_PCA9541
 	  will be called i2c-mux-pca9541.
 
 config I2C_MUX_PCA954x
-	tristate "NXP PCA954x and PCA984x I2C Mux/switches"
+	tristate "NXP PCA954x/PCA984x and Maxim MAX735x/MAX736x I2C Mux/switches"
 	depends on GPIOLIB || COMPILE_TEST
 	help
 	  If you say yes here you get support for the NXP PCA954x
-	  and PCA984x I2C mux/switch devices.
+	  and PCA984x and Maxim MAX735x/MAX736x 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 4ad665757dd8..33b9a6a1fffa 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -4,6 +4,7 @@
  *
  * Copyright (c) 2008-2009 Rodolfo Giometti <giometti@linux.it>
  * Copyright (c) 2008-2009 Eurotech S.p.A. <info@eurotech.it>
+ * Copyright (c) 2022 Patrick Rudolph <patrick.rudolph@9elements.com>
  *
  * This module supports the PCA954x and PCA984x series of I2C multiplexer/switch
  * chips made by NXP Semiconductors.
@@ -11,6 +12,12 @@
  *	 PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547,
  *	 PCA9548, PCA9846, PCA9847, PCA9848 and PCA9849.
  *
+ * It's also compatible to Maxims MAX735x I2C switch chips, which are controlled
+ * as the NXP PCA9548 and the MAX736x chips that act like the PCA9544.
+ *
+ * This includes the:
+ *	 MAX7356, MAX7357, MAX7358, MAX7367, MAX7368 and MAX7369
+ *
  * 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,
  * four, or eight downstream busses or channels; which of these
@@ -50,7 +57,30 @@
 
 #define PCA954X_IRQ_OFFSET 4
 
+/*
+ * MAX7357 exposes 7 registers on POR which allow to configure additional
+ * features. Disable interrupts, enable bus locked-up clearing,
+ * isolate only the locked channel instead of all channels.
+ */
+#define MAX7357_CONF_INT_ENABLE			BIT(0)
+#define MAX7357_CONF_FLUSH_OUT			BIT(1)
+#define MAX7357_CONF_RELEASE_INT		BIT(2)
+#define MAX7357_CONF_LOCK_UP_CLEAR		BIT(3)
+#define MAX7357_CONF_DISCON_SINGLE_CHAN		BIT(4)
+#define MAX7357_CONF_BUS_LOCKUP_DETECTION	BIT(5)
+#define MAX7357_CONF_ENABLE_BASIC_MODE		BIT(6)
+#define MAX7357_CONF_PRECONNECT_TEST		BIT(7)
+
+#define MAX7357_CONF_DEFAULTS (MAX7357_CONF_FLUSH_OUT | \
+	 MAX7357_CONF_DISCON_SINGLE_CHAN)
+
 enum pca_type {
+	max_7367,
+	max_7368,
+	max_7369,
+	max_7356,
+	max_7357,
+	max_7358,
 	pca_9540,
 	pca_9542,
 	pca_9543,
@@ -69,6 +99,7 @@ struct chip_desc {
 	u8 nchans;
 	u8 enable;	/* used for muxes only */
 	u8 has_irq;
+	u8 max7357;
 	enum muxtype {
 		pca954x_ismux = 0,
 		pca954x_isswi
@@ -90,8 +121,42 @@ struct pca954x {
 	raw_spinlock_t lock;
 };
 
-/* Provide specs for the PCA954x types we know about */
+/* Provide specs for the PCA954x and MAX735x types we know about */
 static const struct chip_desc chips[] = {
+	[max_7356] = {
+		.nchans = 8,
+		.muxtype = pca954x_isswi,
+		.id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
+	},
+	[max_7357] = {
+		.nchans = 8,
+		.muxtype = pca954x_isswi,
+		.max7357 = 1,
+		.id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
+	},
+	[max_7358] = {
+		.nchans = 8,
+		.muxtype = pca954x_isswi,
+		.id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
+	},
+	[max_7367] = {
+		.nchans = 4,
+		.muxtype = pca954x_isswi,
+		.has_irq = 1,
+		.id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
+	},
+	[max_7368] = {
+		.nchans = 4,
+		.muxtype = pca954x_isswi,
+		.id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
+	},
+	[max_7369] = {
+		.nchans = 4,
+		.enable = 0x4,
+		.muxtype = pca954x_ismux,
+		.has_irq = 1,
+		.id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
+	},
 	[pca_9540] = {
 		.nchans = 2,
 		.enable = 0x4,
@@ -177,6 +242,12 @@ static const struct chip_desc chips[] = {
 };
 
 static const struct i2c_device_id pca954x_id[] = {
+	{ "max7356", max_7356 },
+	{ "max7357", max_7357 },
+	{ "max7358", max_7358 },
+	{ "max7367", max_7367 },
+	{ "max7368", max_7368 },
+	{ "max7369", max_7369 },
 	{ "pca9540", pca_9540 },
 	{ "pca9542", pca_9542 },
 	{ "pca9543", pca_9543 },
@@ -194,6 +265,12 @@ static const struct i2c_device_id pca954x_id[] = {
 MODULE_DEVICE_TABLE(i2c, pca954x_id);
 
 static const struct of_device_id pca954x_of_match[] = {
+	{ .compatible = "maxim,max7356", .data = &chips[max_7356] },
+	{ .compatible = "maxim,max7357", .data = &chips[max_7357] },
+	{ .compatible = "maxim,max7358", .data = &chips[max_7358] },
+	{ .compatible = "maxim,max7367", .data = &chips[max_7367] },
+	{ .compatible = "maxim,max7368", .data = &chips[max_7368] },
+	{ .compatible = "maxim,max7369", .data = &chips[max_7369] },
 	{ .compatible = "nxp,pca9540", .data = &chips[pca_9540] },
 	{ .compatible = "nxp,pca9542", .data = &chips[pca_9542] },
 	{ .compatible = "nxp,pca9543", .data = &chips[pca_9543] },
@@ -401,9 +478,16 @@ static int pca954x_init(struct i2c_client *client, struct pca954x *data)
 	else
 		data->last_chan = 0; /* Disconnect multiplexer */
 
-	ret = i2c_smbus_write_byte(client, data->last_chan);
-	if (ret < 0)
-		data->last_chan = 0;
+	if (data->chip->max7357) {
+		ret = i2c_smbus_write_byte_data(client, data->last_chan,
+						MAX7357_CONF_DEFAULTS);
+		if (ret < 0)
+			data->last_chan = 0;
+	} else {
+		ret = i2c_smbus_write_byte(client, data->last_chan);
+		if (ret < 0)
+			data->last_chan = 0;
+	}
 
 	return ret;
 }
-- 
2.34.1

[v6 3/3] i2c: muxes: pca954x: Add regulator support

[Patrick Rudolph]

Add a vdd regulator and enable it for boards that have the
mux powered off by default.

Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 34 ++++++++++++++++++++++++-----
 1 file changed, 29 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 33b9a6a1fffa..e25383752616 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -49,6 +49,7 @@
 #include <linux/module.h>
 #include <linux/pm.h>
 #include <linux/property.h>
+#include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <dt-bindings/mux/mux.h>
@@ -119,6 +120,7 @@ struct pca954x {
 	struct irq_domain *irq;
 	unsigned int irq_mask;
 	raw_spinlock_t lock;
+	struct regulator *supply;
 };
 
 /* Provide specs for the PCA954x and MAX735x types we know about */
@@ -459,6 +461,9 @@ static void pca954x_cleanup(struct i2c_mux_core *muxc)
 	struct pca954x *data = i2c_mux_priv(muxc);
 	int c, irq;
 
+	if (!IS_ERR_OR_NULL(data->supply))
+		regulator_disable(data->supply);
+
 	if (data->irq) {
 		for (c = 0; c < data->chip->nchans; c++) {
 			irq = irq_find_mapping(data->irq, c);
@@ -513,15 +518,32 @@ static int pca954x_probe(struct i2c_client *client,
 			     pca954x_select_chan, pca954x_deselect_mux);
 	if (!muxc)
 		return -ENOMEM;
+
 	data = i2c_mux_priv(muxc);
 
 	i2c_set_clientdata(client, muxc);
 	data->client = client;
 
+	data->supply = devm_regulator_get(dev, "vdd");
+	if (IS_ERR(data->supply)) {
+		ret = PTR_ERR(data->supply);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "Failed to request regulator: %d\n", ret);
+		return ret;
+	}
+
+	ret = regulator_enable(data->supply);
+	if (ret) {
+		dev_err(dev, "Failed to enable regulator: %d\n", ret);
+		return ret;
+	}
+
 	/* Reset the mux if a reset GPIO is specified. */
 	gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
-	if (IS_ERR(gpio))
-		return PTR_ERR(gpio);
+	if (IS_ERR(gpio)) {
+		ret = PTR_ERR(gpio);
+		goto fail_cleanup;
+	}
 	if (gpio) {
 		udelay(1);
 		gpiod_set_value_cansleep(gpio, 0);
@@ -538,7 +560,7 @@ static int pca954x_probe(struct i2c_client *client,
 
 		ret = i2c_get_device_id(client, &id);
 		if (ret && ret != -EOPNOTSUPP)
-			return ret;
+			goto fail_cleanup;
 
 		if (!ret &&
 		    (id.manufacturer_id != data->chip->id.manufacturer_id ||
@@ -546,7 +568,8 @@ static int pca954x_probe(struct i2c_client *client,
 			dev_warn(dev, "unexpected device id %03x-%03x-%x\n",
 				 id.manufacturer_id, id.part_id,
 				 id.die_revision);
-			return -ENODEV;
+			ret = -ENODEV;
+			goto fail_cleanup;
 		}
 	}
 
@@ -565,7 +588,8 @@ static int pca954x_probe(struct i2c_client *client,
 	ret = pca954x_init(client, data);
 	if (ret < 0) {
 		dev_warn(dev, "probe failed\n");
-		return -ENODEV;
+		ret = -ENODEV;
+		goto fail_cleanup;
 	}
 
 	ret = pca954x_irq_setup(muxc);
-- 
2.34.1

Re: [v6 1/3] dt-bindings: i2c: Add Maxim MAX735x/MAX736x variants

[Rob Herring]

On Wed, 16 Feb 2022 08:46:10 +0100, Patrick Rudolph wrote:
> Update the pca954x bindings to add support for the Maxim MAX735x/MAX736x
> chips. The functionality will be provided by the exisintg pca954x driver.
> 
> While on it make the interrupts support conditionally as not all of the
> existing chips have interrupts.
> 
> For chips that are powered off by default add an optional regulator
> called vdd-supply.
> 
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> ---
>  .../bindings/i2c/i2c-mux-pca954x.yaml         | 44 ++++++++++++++-----
>  1 file changed, 34 insertions(+), 10 deletions(-)
> 


Please add Acked-by/Reviewed-by tags when posting new versions. However,
there's no need to repost patches *only* to add the tags. The upstream
maintainer will do that for acks received on the version they apply.

If a tag was not added on purpose, please state why and what changed.

Re: [v6 2/3] i2c: muxes: pca954x: Add MAX735x/MAX736x support

[Wolfram Sang]

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

On Wed, Feb 16, 2022 at 08:46:11AM +0100, Patrick Rudolph wrote:
> Add support for the following Maxim chips using the existing PCA954x
> driver:
> - MAX7356
> - MAX7357
> - MAX7358
> - MAX7367
> - MAX7368
> - MAX7369
> 
> All added Maxim chips behave like the PCA954x, where a single SMBUS byte
> write selects up to 8 channels to be bridged to the primary bus.
> 
> The MAX7357 exposes 6 additional registers at Power-On-Reset and is
> configured to:
>  - Disabled interrupts on bus locked up detection
>  - Enable bus locked-up clearing
>  - Disconnect only locked bus instead of all channels
> 
> While the MAX7357/MAX7358 have interrupt support, they don't act as
> interrupt controller like the PCA9545 does. Thus don't enable IRQ support
> and handle them like the PCA9548.
> 
> Tested using the MAX7357 and verified that the stalled bus is disconnected
> while the other channels remain operational.
> 
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>

Peter, are you happy with this patch series?

All the best,

   Wolfram


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

Re: [v6 2/3] i2c: muxes: pca954x: Add MAX735x/MAX736x support

[Peter Rosin]

Hi!

Sorry for the slow review and thanks for your patience...

On 2022-02-16 08:46, Patrick Rudolph wrote:
> Add support for the following Maxim chips using the existing PCA954x
> driver:
> - MAX7356
> - MAX7357
> - MAX7358
> - MAX7367
> - MAX7368
> - MAX7369
> 
> All added Maxim chips behave like the PCA954x, where a single SMBUS byte
> write selects up to 8 channels to be bridged to the primary bus.
> 
> The MAX7357 exposes 6 additional registers at Power-On-Reset and is

MAX7358 also has the same enhanced mode as the 7357, no?

And what do you mean that they are exposed at POR? I can see why they
are not exposed /before/ POR, but are they ever /not/ exposed? If they
are always exposed when the chip is "alive", then I suggest that the
POR wording is dropped, otherwise that the above is reworded to
describe when the register are no longer exposed.

> configured to:
>  - Disabled interrupts on bus locked up detection
>  - Enable bus locked-up clearing
>  - Disconnect only locked bus instead of all channels
> 
> While the MAX7357/MAX7358 have interrupt support, they don't act as
> interrupt controller like the PCA9545 does. Thus don't enable IRQ support
> and handle them like the PCA9548.
> 
> Tested using the MAX7357 and verified that the stalled bus is disconnected
> while the other channels remain operational.
> 
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> ---
>  drivers/i2c/muxes/Kconfig           |  4 +-
>  drivers/i2c/muxes/i2c-mux-pca954x.c | 92 +++++++++++++++++++++++++++--
>  2 files changed, 90 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
> index 1708b1a82da2..2ac99d044199 100644
> --- a/drivers/i2c/muxes/Kconfig
> +++ b/drivers/i2c/muxes/Kconfig
> @@ -65,11 +65,11 @@ config I2C_MUX_PCA9541
>  	  will be called i2c-mux-pca9541.
>  
>  config I2C_MUX_PCA954x
> -	tristate "NXP PCA954x and PCA984x I2C Mux/switches"
> +	tristate "NXP PCA954x/PCA984x and Maxim MAX735x/MAX736x I2C Mux/switches"
>  	depends on GPIOLIB || COMPILE_TEST
>  	help
>  	  If you say yes here you get support for the NXP PCA954x
> -	  and PCA984x I2C mux/switch devices.
> +	  and PCA984x and Maxim MAX735x/MAX736x I2C mux/switch devices.

and and and... :-) Maybe like this?

	  If you say yes here you get support for NXP PCA954x/PCA984x
	  and Maxim MAX735x/MAX736x 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 4ad665757dd8..33b9a6a1fffa 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -4,6 +4,7 @@
>   *
>   * Copyright (c) 2008-2009 Rodolfo Giometti <giometti@linux.it>
>   * Copyright (c) 2008-2009 Eurotech S.p.A. <info@eurotech.it>
> + * Copyright (c) 2022 Patrick Rudolph <patrick.rudolph@9elements.com>
>   *
>   * This module supports the PCA954x and PCA984x series of I2C multiplexer/switch
>   * chips made by NXP Semiconductors.
> @@ -11,6 +12,12 @@
>   *	 PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547,
>   *	 PCA9548, PCA9846, PCA9847, PCA9848 and PCA9849.
>   *
> + * It's also compatible to Maxims MAX735x I2C switch chips, which are controlled
> + * as the NXP PCA9548 and the MAX736x chips that act like the PCA9544.
> + *
> + * This includes the:
> + *	 MAX7356, MAX7357, MAX7358, MAX7367, MAX7368 and MAX7369
> + *
>   * 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,
>   * four, or eight downstream busses or channels; which of these
> @@ -50,7 +57,30 @@
>  
>  #define PCA954X_IRQ_OFFSET 4
>  
> +/*
> + * MAX7357 exposes 7 registers on POR which allow to configure additional
> + * features. Disable interrupts, enable bus locked-up clearing,
> + * isolate only the locked channel instead of all channels.

Same MAX7358 and POR comments as above.

The way I understands things are:

 * MAX7357/MAX7358 exposes 7 registers which allow setup of
 * enhanced mode features. The first of these registers is the
 * switch control register that is present in some form on all
 * chips supported by this driver.
 * The second register is the configuration register, which allows
 * to configure additional features. E.g. disable interrupts,
 * enable bus locked-up clearing and isolate only the locked
 * channel instead of all channels.
 * The remaining 5 registers are left as is by this driver.

> + */
> +#define MAX7357_CONF_INT_ENABLE			BIT(0)
> +#define MAX7357_CONF_FLUSH_OUT			BIT(1)
> +#define MAX7357_CONF_RELEASE_INT		BIT(2)
> +#define MAX7357_CONF_LOCK_UP_CLEAR		BIT(3)
> +#define MAX7357_CONF_DISCON_SINGLE_CHAN		BIT(4)
> +#define MAX7357_CONF_BUS_LOCKUP_DETECTION	BIT(5)
> +#define MAX7357_CONF_ENABLE_BASIC_MODE		BIT(6)
> +#define MAX7357_CONF_PRECONNECT_TEST		BIT(7)
> +
> +#define MAX7357_CONF_DEFAULTS (MAX7357_CONF_FLUSH_OUT | \
> +	 MAX7357_CONF_DISCON_SINGLE_CHAN)
> +
>  enum pca_type {
> +	max_7367,
> +	max_7368,
> +	max_7369,
> +	max_7356,
> +	max_7357,
> +	max_7358,
>  	pca_9540,
>  	pca_9542,
>  	pca_9543,
> @@ -69,6 +99,7 @@ struct chip_desc {
>  	u8 nchans;
>  	u8 enable;	/* used for muxes only */
>  	u8 has_irq;
> +	u8 max7357;

Perhaps maxim_enhanced_mode is a better name?

>  	enum muxtype {
>  		pca954x_ismux = 0,
>  		pca954x_isswi
> @@ -90,8 +121,42 @@ struct pca954x {
>  	raw_spinlock_t lock;
>  };
>  
> -/* Provide specs for the PCA954x types we know about */
> +/* Provide specs for the PCA954x and MAX735x types we know about */
>  static const struct chip_desc chips[] = {
> +	[max_7356] = {
> +		.nchans = 8,
> +		.muxtype = pca954x_isswi,
> +		.id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
> +	},
> +	[max_7357] = {
> +		.nchans = 8,
> +		.muxtype = pca954x_isswi,
> +		.max7357 = 1,
> +		.id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
> +	},
> +	[max_7358] = {
> +		.nchans = 8,
> +		.muxtype = pca954x_isswi,
> +		.id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
> +	},
> +	[max_7367] = {
> +		.nchans = 4,
> +		.muxtype = pca954x_isswi,
> +		.has_irq = 1,
> +		.id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
> +	},
> +	[max_7368] = {
> +		.nchans = 4,
> +		.muxtype = pca954x_isswi,
> +		.id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
> +	},
> +	[max_7369] = {
> +		.nchans = 4,
> +		.enable = 0x4,
> +		.muxtype = pca954x_ismux,
> +		.has_irq = 1,
> +		.id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
> +	},
>  	[pca_9540] = {
>  		.nchans = 2,
>  		.enable = 0x4,
> @@ -177,6 +242,12 @@ static const struct chip_desc chips[] = {
>  };
>  
>  static const struct i2c_device_id pca954x_id[] = {
> +	{ "max7356", max_7356 },
> +	{ "max7357", max_7357 },
> +	{ "max7358", max_7358 },
> +	{ "max7367", max_7367 },
> +	{ "max7368", max_7368 },
> +	{ "max7369", max_7369 },
>  	{ "pca9540", pca_9540 },
>  	{ "pca9542", pca_9542 },
>  	{ "pca9543", pca_9543 },
> @@ -194,6 +265,12 @@ static const struct i2c_device_id pca954x_id[] = {
>  MODULE_DEVICE_TABLE(i2c, pca954x_id);
>  
>  static const struct of_device_id pca954x_of_match[] = {
> +	{ .compatible = "maxim,max7356", .data = &chips[max_7356] },
> +	{ .compatible = "maxim,max7357", .data = &chips[max_7357] },
> +	{ .compatible = "maxim,max7358", .data = &chips[max_7358] },
> +	{ .compatible = "maxim,max7367", .data = &chips[max_7367] },
> +	{ .compatible = "maxim,max7368", .data = &chips[max_7368] },
> +	{ .compatible = "maxim,max7369", .data = &chips[max_7369] },
>  	{ .compatible = "nxp,pca9540", .data = &chips[pca_9540] },
>  	{ .compatible = "nxp,pca9542", .data = &chips[pca_9542] },
>  	{ .compatible = "nxp,pca9543", .data = &chips[pca_9543] },
> @@ -401,9 +478,16 @@ static int pca954x_init(struct i2c_client *client, struct pca954x *data)
>  	else
>  		data->last_chan = 0; /* Disconnect multiplexer */
>  
> -	ret = i2c_smbus_write_byte(client, data->last_chan);
> -	if (ret < 0)
> -		data->last_chan = 0;
> +	if (data->chip->max7357) {
> +		ret = i2c_smbus_write_byte_data(client, data->last_chan,
> +						MAX7357_CONF_DEFAULTS);
> +		if (ret < 0)
> +			data->last_chan = 0;
> +	} else {
> +		ret = i2c_smbus_write_byte(client, data->last_chan);
> +		if (ret < 0)
> +			data->last_chan = 0;
> +	}
>  
>  	return ret;
>  }

The actual code is simple enough, and looks good.

Cheers,
Peter

Re: [v6 3/3] i2c: muxes: pca954x: Add regulator support

[Peter Rosin]

On 2022-02-16 08:46, Patrick Rudolph wrote:
> Add a vdd regulator and enable it for boards that have the
> mux powered off by default.
> 
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> ---
>  drivers/i2c/muxes/i2c-mux-pca954x.c | 34 ++++++++++++++++++++++++-----
>  1 file changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 33b9a6a1fffa..e25383752616 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -49,6 +49,7 @@
>  #include <linux/module.h>
>  #include <linux/pm.h>
>  #include <linux/property.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
>  #include <dt-bindings/mux/mux.h>
> @@ -119,6 +120,7 @@ struct pca954x {
>  	struct irq_domain *irq;
>  	unsigned int irq_mask;
>  	raw_spinlock_t lock;
> +	struct regulator *supply;
>  };
>  
>  /* Provide specs for the PCA954x and MAX735x types we know about */
> @@ -459,6 +461,9 @@ static void pca954x_cleanup(struct i2c_mux_core *muxc)
>  	struct pca954x *data = i2c_mux_priv(muxc);
>  	int c, irq;
>  
> +	if (!IS_ERR_OR_NULL(data->supply))
> +		regulator_disable(data->supply);
> +
>  	if (data->irq) {
>  		for (c = 0; c < data->chip->nchans; c++) {
>  			irq = irq_find_mapping(data->irq, c);
> @@ -513,15 +518,32 @@ static int pca954x_probe(struct i2c_client *client,
>  			     pca954x_select_chan, pca954x_deselect_mux);
>  	if (!muxc)
>  		return -ENOMEM;
> +
>  	data = i2c_mux_priv(muxc);
>  
>  	i2c_set_clientdata(client, muxc);
>  	data->client = client;
>  
> +	data->supply = devm_regulator_get(dev, "vdd");
> +	if (IS_ERR(data->supply)) {
> +		ret = PTR_ERR(data->supply);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "Failed to request regulator: %d\n", ret);
> +		return ret;
> +	}
> +

I think you need enclose the below in "if (data->supply)"

Cheers,
Peter

> +	ret = regulator_enable(data->supply);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable regulator: %d\n", ret);
> +		return ret;
> +	}
> +
>  	/* Reset the mux if a reset GPIO is specified. */
>  	gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> -	if (IS_ERR(gpio))
> -		return PTR_ERR(gpio);
> +	if (IS_ERR(gpio)) {
> +		ret = PTR_ERR(gpio);
> +		goto fail_cleanup;
> +	}
>  	if (gpio) {
>  		udelay(1);
>  		gpiod_set_value_cansleep(gpio, 0);
> @@ -538,7 +560,7 @@ static int pca954x_probe(struct i2c_client *client,
>  
>  		ret = i2c_get_device_id(client, &id);
>  		if (ret && ret != -EOPNOTSUPP)
> -			return ret;
> +			goto fail_cleanup;
>  
>  		if (!ret &&
>  		    (id.manufacturer_id != data->chip->id.manufacturer_id ||
> @@ -546,7 +568,8 @@ static int pca954x_probe(struct i2c_client *client,
>  			dev_warn(dev, "unexpected device id %03x-%03x-%x\n",
>  				 id.manufacturer_id, id.part_id,
>  				 id.die_revision);
> -			return -ENODEV;
> +			ret = -ENODEV;
> +			goto fail_cleanup;
>  		}
>  	}
>  
> @@ -565,7 +588,8 @@ static int pca954x_probe(struct i2c_client *client,
>  	ret = pca954x_init(client, data);
>  	if (ret < 0) {
>  		dev_warn(dev, "probe failed\n");
> -		return -ENODEV;
> +		ret = -ENODEV;
> +		goto fail_cleanup;
>  	}
>  
>  	ret = pca954x_irq_setup(muxc);

Re: [v6 3/3] i2c: muxes: pca954x: Add regulator support

[Peter Rosin]

On 2022-03-19 15:41, Peter Rosin wrote:
> On 2022-02-16 08:46, Patrick Rudolph wrote:
>> Add a vdd regulator and enable it for boards that have the
>> mux powered off by default.
>>
>> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
>> ---
>>  drivers/i2c/muxes/i2c-mux-pca954x.c | 34 ++++++++++++++++++++++++-----
>>  1 file changed, 29 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
>> index 33b9a6a1fffa..e25383752616 100644
>> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
>> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
>> @@ -49,6 +49,7 @@
>>  #include <linux/module.h>
>>  #include <linux/pm.h>
>>  #include <linux/property.h>
>> +#include <linux/regulator/consumer.h>
>>  #include <linux/slab.h>
>>  #include <linux/spinlock.h>
>>  #include <dt-bindings/mux/mux.h>
>> @@ -119,6 +120,7 @@ struct pca954x {
>>  	struct irq_domain *irq;
>>  	unsigned int irq_mask;
>>  	raw_spinlock_t lock;
>> +	struct regulator *supply;
>>  };
>>  
>>  /* Provide specs for the PCA954x and MAX735x types we know about */
>> @@ -459,6 +461,9 @@ static void pca954x_cleanup(struct i2c_mux_core *muxc)
>>  	struct pca954x *data = i2c_mux_priv(muxc);
>>  	int c, irq;
>>  
>> +	if (!IS_ERR_OR_NULL(data->supply))

Hmm, this is a bit confusing, but I guess it's fine since the
cleanup function is then ok even if it at some point in the future
is called before data->supply has been filled in. But this is
what confused me into the below comment...

>> +		regulator_disable(data->supply);
>> +
>>  	if (data->irq) {
>>  		for (c = 0; c < data->chip->nchans; c++) {
>>  			irq = irq_find_mapping(data->irq, c);
>> @@ -513,15 +518,32 @@ static int pca954x_probe(struct i2c_client *client,
>>  			     pca954x_select_chan, pca954x_deselect_mux);
>>  	if (!muxc)
>>  		return -ENOMEM;
>> +
>>  	data = i2c_mux_priv(muxc);
>>  
>>  	i2c_set_clientdata(client, muxc);
>>  	data->client = client;
>>  
>> +	data->supply = devm_regulator_get(dev, "vdd");
>> +	if (IS_ERR(data->supply)) {
>> +		ret = PTR_ERR(data->supply);
>> +		if (ret != -EPROBE_DEFER)
>> +			dev_err(dev, "Failed to request regulator: %d\n", ret);
>> +		return ret;
>> +	}
>> +
> 
> I think you need enclose the below in "if (data->supply)"

I just realized that no, you don't, because it falls back to the
dummy regulator. But then, data->supply cannot be NULL, but see
above for why it's a good thing to check for it either way when
cleaning up.

All is fine as-is.

Reviewed-by: Peter Rosin <peda@axentia.se>

Cheers,
Peter

>> +	ret = regulator_enable(data->supply);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to enable regulator: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>>  	/* Reset the mux if a reset GPIO is specified. */
>>  	gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
>> -	if (IS_ERR(gpio))
>> -		return PTR_ERR(gpio);
>> +	if (IS_ERR(gpio)) {
>> +		ret = PTR_ERR(gpio);
>> +		goto fail_cleanup;
>> +	}
>>  	if (gpio) {
>>  		udelay(1);
>>  		gpiod_set_value_cansleep(gpio, 0);
>> @@ -538,7 +560,7 @@ static int pca954x_probe(struct i2c_client *client,
>>  
>>  		ret = i2c_get_device_id(client, &id);
>>  		if (ret && ret != -EOPNOTSUPP)
>> -			return ret;
>> +			goto fail_cleanup;
>>  
>>  		if (!ret &&
>>  		    (id.manufacturer_id != data->chip->id.manufacturer_id ||
>> @@ -546,7 +568,8 @@ static int pca954x_probe(struct i2c_client *client,
>>  			dev_warn(dev, "unexpected device id %03x-%03x-%x\n",
>>  				 id.manufacturer_id, id.part_id,
>>  				 id.die_revision);
>> -			return -ENODEV;
>> +			ret = -ENODEV;
>> +			goto fail_cleanup;
>>  		}
>>  	}
>>  
>> @@ -565,7 +588,8 @@ static int pca954x_probe(struct i2c_client *client,
>>  	ret = pca954x_init(client, data);
>>  	if (ret < 0) {
>>  		dev_warn(dev, "probe failed\n");
>> -		return -ENODEV;
>> +		ret = -ENODEV;
>> +		goto fail_cleanup;
>>  	}
>>  
>>  	ret = pca954x_irq_setup(muxc);

Re: [v6 2/3] i2c: muxes: pca954x: Add MAX735x/MAX736x support

[Patrick Rudolph]

Hi Peter,
thanks for the review.
The MAX7358 has indeed the same registers as the MAX7357, but they
need to be "unlocked" by sending a magic sequence first.
As this isn't implemented by the driver it behaves like the MAX7356
with a single register.
The additional registers can be hidden again by setting a bit in the
config space.
Which wording would you prefer to describe this feature?

I'll change it to maxim_enhanced_mode.

Regards,
Patrick

On Sat, Mar 19, 2022 at 3:41 PM Peter Rosin <peda@axentia.se> wrote:
>
> Hi!
>
> Sorry for the slow review and thanks for your patience...
>
> On 2022-02-16 08:46, Patrick Rudolph wrote:
> > Add support for the following Maxim chips using the existing PCA954x
> > driver:
> > - MAX7356
> > - MAX7357
> > - MAX7358
> > - MAX7367
> > - MAX7368
> > - MAX7369
> >
> > All added Maxim chips behave like the PCA954x, where a single SMBUS byte
> > write selects up to 8 channels to be bridged to the primary bus.
> >
> > The MAX7357 exposes 6 additional registers at Power-On-Reset and is
>
> MAX7358 also has the same enhanced mode as the 7357, no?
>
> And what do you mean that they are exposed at POR? I can see why they
> are not exposed /before/ POR, but are they ever /not/ exposed? If they
> are always exposed when the chip is "alive", then I suggest that the
> POR wording is dropped, otherwise that the above is reworded to
> describe when the register are no longer exposed.
>
> > configured to:
> >  - Disabled interrupts on bus locked up detection
> >  - Enable bus locked-up clearing
> >  - Disconnect only locked bus instead of all channels
> >
> > While the MAX7357/MAX7358 have interrupt support, they don't act as
> > interrupt controller like the PCA9545 does. Thus don't enable IRQ support
> > and handle them like the PCA9548.
> >
> > Tested using the MAX7357 and verified that the stalled bus is disconnected
> > while the other channels remain operational.
> >
> > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> > ---
> >  drivers/i2c/muxes/Kconfig           |  4 +-
> >  drivers/i2c/muxes/i2c-mux-pca954x.c | 92 +++++++++++++++++++++++++++--
> >  2 files changed, 90 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
> > index 1708b1a82da2..2ac99d044199 100644
> > --- a/drivers/i2c/muxes/Kconfig
> > +++ b/drivers/i2c/muxes/Kconfig
> > @@ -65,11 +65,11 @@ config I2C_MUX_PCA9541
> >         will be called i2c-mux-pca9541.
> >
> >  config I2C_MUX_PCA954x
> > -     tristate "NXP PCA954x and PCA984x I2C Mux/switches"
> > +     tristate "NXP PCA954x/PCA984x and Maxim MAX735x/MAX736x I2C Mux/switches"
> >       depends on GPIOLIB || COMPILE_TEST
> >       help
> >         If you say yes here you get support for the NXP PCA954x
> > -       and PCA984x I2C mux/switch devices.
> > +       and PCA984x and Maxim MAX735x/MAX736x I2C mux/switch devices.
>
> and and and... :-) Maybe like this?
>
>           If you say yes here you get support for NXP PCA954x/PCA984x
>           and Maxim MAX735x/MAX736x 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 4ad665757dd8..33b9a6a1fffa 100644
> > --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> > +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> > @@ -4,6 +4,7 @@
> >   *
> >   * Copyright (c) 2008-2009 Rodolfo Giometti <giometti@linux.it>
> >   * Copyright (c) 2008-2009 Eurotech S.p.A. <info@eurotech.it>
> > + * Copyright (c) 2022 Patrick Rudolph <patrick.rudolph@9elements.com>
> >   *
> >   * This module supports the PCA954x and PCA984x series of I2C multiplexer/switch
> >   * chips made by NXP Semiconductors.
> > @@ -11,6 +12,12 @@
> >   *    PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547,
> >   *    PCA9548, PCA9846, PCA9847, PCA9848 and PCA9849.
> >   *
> > + * It's also compatible to Maxims MAX735x I2C switch chips, which are controlled
> > + * as the NXP PCA9548 and the MAX736x chips that act like the PCA9544.
> > + *
> > + * This includes the:
> > + *    MAX7356, MAX7357, MAX7358, MAX7367, MAX7368 and MAX7369
> > + *
> >   * 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,
> >   * four, or eight downstream busses or channels; which of these
> > @@ -50,7 +57,30 @@
> >
> >  #define PCA954X_IRQ_OFFSET 4
> >
> > +/*
> > + * MAX7357 exposes 7 registers on POR which allow to configure additional
> > + * features. Disable interrupts, enable bus locked-up clearing,
> > + * isolate only the locked channel instead of all channels.
>
> Same MAX7358 and POR comments as above.
>
> The way I understands things are:
>
>  * MAX7357/MAX7358 exposes 7 registers which allow setup of
>  * enhanced mode features. The first of these registers is the
>  * switch control register that is present in some form on all
>  * chips supported by this driver.
>  * The second register is the configuration register, which allows
>  * to configure additional features. E.g. disable interrupts,
>  * enable bus locked-up clearing and isolate only the locked
>  * channel instead of all channels.
>  * The remaining 5 registers are left as is by this driver.
>
> > + */
> > +#define MAX7357_CONF_INT_ENABLE                      BIT(0)
> > +#define MAX7357_CONF_FLUSH_OUT                       BIT(1)
> > +#define MAX7357_CONF_RELEASE_INT             BIT(2)
> > +#define MAX7357_CONF_LOCK_UP_CLEAR           BIT(3)
> > +#define MAX7357_CONF_DISCON_SINGLE_CHAN              BIT(4)
> > +#define MAX7357_CONF_BUS_LOCKUP_DETECTION    BIT(5)
> > +#define MAX7357_CONF_ENABLE_BASIC_MODE               BIT(6)
> > +#define MAX7357_CONF_PRECONNECT_TEST         BIT(7)
> > +
> > +#define MAX7357_CONF_DEFAULTS (MAX7357_CONF_FLUSH_OUT | \
> > +      MAX7357_CONF_DISCON_SINGLE_CHAN)
> > +
> >  enum pca_type {
> > +     max_7367,
> > +     max_7368,
> > +     max_7369,
> > +     max_7356,
> > +     max_7357,
> > +     max_7358,
> >       pca_9540,
> >       pca_9542,
> >       pca_9543,
> > @@ -69,6 +99,7 @@ struct chip_desc {
> >       u8 nchans;
> >       u8 enable;      /* used for muxes only */
> >       u8 has_irq;
> > +     u8 max7357;
>
> Perhaps maxim_enhanced_mode is a better name?
>
> >       enum muxtype {
> >               pca954x_ismux = 0,
> >               pca954x_isswi
> > @@ -90,8 +121,42 @@ struct pca954x {
> >       raw_spinlock_t lock;
> >  };
> >
> > -/* Provide specs for the PCA954x types we know about */
> > +/* Provide specs for the PCA954x and MAX735x types we know about */
> >  static const struct chip_desc chips[] = {
> > +     [max_7356] = {
> > +             .nchans = 8,
> > +             .muxtype = pca954x_isswi,
> > +             .id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
> > +     },
> > +     [max_7357] = {
> > +             .nchans = 8,
> > +             .muxtype = pca954x_isswi,
> > +             .max7357 = 1,
> > +             .id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
> > +     },
> > +     [max_7358] = {
> > +             .nchans = 8,
> > +             .muxtype = pca954x_isswi,
> > +             .id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
> > +     },
> > +     [max_7367] = {
> > +             .nchans = 4,
> > +             .muxtype = pca954x_isswi,
> > +             .has_irq = 1,
> > +             .id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
> > +     },
> > +     [max_7368] = {
> > +             .nchans = 4,
> > +             .muxtype = pca954x_isswi,
> > +             .id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
> > +     },
> > +     [max_7369] = {
> > +             .nchans = 4,
> > +             .enable = 0x4,
> > +             .muxtype = pca954x_ismux,
> > +             .has_irq = 1,
> > +             .id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
> > +     },
> >       [pca_9540] = {
> >               .nchans = 2,
> >               .enable = 0x4,
> > @@ -177,6 +242,12 @@ static const struct chip_desc chips[] = {
> >  };
> >
> >  static const struct i2c_device_id pca954x_id[] = {
> > +     { "max7356", max_7356 },
> > +     { "max7357", max_7357 },
> > +     { "max7358", max_7358 },
> > +     { "max7367", max_7367 },
> > +     { "max7368", max_7368 },
> > +     { "max7369", max_7369 },
> >       { "pca9540", pca_9540 },
> >       { "pca9542", pca_9542 },
> >       { "pca9543", pca_9543 },
> > @@ -194,6 +265,12 @@ static const struct i2c_device_id pca954x_id[] = {
> >  MODULE_DEVICE_TABLE(i2c, pca954x_id);
> >
> >  static const struct of_device_id pca954x_of_match[] = {
> > +     { .compatible = "maxim,max7356", .data = &chips[max_7356] },
> > +     { .compatible = "maxim,max7357", .data = &chips[max_7357] },
> > +     { .compatible = "maxim,max7358", .data = &chips[max_7358] },
> > +     { .compatible = "maxim,max7367", .data = &chips[max_7367] },
> > +     { .compatible = "maxim,max7368", .data = &chips[max_7368] },
> > +     { .compatible = "maxim,max7369", .data = &chips[max_7369] },
> >       { .compatible = "nxp,pca9540", .data = &chips[pca_9540] },
> >       { .compatible = "nxp,pca9542", .data = &chips[pca_9542] },
> >       { .compatible = "nxp,pca9543", .data = &chips[pca_9543] },
> > @@ -401,9 +478,16 @@ static int pca954x_init(struct i2c_client *client, struct pca954x *data)
> >       else
> >               data->last_chan = 0; /* Disconnect multiplexer */
> >
> > -     ret = i2c_smbus_write_byte(client, data->last_chan);
> > -     if (ret < 0)
> > -             data->last_chan = 0;
> > +     if (data->chip->max7357) {
> > +             ret = i2c_smbus_write_byte_data(client, data->last_chan,
> > +                                             MAX7357_CONF_DEFAULTS);
> > +             if (ret < 0)
> > +                     data->last_chan = 0;
> > +     } else {
> > +             ret = i2c_smbus_write_byte(client, data->last_chan);
> > +             if (ret < 0)
> > +                     data->last_chan = 0;
> > +     }
> >
> >       return ret;
> >  }
>
> The actual code is simple enough, and looks good.
>
> Cheers,
> Peter

Re: [v6 2/3] i2c: muxes: pca954x: Add MAX735x/MAX736x support

[Peter Rosin]

On 2022-03-23 13:41, Patrick Rudolph wrote:
> Hi Peter,
> thanks for the review.
> The MAX7358 has indeed the same registers as the MAX7357, but they
> need to be "unlocked" by sending a magic sequence first.
> As this isn't implemented by the driver it behaves like the MAX7356
> with a single register.
> The additional registers can be hidden again by setting a bit in the
> config space.
> Which wording would you prefer to describe this feature?

Perhaps: "exposed at POR" -> "exposed without extra commands"
or something like that. Without the background that the chips
are different like this at POR, the mention of POR is just
confusing...

But why not send this sequence for MAX7358? Then it occurred to me
that I expect the MAX7357 to behave pretty much like any of the
other chips until you touch the enhanced registers. Isn't that the
case? But why is it then needed to touch the enhanced registers at
all?

I suspect you know the answers, so I'm going the lazy route of just
asking instead of looking it up myself, hope that's ok...

Cheers,
Peter

> I'll change it to maxim_enhanced_mode.



> Regards,
> Patrick
> 
> On Sat, Mar 19, 2022 at 3:41 PM Peter Rosin <peda@axentia.se> wrote:
>>
>> Hi!
>>
>> Sorry for the slow review and thanks for your patience...
>>
>> On 2022-02-16 08:46, Patrick Rudolph wrote:
>>> Add support for the following Maxim chips using the existing PCA954x
>>> driver:
>>> - MAX7356
>>> - MAX7357
>>> - MAX7358
>>> - MAX7367
>>> - MAX7368
>>> - MAX7369
>>>
>>> All added Maxim chips behave like the PCA954x, where a single SMBUS byte
>>> write selects up to 8 channels to be bridged to the primary bus.
>>>
>>> The MAX7357 exposes 6 additional registers at Power-On-Reset and is
>>
>> MAX7358 also has the same enhanced mode as the 7357, no?
>>
>> And what do you mean that they are exposed at POR? I can see why they
>> are not exposed /before/ POR, but are they ever /not/ exposed? If they
>> are always exposed when the chip is "alive", then I suggest that the
>> POR wording is dropped, otherwise that the above is reworded to
>> describe when the register are no longer exposed.
>>
>>> configured to:
>>>  - Disabled interrupts on bus locked up detection
>>>  - Enable bus locked-up clearing
>>>  - Disconnect only locked bus instead of all channels
>>>
>>> While the MAX7357/MAX7358 have interrupt support, they don't act as
>>> interrupt controller like the PCA9545 does. Thus don't enable IRQ support
>>> and handle them like the PCA9548.
>>>
>>> Tested using the MAX7357 and verified that the stalled bus is disconnected
>>> while the other channels remain operational.
>>>
>>> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
>>> ---
>>>  drivers/i2c/muxes/Kconfig           |  4 +-
>>>  drivers/i2c/muxes/i2c-mux-pca954x.c | 92 +++++++++++++++++++++++++++--
>>>  2 files changed, 90 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
>>> index 1708b1a82da2..2ac99d044199 100644
>>> --- a/drivers/i2c/muxes/Kconfig
>>> +++ b/drivers/i2c/muxes/Kconfig
>>> @@ -65,11 +65,11 @@ config I2C_MUX_PCA9541
>>>         will be called i2c-mux-pca9541.
>>>
>>>  config I2C_MUX_PCA954x
>>> -     tristate "NXP PCA954x and PCA984x I2C Mux/switches"
>>> +     tristate "NXP PCA954x/PCA984x and Maxim MAX735x/MAX736x I2C Mux/switches"
>>>       depends on GPIOLIB || COMPILE_TEST
>>>       help
>>>         If you say yes here you get support for the NXP PCA954x
>>> -       and PCA984x I2C mux/switch devices.
>>> +       and PCA984x and Maxim MAX735x/MAX736x I2C mux/switch devices.
>>
>> and and and... :-) Maybe like this?
>>
>>           If you say yes here you get support for NXP PCA954x/PCA984x
>>           and Maxim MAX735x/MAX736x 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 4ad665757dd8..33b9a6a1fffa 100644
>>> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
>>> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
>>> @@ -4,6 +4,7 @@
>>>   *
>>>   * Copyright (c) 2008-2009 Rodolfo Giometti <giometti@linux.it>
>>>   * Copyright (c) 2008-2009 Eurotech S.p.A. <info@eurotech.it>
>>> + * Copyright (c) 2022 Patrick Rudolph <patrick.rudolph@9elements.com>
>>>   *
>>>   * This module supports the PCA954x and PCA984x series of I2C multiplexer/switch
>>>   * chips made by NXP Semiconductors.
>>> @@ -11,6 +12,12 @@
>>>   *    PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547,
>>>   *    PCA9548, PCA9846, PCA9847, PCA9848 and PCA9849.
>>>   *
>>> + * It's also compatible to Maxims MAX735x I2C switch chips, which are controlled
>>> + * as the NXP PCA9548 and the MAX736x chips that act like the PCA9544.
>>> + *
>>> + * This includes the:
>>> + *    MAX7356, MAX7357, MAX7358, MAX7367, MAX7368 and MAX7369
>>> + *
>>>   * 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,
>>>   * four, or eight downstream busses or channels; which of these
>>> @@ -50,7 +57,30 @@
>>>
>>>  #define PCA954X_IRQ_OFFSET 4
>>>
>>> +/*
>>> + * MAX7357 exposes 7 registers on POR which allow to configure additional
>>> + * features. Disable interrupts, enable bus locked-up clearing,
>>> + * isolate only the locked channel instead of all channels.
>>
>> Same MAX7358 and POR comments as above.
>>
>> The way I understands things are:
>>
>>  * MAX7357/MAX7358 exposes 7 registers which allow setup of
>>  * enhanced mode features. The first of these registers is the
>>  * switch control register that is present in some form on all
>>  * chips supported by this driver.
>>  * The second register is the configuration register, which allows
>>  * to configure additional features. E.g. disable interrupts,
>>  * enable bus locked-up clearing and isolate only the locked
>>  * channel instead of all channels.
>>  * The remaining 5 registers are left as is by this driver.
>>
>>> + */
>>> +#define MAX7357_CONF_INT_ENABLE                      BIT(0)
>>> +#define MAX7357_CONF_FLUSH_OUT                       BIT(1)
>>> +#define MAX7357_CONF_RELEASE_INT             BIT(2)
>>> +#define MAX7357_CONF_LOCK_UP_CLEAR           BIT(3)
>>> +#define MAX7357_CONF_DISCON_SINGLE_CHAN              BIT(4)
>>> +#define MAX7357_CONF_BUS_LOCKUP_DETECTION    BIT(5)
>>> +#define MAX7357_CONF_ENABLE_BASIC_MODE               BIT(6)
>>> +#define MAX7357_CONF_PRECONNECT_TEST         BIT(7)
>>> +
>>> +#define MAX7357_CONF_DEFAULTS (MAX7357_CONF_FLUSH_OUT | \
>>> +      MAX7357_CONF_DISCON_SINGLE_CHAN)
>>> +
>>>  enum pca_type {
>>> +     max_7367,
>>> +     max_7368,
>>> +     max_7369,
>>> +     max_7356,
>>> +     max_7357,
>>> +     max_7358,
>>>       pca_9540,
>>>       pca_9542,
>>>       pca_9543,
>>> @@ -69,6 +99,7 @@ struct chip_desc {
>>>       u8 nchans;
>>>       u8 enable;      /* used for muxes only */
>>>       u8 has_irq;
>>> +     u8 max7357;
>>
>> Perhaps maxim_enhanced_mode is a better name?
>>
>>>       enum muxtype {
>>>               pca954x_ismux = 0,
>>>               pca954x_isswi
>>> @@ -90,8 +121,42 @@ struct pca954x {
>>>       raw_spinlock_t lock;
>>>  };
>>>
>>> -/* Provide specs for the PCA954x types we know about */
>>> +/* Provide specs for the PCA954x and MAX735x types we know about */
>>>  static const struct chip_desc chips[] = {
>>> +     [max_7356] = {
>>> +             .nchans = 8,
>>> +             .muxtype = pca954x_isswi,
>>> +             .id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
>>> +     },
>>> +     [max_7357] = {
>>> +             .nchans = 8,
>>> +             .muxtype = pca954x_isswi,
>>> +             .max7357 = 1,
>>> +             .id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
>>> +     },
>>> +     [max_7358] = {
>>> +             .nchans = 8,
>>> +             .muxtype = pca954x_isswi,
>>> +             .id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
>>> +     },
>>> +     [max_7367] = {
>>> +             .nchans = 4,
>>> +             .muxtype = pca954x_isswi,
>>> +             .has_irq = 1,
>>> +             .id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
>>> +     },
>>> +     [max_7368] = {
>>> +             .nchans = 4,
>>> +             .muxtype = pca954x_isswi,
>>> +             .id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
>>> +     },
>>> +     [max_7369] = {
>>> +             .nchans = 4,
>>> +             .enable = 0x4,
>>> +             .muxtype = pca954x_ismux,
>>> +             .has_irq = 1,
>>> +             .id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
>>> +     },
>>>       [pca_9540] = {
>>>               .nchans = 2,
>>>               .enable = 0x4,
>>> @@ -177,6 +242,12 @@ static const struct chip_desc chips[] = {
>>>  };
>>>
>>>  static const struct i2c_device_id pca954x_id[] = {
>>> +     { "max7356", max_7356 },
>>> +     { "max7357", max_7357 },
>>> +     { "max7358", max_7358 },
>>> +     { "max7367", max_7367 },
>>> +     { "max7368", max_7368 },
>>> +     { "max7369", max_7369 },
>>>       { "pca9540", pca_9540 },
>>>       { "pca9542", pca_9542 },
>>>       { "pca9543", pca_9543 },
>>> @@ -194,6 +265,12 @@ static const struct i2c_device_id pca954x_id[] = {
>>>  MODULE_DEVICE_TABLE(i2c, pca954x_id);
>>>
>>>  static const struct of_device_id pca954x_of_match[] = {
>>> +     { .compatible = "maxim,max7356", .data = &chips[max_7356] },
>>> +     { .compatible = "maxim,max7357", .data = &chips[max_7357] },
>>> +     { .compatible = "maxim,max7358", .data = &chips[max_7358] },
>>> +     { .compatible = "maxim,max7367", .data = &chips[max_7367] },
>>> +     { .compatible = "maxim,max7368", .data = &chips[max_7368] },
>>> +     { .compatible = "maxim,max7369", .data = &chips[max_7369] },
>>>       { .compatible = "nxp,pca9540", .data = &chips[pca_9540] },
>>>       { .compatible = "nxp,pca9542", .data = &chips[pca_9542] },
>>>       { .compatible = "nxp,pca9543", .data = &chips[pca_9543] },
>>> @@ -401,9 +478,16 @@ static int pca954x_init(struct i2c_client *client, struct pca954x *data)
>>>       else
>>>               data->last_chan = 0; /* Disconnect multiplexer */
>>>
>>> -     ret = i2c_smbus_write_byte(client, data->last_chan);
>>> -     if (ret < 0)
>>> -             data->last_chan = 0;
>>> +     if (data->chip->max7357) {
>>> +             ret = i2c_smbus_write_byte_data(client, data->last_chan,
>>> +                                             MAX7357_CONF_DEFAULTS);
>>> +             if (ret < 0)
>>> +                     data->last_chan = 0;
>>> +     } else {
>>> +             ret = i2c_smbus_write_byte(client, data->last_chan);
>>> +             if (ret < 0)
>>> +                     data->last_chan = 0;
>>> +     }
>>>
>>>       return ret;
>>>  }
>>
>> The actual code is simple enough, and looks good.
>>
>> Cheers,
>> Peter