[PATCH v2] i2c: i2c-mux-gpio: update mux with gpiod_set_array_value_cansleep

[PATCH v2] i2c: i2c-mux-gpio: update mux with gpiod_set_array_value_cansleep

[Peter Rosin]

If the gpio controller supports it and the gpio lines are concentrated
to one gpio chip, the mux controller pins will get updated simultaneously.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/i2c/muxes/i2c-mux-gpio.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

Sorry about any confusion with v1, it had another unrelated patch
underneath that made it hard to apply it. I had forgot about that
one...

Anyway, this version should be better. Sorry again.

Cheers,
Peter

diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
index e5cf26eefa97..62d033a7adf2 100644
--- a/drivers/i2c/muxes/i2c-mux-gpio.c
+++ b/drivers/i2c/muxes/i2c-mux-gpio.c
@@ -21,15 +21,18 @@
 struct gpiomux {
 	struct i2c_mux_gpio_platform_data data;
 	unsigned gpio_base;
+	struct gpio_desc **gpios;
 };
 
 static void i2c_mux_gpio_set(const struct gpiomux *mux, unsigned val)
 {
+	int values[mux->data.n_gpios];
 	int i;
 
 	for (i = 0; i < mux->data.n_gpios; i++)
-		gpio_set_value_cansleep(mux->gpio_base + mux->data.gpios[i],
-					val & (1 << i));
+		values[i] = (val >> i) & 1;
+
+	gpiod_set_array_value_cansleep(mux->data.n_gpios, mux->gpios, values);
 }
 
 static int i2c_mux_gpio_select(struct i2c_mux_core *muxc, u32 chan)
@@ -176,12 +179,14 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
 	if (!parent)
 		return -EPROBE_DEFER;
 
-	muxc = i2c_mux_alloc(parent, &pdev->dev, mux->data.n_values, 0, 0,
+	muxc = i2c_mux_alloc(parent, &pdev->dev, mux->data.n_values,
+			     mux->data.n_values * sizeof(*mux->gpios), 0,
 			     i2c_mux_gpio_select, NULL);
 	if (!muxc) {
 		ret = -ENOMEM;
 		goto alloc_failed;
 	}
+	mux->gpios = muxc->priv;
 	muxc->priv = mux;
 
 	platform_set_drvdata(pdev, muxc);
@@ -219,10 +224,12 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
 			goto err_request_gpio;
 		}
 
+		gpio_desc = gpio_to_desc(gpio_base + mux->data.gpios[i]);
+		mux->gpios[i] = gpio_desc;
+
 		if (!muxc->mux_locked)
 			continue;
 
-		gpio_desc = gpio_to_desc(gpio_base + mux->data.gpios[i]);
 		gpio_dev = &gpio_desc->gdev->dev;
 		muxc->mux_locked = i2c_root_adapter(gpio_dev) == root;
 	}
-- 
2.1.4

Re: [PATCH v2] i2c: i2c-mux-gpio: update mux with gpiod_set_array_value_cansleep

[Wolfram Sang]

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

Hi Peter,

> +	int values[mux->data.n_gpios];

Hmm, my code checkers complain about this line:

  CHECK   drivers/i2c/muxes/i2c-mux-gpio.c
    SPARSE
drivers/i2c/muxes/i2c-mux-gpio.c:29:29: warning: Variable length array is used.
    SMATCH
drivers/i2c/muxes/i2c-mux-gpio.c:29:29: warning: Variable length array is used.

Worth to fix it?

BTW (unrelated to your patch), the compiler complains about:

In file included from drivers/i2c/muxes/i2c-mux-gpio.c:18:0:
drivers/i2c/muxes/../../gpio/gpiolib.h:88:27: warning: ‘gpio_suffixes’ defined but not used [-Wunused-const-variable=]
 static const char * const gpio_suffixes[] = { "gpios", "gpio" };

which pointed out this line to me:

 18 #include "../../gpio/gpiolib.h"

which is probably worth fixing, too?

Thanks,

   Wolfram

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

Re: [PATCH v2] i2c: i2c-mux-gpio: update mux with gpiod_set_array_value_cansleep

