[v4,2/2] i2c: muxes: pca954x: Enable features on MAX7357

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

Hi Patrick,

On Thu, Oct 05, 2023 at 03:45:40PM +0200, 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

/&/and/
/its/it's/

> to enable the same as it may have unexpected side effects.

which side effects?

> These should be validated for proper functioning & detection of devices

/&/and/

it ain't WhatsApp :-)

> in secondary bus as sometimes it can cause secondary bus being disabled.

Is this latest sentence a related to this patch or is it a free
information?

> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com>

[...]

>  	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.
> +			 */
> +			conf |= MAX7357_CONF_RELEASE_INT;

This setting comes as default, what about the dedicated reset
gpio pin? How is the driver going to understand whether from the
irq pin is coming a reset or an interrupt?

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

How are these properties affecting the economy of the driver? Can
we have a brief description?

Andi
  

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.

> +			 */
> +			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.

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

Hi again!

[sorry for replying to self]

2023-10-06 at 09:46, Peter Rosin wrote:
> 2023-10-05 at 15:45, Naresh Solanki wrote:
>> From: Patrick Rudolph <patrick.rudolph@9elements.com>
>>

...

>> +			dev_warn(&client->dev,
>> +				 "Write byte not supported. Cannot enable enhanced mode features");
> 
> Missing \n at the end of the string.
> 

Also, I2C_FUNC_SMBUS_WRITE_BYTE is separate from
I2C_FUNC_SMBUS_WRITE_BYTE_DATA, which makes that "Write byte"
misleading.

Cheers,
Peter
  

Hi Andi,


On Fri, 6 Oct 2023 at 04:30, Andi Shyti <andi.shyti@kernel.org> wrote:
>
> Hi Patrick,
>
> On Thu, Oct 05, 2023 at 03:45:40PM +0200, 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
>
> /&/and/
> /its/it's/
Ack
>
> > to enable the same as it may have unexpected side effects.
>
> which side effects?
>
> > These should be validated for proper functioning & detection of devices
>
> /&/and/
>
> it ain't WhatsApp :-)
Ack
>
> > in secondary bus as sometimes it can cause secondary bus being disabled.
>
> Is this latest sentence a related to this patch or is it a free
> information?
Its related to the potential side effect of the feature if enabled.
>
> > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> > Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com>
>
> [...]
>
> >       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.
> > +                      */
> > +                     conf |= MAX7357_CONF_RELEASE_INT;
>
> This setting comes as default, what about the dedicated reset
> gpio pin? How is the driver going to understand whether from the
> irq pin is coming a reset or an interrupt?
As per datasheet,
 RELEASE_INT bit in conf is cleared at poweron.
 The RST/INT is dual function pin.
>
> > +                     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;
>
> How are these properties affecting the economy of the driver? Can
> we have a brief description?
Not sure what you mean.
Are you asking for impact on the driver or feature description ?
Note: These are chip features & this patch is intended to enable them
based on DT settings.

Regards,
Naresh
>
> Andi
  

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