Re: [PATCH 0/5] i2c-mux-gpio: Split plat- and dt-specific code up

[Peter Rosin]

On 2019-04-25 16:37, Serge Semin wrote:
> On Wed, Apr 24, 2019 at 09:25:24PM +0000, Peter Rosin wrote:
> 
> Hello Peter,
> 
>> On 2019-04-24 14:34, Serge Semin wrote:
>>> The main idea of this patchset was to add the dt-based GPIOs support
>>> in i2c-mux-gpio driver. In particular we needed to have the full GPIOs
>>> specifier being handled including the dt-flags like GPIO_ACTIVE_HIGH,
>>> GPIO_ACTIVE_LOW, etc. Due to using a legacy GPIO interface the former
>>> driver implementation didn't provide this ability.
>>
>> I'm curious why active low/high is of any importance? That will only affect
>> the state numbering, but I fail to see any relevance in that numbering. It's
>> just numbers, no?
>>
>> If all the pins are inverted (anything else seems very strange), just
>> reverse the order. I.e. for a 4-way mux, use 3, 2, 1, 0 instead of
>> 0, 1, 2, 3.
>>
>> Why not?
> 
> I may misunderstood you, but active low/high flag has nothing to do with
> pins ordering. It is relevant to an individual pin setting, most likely
> related with hardware setup.

I was not talking about pin order. I was obviously referring to the
state order.

> Here is a simple example:
> i2cmux {
>                 compatible = "i2c-mux-gpio";
>                 mux-gpios = <&gpioa 0 GPIO_ACTIVE_LOW
>                              &control 2 GPIO_ACTIVE_HIGH
>                              &last 5 GPIO_ACTIVE_LOW>;
> };

So, with this, instead of having two pins active-low and using state 3,
you could use state 6 with all pins active-high. Same thing. I.e., use
"state ^ 5" instead of the "direct" state (whatever that is, the state
numbers have no real meaning, they are just numbers).

> In this setup we've got some i2c-mux with GPIOs-driven channel selector.
> First channel is selected by GPIO#0 of controller &gpioa, second one -
> by GPIO#2 of controller &control and third - by GPIO#3 of controller
> &last. In accordance with the i2c_mux_gpio_set() method of i2c-mux-gpio
> driver a GPIO from this set will be driven high in case of a corresponding
> mux channel being enabled. But as you can see from the "mux-gpios" property
> these GPIOs aren't identical. First of all they belong to different
> controller and most importantly they've got different active-attribute.
> This attribute actually means the straight or inverted activation policy.
> So in case of ACTIVE_HIGH flag you get a straight policy. If you set GPIO'
> value the hardware pin will be driven high, and if you clear it GPIO'
> value the hardware pin will be pushed to ground. In case ACTIVE_LOW flag
> is specified, the GPIO and actual hardware pin values are inverted.
> So if you set GPIO to one, the hardware pin will be driven to zero,
> and vise-versa. All this logic is implemented in the gpiod subsystem
> of the kernel and can be defined in dts scripts, while legacy GPIO
> subsystem relied on the drivers to handle this.
> 
> Yeah, it might be confusing, but some hardware is designed this way, so
> the ordinary GPIO outputs are inverted on the way to the i2c-mux channel
> activators. For instance in case if some level-shifter is used as a single
> channel i2c-mux and we don't want i2c-bus being always connected to a bus
> behind it. Such level-shifters are usually activated by ACTIVE_LOW signals.

See above, you could just adjust the state numbers instead.

> In addition there are other than ACTIVE_LOW/ACTIVE_HIGH flags available for
> GPIOs in dts, like GPIO_PUSH_PULL, GPIO_OPEN_DRAIN, GPIO_OPEN_SOURCE, which are
> also specific not only to the GPIO functionality but to the target port and
> hardware design in general. So the support of dt- GPIO-specifiers is very
> important to properly describe the hardware setup.

These things are another matter. I thought this was supposed to be
controlled with pinctrl drivers, but now that I look I see that gpio
drivers can do this directly without help from pinctrl. That's just
not how it has been done on the platforms I have been using...

So, ok, but open-drain/open-source/push-pull should perhaps have been
mentioned more prominently. (I now see that I managed to read past a
mention of open-drain in the commit message for 5/5)

Cheers,
Peter

> -Sergey
> 
>>
>> Cheers,
>> Peter
>>
>>> On the way of adding the full dt-GPIO flags support a small set of
>>> refactorings has been done in order to keep the platform_data-based
>>> systems support, make the code more readable and the alterations - clearer.
>>> In general the whole changes might be considered as the plat- and dt-
>>> specific code split up. In first patch we unpinned the platform-specific
>>> method of GPIO-chip probing. The second patch makes the driver to return
>>> an error if of-based (last resort path) failed to retrieve the driver
>>> private data. The next three patches is the sequence of initial channel
>>> info retrieval, platform_data-specific code isolation and finally full
>>> dt-based GPIOs request method introduction. The last patch does what
>>> we inteded this patchset for in the first place - adds the full dt-GPIO
>>> specifiers support.
>>>
>>>
>>> Serge Semin (5):
>>>   i2c-mux-gpio: Unpin a platform-based device initialization
>>>   i2c-mux-gpio: Return an error if no config data found
>>>   i2c-mux-gpio: Save initial channel number to the idle data field
>>>   i2c-mux-gpio: Unpin the platform-specific GPIOs request code
>>>   i2c-mux-gpio: Create of-based GPIOs request method
>>>
>>>  drivers/i2c/muxes/i2c-mux-gpio.c | 224 ++++++++++++++++++++-----------
>>>  1 file changed, 146 insertions(+), 78 deletions(-)
>>>
>>