[Peter Rosin]

On 2016-11-24 16:14, Wolfram Sang wrote:
> Hi Peter,
> 
>> +	int values[mux->data.n_gpios];
> 
> Hmm, my code checkers complain about this line:
> 
>   CHECK   drivers/i2c/muxes/i2c-mux-gpio.c
>     SPARSE
> drivers/i2c/muxes/i2c-mux-gpio.c:29:29: warning: Variable length array is used.
>     SMATCH
> drivers/i2c/muxes/i2c-mux-gpio.c:29:29: warning: Variable length array is used.
> 
> Worth to fix it?

Yes, especially since I spotted an unrelated bug in my patch.
Regarding this though, I just thought (smallish) variable length
arrays were ok. But I guess smallish isn't very exact...

> BTW (unrelated to your patch), the compiler complains about:
> 
> In file included from drivers/i2c/muxes/i2c-mux-gpio.c:18:0:
> drivers/i2c/muxes/../../gpio/gpiolib.h:88:27: warning: ‘gpio_suffixes’ defined but not used [-Wunused-const-variable=]
>  static const char * const gpio_suffixes[] = { "gpios", "gpio" };
> 
> which pointed out this line to me:
> 
>  18 #include "../../gpio/gpiolib.h"
> 
> which is probably worth fixing, too?

Yes, I never liked that include, but I don't know how to get
from struct gpio_desc * to the relevant struct device *
without it...

It's this line that needs it:

	gpio_dev = &gpio_dec->gdev->dev;

If you can replace it with something neater, go for it!

Cheers,
Peter

Re: [PATCH v2] i2c: i2c-mux-gpio: update mux with gpiod_set_array_value_cansleep

[Wolfram Sang]

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


> Yes, I never liked that include, but I don't know how to get
> from struct gpio_desc * to the relevant struct device *
> without it...

Looks to me like we should ask the GPIO maintainers if they are willing
to export this value? I am probably not as good as you in explaining the
details why, though...


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

Getting at gpio- and pinctrl-devices as a consumer

[Peter Rosin]

On 2016-11-24 20:52, Wolfram Sang wrote:
>> Yes, I never liked that include, but I don't know how to get
>> from struct gpio_desc * to the relevant struct device *
>> without it...
> 
> Looks to me like we should ask the GPIO maintainers if they are willing
> to export this value? I am probably not as good as you in explaining the
> details why, though...

Right. Adding Linus, Alexandre and the gpio list.

The background is that the gpio- and pinctrl-based i2c-mux drivers
need to know if the device that is used to control the mux of the
i2c-bus is also sitting on that very same i2c-bus. If it is, the
locking has to be different and a bit more relaxed. This relaxed
mode cannot be used always, as that would change the mux behavior
in an unacceptable way for stuff expecting the (traditional)
stricter locking. See Documentation/i2c/i2c-topology for more
details if you need it.

To check this, the i2c mux drivers dig out the device connected to
each gpio-pin (or pinctrl-state) and walks up the device tree to see
if the root i2c adapter that is muxed is in the loop.

When I wrote this code, I could not find a clean way to go from a
struct gpio_desc * to the relevant device, short of doing

	#include "../../gpio/gpiolib.h"

	gpio_dev = &gpio_desc->gdev->dev;

And similarly for pinctrl:

	#include "../../pinctrl/core.h"

	struct pinctrl_setting *setting;
	pinctrl_dev = setting->pctldev->dev;

I'm not very proud of that, and wonder if there is a better way
to get at the needed struct device? If not, then perhaps there
should be?

Cheers,
Peter

Re: Getting at gpio- and pinctrl-devices as a consumer

[Linus Walleij]

On Thu, Nov 24, 2016 at 10:35 PM, Peter Rosin <peda@axentia.se> wrote:

