[PATCH 0/2] i2c-mux-gpio: Add optional 'select-delay' DT property

[PATCH 0/2] i2c-mux-gpio: Add optional 'select-delay' DT property

[Horatiu Vultur]

Add optional property 'select-delay' to add a delay when changing the
mux state. This is required on some platforms to allow GPIO signals to get
stablized.

Horatiu Vultur (2):
  dt-bindings: i2c-mux-gpio: Add optional DT property
  i2c: i2c-mux-gpio: Add support 'select-delay' property

 Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt | 1 +
 drivers/i2c/muxes/i2c-mux-gpio.c                       | 7 +++++++
 2 files changed, 8 insertions(+)

-- 
2.33.0

[PATCH 1/2] dt-bindings: i2c-mux-gpio: Add optional DT property

[Horatiu Vultur]

Add optional property 'select-delay' DT property. In case this is set
then a delay is added when changing mux state. The value is specified in
usec.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt
index d4cf10582a26..d0dacbad491a 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt
@@ -28,6 +28,7 @@ Required properties:
 Optional properties:
 - idle-state: value to set the muxer to when idle. When no value is
   given, it defaults to the last value used.
+- select-delay: GPIO settle delay when changing mux state. In usec.
 
 For each i2c child node, an I2C child bus will be created. They will
 be numbered based on their order in the device tree.
-- 
2.33.0

Re: [PATCH 1/2] dt-bindings: i2c-mux-gpio: Add optional DT property

[Rob Herring]

On Wed, Oct 13, 2021 at 04:10:02PM +0200, Horatiu Vultur wrote:
> Add optional property 'select-delay' DT property. In case this is set
> then a delay is added when changing mux state. The value is specified in
> usec.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt
> index d4cf10582a26..d0dacbad491a 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt
> @@ -28,6 +28,7 @@ Required properties:
>  Optional properties:
>  - idle-state: value to set the muxer to when idle. When no value is
>    given, it defaults to the last value used.
> +- select-delay: GPIO settle delay when changing mux state. In usec.

Seems generally useful. Can we add this first to the mux control 
binding, then use it here (or better yet, use the mux binding if you 
can instead).

Also, properties with units need a standard unit suffix.

>  
>  For each i2c child node, an I2C child bus will be created. They will
>  be numbered based on their order in the device tree.
> -- 
> 2.33.0
> 
> 

Re: [PATCH 1/2] dt-bindings: i2c-mux-gpio: Add optional DT property

[Peter Rosin]

On 2021-10-26 22:29, Rob Herring wrote:
> On Wed, Oct 13, 2021 at 04:10:02PM +0200, Horatiu Vultur wrote:
>> Add optional property 'select-delay' DT property. In case this is set
>> then a delay is added when changing mux state. The value is specified in
>> usec.
>>
>> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
>> ---
>>  Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt
>> index d4cf10582a26..d0dacbad491a 100644
>> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt
>> @@ -28,6 +28,7 @@ Required properties:
>>  Optional properties:
>>  - idle-state: value to set the muxer to when idle. When no value is
>>    given, it defaults to the last value used.
>> +- select-delay: GPIO settle delay when changing mux state. In usec.
> 
> Seems generally useful. Can we add this first to the mux control 
> binding, then use it here (or better yet, use the mux binding if you 
> can instead).

It is actually not very useful here, nor in the mux-control binding. The
same gpio lines (or mux-control) could be used to control several muxes,
all with vastly different needs as to how long the settle time needs to
be. I.e. it is not the gpio lines (or mux-control) that need to settle,
it is the signal(s) that travel through the controlled mux(es) that need
to settle.

In this case, a settle time property was added to the io-channel-mux
binding, which makes much more sense.

https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git/commit/Documentation/devicetree/bindings/iio/multiplexer/io-channel-mux.yaml?h=char-misc-next&id=b9221f71c285

Cheers,
Peter

> Also, properties with units need a standard unit suffix.
> 
>>  
>>  For each i2c child node, an I2C child bus will be created. They will
>>  be numbered based on their order in the device tree.
>> -- 
>> 2.33.0
>>
>>

Re: [PATCH 1/2] dt-bindings: i2c-mux-gpio: Add optional DT property

[Peter Rosin]

