Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 8 Oct 2022 15:54:36 +0300
From:   Serge Semin <fancer.lancer@gmail.com>
To:     Patrick Rudolph <patrick.rudolph@9elements.com>
Cc:     Peter Rosin <peda@axentia.se>, robh@kernel.org,
        laurent.pinchart@ideasonboard.com, wsa@kernel.org,
        linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [v9 3/4] i2c: muxes: pca954x: Configure MAX7357 in enhanced mode
Message-ID: <20221008125436.ndj2nwesx5lgppsf@mobilestation>
References: <20221007075354.568752-1-patrick.rudolph@9elements.com>
 <20221007075354.568752-4-patrick.rudolph@9elements.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221007075354.568752-4-patrick.rudolph@9elements.com>
Precedence: bulk

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
>