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
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 <[email protected]>
---
.../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 <[email protected]>
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
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 <[email protected]>
---
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 <[email protected]>
* Copyright (c) 2008-2009 Eurotech S.p.A. <[email protected]>
+ * Copyright (c) 2022 Patrick Rudolph <[email protected]>
*
* 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
Add a vdd regulator and enable it for boards that have the
mux powered off by default.
Signed-off-by: Patrick Rudolph <[email protected]>
---
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
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 <[email protected]>
> ---
> .../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.
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 <[email protected]>
Peter, are you happy with this patch series?
All the best,
Wolfram
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 <[email protected]>
> ---
> 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);
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 <[email protected]>
> ---
> 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 <[email protected]>
> * Copyright (c) 2008-2009 Eurotech S.p.A. <[email protected]>
> + * Copyright (c) 2022 Patrick Rudolph <[email protected]>
> *
> * 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
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 <[email protected]>
>> ---
>> 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 <[email protected]>
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);
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 <[email protected]> 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 <[email protected]>
>>> ---
>>> 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 <[email protected]>
>>> * Copyright (c) 2008-2009 Eurotech S.p.A. <[email protected]>
>>> + * Copyright (c) 2022 Patrick Rudolph <[email protected]>
>>> *
>>> * 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
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 <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
> > * Copyright (c) 2008-2009 Eurotech S.p.A. <[email protected]>
> > + * Copyright (c) 2022 Patrick Rudolph <[email protected]>
> > *
> > * 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