> The background is that the gpio- and pinctrl-based i2c-mux drivers
> need to know if the device that is used to control the mux of the
> i2c-bus is also sitting on that very same i2c-bus. If it is, the
> locking has to be different and a bit more relaxed. This relaxed
> mode cannot be used always, as that would change the mux behavior
> in an unacceptable way for stuff expecting the (traditional)
> stricter locking. See Documentation/i2c/i2c-topology for more
> details if you need it.
>
> To check this, the i2c mux drivers dig out the device connected to
> each gpio-pin (or pinctrl-state) and walks up the device tree to see
> if the root i2c adapter that is muxed is in the loop.
>
> When I wrote this code, I could not find a clean way to go from a
> struct gpio_desc * to the relevant device, short of doing
>
>         #include "../../gpio/gpiolib.h"
>
>         gpio_dev = &gpio_desc->gdev->dev;
>
> And similarly for pinctrl:
>
>         #include "../../pinctrl/core.h"
>
>         struct pinctrl_setting *setting;
>         pinctrl_dev = setting->pctldev->dev;
>
> I'm not very proud of that, and wonder if there is a better way
> to get at the needed struct device? If not, then perhaps there
> should be?

Surely if I can be convinced that we need helpers for this
in GPIO and/or pin control we can add them.

They just need to be named something reasonable and
be generally useful for other situations of similar nature.

struct device *gpiod_get_backing_device(struct gpio_desc *d);

Is simple but is it really what you want?

Yours,
Linus Walleij

Re: Getting at gpio- and pinctrl-devices as a consumer

[Peter Rosin]

On 2016-11-25 00:29, Linus Walleij wrote:
> On Thu, Nov 24, 2016 at 10:35 PM, Peter Rosin <peda@axentia.se> wrote:
> 
>> The background is that the gpio- and pinctrl-based i2c-mux drivers
>> need to know if the device that is used to control the mux of the
>> i2c-bus is also sitting on that very same i2c-bus. If it is, the
>> locking has to be different and a bit more relaxed. This relaxed
>> mode cannot be used always, as that would change the mux behavior
>> in an unacceptable way for stuff expecting the (traditional)
>> stricter locking. See Documentation/i2c/i2c-topology for more
>> details if you need it.
>>
>> To check this, the i2c mux drivers dig out the device connected to
>> each gpio-pin (or pinctrl-state) and walks up the device tree to see
>> if the root i2c adapter that is muxed is in the loop.
>>
>> When I wrote this code, I could not find a clean way to go from a
>> struct gpio_desc * to the relevant device, short of doing
>>
>>         #include "../../gpio/gpiolib.h"
>>
>>         gpio_dev = &gpio_desc->gdev->dev;
>>
>> And similarly for pinctrl:
>>
>>         #include "../../pinctrl/core.h"
>>
>>         struct pinctrl_setting *setting;
>>         pinctrl_dev = setting->pctldev->dev;
>>
>> I'm not very proud of that, and wonder if there is a better way
>> to get at the needed struct device? If not, then perhaps there
>> should be?
> 
> Surely if I can be convinced that we need helpers for this
> in GPIO and/or pin control we can add them.
> 
> They just need to be named something reasonable and
> be generally useful for other situations of similar nature.
> 
> struct device *gpiod_get_backing_device(struct gpio_desc *d);
> 
> Is simple but is it really what you want?

Well, my first attempt was to simply have a property in the
devicetree stating that the mux was controlled from the same
i2c bus it was muxing, but that was shot down because it
should be possible to deduce this from the implementation (or
something of that meaning, it was a while ago), which to me
meant examining the "struct device"-tree.

For the gpio_desc it is easy. However, it is worse for the
pinctrl case. The pinctrl consumer currently deals in states,
but each state has a number of pinctrl_settings and it is
these settings that are backed by a device implementing that
state. It is in other words possibly several devices involved
with one pinctrl_state. This can be handled in in three ways
that spring to mind.

1. Return a single device connected to a pinctrl state, if it
is the same device backing all settings, and error out if more
than one device is involved. I mean, how silly would it me to
control a mux from pins not controlled by the same pinctrl?
That just seems extremely odd, like one pin each on two
different i2c-controlled io-expanders sitting on the same
i2c-bus that is also muxed using these two pins. The risk for
regression because of this should be ... low.

