Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
MIME-Version: 1.0
References: <20231005134541.947727-1-naresh.solanki@9elements.com>
 <20231005134541.947727-2-naresh.solanki@9elements.com> <29485a37-1f45-9f7f-9cc5-5d6a9fcc5e08@axentia.se>
In-Reply-To: <29485a37-1f45-9f7f-9cc5-5d6a9fcc5e08@axentia.se>
From:   Naresh Solanki <naresh.solanki@9elements.com>
Date:   Tue, 17 Oct 2023 20:28:36 +0530
Message-ID: <CABqG17g=Q4nhH_RaVQJHhciWMfc-1hLyPaB_+WaincgLtH37CA@mail.gmail.com>
Subject: Re: [PATCH v4 2/2] i2c: muxes: pca954x: Enable features on MAX7357
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

On Fri, 6 Oct 2023 at 13:16, Peter Rosin <peda@axentia.se> wrote:
>
> Hi!
>
> 2023-10-05 at 15:45, Naresh Solanki wrote:
> > From: Patrick Rudolph <patrick.rudolph@9elements.com>
> >
> > 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.
> >
> > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> > Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com>
> > ---
> > Changes in V4:
> > - Drop max7358
> > - Update #define
> > - Move conf variable
> > - Print warning when I2C_FUNC_SMBUS_WRITE_BYTE_DATA isn't supported
> > 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 | 44 ++++++++++++++++++++++++++++-
> >  1 file changed, 43 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> > index 2219062104fb..f37ce332078c 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)
> > +#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             MAX7357_CONF_INT_ENABLE
> > +
> >  enum pca_type {
> >       max_7356,
> >       max_7357,
> > @@ -470,7 +484,35 @@ 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")) {
> > +             if (i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) {
> > +                     u8 conf = MAX7357_POR_DEFAULT_CONF;
> > +                     /*
> > +                      * 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.
>
> I'd suggest dropping the word "yet". The interrupt isn't serviced for
> max7357, period. The "yet" in combination with that 1.6 second window is
> a bit cunfusing and readers might think that the interrupt is serviced
> at some later stage or something, when I think the intention of "The
> interrupt isn't serviced yet" comes with the silent implication that it
> is simply not implemented yet.
Sure.
>
> > +                      */
> > +                     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 {
> > +                     dev_warn(&client->dev,
> > +                              "Write byte not supported. Cannot enable enhanced mode features");
>
> Missing \n at the end of the string.
Sure. Will also update it as
'Write byte data not supported. Cannot enable enhanced mode features\n'

Regards,
Naresh
>
> Cheers,
> Peter
>
> > +                     ret = i2c_smbus_write_byte(client, data->last_chan);
> > +             }
> > +     } else {
> > +             ret = i2c_smbus_write_byte(client, data->last_chan);
> > +     }
> > +
> >       if (ret < 0)
> >               data->last_chan = 0;
> >