serial: st-asc: Use new GPIOD API to obtain RTS pin

Message ID 20170208092425.azqkx3nwf4jhsfz5@dell
State New
Headers show

Commit Message

Lee Jones Feb. 8, 2017, 9:24 a.m.
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

Comments

Greg KH Feb. 8, 2017, 1 p.m. | #1
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
kbuild test robot Feb. 8, 2017, 1:48 p.m. | #2
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
Andy Shevchenko Feb. 8, 2017, 4:31 p.m. | #3
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
Greg KH Feb. 8, 2017, 5:47 p.m. | #4
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
Andy Shevchenko Feb. 8, 2017, 7:42 p.m. | #5
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
Stephen Rothwell Feb. 8, 2017, 9:24 p.m. | #6
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
Lee Jones Feb. 9, 2017, 8:21 a.m. | #7
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
Greg KH Feb. 9, 2017, 8:51 a.m. | #8
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
Lee Jones Feb. 13, 2017, 9:59 a.m. | #9
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
Linus Walleij Feb. 21, 2017, 10:26 a.m. | #10
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

Patch hide | download patch | download mbox

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;
-			}
 		}
 	}