| Message ID | 20170208092425.azqkx3nwf4jhsfz5@dell |
|---|---|
| State | New |
| Headers | show |
On Wed, Feb 08, 2017 at 09:24:25AM +0000, Lee Jones wrote: > The commits mentioned below adapt the GPIO API to allow more information > to be passed directly through devm_get_gpiod_from_child() in the first > instance. This facilitates the removal of subsequent calls, such as > gpiod_direction_output(). This patch firstly moves to utilise the new > API and secondly removes the now superfluous call do set the direction. > > Fixes: a264d10ff45c ("gpiolib: Convert fwnode_get_named_gpiod() to configure GPIO") > Fixes: b2987d7438e0 ("gpio: Pass GPIO label down to gpiod_request") > Fixes: 4b0947974e59 ("gpio: Rename devm_get_gpiod_from_child()") > Reported-by: Stephen Rothwell <sfr@canb.auug.org.au> > Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com> > Signed-off-by: Lee Jones <lee.jones@linaro.org> > --- > drivers/tty/serial/st-asc.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c > index bcf1d33..c334bcc 100644 > --- a/drivers/tty/serial/st-asc.c > +++ b/drivers/tty/serial/st-asc.c > @@ -575,12 +575,13 @@ static void asc_set_termios(struct uart_port *port, struct ktermios *termios, > pinctrl_select_state(ascport->pinctrl, > ascport->states[NO_HW_FLOWCTRL]); > > - gpiod = devm_get_gpiod_from_child(port->dev, "rts", > - &np->fwnode); > - if (!IS_ERR(gpiod)) { > - gpiod_direction_output(gpiod, 0); > + gpiod = devm_fwnode_get_gpiod_from_child(port->dev, > + "rts", > + &np->fwnode, > + GPIOD_OUT_LOW, > + np->name); I can't apply this :( Usually, when you move apis around, you add it, then convert it, wait a kernel release, then remove the old one. That allows for issues like this when new code is added in one maintainer's branch but not yours. So how about reverting your "drop the function" patch and then wait for -rc2 to really remove it? thanks, greg k-h
Hi Lee,
[auto build test ERROR on tty/tty-testing]
[cannot apply to v4.10-rc7 next-20170208]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Lee-Jones/serial-st-asc-Use-new-GPIOD-API-to-obtain-RTS-pin/20170208-180609
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/tty/serial/st-asc.c: In function 'asc_set_termios':
>> drivers/tty/serial/st-asc.c:578:12: error: implicit declaration of function 'devm_fwnode_get_gpiod_from_child' [-Werror=implicit-function-declaration]
gpiod = devm_fwnode_get_gpiod_from_child(port->dev,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/tty/serial/st-asc.c:578:10: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
gpiod = devm_fwnode_get_gpiod_from_child(port->dev,
^
cc1: some warnings being treated as errors
vim +/devm_fwnode_get_gpiod_from_child +578 drivers/tty/serial/st-asc.c
572 } else {
573 /* If flow-control disabled, it's safe to handle RTS manually */
574 if (!ascport->rts && ascport->states[NO_HW_FLOWCTRL]) {
575 pinctrl_select_state(ascport->pinctrl,
576 ascport->states[NO_HW_FLOWCTRL]);
577
> 578 gpiod = devm_fwnode_get_gpiod_from_child(port->dev,
579 "rts",
580 &np->fwnode,
581 GPIOD_OUT_LOW,
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Wed, 2017-02-08 at 21:48 +0800, kbuild test robot wrote: > Hi Lee, > > [auto build test ERROR on tty/tty-testing] > [cannot apply to v4.10-rc7 next-20170208] > [if your patch is applied to the wrong git tree, please drop us a note > to help improve the system] > > url: https://github.com/0day-ci/linux/commits/Lee-Jones/serial-st-a > sc-Use-new-GPIOD-API-to-obtain-RTS-pin/20170208-180609 > base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git > tty-testing > config: x86_64-allmodconfig (attached as .config) > compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 > reproduce: > # save the attached .config to linux build tree > make ARCH=x86_64 > It requires to have immutable branch in one of the subsystem which the other one can pull. > All errors (new ones prefixed by >>): > > drivers/tty/serial/st-asc.c: In function 'asc_set_termios': > > > drivers/tty/serial/st-asc.c:578:12: error: implicit declaration of > > > function 'devm_fwnode_get_gpiod_from_child' [-Werror=implicit- > > > function-declaration] > > gpiod = devm_fwnode_get_gpiod_from_child(port->dev, > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > drivers/tty/serial/st-asc.c:578:10: warning: assignment makes > pointer from integer without a cast [-Wint-conversion] > gpiod = devm_fwnode_get_gpiod_from_child(port->dev, > ^ > cc1: some warnings being treated as errors > > vim +/devm_fwnode_get_gpiod_from_child +578 drivers/tty/serial/st- > asc.c > > 572 } else { > 573 /* If flow-control disabled, it's safe > to handle RTS manually */ > 574 if (!ascport->rts && ascport- > >states[NO_HW_FLOWCTRL]) { > 575 pinctrl_select_state(ascport- > >pinctrl, > 576 ascport- > >states[NO_HW_FLOWCTRL]); > 577 > > 578 gpiod = > devm_fwnode_get_gpiod_from_child(port->dev, > 579 > "rts", > 580 > &np->fwnode, > 581 > GPIOD_OUT_LOW, > > --- > 0-DAY kernel test infrastructure Open Source Technology > Center > https://lists.01.org/pipermail/kbuild-all Intel > Corporation -- Andy Shevchenko <andriy.shevchenko@linux.intel.com> Intel Finland Oy
On Wed, Feb 08, 2017 at 06:31:10PM +0200, Andy Shevchenko wrote: > On Wed, 2017-02-08 at 21:48 +0800, kbuild test robot wrote: > > Hi Lee, > > > > [auto build test ERROR on tty/tty-testing] > > [cannot apply to v4.10-rc7 next-20170208] > > [if your patch is applied to the wrong git tree, please drop us a note > > to help improve the system] > > > > url: https://github.com/0day-ci/linux/commits/Lee-Jones/serial-st-a > > sc-Use-new-GPIOD-API-to-obtain-RTS-pin/20170208-180609 > > base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git > > tty-testing > > config: x86_64-allmodconfig (attached as .config) > > compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 > > reproduce: > > # save the attached .config to linux build tree > > make ARCH=x86_64 > > > > It requires to have immutable branch in one of the subsystem which the > other one can pull. Which sucks, and is why you should not do api changes this way! greg k-h
On Wed, 2017-02-08 at 18:47 +0100, Greg KH wrote: > On Wed, Feb 08, 2017 at 06:31:10PM +0200, Andy Shevchenko wrote: > > On Wed, 2017-02-08 at 21:48 +0800, kbuild test robot wrote: > > > Hi Lee, > > > > > > [auto build test ERROR on tty/tty-testing] > > > [cannot apply to v4.10-rc7 next-20170208] > > > [if your patch is applied to the wrong git tree, please drop us a > > > note > > > to help improve the system] > > > > > > url: https://github.com/0day-ci/linux/commits/Lee-Jones/serial- > > > st-a > > > sc-Use-new-GPIOD-API-to-obtain-RTS-pin/20170208-180609 > > > base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty > > > .git > > > tty-testing > > > config: x86_64-allmodconfig (attached as .config) > > > compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 > > > reproduce: > > > # save the attached .config to linux build tree > > > make ARCH=x86_64 > > > > > > > It requires to have immutable branch in one of the subsystem which > > the > > other one can pull. > > Which sucks, and is why you should not do api changes this way! Not only me :-) If above will not work we may do something like below for this cycle: static inline ... devm_get_gpiod_from_child() { return devm_fwnode_get_gpiod_from_child(..., GPIO_AS_IS, "?"); } in GPIO tree. -- Andy Shevchenko <andriy.shevchenko@linux.intel.com> Intel Finland Oy
Hi all, On Wed, 08 Feb 2017 21:42:47 +0200 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, 2017-02-08 at 18:47 +0100, Greg KH wrote: > > On Wed, Feb 08, 2017 at 06:31:10PM +0200, Andy Shevchenko wrote: > > > > > > It requires to have immutable branch in one of the subsystem which > > > the > > > other one can pull. > > > > Which sucks, and is why you should not do api changes this way! > > Not only me :-) > > If above will not work we may do something like below for this cycle: > > static inline ... devm_get_gpiod_from_child() > { > return devm_fwnode_get_gpiod_from_child(..., GPIO_AS_IS, "?"); > } > > in GPIO tree. I will use Lee's patch as a merge resolution when I merge the gpio tree (as that is later in my list) from now on. All that has to happen now is that whichever tree is merged last by Linus (Torvalds) has to have this same merge resolution applied. In general, it is better if API changes can be done either as Greg suggested or with a separate immutable topic branch merged into whichever trees need it, but it doesn't happen that way very often, so this is what we generally do. -- Cheers, Stephen Rothwell
On Wed, 08 Feb 2017, Greg KH wrote: > On Wed, Feb 08, 2017 at 09:24:25AM +0000, Lee Jones wrote: > > The commits mentioned below adapt the GPIO API to allow more information > > to be passed directly through devm_get_gpiod_from_child() in the first > > instance. This facilitates the removal of subsequent calls, such as > > gpiod_direction_output(). This patch firstly moves to utilise the new > > API and secondly removes the now superfluous call do set the direction. > > > > Fixes: a264d10ff45c ("gpiolib: Convert fwnode_get_named_gpiod() to configure GPIO") > > Fixes: b2987d7438e0 ("gpio: Pass GPIO label down to gpiod_request") > > Fixes: 4b0947974e59 ("gpio: Rename devm_get_gpiod_from_child()") > > Reported-by: Stephen Rothwell <sfr@canb.auug.org.au> > > Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com> > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > --- > > drivers/tty/serial/st-asc.c | 11 ++++++----- > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c > > index bcf1d33..c334bcc 100644 > > --- a/drivers/tty/serial/st-asc.c > > +++ b/drivers/tty/serial/st-asc.c > > @@ -575,12 +575,13 @@ static void asc_set_termios(struct uart_port *port, struct ktermios *termios, > > pinctrl_select_state(ascport->pinctrl, > > ascport->states[NO_HW_FLOWCTRL]); > > > > - gpiod = devm_get_gpiod_from_child(port->dev, "rts", > > - &np->fwnode); > > - if (!IS_ERR(gpiod)) { > > - gpiod_direction_output(gpiod, 0); > > + gpiod = devm_fwnode_get_gpiod_from_child(port->dev, > > + "rts", > > + &np->fwnode, > > + GPIOD_OUT_LOW, > > + np->name); > > I can't apply this :( > > Usually, when you move apis around, you add it, then convert it, wait a > kernel release, then remove the old one. That allows for issues like > this when new code is added in one maintainer's branch but not yours. > > So how about reverting your "drop the function" patch and then wait for > -rc2 to really remove it? I assume this is a question for LinusW? -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
On Thu, Feb 09, 2017 at 08:21:50AM +0000, Lee Jones wrote: > On Wed, 08 Feb 2017, Greg KH wrote: > > > On Wed, Feb 08, 2017 at 09:24:25AM +0000, Lee Jones wrote: > > > The commits mentioned below adapt the GPIO API to allow more information > > > to be passed directly through devm_get_gpiod_from_child() in the first > > > instance. This facilitates the removal of subsequent calls, such as > > > gpiod_direction_output(). This patch firstly moves to utilise the new > > > API and secondly removes the now superfluous call do set the direction. > > > > > > Fixes: a264d10ff45c ("gpiolib: Convert fwnode_get_named_gpiod() to configure GPIO") > > > Fixes: b2987d7438e0 ("gpio: Pass GPIO label down to gpiod_request") > > > Fixes: 4b0947974e59 ("gpio: Rename devm_get_gpiod_from_child()") > > > Reported-by: Stephen Rothwell <sfr@canb.auug.org.au> > > > Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com> > > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > > --- > > > drivers/tty/serial/st-asc.c | 11 ++++++----- > > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c > > > index bcf1d33..c334bcc 100644 > > > --- a/drivers/tty/serial/st-asc.c > > > +++ b/drivers/tty/serial/st-asc.c > > > @@ -575,12 +575,13 @@ static void asc_set_termios(struct uart_port *port, struct ktermios *termios, > > > pinctrl_select_state(ascport->pinctrl, > > > ascport->states[NO_HW_FLOWCTRL]); > > > > > > - gpiod = devm_get_gpiod_from_child(port->dev, "rts", > > > - &np->fwnode); > > > - if (!IS_ERR(gpiod)) { > > > - gpiod_direction_output(gpiod, 0); > > > + gpiod = devm_fwnode_get_gpiod_from_child(port->dev, > > > + "rts", > > > + &np->fwnode, > > > + GPIOD_OUT_LOW, > > > + np->name); > > > > I can't apply this :( > > > > Usually, when you move apis around, you add it, then convert it, wait a > > kernel release, then remove the old one. That allows for issues like > > this when new code is added in one maintainer's branch but not yours. > > > > So how about reverting your "drop the function" patch and then wait for > > -rc2 to really remove it? > > I assume this is a question for LinusW? It's for whom ever is causing this breakage by removing an api in this manner. greg k-h
On Thu, 09 Feb 2017, Greg KH wrote: > On Thu, Feb 09, 2017 at 08:21:50AM +0000, Lee Jones wrote: > > On Wed, 08 Feb 2017, Greg KH wrote: > > > > > On Wed, Feb 08, 2017 at 09:24:25AM +0000, Lee Jones wrote: > > > > The commits mentioned below adapt the GPIO API to allow more information > > > > to be passed directly through devm_get_gpiod_from_child() in the first > > > > instance. This facilitates the removal of subsequent calls, such as > > > > gpiod_direction_output(). This patch firstly moves to utilise the new > > > > API and secondly removes the now superfluous call do set the direction. > > > > > > > > Fixes: a264d10ff45c ("gpiolib: Convert fwnode_get_named_gpiod() to configure GPIO") > > > > Fixes: b2987d7438e0 ("gpio: Pass GPIO label down to gpiod_request") > > > > Fixes: 4b0947974e59 ("gpio: Rename devm_get_gpiod_from_child()") > > > > Reported-by: Stephen Rothwell <sfr@canb.auug.org.au> > > > > Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com> > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > > > --- > > > > drivers/tty/serial/st-asc.c | 11 ++++++----- > > > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c > > > > index bcf1d33..c334bcc 100644 > > > > --- a/drivers/tty/serial/st-asc.c > > > > +++ b/drivers/tty/serial/st-asc.c > > > > @@ -575,12 +575,13 @@ static void asc_set_termios(struct uart_port *port, struct ktermios *termios, > > > > pinctrl_select_state(ascport->pinctrl, > > > > ascport->states[NO_HW_FLOWCTRL]); > > > > > > > > - gpiod = devm_get_gpiod_from_child(port->dev, "rts", > > > > - &np->fwnode); > > > > - if (!IS_ERR(gpiod)) { > > > > - gpiod_direction_output(gpiod, 0); > > > > + gpiod = devm_fwnode_get_gpiod_from_child(port->dev, > > > > + "rts", > > > > + &np->fwnode, > > > > + GPIOD_OUT_LOW, > > > > + np->name); > > > > > > I can't apply this :( > > > > > > Usually, when you move apis around, you add it, then convert it, wait a > > > kernel release, then remove the old one. That allows for issues like > > > this when new code is added in one maintainer's branch but not yours. > > > > > > So how about reverting your "drop the function" patch and then wait for > > > -rc2 to really remove it? > > > > I assume this is a question for LinusW? > > It's for whom ever is causing this breakage by removing an api in this > manner. +1 -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
On Wed, Feb 8, 2017 at 2:00 PM, Greg KH <greg@kroah.com> wrote: >> - gpiod = devm_get_gpiod_from_child(port->dev, "rts", >> - &np->fwnode); >> - if (!IS_ERR(gpiod)) { >> - gpiod_direction_output(gpiod, 0); >> + gpiod = devm_fwnode_get_gpiod_from_child(port->dev, >> + "rts", >> + &np->fwnode, >> + GPIOD_OUT_LOW, >> + np->name); > > I can't apply this :( > > Usually, when you move apis around, you add it, then convert it, wait a > kernel release, then remove the old one. That allows for issues like > this when new code is added in one maintainer's branch but not yours. Sorry about this, I guess I got a bit stressed too recently so I was not able to solve this in the ultimate way. We converted over all existing users of the APIs but I guess I optimistically assumed no new users would be added in this kernel cycle, but of course they did.... this new driver was using it, Stephen fixed that up in next and now a patch to that driver arrived on top, ouch. > So how about reverting your "drop the function" patch and then wait for > -rc2 to really remove it? I never did a thing like this before, hm sorry for the inexperience. :( I did make an immutable branch like Andy suggested but didn't advertise it well enough. But as stated that approach sucks anyways. Typically I saw this suggestion right after sending the pull request to Torvalds (yeah I should have seen it first, my inbox is chaotic too, mea culpa). I'll follow up on it asking him not to pull that and look for a resolution like you suggest instead. Since it is three patches that then have users on top I guess it is best to add back the old prototype helper for this driver exclusively then fix it for -rc2. Yours, Linus Walleij
diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c index bcf1d33..c334bcc 100644 --- a/drivers/tty/serial/st-asc.c +++ b/drivers/tty/serial/st-asc.c @@ -575,12 +575,13 @@ static void asc_set_termios(struct uart_port *port, struct ktermios *termios, pinctrl_select_state(ascport->pinctrl, ascport->states[NO_HW_FLOWCTRL]); - gpiod = devm_get_gpiod_from_child(port->dev, "rts", - &np->fwnode); - if (!IS_ERR(gpiod)) { - gpiod_direction_output(gpiod, 0); + gpiod = devm_fwnode_get_gpiod_from_child(port->dev, + "rts", + &np->fwnode, + GPIOD_OUT_LOW, + np->name); + if (!IS_ERR(gpiod)) ascport->rts = gpiod; - } } }
The commits mentioned below adapt the GPIO API to allow more information to be passed directly through devm_get_gpiod_from_child() in the first instance. This facilitates the removal of subsequent calls, such as gpiod_direction_output(). This patch firstly moves to utilise the new API and secondly removes the now superfluous call do set the direction. Fixes: a264d10ff45c ("gpiolib: Convert fwnode_get_named_gpiod() to configure GPIO") Fixes: b2987d7438e0 ("gpio: Pass GPIO label down to gpiod_request") Fixes: 4b0947974e59 ("gpio: Rename devm_get_gpiod_from_child()") Reported-by: Stephen Rothwell <sfr@canb.auug.org.au> Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com> Signed-off-by: Lee Jones <lee.jones@linaro.org> --- drivers/tty/serial/st-asc.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) -- 2.9.3