Re: [PATCH 2/2] i2c: muxes: Enable features on MAX7357

[Andi Shyti]

Hi Naresh,

On Wed, Aug 30, 2023 at 01:57:43PM +0200, Naresh Solanki wrote:
> From: Patrick Rudolph <patrick.rudolph@xxxxxxxxxxxxx>
> 
> Detect that max7357 is being used and run custom init sequence.
> 
> By default MAX7357 disconnects all channels on a bus lock-up and
> signals	this condition to the bus master using an interrupt.

please replace this tab with a space.

> Disable the interrupt as it's not useful within the kernel and
> it might conflict with the reset functionality that shares the same
> pin.
> 
> Use the introduced 'maxim,bus-lockup-fix' property to enable
> faulty channel isolation and flush-out sequence generation.
> 
> Signed-off-by: Patrick Rudolph <patrick.rudolph@xxxxxxxxxxxxx>
> Signed-off-by: Naresh Solanki <Naresh.Solanki@xxxxxxxxxxxxx>
> ---
>  drivers/i2c/muxes/i2c-mux-pca954x.c | 56 ++++++++++++++++++++++++++++-
>  1 file changed, 55 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 2219062104fb..0c1ff1438e7c 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -57,6 +57,21 @@
>  
>  #define PCA954X_IRQ_OFFSET 4
>  
> +/*
> + * MAX7357 exposes 7 registers on POR which allow to configure additional
> + * features. 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_LOCK_UP_CLEAR		BIT(3)
> +#define MAX7357_CONF_DISCON_SINGLE_CHAN		BIT(4)
> +#define MAX7357_CONF_BUS_LOCKUP_DETECT_DIS	BIT(5)
> +#define MAX7357_CONF_ENABLE_BASIC_MODE		BIT(6)
> +#define MAX7357_CONF_PRECONNECT_TEST		BIT(7)

Not all these defines are are used, can we remove those that we
don't need?

> +#define MAX7357_POR_DEFAULT_CONF		BIT(0)

I think:

   #define MAX7357_POR_DEFAULT_CONF	MAX7357_CONF_INT_ENABLE

has a better meaning... but overall, do we need it?

> +
>  enum pca_type {
>  	max_7356,
>  	max_7357,
> @@ -477,6 +492,41 @@ static int pca954x_init(struct i2c_client *client, struct pca954x *data)
>  	return ret;
>  }
>  
> +static int max7357_init(struct i2c_client *client, struct pca954x *data)
> +{
> +	struct i2c_adapter *adap = client->adapter;
> +	u8 conf = MAX7357_POR_DEFAULT_CONF;
> +	int ret;
> +
> +	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_WRITE_BYTE_DATA))
> +		return pca954x_init(client, data);
> +
> +	if (data->idle_state >= 0)
> +		data->last_chan = pca954x_regval(data, data->idle_state);
> +	else
> +		data->last_chan = 0; /* Disconnect multiplexer */
> +
> +	/*
> +	 * The interrupt signals downstream channels that are stuck, but
> +	 * there's nothing to do and it prevents using the shared pin as reset.
> +	 */
> +	conf &= MAX7357_CONF_INT_ENABLE;
> +
> +	/*
> +	 * On bus lock-up isolate the failing channel and try to clear the
> +	 * fault by sending the flush-out sequence.
> +	 */
> +	if (device_property_read_bool(&client->dev, "maxim,bus-lockup-fix"))
> +		conf |= MAX7357_CONF_DISCON_SINGLE_CHAN |
> +			MAX7357_CONF_FLUSH_OUT;

this function is identical to pca954x_init() except for the
conf.

If you:

	u8 conf = 0;

	...

	if (i2c_check_functionality(adap, I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) {
		conf &= MAX7357_CONF_INT_ENABLE;

		if (device_property_read_bool(&client->dev,
					      "maxim,bus-lockup-fix"))
			conf |= MAX7357_CONF_DISCON_SINGLE_CHAN |
				MAX7357_CONF_FLUSH_OUT;
	}

	ret = i2c_smbus_write_byte_data(client, data->last_chan, conf);
	...


You basically should obtain the same thing, I guess and we make
things easier.


> +	ret = i2c_smbus_write_byte_data(client, data->last_chan, conf);
> +	if (ret < 0)
> +		data->last_chan = 0;
> +	return ret;
> +}
> +
>  /*
>   * I2C init/probing/exit functions
>   */
> @@ -560,7 +610,11 @@ static int pca954x_probe(struct i2c_client *client)
>  	 * initializes the mux to a channel
>  	 * or disconnected state.
>  	 */
> -	ret = pca954x_init(client, data);
> +	if ((dev->of_node && of_device_is_compatible(dev->of_node, "maxim,max7357")) ||
> +	    id->driver_data == max_7357)
> +		ret = max7357_init(client, data);

what happens if this is true and in max7357_init(); the i2c
functionality check fails?

Which of the two if's is redundant? Should they be merged?

Andi

> +	else
> +		ret = pca954x_init(client, data);
>  	if (ret < 0) {
>  		dev_warn(dev, "probe failed\n");
>  		ret = -ENODEV;
> -- 
> 2.41.0
>