On 2021-10-26 23:02, Peter Rosin wrote:
> 
> 
> On 2021-10-26 22:29, Rob Herring wrote:
>> On Wed, Oct 13, 2021 at 04:10:02PM +0200, Horatiu Vultur wrote:
>>> Add optional property 'select-delay' DT property. In case this is set
>>> then a delay is added when changing mux state. The value is specified in
>>> usec.
>>>
>>> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
>>> ---
>>>  Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt
>>> index d4cf10582a26..d0dacbad491a 100644
>>> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt
>>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt
>>> @@ -28,6 +28,7 @@ Required properties:
>>>  Optional properties:
>>>  - idle-state: value to set the muxer to when idle. When no value is
>>>    given, it defaults to the last value used.
>>> +- select-delay: GPIO settle delay when changing mux state. In usec.
>>
>> Seems generally useful. Can we add this first to the mux control 
>> binding, then use it here (or better yet, use the mux binding if you 
>> can instead).
> 
> It is actually not very useful here, nor in the mux-control binding. The
> same gpio lines (or mux-control) could be used to control several muxes,
> all with vastly different needs as to how long the settle time needs to
> be. I.e. it is not the gpio lines (or mux-control) that need to settle,
> it is the signal(s) that travel through the controlled mux(es) that need
> to settle.
> 
> In this case, a settle time property was added to the io-channel-mux
> binding, which makes much more sense.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git/commit/Documentation/devicetree/bindings/iio/multiplexer/io-channel-mux.yaml?h=char-misc-next&id=b9221f71c285

Oh crap, sorry. This series went right past me since it somehow didn't get
flagged in my inbox. But Robs answer did, and then I assumed it was a late
answer to this series:

https://lore.kernel.org/lkml/20211004153640.20650-1-vincent.whitchurch@axis.com/

Which it of course isn't. But in my mind it was. Result: my above response
does not make any sense. I'll have to go to bed now, but I promise to write
a proper answer tomorrow.

Cheers,
Peter

> Cheers,
> Peter
> 
>> Also, properties with units need a standard unit suffix.
>>
>>>  
>>>  For each i2c child node, an I2C child bus will be created. They will
>>>  be numbered based on their order in the device tree.
>>> -- 
>>> 2.33.0
>>>
>>>

Re: [PATCH 1/2] dt-bindings: i2c-mux-gpio: Add optional DT property

[Peter Rosin]

On 2021-10-26 22:29, Rob Herring wrote:
> On Wed, Oct 13, 2021 at 04:10:02PM +0200, Horatiu Vultur wrote:
>> Add optional property 'select-delay' DT property. In case this is set
>> then a delay is added when changing mux state. The value is specified in
>> usec.
>>
>> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
>> ---
>>  Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt
>> index d4cf10582a26..d0dacbad491a 100644
>> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt
>> @@ -28,6 +28,7 @@ Required properties:
>>  Optional properties:
>>  - idle-state: value to set the muxer to when idle. When no value is
>>    given, it defaults to the last value used.
>> +- select-delay: GPIO settle delay when changing mux state. In usec.
> 
> Seems generally useful. Can we add this first to the mux control 
> binding, then use it here (or better yet, use the mux binding if you 
> can instead)
I do not think that adding a delay to the mux-control binding is what
we want. Any change to the mux-control will be quick. What may take time
is rearranging the routing inside the mux and for the muxed signals
affected by those changes to then settle.

This might sound like splitting hairs, but the distinction does provide
a hint for where the delay fits best. And I think that any user of a
mux-control that might need a delay after changed mux state knows best
what that delay really is. Considering the case where a single
mux-control is used for two different muxes that are routing signals
with different settling time requirements. Why should a quick path
suffer the delays enforced for some other, slow, path? So, I don't
think the delay fits as a property of the mux-control.

> Also, properties with units need a standard unit suffix.

Another naming issue is that the io-channel-mux binding recently added
a settle-time-us property for this exact purpose, it might be good to
follow that lead.

However, since I'm not really happy with the implementation, it might
be easier to instead use a generic mux-control and extend the
i2c-mux-gpmux driver with this settling time. See comments on patch
2/2 for details.

Cheers,
Peter

>>  
>>  For each i2c child node, an I2C child bus will be created. They will
>>  be numbered based on their order in the device tree.
>> -- 
>> 2.33.0
>>