2. Return an array of devices, one for each pinctrl setting
connected to the state. This also needs to expose the number
of settings for a state.

3. Introduce some kind of list_for_each_entry wrapper (or
something) so that the consumer can iterate over the settings
connected to a state, coupled with a function to get the
struct device from a setting.

#1 is not as generally useful as #2 or #3, as it assumes
that the corner case is not interesting. But it might very
well be the case that the next user is looking for exactly
this corner case. But at the same time, the next user isn't
here yet, at least not that I know of...

#2 and #3 exposes more of the internals to the consumer, since
the consumer currently does not need to worry about neither
the number of settings connected to a state nor that fact
that there are settings at all. #3 would be closest to the
current implementation in i2c-mux-pinctrl (included below).

So, for the case in question I deem #1 good enough, and it
exposes less of the internals. But #2 or #3 might be better for
the next case, and also makes the current case not risk the
unlikely regression. Any preference?

Cheers,
Peter


static struct i2c_adapter *i2c_mux_pinctrl_root_adapter(
	struct pinctrl_state *state)
{
	struct i2c_adapter *root = NULL;
	struct pinctrl_setting *setting;
	struct i2c_adapter *pin_root;

	list_for_each_entry(setting, &state->settings, node) {
		pin_root = i2c_root_adapter(setting->pctldev->dev);
		if (!pin_root)
			return NULL;
		if (!root)
			root = pin_root;
		else if (root != pin_root)
			return NULL;
	}

	return root;
}

Re: Getting at gpio- and pinctrl-devices as a consumer

[Linus Walleij]

On Fri, Nov 25, 2016 at 10:24 AM, Peter Rosin <peda@axentia.se> wrote:
>[Me]
>> struct device *gpiod_get_backing_device(struct gpio_desc *d);
>>
>> Is simple but is it really what you want?
>
> Well, my first attempt was to simply have a property in the
> devicetree stating that the mux was controlled from the same
> i2c bus it was muxing, but that was shot down because it
> should be possible to deduce this from the implementation (or
> something of that meaning, it was a while ago), which to me
> meant examining the "struct device"-tree.

The problem goes into any subsystem providing resources for
a mux in this case, generally for example it is not OK for a
device to runtime suspend or shut down its regulator or turn off
its clock if it is acting as a mux. GPIO and pin control just happens
to be two resource in this specific case.

> For the gpio_desc it is easy. However, it is worse for the
> pinctrl case.

It is annoying to do this in a sense, because it starts to kill
the abstraction we have created exactly in order to avoid
consumers having to worry much about their providers
internals. No we are opening the can and letting the stuff
out all over the place.

Have you looked into the discussion about device dependencies
in general? Isn't this problem mappable as a subset of that?
This was discussed at length at the last kernel summit:
https://lwn.net/Articles/705852/

See especially Rafael's commit
9ed9895370aedd6032af2a9181c62c394d08223b
to driver core in linux-next

Yours,
Linus Walleij

Re: Getting at gpio- and pinctrl-devices as a consumer

[Peter Rosin]

On 2016-11-25 14:39, Linus Walleij wrote:
> On Fri, Nov 25, 2016 at 10:24 AM, Peter Rosin <peda@axentia.se> wrote:
>> [Me]
>>> struct device *gpiod_get_backing_device(struct gpio_desc *d);
>>>
>>> Is simple but is it really what you want?
>>
>> Well, my first attempt was to simply have a property in the
>> devicetree stating that the mux was controlled from the same
>> i2c bus it was muxing, but that was shot down because it
>> should be possible to deduce this from the implementation (or
>> something of that meaning, it was a while ago), which to me
>> meant examining the "struct device"-tree.
> 
> The problem goes into any subsystem providing resources for
> a mux in this case, generally for example it is not OK for a
> device to runtime suspend or shut down its regulator or turn off
> its clock if it is acting as a mux. GPIO and pin control just happens
> to be two resource in this specific case.

Just to be clear, we are only talking of muxers used to mux an
i2c-bus. But if some other popular bus is muxed the problems would
probably be similar.

