[v9 0/4] Add support for Maxim MAX735x/MAX736x variants

[v9 0/4] Add support for Maxim MAX735x/MAX736x variants

[Patrick Rudolph]

v9:
- Fix 'then' not aligned with 'if' in dt-bindings
- Split enhanced mode configuration into separate patch
- Add MAX7357/MAX7358 register definitions
- Rename config register defines
- Update comments and explain non default config being applied on MAX7357
- Check for I2C_FUNC_SMBUS_WRITE_BYTE_DATA functionality

v8:
- Move allOf in dt-binding and use double negation

v7:
- Reworked the commit message, comments and renamed a struct
  field. No functional change.

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 (4):
  dt-bindings: i2c: Add Maxim MAX735x/MAX736x variants
  i2c: muxes: pca954x: Add MAX735x/MAX736x support
  i2c: muxes: pca954x: Configure MAX7357 in enhanced mode
  i2c: muxes: pca954x: Add regulator support

 .../bindings/i2c/i2c-mux-pca954x.yaml         |  39 ++++-
 drivers/i2c/muxes/Kconfig                     |   6 +-
 drivers/i2c/muxes/i2c-mux-pca954x.c           | 140 +++++++++++++++++-
 3 files changed, 170 insertions(+), 15 deletions(-)

-- 
2.37.3

[v9 1/4] 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         | 39 ++++++++++++++++---
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml
index 9f1726d0356b..efad0a95806f 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml
@@ -4,21 +4,25 @@
 $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.
-
-allOf:
-  - $ref: /schemas/i2c/i2c-mux.yaml#
+  The binding supports NXP PCA954x and PCA984x I2C mux/switch devices,
+  and the Maxim MAX735x and MAX736x I2C mux/switch devices.
 
 properties:
   compatible:
     oneOf:
       - enum:
+          - maxim,max7356
+          - maxim,max7357
+          - maxim,max7358
+          - maxim,max7367
+          - maxim,max7368
+          - maxim,max7369
           - nxp,pca9540
           - nxp,pca9542
           - nxp,pca9543
@@ -59,10 +63,33 @@ 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
 
+allOf:
+  - $ref: /schemas/i2c/i2c-mux.yaml#
+  - if:
+      not:
+        properties:
+          compatible:
+            contains:
+              enum:
+                - maxim,max7367
+                - maxim,max7369
+                - nxp,pca9542
+                - nxp,pca9543
+                - nxp,pca9544
+                - nxp,pca9545
+    then:
+      properties:
+        interrupts: false
+        "#interrupt-cells": false
+        interrupt-controller: false
+
 unevaluatedProperties: false
 
 examples:
@@ -79,6 +106,8 @@ examples:
             #size-cells = <0>;
             reg = <0x74>;
 
+            vdd-supply = <&p3v3>;
+
             interrupt-parent = <&ipic>;
             interrupts = <17 IRQ_TYPE_LEVEL_LOW>;
             interrupt-controller;
-- 
2.37.3

[v9 2/4] 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.

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           |  6 +--
 drivers/i2c/muxes/i2c-mux-pca954x.c | 60 ++++++++++++++++++++++++++++-
 2 files changed, 62 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