[PATCH 2/2] i2c: i2c-mux-gpio: Add support 'select-delay' property

[Horatiu Vultur]

Use select-delay property to add a delay once the mux state is changed.
This is required on some platforms to allow the GPIO signals to get
stabilized.

Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 drivers/i2c/muxes/i2c-mux-gpio.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
index bac415a52b78..1cc69eb67221 100644
--- a/drivers/i2c/muxes/i2c-mux-gpio.c
+++ b/drivers/i2c/muxes/i2c-mux-gpio.c
@@ -13,6 +13,8 @@
 #include <linux/slab.h>
 #include <linux/bits.h>
 #include <linux/gpio/consumer.h>
+#include <linux/delay.h>
+
 /* FIXME: stop poking around inside gpiolib */
 #include "../../gpio/gpiolib.h"
 
@@ -20,6 +22,7 @@ struct gpiomux {
 	struct i2c_mux_gpio_platform_data data;
 	int ngpios;
 	struct gpio_desc **gpios;
+	int select_delay;
 };
 
 static void i2c_mux_gpio_set(const struct gpiomux *mux, unsigned val)
@@ -29,6 +32,8 @@ static void i2c_mux_gpio_set(const struct gpiomux *mux, unsigned val)
 	values[0] = val;
 
 	gpiod_set_array_value_cansleep(mux->ngpios, mux->gpios, NULL, values);
+	if (mux->select_delay)
+		udelay(mux->select_delay);
 }
 
 static int i2c_mux_gpio_select(struct i2c_mux_core *muxc, u32 chan)
@@ -153,6 +158,8 @@ static int i2c_mux_gpio_probe_fw(struct gpiomux *mux,
 	if (fwnode_property_read_u32(dev->fwnode, "idle-state", &mux->data.idle))
 		mux->data.idle = I2C_MUX_GPIO_NO_IDLE;
 
+	fwnode_property_read_u32(dev->fwnode, "select-delay", &mux->select_delay);
+
 	return 0;
 }
 
-- 
2.33.0

Re: [PATCH 2/2] i2c: i2c-mux-gpio: Add support 'select-delay' property

[Peter Rosin]

Hi!

I'm sorry for the slow response...

On 2021-10-13 16:10, Horatiu Vultur wrote:
> Use select-delay property to add a delay once the mux state is changed.
> This is required on some platforms to allow the GPIO signals to get
> stabilized.
> 
> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  drivers/i2c/muxes/i2c-mux-gpio.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
> index bac415a52b78..1cc69eb67221 100644
> --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> @@ -13,6 +13,8 @@
>  #include <linux/slab.h>
>  #include <linux/bits.h>
>  #include <linux/gpio/consumer.h>
> +#include <linux/delay.h>
> +
>  /* FIXME: stop poking around inside gpiolib */
>  #include "../../gpio/gpiolib.h"
>  
> @@ -20,6 +22,7 @@ struct gpiomux {
>  	struct i2c_mux_gpio_platform_data data;
>  	int ngpios;
>  	struct gpio_desc **gpios;
> +	int select_delay;
>  };
>  
>  static void i2c_mux_gpio_set(const struct gpiomux *mux, unsigned val)
> @@ -29,6 +32,8 @@ static void i2c_mux_gpio_set(const struct gpiomux *mux, unsigned val)
>  	values[0] = val;
>  
>  	gpiod_set_array_value_cansleep(mux->ngpios, mux->gpios, NULL, values);
> +	if (mux->select_delay)
> +		udelay(mux->select_delay);

Use fsleep(mux->select_delay) if you don't know how long the delay really
is.

However, you needlessly invoke the delay even if you do not actually change
the state of the mux. In order to fix that, you need to keep track of the
current state of the mux, but that's a chunk of boring code to write. If you
instead switch to using a mux-gpio from the mux subsystem and point an
i2c-mux-gpmux to that instance, you get that for free, and you can make simple
changes to the i2c-mux-gpmux driver to get this sorted properly, basically
exactly as this patch but with this

-	ret = mux_control_select(mux->control, chan->channel);
+	ret = mux_control_select_delay(mux->control, chan->channel,
+				       mux->delay_us);

instead of the udelay/fsleep in this patch. That will invoke the requested
delay, but only if too little time has gone by since the latest state change.

