Hi Andy,
On Sun, 3 Sept 2023 at 00:13, Andi Shyti <andi.shyti@xxxxxxxxxx> wrote:
>
> 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.
Ack
>
> > 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?
Ack. Will keep the ones that are used.
>
> > +#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?
Ack. Will keep the ones that are in use.
>
> > +
> > 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.
Ack. Will do the changes as suggested.
Also based on feedback from Krzysztof, it was suggested that the dt
property might
not be needed & the settings can be configured/enabled by default. So
will remove
the DT property check ?
Ref: https://lore.kernel.org/lkml/9100e41b-291e-9723-4188-b4d3e5adb6f8@xxxxxxxxxx/#t
Regards,
Naresh
>
>
> > + 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
> >
