[PATCH -next] serial: etraxfs-uart: Update gpiod API

[PATCH -next] serial: etraxfs-uart: Update gpiod API

[Guenter Roeck]

Commit b17d1bf16cc7 ("gpio: make flags mandatory for gpiod_get functions")
makes the flags argument to devm_gpiod_get_optional mandatory but does not
update all users. This results in the following build error.

drivers/tty/serial/etraxfs-uart.c:933:16: error:
	too few arguments to function ‘devm_gpiod_get_optional’

Fixes: b17d1bf16cc7 ("gpio: make flags mandatory for gpiod_get functions")
Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/tty/serial/etraxfs-uart.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/etraxfs-uart.c b/drivers/tty/serial/etraxfs-uart.c
index a57301a6fe42..e35a19b214d5 100644
--- a/drivers/tty/serial/etraxfs-uart.c
+++ b/drivers/tty/serial/etraxfs-uart.c
@@ -930,10 +930,10 @@ static int etraxfs_uart_probe(struct platform_device *pdev)
 
 	up->irq = irq_of_parse_and_map(np, 0);
 	up->regi_ser = of_iomap(np, 0);
-	up->dtr_pin = devm_gpiod_get_optional(&pdev->dev, "dtr");
-	up->dsr_pin = devm_gpiod_get_optional(&pdev->dev, "dsr");
-	up->ri_pin = devm_gpiod_get_optional(&pdev->dev, "ri");
-	up->cd_pin = devm_gpiod_get_optional(&pdev->dev, "cd");
+	up->dtr_pin = devm_gpiod_get_optional(&pdev->dev, "dtr", GPIOD_ASIS);
+	up->dsr_pin = devm_gpiod_get_optional(&pdev->dev, "dsr", GPIOD_ASIS);
+	up->ri_pin = devm_gpiod_get_optional(&pdev->dev, "ri", GPIOD_ASIS);
+	up->cd_pin = devm_gpiod_get_optional(&pdev->dev, "cd", GPIOD_ASIS);
 	up->port.dev = &pdev->dev;
 	cris_serial_port_init(&up->port, dev_id);
 
-- 
2.1.0

Re: [PATCH -next] serial: etraxfs-uart: Update gpiod API

[Uwe Kleine-König]

On Tue, Jul 21, 2015 at 01:34:52PM -0700, Guenter Roeck wrote:
> Commit b17d1bf16cc7 ("gpio: make flags mandatory for gpiod_get functions")
> makes the flags argument to devm_gpiod_get_optional mandatory but does not
> update all users. This results in the following build error.
> 
> drivers/tty/serial/etraxfs-uart.c:933:16: error:
> 	too few arguments to function ‘devm_gpiod_get_optional’
> 
> Fixes: b17d1bf16cc7 ("gpio: make flags mandatory for gpiod_get functions")
> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
We discussed this driver while I worked on updating the gpiod API. I
don't care much about this driver, but if you want to do something about
the build failure, I'd prefer:

diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index 76e65b714471..3a6a7c235f09 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -1066,6 +1066,7 @@ config SERIAL_VT8500_CONSOLE
 config SERIAL_ETRAXFS
 	bool "ETRAX FS serial port support"
 	depends on ETRAX_ARCH_V32 && OF
+	depends on BROKEN
 	select SERIAL_CORE
 
 config SERIAL_ETRAXFS_CONSOLE

Best regards
Uwe
-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH -next] serial: etraxfs-uart: Update gpiod API

[Guenter Roeck]

On 07/21/2015 02:01 PM, Uwe Kleine-König wrote:
> On Tue, Jul 21, 2015 at 01:34:52PM -0700, Guenter Roeck wrote:
>> Commit b17d1bf16cc7 ("gpio: make flags mandatory for gpiod_get functions")
>> makes the flags argument to devm_gpiod_get_optional mandatory but does not
>> update all users. This results in the following build error.
>>
>> drivers/tty/serial/etraxfs-uart.c:933:16: error:
>> 	too few arguments to function ‘devm_gpiod_get_optional’
>>
>> Fixes: b17d1bf16cc7 ("gpio: make flags mandatory for gpiod_get functions")
>> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> We discussed this driver while I worked on updating the gpiod API. I
> don't care much about this driver, but if you want to do something about
> the build failure, I'd prefer:
>
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index 76e65b714471..3a6a7c235f09 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -1066,6 +1066,7 @@ config SERIAL_VT8500_CONSOLE
>   config SERIAL_ETRAXFS
>   	bool "ETRAX FS serial port support"
>   	depends on ETRAX_ARCH_V32 && OF
> +	depends on BROKEN
>   	select SERIAL_CORE
>
>   config SERIAL_ETRAXFS_CONSOLE
>
> Best regards
> Uwe
>

