From: Patrick Rudolph <[email protected]>
Maxim Max7357 has a configuration register to enable additional
features. These features aren't enabled by default & its up to
board designer to enable the same as it may have unexpected side effects.
These should be validated for proper functioning & detection of devices
in secondary bus as sometimes it can cause secondary bus being disabled.
Add booleans for:
- maxim,isolate-stuck-channel
- maxim,send-flush-out-sequence
- maxim,preconnection-wiggle-test-enable
Signed-off-by: Patrick Rudolph <[email protected]>
Signed-off-by: Naresh Solanki <[email protected]>
---
Changes in V4:
- Drop max7358.
Changes in V3:
- Update commit message
Changes in V2:
- Update properties.
---
.../bindings/i2c/i2c-mux-pca954x.yaml | 30 +++++++++++++++++++
1 file changed, 30 insertions(+)
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml
index 2d7bb998b0e9..9aa0585200c9 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml
@@ -71,6 +71,23 @@ properties:
description: A voltage regulator supplying power to the chip. On PCA9846
the regulator supplies power to VDD2 (core logic) and optionally to VDD1.
+ maxim,isolate-stuck-channel:
+ type: boolean
+ description: Allows to use non faulty channels while a stuck channel is
+ isolated from the upstream bus. If not set all channels are isolated from
+ the upstream bus until the fault is cleared.
+
+ maxim,send-flush-out-sequence:
+ type: boolean
+ description: Send a flush-out sequence to stuck auxiliary buses
+ automatically after a stuck channel is being detected.
+
+ maxim,preconnection-wiggle-test-enable:
+ type: boolean
+ description: Send a STOP condition to the auxiliary buses when the switch
+ register activates a channel to detect a stuck high fault. On fault the
+ channel is isolated from the upstream bus.
+
required:
- compatible
- reg
@@ -95,6 +112,19 @@ allOf:
"#interrupt-cells": false
interrupt-controller: false
+ - if:
+ not:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - maxim,max7357
+ then:
+ properties:
+ maxim,isolate-stuck-channel: false
+ maxim,send-flush-out-sequence: false
+ maxim,preconnection-wiggle-test-enable: false
+
unevaluatedProperties: false
examples:
base-commit: b5d463c0eecb4a690e631fa38cb6771a906f7620
--
2.41.0
From: Patrick Rudolph <[email protected]>
Enable additional features based on DT settings and unconditionally
release the shared interrupt pin after 1.6 seconds and allow to use
it as reset.
These features aren't enabled by default & its up to board designer
to enable the same as it may have unexpected side effects.
These should be validated for proper functioning & detection of devices
in secondary bus as sometimes it can cause secondary bus being disabled.
Signed-off-by: Patrick Rudolph <[email protected]>
Signed-off-by: Naresh Solanki <[email protected]>
---
Changes in V4:
- Drop max7358
- Update #define
- Move conf variable
- Print warning when I2C_FUNC_SMBUS_WRITE_BYTE_DATA isn't supported
Changes in V3:
- Delete unused #define
- Update pca954x_init
- Update commit message
Changes in V2:
- Update comments
- Update check for DT properties
---
drivers/i2c/muxes/i2c-mux-pca954x.c | 44 ++++++++++++++++++++++++++++-
1 file changed, 43 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 2219062104fb..f37ce332078c 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -57,6 +57,20 @@
#define PCA954X_IRQ_OFFSET 4
+/*
+ * MAX7357's configuration register is writeable after POR, but
+ * can be locked by setting the basic mode bit. MAX7358 configuration
+ * register is locked by default and needs to be unlocked first.
+ * The configuration register holds the following settings:
+ */
+#define MAX7357_CONF_INT_ENABLE BIT(0)
+#define MAX7357_CONF_FLUSH_OUT BIT(1)
+#define MAX7357_CONF_RELEASE_INT BIT(2)
+#define MAX7357_CONF_DISCON_SINGLE_CHAN BIT(4)
+#define MAX7357_CONF_PRECONNECT_TEST BIT(7)
+
+#define MAX7357_POR_DEFAULT_CONF MAX7357_CONF_INT_ENABLE
+
enum pca_type {
max_7356,
max_7357,
@@ -470,7 +484,35 @@ 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 (device_is_compatible(&client->dev, "maxim,max7357")) {
+ if (i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) {
+ u8 conf = MAX7357_POR_DEFAULT_CONF;
+ /*
+ * The interrupt signal is shared with the reset pin. Release the
+ * interrupt after 1.6 seconds to allow using the pin as reset.
+ * The interrupt isn't serviced yet.
+ */
+ conf |= MAX7357_CONF_RELEASE_INT;
+
+ if (device_property_read_bool(&client->dev, "maxim,isolate-stuck-channel"))
+ conf |= MAX7357_CONF_DISCON_SINGLE_CHAN;
+ if (device_property_read_bool(&client->dev,
+ "maxim,send-flush-out-sequence"))
+ conf |= MAX7357_CONF_FLUSH_OUT;
+ if (device_property_read_bool(&client->dev,
+ "maxim,preconnection-wiggle-test-enable"))
+ conf |= MAX7357_CONF_PRECONNECT_TEST;
+
+ ret = i2c_smbus_write_byte_data(client, data->last_chan, conf);
+ } else {
+ dev_warn(&client->dev,
+ "Write byte not supported. Cannot enable enhanced mode features");
+ 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.41.0
Hi Patrick,
On Thu, Oct 05, 2023 at 03:45:40PM +0200, Naresh Solanki wrote:
> From: Patrick Rudolph <[email protected]>
>
> Enable additional features based on DT settings and unconditionally
> release the shared interrupt pin after 1.6 seconds and allow to use
> it as reset.
>
> These features aren't enabled by default & its up to board designer
/&/and/
/its/it's/
> to enable the same as it may have unexpected side effects.
which side effects?
> These should be validated for proper functioning & detection of devices
/&/and/
it ain't WhatsApp :-)
> in secondary bus as sometimes it can cause secondary bus being disabled.
Is this latest sentence a related to this patch or is it a free
information?
> Signed-off-by: Patrick Rudolph <[email protected]>
> Signed-off-by: Naresh Solanki <[email protected]>
[...]
> else
> data->last_chan = 0; /* Disconnect multiplexer */
>
> - ret = i2c_smbus_write_byte(client, data->last_chan);
> + if (device_is_compatible(&client->dev, "maxim,max7357")) {
> + if (i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) {
> + u8 conf = MAX7357_POR_DEFAULT_CONF;
> + /*
> + * The interrupt signal is shared with the reset pin. Release the
> + * interrupt after 1.6 seconds to allow using the pin as reset.
> + * The interrupt isn't serviced yet.
> + */
> + conf |= MAX7357_CONF_RELEASE_INT;
This setting comes as default, what about the dedicated reset
gpio pin? How is the driver going to understand whether from the
irq pin is coming a reset or an interrupt?
> + if (device_property_read_bool(&client->dev, "maxim,isolate-stuck-channel"))
> + conf |= MAX7357_CONF_DISCON_SINGLE_CHAN;
> + if (device_property_read_bool(&client->dev,
> + "maxim,send-flush-out-sequence"))
> + conf |= MAX7357_CONF_FLUSH_OUT;
> + if (device_property_read_bool(&client->dev,
> + "maxim,preconnection-wiggle-test-enable"))
> + conf |= MAX7357_CONF_PRECONNECT_TEST;
How are these properties affecting the economy of the driver? Can
we have a brief description?
Andi
Hi!
2023-10-05 at 15:45, Naresh Solanki wrote:
> From: Patrick Rudolph <[email protected]>
>
> Enable additional features based on DT settings and unconditionally
> release the shared interrupt pin after 1.6 seconds and allow to use
> it as reset.
>
> These features aren't enabled by default & its up to board designer
> to enable the same as it may have unexpected side effects.
>
> These should be validated for proper functioning & detection of devices
> in secondary bus as sometimes it can cause secondary bus being disabled.
>
> Signed-off-by: Patrick Rudolph <[email protected]>
> Signed-off-by: Naresh Solanki <[email protected]>
> ---
> Changes in V4:
> - Drop max7358
> - Update #define
> - Move conf variable
> - Print warning when I2C_FUNC_SMBUS_WRITE_BYTE_DATA isn't supported
> Changes in V3:
> - Delete unused #define
> - Update pca954x_init
> - Update commit message
>
> Changes in V2:
> - Update comments
> - Update check for DT properties
> ---
> drivers/i2c/muxes/i2c-mux-pca954x.c | 44 ++++++++++++++++++++++++++++-
> 1 file changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 2219062104fb..f37ce332078c 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -57,6 +57,20 @@
>
> #define PCA954X_IRQ_OFFSET 4
>
> +/*
> + * MAX7357's configuration register is writeable after POR, but
> + * can be locked by setting the basic mode bit. MAX7358 configuration
> + * register is locked by default and needs to be unlocked first.
> + * The configuration register holds the following settings:
> + */
> +#define MAX7357_CONF_INT_ENABLE BIT(0)
> +#define MAX7357_CONF_FLUSH_OUT BIT(1)
> +#define MAX7357_CONF_RELEASE_INT BIT(2)
> +#define MAX7357_CONF_DISCON_SINGLE_CHAN BIT(4)
> +#define MAX7357_CONF_PRECONNECT_TEST BIT(7)
> +
> +#define MAX7357_POR_DEFAULT_CONF MAX7357_CONF_INT_ENABLE
> +
> enum pca_type {
> max_7356,
> max_7357,
> @@ -470,7 +484,35 @@ 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 (device_is_compatible(&client->dev, "maxim,max7357")) {
> + if (i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) {
> + u8 conf = MAX7357_POR_DEFAULT_CONF;
> + /*
> + * The interrupt signal is shared with the reset pin. Release the
> + * interrupt after 1.6 seconds to allow using the pin as reset.
> + * The interrupt isn't serviced yet.
I'd suggest dropping the word "yet". The interrupt isn't serviced for
max7357, period. The "yet" in combination with that 1.6 second window is
a bit cunfusing and readers might think that the interrupt is serviced
at some later stage or something, when I think the intention of "The
interrupt isn't serviced yet" comes with the silent implication that it
is simply not implemented yet.
> + */
> + conf |= MAX7357_CONF_RELEASE_INT;
> +
> + if (device_property_read_bool(&client->dev, "maxim,isolate-stuck-channel"))
> + conf |= MAX7357_CONF_DISCON_SINGLE_CHAN;
> + if (device_property_read_bool(&client->dev,
> + "maxim,send-flush-out-sequence"))
> + conf |= MAX7357_CONF_FLUSH_OUT;
> + if (device_property_read_bool(&client->dev,
> + "maxim,preconnection-wiggle-test-enable"))
> + conf |= MAX7357_CONF_PRECONNECT_TEST;
> +
> + ret = i2c_smbus_write_byte_data(client, data->last_chan, conf);
> + } else {
> + dev_warn(&client->dev,
> + "Write byte not supported. Cannot enable enhanced mode features");
Missing \n at the end of the string.
Cheers,
Peter
> + 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;
>
Hi again!
[sorry for replying to self]
2023-10-06 at 09:46, Peter Rosin wrote:
> 2023-10-05 at 15:45, Naresh Solanki wrote:
>> From: Patrick Rudolph <[email protected]>
>>
...
>> + dev_warn(&client->dev,
>> + "Write byte not supported. Cannot enable enhanced mode features");
>
> Missing \n at the end of the string.
>
Also, I2C_FUNC_SMBUS_WRITE_BYTE is separate from
I2C_FUNC_SMBUS_WRITE_BYTE_DATA, which makes that "Write byte"
misleading.
Cheers,
Peter
On Thu, 05 Oct 2023 15:45:39 +0200, Naresh Solanki wrote:
> From: Patrick Rudolph <[email protected]>
>
> Maxim Max7357 has a configuration register to enable additional
> features. These features aren't enabled by default & its up to
> board designer to enable the same as it may have unexpected side effects.
>
> These should be validated for proper functioning & detection of devices
> in secondary bus as sometimes it can cause secondary bus being disabled.
>
> Add booleans for:
> - maxim,isolate-stuck-channel
> - maxim,send-flush-out-sequence
> - maxim,preconnection-wiggle-test-enable
>
> Signed-off-by: Patrick Rudolph <[email protected]>
> Signed-off-by: Naresh Solanki <[email protected]>
> ---
> Changes in V4:
> - Drop max7358.
> Changes in V3:
> - Update commit message
> Changes in V2:
> - Update properties.
> ---
> .../bindings/i2c/i2c-mux-pca954x.yaml | 30 +++++++++++++++++++
> 1 file changed, 30 insertions(+)
>
Reviewed-by: Rob Herring <[email protected]>
Hi Andi,
On Fri, 6 Oct 2023 at 04:30, Andi Shyti <[email protected]> wrote:
>
> Hi Patrick,
>
> On Thu, Oct 05, 2023 at 03:45:40PM +0200, Naresh Solanki wrote:
> > From: Patrick Rudolph <[email protected]>
> >
> > Enable additional features based on DT settings and unconditionally
> > release the shared interrupt pin after 1.6 seconds and allow to use
> > it as reset.
> >
> > These features aren't enabled by default & its up to board designer
>
> /&/and/
> /its/it's/
Ack
>
> > to enable the same as it may have unexpected side effects.
>
> which side effects?
>
> > These should be validated for proper functioning & detection of devices
>
> /&/and/
>
> it ain't WhatsApp :-)
Ack
>
> > in secondary bus as sometimes it can cause secondary bus being disabled.
>
> Is this latest sentence a related to this patch or is it a free
> information?
Its related to the potential side effect of the feature if enabled.
>
> > Signed-off-by: Patrick Rudolph <[email protected]>
> > Signed-off-by: Naresh Solanki <[email protected]>
>
> [...]
>
> > else
> > data->last_chan = 0; /* Disconnect multiplexer */
> >
> > - ret = i2c_smbus_write_byte(client, data->last_chan);
> > + if (device_is_compatible(&client->dev, "maxim,max7357")) {
> > + if (i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) {
> > + u8 conf = MAX7357_POR_DEFAULT_CONF;
> > + /*
> > + * The interrupt signal is shared with the reset pin. Release the
> > + * interrupt after 1.6 seconds to allow using the pin as reset.
> > + * The interrupt isn't serviced yet.
> > + */
> > + conf |= MAX7357_CONF_RELEASE_INT;
>
> This setting comes as default, what about the dedicated reset
> gpio pin? How is the driver going to understand whether from the
> irq pin is coming a reset or an interrupt?
As per datasheet,
RELEASE_INT bit in conf is cleared at poweron.
The RST/INT is dual function pin.
>
> > + if (device_property_read_bool(&client->dev, "maxim,isolate-stuck-channel"))
> > + conf |= MAX7357_CONF_DISCON_SINGLE_CHAN;
> > + if (device_property_read_bool(&client->dev,
> > + "maxim,send-flush-out-sequence"))
> > + conf |= MAX7357_CONF_FLUSH_OUT;
> > + if (device_property_read_bool(&client->dev,
> > + "maxim,preconnection-wiggle-test-enable"))
> > + conf |= MAX7357_CONF_PRECONNECT_TEST;
>
> How are these properties affecting the economy of the driver? Can
> we have a brief description?
Not sure what you mean.
Are you asking for impact on the driver or feature description ?
Note: These are chip features & this patch is intended to enable them
based on DT settings.
Regards,
Naresh
>
> Andi
Hi
On Fri, 6 Oct 2023 at 13:16, Peter Rosin <[email protected]> wrote:
>
> Hi!
>
> 2023-10-05 at 15:45, Naresh Solanki wrote:
> > From: Patrick Rudolph <[email protected]>
> >
> > Enable additional features based on DT settings and unconditionally
> > release the shared interrupt pin after 1.6 seconds and allow to use
> > it as reset.
> >
> > These features aren't enabled by default & its up to board designer
> > to enable the same as it may have unexpected side effects.
> >
> > These should be validated for proper functioning & detection of devices
> > in secondary bus as sometimes it can cause secondary bus being disabled.
> >
> > Signed-off-by: Patrick Rudolph <[email protected]>
> > Signed-off-by: Naresh Solanki <[email protected]>
> > ---
> > Changes in V4:
> > - Drop max7358
> > - Update #define
> > - Move conf variable
> > - Print warning when I2C_FUNC_SMBUS_WRITE_BYTE_DATA isn't supported
> > Changes in V3:
> > - Delete unused #define
> > - Update pca954x_init
> > - Update commit message
> >
> > Changes in V2:
> > - Update comments
> > - Update check for DT properties
> > ---
> > drivers/i2c/muxes/i2c-mux-pca954x.c | 44 ++++++++++++++++++++++++++++-
> > 1 file changed, 43 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> > index 2219062104fb..f37ce332078c 100644
> > --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> > +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> > @@ -57,6 +57,20 @@
> >
> > #define PCA954X_IRQ_OFFSET 4
> >
> > +/*
> > + * MAX7357's configuration register is writeable after POR, but
> > + * can be locked by setting the basic mode bit. MAX7358 configuration
> > + * register is locked by default and needs to be unlocked first.
> > + * The configuration register holds the following settings:
> > + */
> > +#define MAX7357_CONF_INT_ENABLE BIT(0)
> > +#define MAX7357_CONF_FLUSH_OUT BIT(1)
> > +#define MAX7357_CONF_RELEASE_INT BIT(2)
> > +#define MAX7357_CONF_DISCON_SINGLE_CHAN BIT(4)
> > +#define MAX7357_CONF_PRECONNECT_TEST BIT(7)
> > +
> > +#define MAX7357_POR_DEFAULT_CONF MAX7357_CONF_INT_ENABLE
> > +
> > enum pca_type {
> > max_7356,
> > max_7357,
> > @@ -470,7 +484,35 @@ 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 (device_is_compatible(&client->dev, "maxim,max7357")) {
> > + if (i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) {
> > + u8 conf = MAX7357_POR_DEFAULT_CONF;
> > + /*
> > + * The interrupt signal is shared with the reset pin. Release the
> > + * interrupt after 1.6 seconds to allow using the pin as reset.
> > + * The interrupt isn't serviced yet.
>
> I'd suggest dropping the word "yet". The interrupt isn't serviced for
> max7357, period. The "yet" in combination with that 1.6 second window is
> a bit cunfusing and readers might think that the interrupt is serviced
> at some later stage or something, when I think the intention of "The
> interrupt isn't serviced yet" comes with the silent implication that it
> is simply not implemented yet.
Sure.
>
> > + */
> > + conf |= MAX7357_CONF_RELEASE_INT;
> > +
> > + if (device_property_read_bool(&client->dev, "maxim,isolate-stuck-channel"))
> > + conf |= MAX7357_CONF_DISCON_SINGLE_CHAN;
> > + if (device_property_read_bool(&client->dev,
> > + "maxim,send-flush-out-sequence"))
> > + conf |= MAX7357_CONF_FLUSH_OUT;
> > + if (device_property_read_bool(&client->dev,
> > + "maxim,preconnection-wiggle-test-enable"))
> > + conf |= MAX7357_CONF_PRECONNECT_TEST;
> > +
> > + ret = i2c_smbus_write_byte_data(client, data->last_chan, conf);
> > + } else {
> > + dev_warn(&client->dev,
> > + "Write byte not supported. Cannot enable enhanced mode features");
>
> Missing \n at the end of the string.
Sure. Will also update it as
'Write byte data not supported. Cannot enable enhanced mode features\n'
Regards,
Naresh
>
> Cheers,
> Peter
>
> > + 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;
> >