[PATCH v2 0/2] i2c: mux: demux-pinctrl: improve device relationships

[PATCH v2 0/2] i2c: mux: demux-pinctrl: improve device relationships

[Wolfram Sang]

While researching some PM behaviour within I2C, I found out that the i2c-demux
driver does not play well with that due to broken relationship with other
devices. Patch 1 ensures the right parent-child relationship. Patch 2 makes the
connection between the demux device and the demuxed bus similar to that we have
in the mux core.

Tested on a Renesas Lager board (R-Car H2).

Changes since v1:

* added error check in patch 2

Wolfram Sang (2):
  i2c: mux: demux-pinctrl: use proper parent device for demux adapter
  i2c: mux: demux-pinctrl: add symlinks to the demux device in sysfs

 drivers/i2c/muxes/i2c-demux-pinctrl.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

-- 
2.11.0

[PATCH v2 1/2] i2c: mux: demux-pinctrl: use proper parent device for demux adapter

[Wolfram Sang]

Due to a typo, the wrong parent device was assigned to the newly created
demuxing adapter device. It got connected to the demuxing platform
device but not to the selected parent I2C adapter device. Fix it to get
a proper parent-child relationship of the demuxed busses.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/muxes/i2c-demux-pinctrl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/muxes/i2c-demux-pinctrl.c b/drivers/i2c/muxes/i2c-demux-pinctrl.c
index 428de4c97fb2..035032e20327 100644
--- a/drivers/i2c/muxes/i2c-demux-pinctrl.c
+++ b/drivers/i2c/muxes/i2c-demux-pinctrl.c
@@ -106,7 +106,7 @@ static int i2c_demux_activate_master(struct i2c_demux_pinctrl_priv *priv, u32 ne
 	priv->cur_adap.owner = THIS_MODULE;
 	priv->cur_adap.algo = &priv->algo;
 	priv->cur_adap.algo_data = priv;
-	priv->cur_adap.dev.parent = priv->dev;
+	priv->cur_adap.dev.parent = &adap->dev;
 	priv->cur_adap.class = adap->class;
 	priv->cur_adap.retries = adap->retries;
 	priv->cur_adap.timeout = adap->timeout;
-- 
2.11.0

[PATCH v2 2/2] i2c: mux: demux-pinctrl: add symlinks to the demux device in sysfs

[Wolfram Sang]

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

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

Re: [PATCH v2 2/2] i2c: mux: demux-pinctrl: add symlinks to the demux device in sysfs

[Peter Rosin]

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

Re: [PATCH v2 2/2] i2c: mux: demux-pinctrl: add symlinks to the demux device in sysfs

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1735 bytes --]

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/2] i2c: mux: demux-pinctrl: add symlinks to the demux device in sysfs

[Simon Horman]

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
> 

Re: [PATCH v2 1/2] i2c: mux: demux-pinctrl: use proper parent device for demux adapter

[Simon Horman]

On Mon, May 21, 2018 at 09:29:38AM +0200, Wolfram Sang wrote:
> Due to a typo, the wrong parent device was assigned to the newly created
> demuxing adapter device. It got connected to the demuxing platform
> device but not to the selected parent I2C adapter device. Fix it to get
> a proper parent-child relationship of the demuxed busses.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

Re: [PATCH v2 2/2] i2c: mux: demux-pinctrl: add symlinks to the demux device in sysfs

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 501 bytes --]


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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/2] i2c: mux: demux-pinctrl: use proper parent device for demux adapter

[Peter Rosin]

On 2018-05-21 09:29, Wolfram Sang wrote:
> Due to a typo, the wrong parent device was assigned to the newly created
> demuxing adapter device. It got connected to the demuxing platform
> device but not to the selected parent I2C adapter device. Fix it to get
> a proper parent-child relationship of the demuxed busses.

Applied to i2c-mux/for-next.

Cheers,
Peter

Re: [PATCH v2 2/2] i2c: mux: demux-pinctrl: add symlinks to the demux device in sysfs

[Peter Rosin]

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