On 2019-06-05 00:43, Serge Semin wrote: > Hello Peter > > On Tue, Jun 04, 2019 at 09:06:22PM +0000, Peter Rosin wrote: >> On 2019-06-04 00:08, Linus Walleij wrote: >>> This switches the i801 GPIO mux to use GPIO descriptors for >>> handling the GPIO lines. The previous hack which was reaching >>> inside the GPIO chips etc cannot live on. We pass descriptors >>> along with the GPIO mux device at creation instead. >>> >>> The GPIO mux was only used by way of platform data with a >>> platform device from one place in the kernel: the i801 i2c bus >>> driver. Let's just associate the GPIO descriptor table with >>> the actual device like everyone else and dynamically create >>> a descriptor table passed along with the GPIO i2c mux. >>> >>> This enables simplification of the GPIO i2c mux driver to >>> use only the descriptor API and the OF probe path gets >>> simplified in the process. >>> >>> The i801 driver was registering the GPIO i2c mux with >>> PLATFORM_DEVID_AUTO which would make it hard to predict the >>> device name and assign the descriptor table properly, but >>> this seems to be a mistake to begin with: all of the >>> GPIO mux devices are hardcoded to look up GPIO lines from >>> the "gpio_ich" GPIO chip. If there are more than one mux, >>> there is certainly more than one gpio chip as well, and >>> then we have more serious problems. Switch to >>> PLATFORM_DEVID_NONE instead. There can be only one. >>> >>> Cc: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx> >>> Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> >>> Cc: Peter Rosin <peda@xxxxxxxxxx> >>> Cc: Jean Delvare <jdelvare@xxxxxxxx> >>> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx> >>> --- >>> ChangeLog v2->v3: >>> - Reorder variable declarations to inverse christmas tree. >>> - Stash away GPIO lookup table reference and make sure to >>> remove it on deletion and on error path. >>> - Insert some nasty FIXME comments about poking around >>> in gpiolib internals. >>> ChangeLog v1->v2: >>> - Found some unused vars when compiling for DT, mea culpa. >>> >>> Folks, you surely see what I am trying to do. Would >>> appreciate help fixing any bugs (it compiles). >> >> Before I dive in, how does this compare to what Serge Semin >> is doing >> >> https://patchwork.ozlabs.org/cover/1091119/ >> >> Any chance of some cooperation? The work seem related at >> first glance. >> >> [I'm quoting the whole message for some context for Serge.] >> >> Cheers, >> Peter >> > > Yes, our works have something in common, but I would appreciate if you > reviewed/accepted my patchset first for several reasons: I asked for cooperation. Calling out "me first" is just not it. > 1. It had been submitted and one more time updated long time ago. Even before the > recent merge window had been opened, since then I haven't got any comments > and you said you get to it after the merge window is closed. Your series was too late for the 5.2 merge window (it was sent a week or two before it opened, and I had no time to rush it). End result, I now have two "competing" pieces of work for the 5.3 merge window. Which of these works was sent first has little bearing on my ordering issues. I you guys don't sort this out, I will go whichever way I find more pleasing. > 2. My patchset splits platform_data-based and OF-based code up, which improves the > i2c-mux-gpio driver readability. Linus' patch doesn't provide the same way of > simplification, but no doubt simplify the code a bit. So, I had a closer look, and yes, your patches do split up the of and plat code. But is this the right approach? The only user of the plat interface is the i2c-i801 driver (and I would really like for the plat interface to just go away). Linus' approach seem (to at least attempt) to consolidate the plat and of code paths so that the i2c-mux-gpio code can ignore the platform interface and only work with the gpiod interface. I like that. > 3. It doesn't break current platform_data-based driver interface as Linus' does. > So if something goes wrong with this patch a user always can get back to my > version of the driver. Linus touches both users of that interface (ignoring any and all out-of-tree losers). But sure, there might be some mistake lurking. He did ask for help though! > 4. It utilized the modern GPIOd API to fully parse of/dtb/fdt/dts nodes. > Linus' alteration also does that, but I have a doubt it is correctly implemented. > > It seems to me, that Linus' patch hasn't been tested (?) on a OF-based platform, > since gpiod_*-methods aren't used correctly ("mux-gpios" con_id utilization > is completely removed from the methods invocations, which may cause failures > in attempts to find the GPIOs count and get GPIO descriptors). Yes, it seems that gpiod_count() and devm_gpiod_get_index() calls can't unconditionally pass NULL as the 2nd argument. Linus? > So I suggest to finish my patchset review first. Then rebase this patch on top of > it. By doing so we'll get to have a simpler driver code with step-by-step history > of alterations from current code state, through platform_data- and OF-based code > split up, modern GPIOd API utilization for OF-based path, to the final > simplifications Linus' patch provides. This patch shall also get to be simpler for > review due to the code split up my patchset provides. I'll help with the review and > test it on my platform when the rebase's done. I'm not so sure. If Linus' patch works out, there will simply be no need to separate the plat/of code paths. Which is of course neat and tidy. Cheers, Peter > Regards, > -Sergey
