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

[Naresh Solanki]

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
> >