Uwe,

I understand the value of BROKEN, but I do not think it would be
appropriate here. It would be appropriate for a driver which is really
broken, not one that doesn't build anymore because an API it uses
was updated and the driver code wasn't updated for some reason.

With your suggested patch I would no longer be able to run my qemu tests
for this architecture, which I would not entirely be happy about.

You may not care, but maybe others do.

Makes me wonder: It used to be that API changes were handled by those
making the API changes, not by those responsible for the code using
the API. Has this changed recently, ie is it now acceptable to not
update all callers of a modified API (on purpose, as it looks like
it was done here) ?

Thanks,
Guenter

Re: [PATCH -next] serial: etraxfs-uart: Update gpiod API

[Uwe Kleine-König]

Hello,

On Tue, Jul 21, 2015 at 03:17:50PM -0700, Guenter Roeck wrote:
> On 07/21/2015 02:01 PM, Uwe Kleine-König wrote:
> >On Tue, Jul 21, 2015 at 01:34:52PM -0700, Guenter Roeck wrote:
> >>Commit b17d1bf16cc7 ("gpio: make flags mandatory for gpiod_get functions")
> >>makes the flags argument to devm_gpiod_get_optional mandatory but does not
> >>update all users. This results in the following build error.
> >>
> >>drivers/tty/serial/etraxfs-uart.c:933:16: error:
> >>	too few arguments to function ‘devm_gpiod_get_optional’
> >>
> >>Fixes: b17d1bf16cc7 ("gpio: make flags mandatory for gpiod_get functions")
> >>Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> >>Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> >We discussed this driver while I worked on updating the gpiod API. I
> >don't care much about this driver, but if you want to do something about
> >the build failure, I'd prefer:
> >
> >diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> >index 76e65b714471..3a6a7c235f09 100644
> >--- a/drivers/tty/serial/Kconfig
> >+++ b/drivers/tty/serial/Kconfig
> >@@ -1066,6 +1066,7 @@ config SERIAL_VT8500_CONSOLE
> >  config SERIAL_ETRAXFS
> >  	bool "ETRAX FS serial port support"
> >  	depends on ETRAX_ARCH_V32 && OF
> >+	depends on BROKEN
> >  	select SERIAL_CORE
> >
> >  config SERIAL_ETRAXFS_CONSOLE
> >
> >Best regards
> >Uwe
> >
> 
> Uwe,
> 
> I understand the value of BROKEN, but I do not think it would be
> appropriate here. It would be appropriate for a driver which is really
> broken, not one that doesn't build anymore because an API it uses
> was updated and the driver code wasn't updated for some reason.
> 
> With your suggested patch I would no longer be able to run my qemu tests
> for this architecture, which I would not entirely be happy about.
> 
> You may not care, but maybe others do.
> 
> Makes me wonder: It used to be that API changes were handled by those
> making the API changes, not by those responsible for the code using
> the API. Has this changed recently, ie is it now acceptable to not
> update all callers of a modified API (on purpose, as it looks like
> it was done here) ?
While I stumbled over the calls to gpiod_get variants in the etraxfs
serial driver I noticed several problems with that driver. I pointed
them out in

	http://www.spinics.net/lists/linux-serial/msg17794.html