index ea838dbae32e..db1b9057612a 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.
+	  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 a5f458b635df..4b63b1eb669e 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 9elements GmbH <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
@@ -51,6 +58,12 @@
 #define PCA954X_IRQ_OFFSET 4
 
 enum pca_type {
+	max_7367,
+	max_7368,
+	max_7369,
+	max_7356,
+	max_7357,
+	max_7358,
 	pca_9540,
 	pca_9542,
 	pca_9543,
@@ -90,8 +103,41 @@ struct pca954x {
 	raw_spinlock_t lock;
 };
 
-/* Provide specs for the PCA954x types we know about */
+/* Provide specs for the MAX735x, PCA954x and PCA984x 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,
+		.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 +223,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 +246,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] },
-- 
2.37.3

[v9 3/4] i2c: muxes: pca954x: Configure MAX7357 in enhanced mode

[Patrick Rudolph]

The MAX7357 and MAX7358 have 6 additional registers called enhanced mode
in the following paragraphs. While the MAX7357 exposes those registers
without a special sequence, the MAX7358 requires an unlock sequence.
The enhanced mode allows to configure optional features which are nice to
have on an I2C mux, but are not mandatory for it's general operation.

As I don't have a MAX7358 for testing the special unlock sequence the
enhanced mode isn't used on the MAX7358, but it could be added later
if required.

The MAX7357 enhanced mode is used to configure the chip to
 - Disable interrupts on bus locked up detection
 - Enable bus locked-up clearing
 - Disconnect only locked bus instead of all channels

This configuration protects the I2C tree from total failure and attempts
to unbrick the faulty bus. It's unclear why this isn't the default
configuration.

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/i2c-mux-pca954x.c | 46 ++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 4b63b1eb669e..992976fa6798 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -57,6 +57,37 @@
 
 #define PCA954X_IRQ_OFFSET 4
 
+/*
+ * The MAX7357 and MAX7358 have 6 additional registers called enhanced mode
+ * in the following paragraphs. While the MAX7357 exposes those registers
+ * without a special sequence, the MAX7358 requires an unlock sequence.
+ *
+ * The first enhanced mode register called CONF allows to configure
+ * additional features.
+ */
+#define MAX7357_REG_SWITCH				0
+#define MAX7357_REG_CONF				1
+#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_ON_READ		BIT(3)
+#define  MAX7357_CONF_DISCON_SINGLE_CHAN		BIT(4)
+#define  MAX7357_CONF_BUS_LOCKUP_DETECTION_DISABLE	BIT(5)
+#define  MAX7357_CONF_ENABLE_BASIC_MODE			BIT(6)
+#define  MAX7357_CONF_PRECONNECT_TEST			BIT(7)
+
+/*
+ * On boot the MAX735x behave like a regular MUX. Apply a fixed
+ * default configuration on MAX7357 that:
+ * - disables interrupts
+ * - sents automatically flush-out sequence on locked-up channels
+     when a lock-up condition is detected
+ * - isolates only the locked channel instead of all channels
+ * - doesn't disable bus lock-up detection.
+ */
+#define MAX7357_CONF_DEFAULTS (MAX7357_CONF_FLUSH_OUT | \
+	 MAX7357_CONF_DISCON_SINGLE_CHAN)
+
 enum pca_type {
 	max_7367,
 	max_7368,
@@ -82,6 +113,7 @@ struct chip_desc {
 	u8 nchans;
 	u8 enable;	/* used for muxes only */
 	u8 has_irq;
+	u8 maxim_enhanced_mode;
 	enum muxtype {
 		pca954x_ismux = 0,
 		pca954x_isswi
@@ -113,6 +145,7 @@ static const struct chip_desc chips[] = {
 	[max_7357] = {
 		.nchans = 8,
 		.muxtype = pca954x_isswi,
+		.maxim_enhanced_mode = 1,
 		.id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
 	},
 	[max_7358] = {
@@ -452,6 +485,7 @@ static void pca954x_cleanup(struct i2c_mux_core *muxc)
 
 static int pca954x_init(struct i2c_client *client, struct pca954x *data)
 {
+	struct i2c_adapter *adap = client->adapter;
 	int ret;
 
 	if (data->idle_state >= 0)
@@ -459,7 +493,17 @@ 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 (data->chip->maxim_enhanced_mode) {
+		if (i2c_check_functionality(adap, I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) {
+			ret = i2c_smbus_write_byte_data(client, data->last_chan,
+							MAX7357_CONF_DEFAULTS);
+		} else {
+			dev_warn(&client->dev, "Didn't configure enhanced defaults\n");
+			ret = i2c_smbus_write_byte(client, data->last_chan);
+		}
+	} else {
+		ret = i2c_smbus_write_byte(client, data->last_chan);
+	}
 	if (ret < 0)
 		data->last_chan = 0;
 
-- 
2.37.3

[v9 4/4] 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>
Reviewed-by: Peter Rosin <peda@axentia.se>
---
 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 992976fa6798..857a4ec387be 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>
@@ -133,6 +134,7 @@ struct pca954x {
 	struct irq_domain *irq;
 	unsigned int irq_mask;
 	raw_spinlock_t lock;
+	struct regulator *supply;
 };
 
 /* Provide specs for the MAX735x, PCA954x and PCA984x types we know about */
@@ -473,6 +475,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);
@@ -531,15 +536,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);
@@ -556,7 +578,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 ||
@@ -564,7 +586,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;
 		}
 	}
 
@@ -583,7 +606,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.37.3

Re: [v9 1/4] dt-bindings: i2c: Add Maxim MAX735x/MAX736x variants

[Serge Semin]

On Fri, Oct 07, 2022 at 09:53:50AM +0200, 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         | 39 ++++++++++++++++---
>  1 file changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml
> index 9f1726d0356b..efad0a95806f 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml
> @@ -4,21 +4,25 @@
>  $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.
> -

> -allOf:
> -  - $ref: /schemas/i2c/i2c-mux.yaml#

Why do you move the allOf statement to the bottom of the schema?

> +  The binding supports NXP PCA954x and PCA984x I2C mux/switch devices,
> +  and the Maxim MAX735x and MAX736x I2C mux/switch devices.

What about combining the sentence: "The binding supports NXP
PCA954x/PCA984x and Maxim MAX735x/MAX736x I2C mux/switch devices." ?
Currently it does look a bit bulky.

>  
>  properties:
>    compatible:
>      oneOf:
>        - enum:
> +          - maxim,max7356
> +          - maxim,max7357
> +          - maxim,max7358
> +          - maxim,max7367
> +          - maxim,max7368
> +          - maxim,max7369
>            - nxp,pca9540
>            - nxp,pca9542
>            - nxp,pca9543
> @@ -59,10 +63,33 @@ 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
>  
> +allOf:
> +  - $ref: /schemas/i2c/i2c-mux.yaml#
> +  - if:
> +      not:
> +        properties:
> +          compatible:
> +            contains:
> +              enum:
> +                - maxim,max7367
> +                - maxim,max7369
> +                - nxp,pca9542
> +                - nxp,pca9543
> +                - nxp,pca9544
> +                - nxp,pca9545
> +    then:

> +      properties:
> +        interrupts: false
> +        "#interrupt-cells": false
> +        interrupt-controller: false

I'd suggest to add an opposite definition. Evaluate the properties for
the devices which expect them being evaluated instead of falsing their
existence for the devices which don't support the interrupts.

-Sergey

> +
>  unevaluatedProperties: false
>  
>  examples:
> @@ -79,6 +106,8 @@ examples:
>              #size-cells = <0>;
>              reg = <0x74>;
>  
> +            vdd-supply = <&p3v3>;
> +
>              interrupt-parent = <&ipic>;
>              interrupts = <17 IRQ_TYPE_LEVEL_LOW>;
>              interrupt-controller;
> -- 
> 2.37.3
> 

Re: [v9 3/4] i2c: muxes: pca954x: Configure MAX7357 in enhanced mode

[Serge Semin]

On Fri, Oct 07, 2022 at 09:53:52AM +0200, Patrick Rudolph wrote:
> The MAX7357 and MAX7358 have 6 additional registers called enhanced mode
> in the following paragraphs. While the MAX7357 exposes those registers
> without a special sequence, the MAX7358 requires an unlock sequence.
> The enhanced mode allows to configure optional features which are nice to
> have on an I2C mux, but are not mandatory for it's general operation.
> 

> As I don't have a MAX7358 for testing the special unlock sequence the
> enhanced mode isn't used on the MAX7358, but it could be added later
> if required.

Not that hard to do. Just place four I2C_SMBUS_QUICK messages in a
single transfer:
S Addr Wr [A] Sr Addr Rd [A] Sr Addr Wr [A] Sr Addr Rd [A] P
it can be easily done by means of the i2c_transfer() method called
with four i2c_msg instances (Wr/Rd/Wr/Rd with zero length) passed.
See the way the quicks smbus-messages are implemented in
i2c_smbus_xfer_emulated((). Just place four if them in single array
and pass to the i2c_transfer() method.

Note some drivers may unsupport the I2C-level transfers. Also note
some adapters may unsupport the zero-length I2C-transfers. AFAIR we
had such problem with the Synopsys DW I2C controller.

> 
> The MAX7357 enhanced mode is used to configure the chip to
>  - Disable interrupts on bus locked up detection
>  - Enable bus locked-up clearing
>  - Disconnect only locked bus instead of all channels
> 
> This configuration protects the I2C tree from total failure and attempts
> to unbrick the faulty bus. It's unclear why this isn't the default
> configuration.
> 
> 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/i2c-mux-pca954x.c | 46 ++++++++++++++++++++++++++++-
>  1 file changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 4b63b1eb669e..992976fa6798 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -57,6 +57,37 @@
>  
>  #define PCA954X_IRQ_OFFSET 4
>  
> +/*
> + * The MAX7357 and MAX7358 have 6 additional registers called enhanced mode
> + * in the following paragraphs. While the MAX7357 exposes those registers
> + * without a special sequence, the MAX7358 requires an unlock sequence.
> + *
> + * The first enhanced mode register called CONF allows to configure
> + * additional features.
> + */
> +#define MAX7357_REG_SWITCH				0
> +#define MAX7357_REG_CONF				1
> +#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_ON_READ		BIT(3)
> +#define  MAX7357_CONF_DISCON_SINGLE_CHAN		BIT(4)
> +#define  MAX7357_CONF_BUS_LOCKUP_DETECTION_DISABLE	BIT(5)
> +#define  MAX7357_CONF_ENABLE_BASIC_MODE			BIT(6)
> +#define  MAX7357_CONF_PRECONNECT_TEST			BIT(7)
> +
> +/*
> + * On boot the MAX735x behave like a regular MUX. Apply a fixed
> + * default configuration on MAX7357 that:
> + * - disables interrupts
> + * - sents automatically flush-out sequence on locked-up channels
> +     when a lock-up condition is detected
> + * - isolates only the locked channel instead of all channels
> + * - doesn't disable bus lock-up detection.
> + */

> +#define MAX7357_CONF_DEFAULTS (MAX7357_CONF_FLUSH_OUT | \
> +	 MAX7357_CONF_DISCON_SINGLE_CHAN)

Moving the macro definition fully to the new line might look a bit
nicer:

+#define MAX7357_CONF_DEFAULTS \
	(MAX7357_CONF_FLUSH_OUT | MAX7357_CONF_DISCON_SINGLE_CHAN)

> +
>  enum pca_type {
>  	max_7367,
>  	max_7368,
> @@ -82,6 +113,7 @@ struct chip_desc {
>  	u8 nchans;
>  	u8 enable;	/* used for muxes only */
>  	u8 has_irq;

> +	u8 maxim_enhanced_mode;

So long name.( What about a shorter version, i.e. max(im)?_enh ?

BTW how to differentiate the devices with the enhanced mode
enabled/disabled by default?

-Sergey

>  	enum muxtype {
>  		pca954x_ismux = 0,
>  		pca954x_isswi
> @@ -113,6 +145,7 @@ static const struct chip_desc chips[] = {
>  	[max_7357] = {
>  		.nchans = 8,
>  		.muxtype = pca954x_isswi,
> +		.maxim_enhanced_mode = 1,
>  		.id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
>  	},
>  	[max_7358] = {
> @@ -452,6 +485,7 @@ static void pca954x_cleanup(struct i2c_mux_core *muxc)
>  
>  static int pca954x_init(struct i2c_client *client, struct pca954x *data)
>  {
> +	struct i2c_adapter *adap = client->adapter;
>  	int ret;
>  
>  	if (data->idle_state >= 0)
> @@ -459,7 +493,17 @@ 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 (data->chip->maxim_enhanced_mode) {
> +		if (i2c_check_functionality(adap, I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) {
> +			ret = i2c_smbus_write_byte_data(client, data->last_chan,
> +							MAX7357_CONF_DEFAULTS);
> +		} else {
> +			dev_warn(&client->dev, "Didn't configure enhanced defaults\n");
> +			ret = i2c_smbus_write_byte(client, data->last_chan);
> +		}
> +	} else {
> +		ret = i2c_smbus_write_byte(client, data->last_chan);
> +	}
>  	if (ret < 0)
>  		data->last_chan = 0;
>  
> -- 
> 2.37.3
> 

Re: [v9 4/4] i2c: muxes: pca954x: Add regulator support

[Serge Semin]

On Fri, Oct 07, 2022 at 09:53:53AM +0200, 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>
> Reviewed-by: Peter Rosin <peda@axentia.se>
> ---
>  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 992976fa6798..857a4ec387be 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>
> @@ -133,6 +134,7 @@ struct pca954x {
>  	struct irq_domain *irq;
>  	unsigned int irq_mask;
>  	raw_spinlock_t lock;
> +	struct regulator *supply;
>  };
>  
>  /* Provide specs for the MAX735x, PCA954x and PCA984x types we know about */
> @@ -473,6 +475,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))

First of all AFAICS the data->supply pointer will never be null on the
pca954x_cleanup() invocations in your current implementation. So
IS_ERR() would be enough here. Second in the next comment I'll suggest
to you to implement the optional regulator semantic, which implies
initializing the data->supply pointer with NULL if the get-regulator
function returns -ENODEV. That shall look easier than the IS_ERR()
macro. So checking the data->supply pointer for being not-null would
be enough here.

> +		regulator_disable(data->supply);
> +
>  	if (data->irq) {
>  		for (c = 0; c < data->chip->nchans; c++) {
>  			irq = irq_find_mapping(data->irq, c);
> @@ -531,15 +536,32 @@ static int pca954x_probe(struct i2c_client *client,
>  			     pca954x_select_chan, pca954x_deselect_mux);
>  	if (!muxc)
>  		return -ENOMEM;

> +

unrelated change...

>  	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)) {

Judging by the DT-bindings the power-supply is supposed to be
optional. Isn't it?  AFAICS from the _regulator_get() semantic if no
vdd-supply is specified and a regulator request method with the
non-optional semantic is called an ugly warning will be printed to the
system log. Most of the users of the driver don't have the
power-supply specified for the device. You don't want to have their
logs polluted with the false warning, do you? If so you should use
the devm_regulator_get_optional() method here. If it returns the
-ENODEV error just overwrite the data->supply with NULL. In case of
any other error halt the device probe procedure.

> +		ret = PTR_ERR(data->supply);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "Failed to request regulator: %d\n", ret);
> +		return ret;

dev_err_probe() ?

-Sergey

> +	}
> +
> +	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);
> @@ -556,7 +578,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 ||
> @@ -564,7 +586,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;
>  		}
>  	}
>  
> @@ -583,7 +606,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.37.3
> 

Re: [v9 1/4] dt-bindings: i2c: Add Maxim MAX735x/MAX736x variants

[Krzysztof Kozlowski]

On 08/10/2022 13:50, Serge Semin wrote:
> On Fri, Oct 07, 2022 at 09:53:50AM +0200, 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         | 39 ++++++++++++++++---
>>  1 file changed, 34 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml
>> index 9f1726d0356b..efad0a95806f 100644
>> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml
>> @@ -4,21 +4,25 @@
>>  $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.
>> -
> 
>> -allOf:
>> -  - $ref: /schemas/i2c/i2c-mux.yaml#
> 
> Why do you move the allOf statement to the bottom of the schema?

Because it goes with 'ifs' at the bottom of the schema...

> 
>> +  The binding supports NXP PCA954x and PCA984x I2C mux/switch devices,
>> +  and the Maxim MAX735x and MAX736x I2C mux/switch devices.
> 
> What about combining the sentence: "The binding supports NXP
> PCA954x/PCA984x and Maxim MAX735x/MAX736x I2C mux/switch devices." ?
> Currently it does look a bit bulky.

Drop "The binding supports". Instead describe the hardware.

> 
>>  
>>  properties:
>>    compatible:
>>      oneOf:
>>        - enum:
>> +          - maxim,max7356
>> +          - maxim,max7357
>> +          - maxim,max7358
>> +          - maxim,max7367
>> +          - maxim,max7368
>> +          - maxim,max7369
>>            - nxp,pca9540
>>            - nxp,pca9542
>>            - nxp,pca9543
>> @@ -59,10 +63,33 @@ 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
>>  
>> +allOf:
>> +  - $ref: /schemas/i2c/i2c-mux.yaml#
>> +  - if:
>> +      not:
>> +        properties:
>> +          compatible:
>> +            contains:
>> +              enum:
>> +                - maxim,max7367
>> +                - maxim,max7369
>> +                - nxp,pca9542
>> +                - nxp,pca9543
>> +                - nxp,pca9544
>> +                - nxp,pca9545
>> +    then:
> 
>> +      properties:
>> +        interrupts: false
>> +        "#interrupt-cells": false
>> +        interrupt-controller: false
> 
> I'd suggest to add an opposite definition. Evaluate the properties for
> the devices which expect them being evaluated instead of falsing their
> existence for the devices which don't support the interrupts.

The properties rather should be defined in top-level than in "if", so I
am not sure how would you want to achieve opposite way.


Best regards,
Krzysztof

Re: [v9 3/4] i2c: muxes: pca954x: Configure MAX7357 in enhanced mode

[Peter Rosin]

2022-10-08 at 14:54, Serge Semin wrote:
> On Fri, Oct 07, 2022 at 09:53:52AM +0200, Patrick Rudolph wrote:
>> +	u8 maxim_enhanced_mode;
> 
> So long name.( What about a shorter version, i.e. max(im)?_enh ?

No thank you, please keep the long name as is. This is a corner
case and the name is not repeated that many times. Spelling it
out makes the code more readable.

Cheers,
Peter

Re: [v9 1/4] dt-bindings: i2c: Add Maxim MAX735x/MAX736x variants

[Serge Semin]

On Sun, Oct 09, 2022 at 05:25:22PM +0200, Krzysztof Kozlowski wrote:
> On 08/10/2022 13:50, Serge Semin wrote:
> > On Fri, Oct 07, 2022 at 09:53:50AM +0200, 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         | 39 ++++++++++++++++---
> >>  1 file changed, 34 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml
> >> index 9f1726d0356b..efad0a95806f 100644
> >> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml
> >> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml
> >> @@ -4,21 +4,25 @@
> >>  $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.
> >> -
> > 
> >> -allOf:
> >> -  - $ref: /schemas/i2c/i2c-mux.yaml#
> > 
> > Why do you move the allOf statement to the bottom of the schema?
> 

> Because it goes with 'ifs' at the bottom of the schema...

Is there a requirement to move the allOf array to the bottom of the
schema if it contains the 'if' statement? If only there were some
kernel doc with all such implicit conventions...

> 
> > 
> >> +  The binding supports NXP PCA954x and PCA984x I2C mux/switch devices,
> >> +  and the Maxim MAX735x and MAX736x I2C mux/switch devices.
> > 
> > What about combining the sentence: "The binding supports NXP
> > PCA954x/PCA984x and Maxim MAX735x/MAX736x I2C mux/switch devices." ?
> > Currently it does look a bit bulky.
> 
> Drop "The binding supports". Instead describe the hardware.
> 
> > 
> >>  
> >>  properties:
> >>    compatible:
> >>      oneOf:
> >>        - enum:
> >> +          - maxim,max7356
> >> +          - maxim,max7357
> >> +          - maxim,max7358
> >> +          - maxim,max7367
> >> +          - maxim,max7368
> >> +          - maxim,max7369
> >>            - nxp,pca9540
> >>            - nxp,pca9542
> >>            - nxp,pca9543
> >> @@ -59,10 +63,33 @@ 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
> >>  
> >> +allOf:
> >> +  - $ref: /schemas/i2c/i2c-mux.yaml#
> >> +  - if:
> >> +      not:
> >> +        properties:
> >> +          compatible:
> >> +            contains:
> >> +              enum:
> >> +                - maxim,max7367
> >> +                - maxim,max7369
> >> +                - nxp,pca9542
> >> +                - nxp,pca9543
> >> +                - nxp,pca9544
> >> +                - nxp,pca9545
> >> +    then:
> > 
> >> +      properties:
> >> +        interrupts: false
> >> +        "#interrupt-cells": false
> >> +        interrupt-controller: false
> > 
> > I'd suggest to add an opposite definition. Evaluate the properties for
> > the devices which expect them being evaluated instead of falsing their
> > existence for the devices which don't support the interrupts.
> 

> The properties rather should be defined in top-level than in "if", so I
> am not sure how would you want to achieve opposite way.

With one more implicit convention like "preferably define the
properties in the top-level than in if" of course I can't. Otherwise I
thought something like this would work:
+allOf:
+  - ...
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum: [...]
+    then:
+      properties:
+        interrupts: ...
+        "#interrupt-cells": ...
+        interrupt-controller: ...
...
-  interrupts:
-  "#interrupt-cells":
-  interrupt-controller: ...

With unevaluatedProperties set to false and evaluation performed for
the particular compatibles such schema shall work with the same
semantic.

-Sergey

> 
> 
> Best regards,
> Krzysztof
> 

Re: [v9 3/4] i2c: muxes: pca954x: Configure MAX7357 in enhanced mode

[Serge Semin]

On Sun, Oct 09, 2022 at 06:36:52PM +0200, Peter Rosin wrote:
> 2022-10-08 at 14:54, Serge Semin wrote:
> > On Fri, Oct 07, 2022 at 09:53:52AM +0200, Patrick Rudolph wrote:
> >> +	u8 maxim_enhanced_mode;
> > 

> > So long name.( What about a shorter version, i.e. max(im)?_enh ?
> 
> No thank you, please keep the long name as is. This is a corner
> case and the name is not repeated that many times. Spelling it
> out makes the code more readable.

I don't insist. It was just a suggestion.

Anyway seeing there are going to be two variables with the flag
semantic (has_irq and maxim_enhanced_mode) it would be better to
convert them to a single quirk field. Moreover it will be useful
taking into account that a single maxim_enhanced_mode flag can't be
used to distinguish the Maxim I2C-muxes with the enhanced mode
disabled by default. Thus another flag will be needed for such
devices.

One more thing. Using u8 type for the flag variables isn't that
descriptive. It should be of the boolean type.

-Sergey

> 
> Cheers,
> Peter

Re: [v9 1/4] dt-bindings: i2c: Add Maxim MAX735x/MAX736x variants

[Krzysztof Kozlowski]

On 09/10/2022 14:03, Serge Semin wrote:
> On Sun, Oct 09, 2022 at 05:25:22PM +0200, Krzysztof Kozlowski wrote:
>> On 08/10/2022 13:50, Serge Semin wrote:
>>> On Fri, Oct 07, 2022 at 09:53:50AM +0200, 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         | 39 ++++++++++++++++---
>>>>  1 file changed, 34 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml
>>>> index 9f1726d0356b..efad0a95806f 100644
>>>> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml
>>>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml
>>>> @@ -4,21 +4,25 @@
>>>>  $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.
>>>> -
>>>
>>>> -allOf:
>>>> -  - $ref: /schemas/i2c/i2c-mux.yaml#
>>>
>>> Why do you move the allOf statement to the bottom of the schema?
>>
> 
>> Because it goes with 'ifs' at the bottom of the schema...
> 
> Is there a requirement to move the allOf array to the bottom of the
> schema if it contains the 'if' statement? If only there were some
> kernel doc with all such implicit conventions...

It's just a convention, although quite logical because "ifs" can grow
significantly, so putting it before properties is outside of context.
Reader does not know yet to what this if applies.

> 
>>
>>>
>>>> +  The binding supports NXP PCA954x and PCA984x I2C mux/switch devices,
>>>> +  and the Maxim MAX735x and MAX736x I2C mux/switch devices.
>>>
>>> What about combining the sentence: "The binding supports NXP
>>> PCA954x/PCA984x and Maxim MAX735x/MAX736x I2C mux/switch devices." ?
>>> Currently it does look a bit bulky.
>>
>> Drop "The binding supports". Instead describe the hardware.
>>
>>>
>>>>  
>>>>  properties:
>>>>    compatible:
>>>>      oneOf:
>>>>        - enum:
>>>> +          - maxim,max7356
>>>> +          - maxim,max7357
>>>> +          - maxim,max7358
>>>> +          - maxim,max7367
>>>> +          - maxim,max7368
>>>> +          - maxim,max7369
>>>>            - nxp,pca9540
>>>>            - nxp,pca9542
>>>>            - nxp,pca9543
>>>> @@ -59,10 +63,33 @@ 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
>>>>  
>>>> +allOf:
>>>> +  - $ref: /schemas/i2c/i2c-mux.yaml#
>>>> +  - if:
>>>> +      not:
>>>> +        properties:
>>>> +          compatible:
>>>> +            contains:
>>>> +              enum:
>>>> +                - maxim,max7367
>>>> +                - maxim,max7369
>>>> +                - nxp,pca9542
>>>> +                - nxp,pca9543
>>>> +                - nxp,pca9544
>>>> +                - nxp,pca9545
>>>> +    then:
>>>
>>>> +      properties:
>>>> +        interrupts: false
>>>> +        "#interrupt-cells": false
>>>> +        interrupt-controller: false
>>>
>>> I'd suggest to add an opposite definition. Evaluate the properties for
>>> the devices which expect them being evaluated instead of falsing their
>>> existence for the devices which don't support the interrupts.
>>
> 
>> The properties rather should be defined in top-level than in "if", so I
>> am not sure how would you want to achieve opposite way.
> 
> With one more implicit convention like "preferably define the
> properties in the top-level than in if" of course I can't. Otherwise I
> thought something like this would work:
> +allOf:
> +  - ...
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum: [...]
> +    then:
> +      properties:
> +        interrupts: ...
> +        "#interrupt-cells": ...
> +        interrupt-controller: ...
> ...
> -  interrupts:
> -  "#interrupt-cells":
> -  interrupt-controller: ...
> 
> With unevaluatedProperties set to false and evaluation performed for
> the particular compatibles such schema shall work with the same
> semantic.

Yes, this will work, but defining properties inside "if" is usually not
readable.

Best regards,
Krzysztof