Re: [PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver

[Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>]

On 02/14/2013 03:01 AM, Linus Walleij wrote:
> On Thu, Feb 14, 2013 at 1:41 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>> On 02/13/2013 05:34 PM, Doug Anderson wrote:
> 
>>> A little torn here.  It adds a bunch of complexity to the code to
>>> handle this case and there are no known or anticipated users.  I only
>>> wish that the GPIO polarity could be more hidden from drivers (add
>>> functions like gpio_assert, gpio_deassert, etc)...
>>
>> Yes, that would be nice. Alex, LinusW?
> 
> OK so good point since Alex is rewriting the way the external
> API to GPIOs is done.
> 
> So this is one of the evolutionary artifacts of the GPIO subsystem:
> that it has this concept of clients having to know if they want to
> drive the line low or high.
> 
> Either way somewhere in the system the knowledge of whether
> the low or high state counts as asserted must be stored.
> 
> The same goes for inputs really: for example we have a
> platform data flag for drivers/mmc/host/mmci.c that tells
> whether a card is inserted when the line goes low or high. And
> this varies between platforms.
> 
> So that would lead to gpio_get_value() being rewritten
> as gpio_is_asserted() as well.
> 
> We all agree that the meaning of a certain GPIO pins
> high vs low state as asserter or deasserted is a machine-
> specific thing, so it need to come from device tree or platform
> data.
> 
> So what this is really all about is whether that knowledge
> should be part of the consumer DT node/platform data
> or the producer DT node/platform data.
> 
> I.e. in the MMCI case whether we encode that into the
> DT node/pdata for the GPIO controller or the MMCI
> controller.
> 
> A bit like whether to eat eggs from the big or little end
> you could say :-)
> 
> But changing it would have very drastic effects.
> Consider this snippet from
> arch/arm/boot/dts/snowball.dts:
> 
> // External Micro SD slot
> sdi0_per1@80126000 {
>       arm,primecell-periphid = <0x10480180>;
>       max-frequency = <50000000>;
>       bus-width = <4>;
>       mmc-cap-mmc-highspeed;
>       vmmc-supply = <&ab8500_ldo_aux3_reg>;
> 
>       cd-gpios  = <&gpio6 26 0x4>; // 218
>       cd-inverted;
> 
>       status = "okay";
> };
> 
> Note property "cd-inverted".
> 
> It states whether the GPIO value is active high (default) or
> active low (this flag set). Ironically the binding document is
> incomplete but we have to support device trees like this
> going forward.
> 
> How do I make sure that this device tree continue to work
> as expected if we change the semantic of the GPIO subsystem
> to only provide gpio_is_asserted()?
> 
> You would have to include a function call to the GPIO core
> and tell it what is asserted and what is not, like
> gpiod_set_assertion_polarity() so the driver can also
> tell the GPIO subsystem what is asserted and what is not,
> rather than encoding that at the GPIO end of things.
> 
> So all of a sudden *both* the consumers and the providers
> can define assertion sematics for the pins. What happens
> if they are in disagreement?

I think it's actually binding-specific.

Either a binding assumed that the GPIO specifier might not include an
inversion flag, and hence included its own alternative (cd-inverted), or
it assumed that the GPIO specifier would always include this flag, and
hence relied purely on the GPIO flags.

Drivers are written to support specific bindings, and hence they know
which case they fall into.

Hence, I think we want something like:

Case 1: Just use GPIO specifier flags:

gpio = of_get_named_gpio_flags(np, name, index, &flags);
gpio_request_with_flags(gpio, flags);

Case 2: Just use binding-specific property:

gpio = of_get_named_gpio(np, name, index);
flags = 0;
if (of_property_read_bool(np, name))
    flags |= FLAG_INVERTED;
gpio_request_with_flags(gpio, flags);

Or something like that.

That seems simple enough?