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

[Horatiu Vultur <horatiu.vultur@microchip.com>]

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