. If you read the follow-ups Jesper Nilsson then said:

	I can push a patch that just drops all signal handling (since
	it's bogus ATM), or you can ignore the driver as currently
	implemented.

I explicitly did the latter. Apart from that single driver I fixed all
users. @Jesper: You said you poked Niklas to care for that. Any news
from that front?

I'm all for the usual habit that API changes should be handled by the
API changer. But if the code was already relying on undefined behaviour
before the change and fixing that needs testing on the respective
hardware, I think pushing the needed work back to the driver owner is
fine. IMHO a simple build fix that doesn't fix the brokeness isn't that
helpful here.

Having said that I don't care for that driver. So if you say it's good
enough for you to simply add GPIOD_ASIS and the responsible maintainers
are ok, too, I won't stand in the way.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH -next] serial: etraxfs-uart: Update gpiod API

[Guenter Roeck]

On 07/22/2015 12:06 AM, Uwe Kleine-König wrote:

> While I stumbled over the calls to gpiod_get variants in the etraxfs
> serial driver I noticed several problems with that driver. I pointed
> them out in
>
> 	http://www.spinics.net/lists/linux-serial/msg17794.html
>
> . If you read the follow-ups Jesper Nilsson then said:
>
> 	I can push a patch that just drops all signal handling (since
> 	it's bogus ATM), or you can ignore the driver as currently
> 	implemented.
>

If you want to mark the driver as BROKEN due to those other problems,
go ahead, and I'll drop the architecture from my tests. But it should
not be marked BROKEN because you did not update an API.

> I explicitly did the latter. Apart from that single driver I fixed all
> users. @Jesper: You said you poked Niklas to care for that. Any news
> from that front?
>
> I'm all for the usual habit that API changes should be handled by the
> API changer. But if the code was already relying on undefined behaviour
> before the change and fixing that needs testing on the respective
> hardware, I think pushing the needed work back to the driver owner is
> fine. IMHO a simple build fix that doesn't fix the brokeness isn't that
> helpful here.
>
The driver works fine for me (in qemu), possibly because qemu doesn't
use/need modem lines.

Guenter

Re: [PATCH -next] serial: etraxfs-uart: Update gpiod API

[Niklas Cassel]

On 07/22/2015 09:06 AM, Uwe Kleine-König wrote:
> Hello,
> 
> On Tue, Jul 21, 2015 at 03:17:50PM -0700, Guenter Roeck wrote:
>> On 07/21/2015 02:01 PM, Uwe Kleine-König wrote:
>>> On Tue, Jul 21, 2015 at 01:34:52PM -0700, Guenter Roeck wrote:
>>>> Commit b17d1bf16cc7 ("gpio: make flags mandatory for gpiod_get functions")
>>>> makes the flags argument to devm_gpiod_get_optional mandatory but does not
>>>> update all users. This results in the following build error.
>>>>
>>>> drivers/tty/serial/etraxfs-uart.c:933:16: error:
>>>> 	too few arguments to function ‘devm_gpiod_get_optional’
>>>>
>>>> Fixes: b17d1bf16cc7 ("gpio: make flags mandatory for gpiod_get functions")
>>>> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>> We discussed this driver while I worked on updating the gpiod API. I
>>> don't care much about this driver, but if you want to do something about
>>> the build failure, I'd prefer:
>>>
>>> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
>>> index 76e65b714471..3a6a7c235f09 100644
>>> --- a/drivers/tty/serial/Kconfig
>>> +++ b/drivers/tty/serial/Kconfig
>>> @@ -1066,6 +1066,7 @@ config SERIAL_VT8500_CONSOLE
>>>  config SERIAL_ETRAXFS
>>>  	bool "ETRAX FS serial port support"
>>>  	depends on ETRAX_ARCH_V32 && OF
>>> +	depends on BROKEN
>>>  	select SERIAL_CORE
>>>
>>>  config SERIAL_ETRAXFS_CONSOLE
>>>
>>> Best regards
>>> Uwe
>>>
>>
>> Uwe,
>>
>> I understand the value of BROKEN, but I do not think it would be
>> appropriate here. It would be appropriate for a driver which is really
>> broken, not one that doesn't build anymore because an API it uses
>> was updated and the driver code wasn't updated for some reason.
>>
>> With your suggested patch I would no longer be able to run my qemu tests
>> for this architecture, which I would not entirely be happy about.
>>
>> You may not care, but maybe others do.
>>
>> Makes me wonder: It used to be that API changes were handled by those
>> making the API changes, not by those responsible for the code using
>> the API. Has this changed recently, ie is it now acceptable to not
>> update all callers of a modified API (on purpose, as it looks like
>> it was done here) ?
> While I stumbled over the calls to gpiod_get variants in the etraxfs
> serial driver I noticed several problems with that driver. I pointed
> them out in
> 
> 	http://www.spinics.net/lists/linux-serial/msg17794.html
> 
> . If you read the follow-ups Jesper Nilsson then said:
> 
> 	I can push a patch that just drops all signal handling (since
> 	it's bogus ATM), or you can ignore the driver as currently
> 	implemented.
> 
> I explicitly did the latter. Apart from that single driver I fixed all
> users. @Jesper: You said you poked Niklas to care for that. Any news
> from that front?

I've talked to Rabin Vincent, who has created an extraxfs gpio driver
that supports gpiolib (will come with 4.2).
Because of that, it might be nice to keep the modem control stuff.

The proper fix is probably to remove the devm_gpiod_get_optional calls
and instead use the serial_mctrl_gpio.c API.

I'll cook a patch this weekend.

> 
> I'm all for the usual habit that API changes should be handled by the
> API changer. But if the code was already relying on undefined behaviour
> before the change and fixing that needs testing on the respective
> hardware, I think pushing the needed work back to the driver owner is
> fine. IMHO a simple build fix that doesn't fix the brokeness isn't that
> helpful here.
> 
> Having said that I don't care for that driver. So if you say it's good
> enough for you to simply add GPIOD_ASIS and the responsible maintainers
> are ok, too, I won't stand in the way.
> 
> Best regards
> Uwe
> 

Re: [PATCH -next] serial: etraxfs-uart: Update gpiod API

[Greg Kroah-Hartman]

On Tue, Jul 21, 2015 at 01:34:52PM -0700, Guenter Roeck wrote:
> Commit b17d1bf16cc7 ("gpio: make flags mandatory for gpiod_get functions")
> makes the flags argument to devm_gpiod_get_optional mandatory but does not
> update all users. This results in the following build error.
> 
> drivers/tty/serial/etraxfs-uart.c:933:16: error:
> 	too few arguments to function ‘devm_gpiod_get_optional’
> 
> Fixes: b17d1bf16cc7 ("gpio: make flags mandatory for gpiod_get functions")

This patch isn't in Linus's tree, so whatever tree this commit is in,
needs to also take this fix.

thanks,

greg k-h

Re: [PATCH -next] serial: etraxfs-uart: Update gpiod API

[Uwe Kleine-König]

On Thu, Jul 23, 2015 at 03:23:49PM -0700, Greg Kroah-Hartman wrote:
> On Tue, Jul 21, 2015 at 01:34:52PM -0700, Guenter Roeck wrote:
> > Commit b17d1bf16cc7 ("gpio: make flags mandatory for gpiod_get functions")
> > makes the flags argument to devm_gpiod_get_optional mandatory but does not
> > update all users. This results in the following build error.
> > 
> > drivers/tty/serial/etraxfs-uart.c:933:16: error:
> > 	too few arguments to function ‘devm_gpiod_get_optional’
> > 
> > Fixes: b17d1bf16cc7 ("gpio: make flags mandatory for gpiod_get functions")
> 
> This patch isn't in Linus's tree, so whatever tree this commit is in,
> needs to also take this fix.
It is in Linus's tree. The Walleij one though, not Torvalds :-)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH -next] serial: etraxfs-uart: Update gpiod API

[Linus Walleij]

On Fri, Jul 24, 2015 at 12:23 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Tue, Jul 21, 2015 at 01:34:52PM -0700, Guenter Roeck wrote:
>> Commit b17d1bf16cc7 ("gpio: make flags mandatory for gpiod_get functions")
>> makes the flags argument to devm_gpiod_get_optional mandatory but does not
>> update all users. This results in the following build error.
>>
>> drivers/tty/serial/etraxfs-uart.c:933:16: error:
>>       too few arguments to function ‘devm_gpiod_get_optional’
>>
>> Fixes: b17d1bf16cc7 ("gpio: make flags mandatory for gpiod_get functions")
>
> This patch isn't in Linus's tree, so whatever tree this commit is in,
> needs to also take this fix.

OK I'll take it. It's a bit messy with this breakage but it is for
a good cause.

Niklas: send me your patch when finished.

Yours,
Linus Walleij

Re: [PATCH -next] serial: etraxfs-uart: Update gpiod API

[Niklas Cassel]

On 07/27/2015 03:16 PM, Linus Walleij wrote:
> On Fri, Jul 24, 2015 at 12:23 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>> On Tue, Jul 21, 2015 at 01:34:52PM -0700, Guenter Roeck wrote:
>>> Commit b17d1bf16cc7 ("gpio: make flags mandatory for gpiod_get functions")
>>> makes the flags argument to devm_gpiod_get_optional mandatory but does not
>>> update all users. This results in the following build error.
>>>
>>> drivers/tty/serial/etraxfs-uart.c:933:16: error:
>>>       too few arguments to function ‘devm_gpiod_get_optional’
>>>
>>> Fixes: b17d1bf16cc7 ("gpio: make flags mandatory for gpiod_get functions")
>>
>> This patch isn't in Linus's tree, so whatever tree this commit is in,
>> needs to also take this fix.
> 
> OK I'll take it. It's a bit messy with this breakage but it is for
> a good cause.
> 
> Niklas: send me your patch when finished.

I've already sent them to the linux-serial and linux-kernel mailing lists.

I'm sorry that I didn't think of CC:ing you Linus.
It might be better if you take them, since you got
the devm_gpiod_get_optional patches in your tree.


http://marc.info/?l=linux-serial&m=143778406314867

http://marc.info/?l=linux-serial&m=143778407714871

Re: [PATCH -next] serial: etraxfs-uart: Update gpiod API

[Uwe Kleine-König]

On Mon, Jul 27, 2015 at 03:29:37PM +0200, Niklas Cassel wrote:
> On 07/27/2015 03:16 PM, Linus Walleij wrote:
> > On Fri, Jul 24, 2015 at 12:23 AM, Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> >> On Tue, Jul 21, 2015 at 01:34:52PM -0700, Guenter Roeck wrote:
> >>> Commit b17d1bf16cc7 ("gpio: make flags mandatory for gpiod_get functions")
> >>> makes the flags argument to devm_gpiod_get_optional mandatory but does not
> >>> update all users. This results in the following build error.
> >>>
> >>> drivers/tty/serial/etraxfs-uart.c:933:16: error:
> >>>       too few arguments to function ‘devm_gpiod_get_optional’
> >>>
> >>> Fixes: b17d1bf16cc7 ("gpio: make flags mandatory for gpiod_get functions")
> >>
> >> This patch isn't in Linus's tree, so whatever tree this commit is in,
> >> needs to also take this fix.
> > 
> > OK I'll take it. It's a bit messy with this breakage but it is for
> > a good cause.
> > 
> > Niklas: send me your patch when finished.
> 
> I've already sent them to the linux-serial and linux-kernel mailing lists.
> 
> I'm sorry that I didn't think of CC:ing you Linus.
> It might be better if you take them, since you got
> the devm_gpiod_get_optional patches in your tree.
If you use devm_gpiod_get_optional with the additional flags parameter
it should already now work for you. So if your change goes in before the
gpio change (which only makes the flags parameter mandatory) everything
is fine. Not sure this makes it easier though.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH -next] serial: etraxfs-uart: Update gpiod API

[Linus Walleij]

On Mon, Jul 27, 2015 at 3:29 PM, Niklas Cassel <niklas.cassel@axis.com> wrote:
> On 07/27/2015 03:16 PM, Linus Walleij wrote:

>> Niklas: send me your patch when finished.
>
> I've already sent them to the linux-serial and linux-kernel mailing lists.
>
> I'm sorry that I didn't think of CC:ing you Linus.
> It might be better if you take them, since you got
> the devm_gpiod_get_optional patches in your tree.
>
> http://marc.info/?l=linux-serial&m=143778406314867
> http://marc.info/?l=linux-serial&m=143778407714871

This should work also with the current kernel, so better if Greg
take them I guess. I'll go in and ACK it.

Yours,
Linus Walleij