That interface (mux_control_select_delay) is brand new though, but available
in linux-next and scheduled for the next merge window. But, since I fumbled
this series it's a bit late for this merge window anyway (sorry again) so
that should not be an issue.

Cheers,
Peter

>  }
>  
>  static int i2c_mux_gpio_select(struct i2c_mux_core *muxc, u32 chan)
> @@ -153,6 +158,8 @@ static int i2c_mux_gpio_probe_fw(struct gpiomux *mux,
>  	if (fwnode_property_read_u32(dev->fwnode, "idle-state", &mux->data.idle))
>  		mux->data.idle = I2C_MUX_GPIO_NO_IDLE;
>  
> +	fwnode_property_read_u32(dev->fwnode, "select-delay", &mux->select_delay);
> +
>  	return 0;
>  }
>  
> 

Re: [PATCH 2/2] i2c: i2c-mux-gpio: Add support 'select-delay' property

[Horatiu Vultur]

The 10/27/2021 12:41, Peter Rosin wrote:
> 
> Hi!

Hi Peter,

> 
> I'm sorry for the slow response...
> 
> On 2021-10-13 16:10, Horatiu Vultur wrote:
> > Use select-delay property to add a delay once the mux state is changed.
> > This is required on some platforms to allow the GPIO signals to get
> > stabilized.
> >
> > Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > ---
> >  drivers/i2c/muxes/i2c-mux-gpio.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
> > index bac415a52b78..1cc69eb67221 100644
> > --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> > +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> > @@ -13,6 +13,8 @@
> >  #include <linux/slab.h>
> >  #include <linux/bits.h>
> >  #include <linux/gpio/consumer.h>
> > +#include <linux/delay.h>
> > +
> >  /* FIXME: stop poking around inside gpiolib */
> >  #include "../../gpio/gpiolib.h"
> >
> > @@ -20,6 +22,7 @@ struct gpiomux {
> >       struct i2c_mux_gpio_platform_data data;
> >       int ngpios;
> >       struct gpio_desc **gpios;
> > +     int select_delay;
> >  };
> >
> >  static void i2c_mux_gpio_set(const struct gpiomux *mux, unsigned val)
> > @@ -29,6 +32,8 @@ static void i2c_mux_gpio_set(const struct gpiomux *mux, unsigned val)
> >       values[0] = val;
> >
> >       gpiod_set_array_value_cansleep(mux->ngpios, mux->gpios, NULL, values);
> > +     if (mux->select_delay)
> > +             udelay(mux->select_delay);
> 
> Use fsleep(mux->select_delay) if you don't know how long the delay really
> is.
> 
> However, you needlessly invoke the delay even if you do not actually change
> the state of the mux. In order to fix that, you need to keep track of the
> current state of the mux, but that's a chunk of boring code to write. If you
> instead switch to using a mux-gpio from the mux subsystem and point an
> i2c-mux-gpmux to that instance, you get that for free, and you can make simple
> changes to the i2c-mux-gpmux driver to get this sorted properly, basically
> exactly as this patch but with this
> 
> -       ret = mux_control_select(mux->control, chan->channel);
> +       ret = mux_control_select_delay(mux->control, chan->channel,
> +                                      mux->delay_us);
> 
> instead of the udelay/fsleep in this patch. That will invoke the requested
> delay, but only if too little time has gone by since the latest state change.

Thanks for the advice! I will change to use i2c-mux-gpmux and make the
changes there.

> 
> That interface (mux_control_select_delay) is brand new though, but available
> in linux-next and scheduled for the next merge window. But, since I fumbled
> this series it's a bit late for this merge window anyway (sorry again) so
> that should not be an issue.

No worries, I will try to send a new version.

> 
> Cheers,
> Peter
> 
> >  }
> >
> >  static int i2c_mux_gpio_select(struct i2c_mux_core *muxc, u32 chan)
> > @@ -153,6 +158,8 @@ static int i2c_mux_gpio_probe_fw(struct gpiomux *mux,
> >       if (fwnode_property_read_u32(dev->fwnode, "idle-state", &mux->data.idle))
> >               mux->data.idle = I2C_MUX_GPIO_NO_IDLE;
> >
> > +     fwnode_property_read_u32(dev->fwnode, "select-delay", &mux->select_delay);
> > +
> >       return 0;
> >  }
> >
> >

-- 
/Horatiu