| Message ID | 20221007075354.568752-4-patrick.rudolph@9elements.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for Maxim MAX735x/MAX736x variants | expand |
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 >
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
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
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;
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(-)