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

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

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

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