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
>