If there is any action (that is not aware of the mux) on the muxed
i2c-bus as the result of a i2c-mux-foo driver requesting a change to
update the mux state, this will deadlock. Unless of course the mux
is operated with the more relaxed locking (mux-locked instead of
parent-locked). So yes, if some multi-function chip provides a
gpio pin to some i2c-mux, and if this chip for some reason
triggers some strange action when a gpio change is requested, and
this action in turn triggers a transfer on the muxed i2c-bus, it
will not work as intended (it will deadlock). I.e., there are all
kinds of ways to defeat the current check to determine if
i2c-mux-gpio and i2c-mux-pinctrl need relaxed locking. But none
or them worked in this setting before I introduced the more relaxed
mux-locked mode either, so I do not feel all that bad about it.

And the checks can of course be improved if needed.

>> For the gpio_desc it is easy. However, it is worse for the
>> pinctrl case.
> 
> It is annoying to do this in a sense, because it starts to kill
> the abstraction we have created exactly in order to avoid
> consumers having to worry much about their providers
> internals. No we are opening the can and letting the stuff
> out all over the place.

Sorry about that. If it matters I'd be just as happy to have the
devicetree author declare the situation explicitly for the i2c-mux
driver, and I didn't really try to argue for that. Maybe it can
be argued that the needed little piece of info isn't really all that
specific to the linux implementation?

Let me try:

-----------8<-----------

It is unreasonable to require that the i2c-mux code can determine
if the devices needed to operate the mux needs to use the i2c-bus
that is muxed. This requires domain knowledge of the whole system,
and fingers everywhere. There should not be a design restriction
on the system such that this question must have answer.

Assuming everybody agrees with the above, the question becomes: Is
it required that the i2c-mux code knows if any devices needed to
operate the mux needs to use the i2c-bus that is muxed?

I say yes.

Case one: Normal iomem gpios directly from some SOC are used to
mux an i2c-bus. If the i2c-bus is not locked as these gpios are
updated, you might see half a transfer on one i2c child channel,
and the other half on some other channel. Or worse. This might
wreak havoc among the client devices. I.e. the i2c-mux must in
this case lock the i2c-bus during the gpio update to make sure
nothing bad hits the clients devices.

Case two: A gpio update might need to use the muxed i2c-bus. The
simplest case is if the io-expander used to operate an i2c-mux sits
on the same i2c-bus that it is muxing, but it might be a lot more
complex with a chain of dependencies ending up causing an i2c
transfer (also, the client devices are known to handle broken crap
on the i2c-bus and are not affected by a little bit of garbage).
I.e. the i2c-mux must in this case make sure to not lock the
i2c-bus during the gpio update.

In other words, without a full view of the system, it is not
possible to know if the i2c-bus should or should not be locked
when updating the gpios used to operate a mux on the i2c-bus.

-----------8<-----------

I can take the above argument to the dt list and perhaps win them
over. But that doesn't mean that we can simply remove the code,
since doing so would regress anything depending on the current
behavior of (at least sometimes) automatically finding the i2c
dependency. But maybe the users are few and able to add a dt
property in case it's needed?

And I like code that works without configuration, so I would
prefer it to remain and continue to discover the easy case.

> Have you looked into the discussion about device dependencies
> in general? Isn't this problem mappable as a subset of that?

Haven't seen it before. My understanding of the driver model is
not the best, and the docs are behind which makes it hard to
know what to believe. The discussions generally fly a bit over
my head, I guess I simply need to dig into the code a bit more...

> This was discussed at length at the last kernel summit:
> https://lwn.net/Articles/705852/
> 
> See especially Rafael's commit
> 9ed9895370aedd6032af2a9181c62c394d08223b
> to driver core in linux-next

This looks like it could be useful; given a gpio, look up its
device, then do a deep search of the supplier list and look
for any device sitting on the muxed i2c-bus. But of course
consumers might also get a notifier and react. I think it's
next to impossible to automatically cover all cases...

An that still requires the i2c-mux driver to lookup the backing
device of gpios (or device_s_ for pinctrl-states).

Cheers,
Peter