Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
MIME-Version: 1.0
References: <20230922093117.3030977-2-naresh.solanki@9elements.com> <8ec35702-54a7-ad6e-99c5-7ed49667c94b@axentia.se>
In-Reply-To: <8ec35702-54a7-ad6e-99c5-7ed49667c94b@axentia.se>
From:   Naresh Solanki <naresh.solanki@9elements.com>
Date:   Thu, 5 Oct 2023 14:03:30 +0530
Message-ID: <CABqG17h2PMunuyeiEPbY4Z6QMZFAmwna2t3AGn5c8YztkzSvNg@mail.gmail.com>
Subject: Re: [RESEND PATCH v3 2/2] i2c: muxes: pca954x: Enable features on MAX7357/MAX7358
To:     Peter Rosin <peda@axentia.se>
Cc:     Patrick Rudolph <patrick.rudolph@9elements.com>,
        linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Peter,

On Fri, 22 Sept 2023 at 16:02, Peter Rosin <peda@axentia.se> wrote:
>
> Hi!
>
> Sorry for being unresponsive...
>
> The subject, description and the bindings patch talk about MAX7358, but
> since it not actually handled it is misleading for the subject to say
> that features are enabled on MAX7358.
Yes will remove reference to max7358.
>
> 2023-09-22 at 11:31, Naresh Solanki wrote:
> > From: Patrick Rudolph <patrick.rudolph@9elements.com>
> >
> > Detect that max7357 is being used and run custom init sequence.
> > Enable additional features based on DT settings and unconditionally
> > release the shared interrupt pin after 1.6 seconds and allow to use
> > it as reset.
> >
> > These features aren't enabled by default & its up to board designer
> > to enable the same as it may have unexpected side effects.
> >
> > These should be validated for proper functioning & detection of devices
> > in secondary bus as sometimes it can cause secondary bus being disabled.
> >
> > The init sequence is not run for max7358 that needs to be unlocked
> > first, but that would need the unimplemented function
> > i2c_probe_func_quick_write().
>
> Is that correct? If that is all that missing, why is it not sufficient to
> open-code it instead?
>
>         i2c_smbus_xfer(client->adapter, client->addr, client->flags,
>                        I2C_SMBUS_WRITE, 0, I2C_SMBUS_QUICK, NULL);
will drop max7358 for now in this patch series.
>
> >
> > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> > Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com>
> > ---
> > Changes in V3:
> > - Delete unused #define
> > - Update pca954x_init
> > - Update commit message
> >
> > Changes in V2:
> > - Update comments
> > - Update check for DT properties
> > ---
> >  drivers/i2c/muxes/i2c-mux-pca954x.c | 38 ++++++++++++++++++++++++++++-
> >  1 file changed, 37 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> > index 2219062104fb..91c7c1d13c89 100644
> > --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> > +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> > @@ -57,6 +57,20 @@
> >
> >  #define PCA954X_IRQ_OFFSET 4
> >
> > +/*
> > + * MAX7357's configuration register is writeable after POR, but
> > + * can be locked by setting the basic mode bit. MAX7358 configuration
> > + * register is locked by default and needs to be unlocked first.
> > + * The configuration register holds the following settings:
> > + */
> > +#define MAX7357_CONF_INT_ENABLE                      BIT(0)
>
> This define isn't used.
Its indirectly used for default POR config. Will use it there.
>
> > +#define MAX7357_CONF_FLUSH_OUT                       BIT(1)
> > +#define MAX7357_CONF_RELEASE_INT             BIT(2)
> > +#define MAX7357_CONF_DISCON_SINGLE_CHAN              BIT(4)
> > +#define MAX7357_CONF_PRECONNECT_TEST         BIT(7)
> > +
> > +#define MAX7357_POR_DEFAULT_CONF             BIT(0)
> > +
> >  enum pca_type {
> >       max_7356,
> >       max_7357,
> > @@ -463,6 +477,7 @@ static void pca954x_cleanup(struct i2c_mux_core *muxc)
> >
> >  static int pca954x_init(struct i2c_client *client, struct pca954x *data)
> >  {
> > +     u8 conf = MAX7357_POR_DEFAULT_CONF;
>
> This line can be moved inside the block below handling max7357. The POR
> default conf is not the same for max7358 anyway.
Sure.
>
> >       int ret;
> >
> >       if (data->idle_state >= 0)
> > @@ -470,7 +485,28 @@ 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 (device_is_compatible(&client->dev, "maxim,max7357") &&
> > +         i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) {
>
> I would have liked a message that any requested extra features cannot
> be enabled if the chip happens to be connected to a bus not capable of
> ...write_byte_data(). That might be plenty helpful for anybody who
> happens to find themself in that hole...
Sure will add that.
>
> > +             /*
> > +              * The interrupt signal is shared with the reset pin. Release the
> > +              * interrupt after 1.6 seconds to allow using the pin as reset.
> > +              * The interrupt isn't serviced yet.
> > +              */
> > +             conf |= MAX7357_CONF_RELEASE_INT;
> > +
> > +             if (device_property_read_bool(&client->dev, "maxim,isolate-stuck-channel"))
> > +                     conf |= MAX7357_CONF_DISCON_SINGLE_CHAN;
> > +             if (device_property_read_bool(&client->dev, "maxim,send-flush-out-sequence"))
> > +                     conf |= MAX7357_CONF_FLUSH_OUT;
> > +             if (device_property_read_bool(&client->dev,
> > +                                           "maxim,preconnection-wiggle-test-enable"))
> > +                     conf |= MAX7357_CONF_PRECONNECT_TEST;
> > +
> > +             ret = i2c_smbus_write_byte_data(client, data->last_chan, conf);
> > +     } else {
> > +             ret = i2c_smbus_write_byte(client, data->last_chan);
> > +     }
> > +
> >       if (ret < 0)
> >               data->last_chan = 0;
> >
>
> Would there be any point in configuring max7357 to be in basic mode?
If the above features arent needed then basic mode will serve the purpose.

>
> Cheers,
> Peter