Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1529452 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] i2c: i2c-mux-gpio: update mux with  gpiod_set_array_value_cansleep Wolfram Sang <wsa-dev@sang-engineering.com> - 2016-11-24 16:30 +0100
    Re: [PATCH v2] i2c: i2c-mux-gpio: update mux with  gpiod_set_array_value_cansleep Wolfram Sang <wsa-dev@sang-engineering.com> - 2016-11-24 21:00 +0100
      Re: Getting at gpio- and pinctrl-devices as a consumer Linus Walleij <linus.walleij@linaro.org> - 2016-11-25 00:30 +0100
        Re: Getting at gpio- and pinctrl-devices as a consumer Linus Walleij <linus.walleij@linaro.org> - 2016-11-25 14:40 +0100

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [next] | [standalone]

#1529669

[Multipart message — attachments visible in raw view] — view raw

> 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...

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web