| Message ID | 20180521072939.2836-3-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
On 2018-05-21 09:29, Wolfram Sang wrote: > Similar to mux devices, create special symlinks to connect the demuxed > bus with the demux device. Hmm, now that I have slept on it, I find this a bit odd. For muxes, all channels and the parent are always present. Here, that is not the case. And don't get me wrong, I see why that is the case, but that doesn't mean that I like it. It would be so much nicer and less disruptive if the client devices were not unbound and rebound (which I think they are, right?) on every master switch. In some cases I think it might be possible to make the switch automatic and seamless, e.g. if there are two masters and one of them is faster but has some glitch(es), and the other is slower but complete (or at least complete enough). The demuxer could then switch to the slower master automatically, on a transaction-by-transaction basis, in order to avoid the glitch(es). Yes, it would have to work a bit differently etc etc, but I don't see any reason why it can't be done. But you probably know more than I on that part of the I2C code... Anyway, since this is ABI (and should be documented) I think it would be good if it could accommodate the automatic master switch case too. And for that to work, the "channel-0" name is wrong. I think channel-N should be reserved for the actual masters, in the order given in the devicetree (I know that you currently can't establish these links since you unbind the inactive masters, but that unbind can probably not happen for the automatic switch case?). There could then be some other symlink, e.g. "current" or "master" or something like that, that reflects the present situation (your "channel-0"). What do you think? Cheers, Peter > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > > Changes since v1: > * check sysfs_create_link and print WARN if something fails > > drivers/i2c/muxes/i2c-demux-pinctrl.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/i2c/muxes/i2c-demux-pinctrl.c b/drivers/i2c/muxes/i2c-demux-pinctrl.c > index 035032e20327..13d1461703f3 100644 > --- a/drivers/i2c/muxes/i2c-demux-pinctrl.c > +++ b/drivers/i2c/muxes/i2c-demux-pinctrl.c > @@ -116,6 +116,13 @@ static int i2c_demux_activate_master(struct i2c_demux_pinctrl_priv *priv, u32 ne > if (ret < 0) > goto err_with_put; > > + WARN(sysfs_create_link(&priv->cur_adap.dev.kobj, &priv->dev->kobj, > + "demux_device"), > + "can't create symlink to mux device\n"); > + WARN(sysfs_create_link(&priv->dev->kobj, &priv->cur_adap.dev.kobj, > + "channel-0"), > + "can't create symlink to channel 0\n"); > + > return 0; > > err_with_put: > @@ -135,6 +142,9 @@ static int i2c_demux_deactivate_master(struct i2c_demux_pinctrl_priv *priv) > if (cur < 0) > return 0; > > + sysfs_remove_link(&priv->dev->kobj, "channel-0"); > + sysfs_remove_link(&priv->cur_adap.dev.kobj, "demux_device"); > + > i2c_del_adapter(&priv->cur_adap); > i2c_put_adapter(priv->chan[cur].parent_adap); > >
Hi Peter, > Hmm, now that I have slept on it, I find this a bit odd. For muxes, all > channels and the parent are always present. Here, that is not the case. > And don't get me wrong, I see why that is the case, but that doesn't > mean that I like it. It would be so much nicer and less disruptive if > the client devices were not unbound and rebound (which I think they are, > right?) on every master switch. Yes, we rebind on every master switch. In the first iteration of this driver, I tried the on-the-fly approach. It turned out to be very flaky because it was stretching the driver model too much. There is no support for multiple parents and no support for switching the parent at runtime. When trying to do that, you find out that e.g. the whole relationship tree needs to be rebuilt, say to keep PM hierarchy consistent. And even, just for I2C, on-the-fly switching is not really supported. Some Renesas R-Car SoCs have two different I2C IP cores which can be muxed to the same pins. One has DMA, the other slave functionality. I don't see a way to combine both into one "virtual" master. This is why I came to the current approach. The use case that the customers decide on the feature set they want to use after booting was said to be good enough. > In some cases I think it might be possible to make the switch automatic > and seamless, e.g. if there are two masters and one of them is faster > but has some glitch(es), and the other is slower but complete (or at > least complete enough). That might work for some cases, yet I'd favor a generic solution. > What do you think? Given my above experiences, I'd just drop the channel symlink and keep the driver as is :) But thanks for thinking about it! Wolfram
On Mon, May 21, 2018 at 09:29:39AM +0200, Wolfram Sang wrote: > Similar to mux devices, create special symlinks to connect the demuxed > bus with the demux device. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Nit below not withstanding: Reviewed-by: Simon Horman <horms+renesas@verge.net.au> > --- > > Changes since v1: > * check sysfs_create_link and print WARN if something fails > > drivers/i2c/muxes/i2c-demux-pinctrl.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/i2c/muxes/i2c-demux-pinctrl.c b/drivers/i2c/muxes/i2c-demux-pinctrl.c > index 035032e20327..13d1461703f3 100644 > --- a/drivers/i2c/muxes/i2c-demux-pinctrl.c > +++ b/drivers/i2c/muxes/i2c-demux-pinctrl.c > @@ -116,6 +116,13 @@ static int i2c_demux_activate_master(struct i2c_demux_pinctrl_priv *priv, u32 ne > if (ret < 0) > goto err_with_put; > > + WARN(sysfs_create_link(&priv->cur_adap.dev.kobj, &priv->dev->kobj, > + "demux_device"), > + "can't create symlink to mux device\n"); > + WARN(sysfs_create_link(&priv->dev->kobj, &priv->cur_adap.dev.kobj, > + "channel-0"), > + "can't create symlink to channel 0\n"); > + Personally I'd rather not rely on side-effects from WARN statements, but perhaps thats just me. > return 0; > > err_with_put: > @@ -135,6 +142,9 @@ static int i2c_demux_deactivate_master(struct i2c_demux_pinctrl_priv *priv) > if (cur < 0) > return 0; > > + sysfs_remove_link(&priv->dev->kobj, "channel-0"); > + sysfs_remove_link(&priv->cur_adap.dev.kobj, "demux_device"); > + > i2c_del_adapter(&priv->cur_adap); > i2c_put_adapter(priv->chan[cur].parent_adap); > > -- > 2.11.0 >
> > + WARN(sysfs_create_link(&priv->cur_adap.dev.kobj, &priv->dev->kobj, > > + "demux_device"), > > + "can't create symlink to mux device\n"); > > + WARN(sysfs_create_link(&priv->dev->kobj, &priv->cur_adap.dev.kobj, > > + "channel-0"), > > + "can't create symlink to channel 0\n"); > > + > > Personally I'd rather not rely on side-effects from WARN statements, > but perhaps thats just me. It's a way to satisfy the __must_check from sysfs_create_link().
On 2018-05-22 21:13, Wolfram Sang wrote: > Hi Peter, > >> Hmm, now that I have slept on it, I find this a bit odd. For muxes, all >> channels and the parent are always present. Here, that is not the case. >> And don't get me wrong, I see why that is the case, but that doesn't >> mean that I like it. It would be so much nicer and less disruptive if >> the client devices were not unbound and rebound (which I think they are, >> right?) on every master switch. > > Yes, we rebind on every master switch. In the first iteration of this > driver, I tried the on-the-fly approach. It turned out to be very flaky > because it was stretching the driver model too much. There is no support > for multiple parents and no support for switching the parent at runtime. > When trying to do that, you find out that e.g. the whole relationship > tree needs to be rebuilt, say to keep PM hierarchy consistent. And even, > just for I2C, on-the-fly switching is not really supported. Some Renesas > R-Car SoCs have two different I2C IP cores which can be muxed to the > same pins. One has DMA, the other slave functionality. I don't see a way > to combine both into one "virtual" master. This is why I came to the > current approach. The use case that the customers decide on the feature > set they want to use after booting was said to be good enough. > >> In some cases I think it might be possible to make the switch automatic >> and seamless, e.g. if there are two masters and one of them is faster >> but has some glitch(es), and the other is slower but complete (or at >> least complete enough). > > That might work for some cases, yet I'd favor a generic solution. > >> What do you think? > > Given my above experiences, I'd just drop the channel symlink and keep > the driver as is :) Ok, I'm dropping this patch. > But thanks for thinking about it! No problem. Cheers, Peter
diff --git a/drivers/i2c/muxes/i2c-demux-pinctrl.c b/drivers/i2c/muxes/i2c-demux-pinctrl.c index 035032e20327..13d1461703f3 100644 --- a/drivers/i2c/muxes/i2c-demux-pinctrl.c +++ b/drivers/i2c/muxes/i2c-demux-pinctrl.c @@ -116,6 +116,13 @@ static int i2c_demux_activate_master(struct i2c_demux_pinctrl_priv *priv, u32 ne if (ret < 0) goto err_with_put; + WARN(sysfs_create_link(&priv->cur_adap.dev.kobj, &priv->dev->kobj, + "demux_device"), + "can't create symlink to mux device\n"); + WARN(sysfs_create_link(&priv->dev->kobj, &priv->cur_adap.dev.kobj, + "channel-0"), + "can't create symlink to channel 0\n"); + return 0; err_with_put: @@ -135,6 +142,9 @@ static int i2c_demux_deactivate_master(struct i2c_demux_pinctrl_priv *priv) if (cur < 0) return 0; + sysfs_remove_link(&priv->dev->kobj, "channel-0"); + sysfs_remove_link(&priv->cur_adap.dev.kobj, "demux_device"); + i2c_del_adapter(&priv->cur_adap); i2c_put_adapter(priv->chan[cur].parent_adap);
Similar to mux devices, create special symlinks to connect the demuxed bus with the demux device. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- Changes since v1: * check sysfs_create_link and print WARN if something fails drivers/i2c/muxes/i2c-demux-pinctrl.c | 10 ++++++++++ 1 file changed, 10 insertions(+)