[PATCH v2 1/3] serial: mctrl_gpio: Avoid probe failures in case of missing gpiolib

[PATCH v2 1/3] serial: mctrl_gpio: Avoid probe failures in case of missing gpiolib

[Schrempf Frieder]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

If CONFIG_GPIOLIB is not enabled, mctrl_gpio_init() and
mctrl_gpio_init_noauto() will currently return an error pointer with
-ENOSYS. As the mctrl GPIOs are usually optional, drivers need to
check for this condition to allow continue probing.

To avoid the need for this check in each driver, we return NULL
instead, as all the mctrl_gpio_*() functions are skipped anyway.
We also adapt mctrl_gpio_to_gpiod() to be in line with this change.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Reviewed-by: Fabio Estevam <festevam@gmail.com>
---
Changes in v2
=============
* Move the sh_sci changes to a separate patch
* Add a patch for the 8250 driver
* Add Fabio's R-b tag
---
 drivers/tty/serial/serial_mctrl_gpio.c | 3 +++
 drivers/tty/serial/serial_mctrl_gpio.h | 6 +++---
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c
index 2b400189be91..54c43e02e375 100644
--- a/drivers/tty/serial/serial_mctrl_gpio.c
+++ b/drivers/tty/serial/serial_mctrl_gpio.c
@@ -61,6 +61,9 @@ EXPORT_SYMBOL_GPL(mctrl_gpio_set);
 struct gpio_desc *mctrl_gpio_to_gpiod(struct mctrl_gpios *gpios,
 				      enum mctrl_gpio_idx gidx)
 {
+	if (gpios == NULL)
+		return NULL;
+
 	return gpios->gpio[gidx];
 }
 EXPORT_SYMBOL_GPL(mctrl_gpio_to_gpiod);
diff --git a/drivers/tty/serial/serial_mctrl_gpio.h b/drivers/tty/serial/serial_mctrl_gpio.h
index b7d3cca48ede..1b2ff503b2c2 100644
--- a/drivers/tty/serial/serial_mctrl_gpio.h
+++ b/drivers/tty/serial/serial_mctrl_gpio.h
@@ -114,19 +114,19 @@ static inline
 struct gpio_desc *mctrl_gpio_to_gpiod(struct mctrl_gpios *gpios,
 				      enum mctrl_gpio_idx gidx)
 {
-	return ERR_PTR(-ENOSYS);
+	return NULL;
 }
 
 static inline
 struct mctrl_gpios *mctrl_gpio_init(struct uart_port *port, unsigned int idx)
 {
-	return ERR_PTR(-ENOSYS);
+	return NULL;
 }
 
 static inline
 struct mctrl_gpios *mctrl_gpio_init_noauto(struct device *dev, unsigned int idx)
 {
-	return ERR_PTR(-ENOSYS);
+	return NULL;
 }
 
 static inline
-- 
2.17.1

[PATCH v2 2/3] serial: sh-sci: Remove check for specific mctrl_gpio_init() return value

[Schrempf Frieder]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

Now that the mctrl_gpio code returns NULL instead of ERR_PTR(-ENOSYS)
in cases when CONFIG_GPIOLIB is disabled, we can safely remove this
check.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 drivers/tty/serial/sh-sci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index d18c680aa64b..249325b65ee0 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -3287,7 +3287,7 @@ static int sci_probe_single(struct platform_device *dev,
 		return ret;
 
 	sciport->gpios = mctrl_gpio_init(&sciport->port, 0);
-	if (IS_ERR(sciport->gpios) && PTR_ERR(sciport->gpios) != -ENOSYS)
+	if (IS_ERR(sciport->gpios))
 		return PTR_ERR(sciport->gpios);
 
 	if (sciport->has_rtscts) {
-- 
2.17.1

[PATCH v2 3/3] serial: 8250: Remove check for specific mctrl_gpio_init() return value

[Schrempf Frieder]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

Now that the mctrl_gpio code returns NULL instead of ERR_PTR(-ENOSYS)
in cases when CONFIG_GPIOLIB is disabled, we can safely remove this
check.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 drivers/tty/serial/8250/8250_core.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index df3bcc0b2d74..e682390ce0de 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -1026,10 +1026,8 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
 		if (!has_acpi_companion(uart->port.dev)) {
 			gpios = mctrl_gpio_init(&uart->port, 0);
 			if (IS_ERR(gpios)) {
-				if (PTR_ERR(gpios) != -ENOSYS) {
-					ret = PTR_ERR(gpios);
-					goto out_unlock;
-				}
+				ret = PTR_ERR(gpios);
+				goto out_unlock;
 			} else {
 				uart->gpios = gpios;
 			}
-- 
2.17.1

Re: [PATCH v2 1/3] serial: mctrl_gpio: Avoid probe failures in case of missing gpiolib

[Uwe Kleine-König]

On Thu, Aug 01, 2019 at 06:45:21PM +0000, Schrempf Frieder wrote:
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> If CONFIG_GPIOLIB is not enabled, mctrl_gpio_init() and
> mctrl_gpio_init_noauto() will currently return an error pointer with
> -ENOSYS. As the mctrl GPIOs are usually optional, drivers need to
> check for this condition to allow continue probing.
> 
> To avoid the need for this check in each driver, we return NULL
> instead, as all the mctrl_gpio_*() functions are skipped anyway.
> We also adapt mctrl_gpio_to_gpiod() to be in line with this change.
> 
> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> Reviewed-by: Fabio Estevam <festevam@gmail.com>

nitpick: put your S-o-b last.

> ---
> Changes in v2
> =============
> * Move the sh_sci changes to a separate patch
> * Add a patch for the 8250 driver
> * Add Fabio's R-b tag
> ---
>  drivers/tty/serial/serial_mctrl_gpio.c | 3 +++
>  drivers/tty/serial/serial_mctrl_gpio.h | 6 +++---
>  2 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c
> index 2b400189be91..54c43e02e375 100644
> --- a/drivers/tty/serial/serial_mctrl_gpio.c
> +++ b/drivers/tty/serial/serial_mctrl_gpio.c
> @@ -61,6 +61,9 @@ EXPORT_SYMBOL_GPL(mctrl_gpio_set);
>  struct gpio_desc *mctrl_gpio_to_gpiod(struct mctrl_gpios *gpios,
>  				      enum mctrl_gpio_idx gidx)
>  {
> +	if (gpios == NULL)
> +		return NULL;
> +

I wonder why you need this. If GPIOLIB is off this code isn't active and
with GPIOLIB calling mctrl_gpio_to_gpiod with a gpios == NULL is a bug
that IMHO should not be silently ignored.

Am I missing something (again)?

>  	return gpios->gpio[gidx];
>  }

Best regards
Uwe

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

Re: [PATCH v2 2/3] serial: sh-sci: Remove check for specific mctrl_gpio_init() return value

[Uwe Kleine-König]

On Thu, Aug 01, 2019 at 06:45:24PM +0000, Schrempf Frieder wrote:
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> Now that the mctrl_gpio code returns NULL instead of ERR_PTR(-ENOSYS)
> in cases when CONFIG_GPIOLIB is disabled, we can safely remove this
> check.

I would mention -ENOSYS in the Subject line. Something like:

	serial: sh-sci: don't check for mctrl_gpio_init returning -ENOSYS

	Now that the mctrl_gpio code returns NULL instead of
	ERR_PTR(-ENOSYS) if CONFIG_GPIOLIB is disabled, we can safely
	remove this check.

Thanks
Uwe

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

Re: [PATCH v2 1/3] serial: mctrl_gpio: Avoid probe failures in case of missing gpiolib

[Schrempf Frieder]

On 01.08.19 22:33, Uwe Kleine-König wrote:
> On Thu, Aug 01, 2019 at 06:45:21PM +0000, Schrempf Frieder wrote:
>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>
>> If CONFIG_GPIOLIB is not enabled, mctrl_gpio_init() and
>> mctrl_gpio_init_noauto() will currently return an error pointer with
>> -ENOSYS. As the mctrl GPIOs are usually optional, drivers need to
>> check for this condition to allow continue probing.
>>
>> To avoid the need for this check in each driver, we return NULL
>> instead, as all the mctrl_gpio_*() functions are skipped anyway.
>> We also adapt mctrl_gpio_to_gpiod() to be in line with this change.
>>
>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>> Reviewed-by: Fabio Estevam <festevam@gmail.com>
> 
> nitpick: put your S-o-b last.

Ok.

>> ---
>> Changes in v2
>> =============
>> * Move the sh_sci changes to a separate patch
>> * Add a patch for the 8250 driver
>> * Add Fabio's R-b tag
>> ---
>>   drivers/tty/serial/serial_mctrl_gpio.c | 3 +++
>>   drivers/tty/serial/serial_mctrl_gpio.h | 6 +++---
>>   2 files changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c
>> index 2b400189be91..54c43e02e375 100644
>> --- a/drivers/tty/serial/serial_mctrl_gpio.c
>> +++ b/drivers/tty/serial/serial_mctrl_gpio.c
>> @@ -61,6 +61,9 @@ EXPORT_SYMBOL_GPL(mctrl_gpio_set);
>>   struct gpio_desc *mctrl_gpio_to_gpiod(struct mctrl_gpios *gpios,
>>   				      enum mctrl_gpio_idx gidx)
>>   {
>> +	if (gpios == NULL)
>> +		return NULL;
>> +
> 
> I wonder why you need this. If GPIOLIB is off this code isn't active and
> with GPIOLIB calling mctrl_gpio_to_gpiod with a gpios == NULL is a bug
> that IMHO should not be silently ignored.
> 
> Am I missing something (again)?

No, you're right. My thoughts were, that if the mctrl_gpio functions are 
allowed to be passed a NULL pointer in general, they all should have a 
NULL check, even if in the current context (GPIOLIB disabled) this code 
is not active. Apparently there are other cases when a NULL pointer is 
passed, see [1]. So you can't really consider gpios == NULL to be a bug 
as this seems to be allowed in general.

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=434be0ae7aa746f481c3a22139e472dbc3f4f817

> 
>>   	return gpios->gpio[gidx];
>>   }
> 
> Best regards
> Uwe
> 

Re: [PATCH v2 2/3] serial: sh-sci: Remove check for specific mctrl_gpio_init() return value

[Schrempf Frieder]

On 01.08.19 22:39, Uwe Kleine-König wrote:
> On Thu, Aug 01, 2019 at 06:45:24PM +0000, Schrempf Frieder wrote:
>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>
>> Now that the mctrl_gpio code returns NULL instead of ERR_PTR(-ENOSYS)
>> in cases when CONFIG_GPIOLIB is disabled, we can safely remove this
>> check.
> 
> I would mention -ENOSYS in the Subject line. Something like:
> 
> 	serial: sh-sci: don't check for mctrl_gpio_init returning -ENOSYS
> 
> 	Now that the mctrl_gpio code returns NULL instead of
> 	ERR_PTR(-ENOSYS) if CONFIG_GPIOLIB is disabled, we can safely
> 	remove this check.

Indeed, I failed to come up with a better subject line. I will adopt 
your proposal.

> Thanks
> Uwe
> 

Re: [PATCH v2 1/3] serial: mctrl_gpio: Avoid probe failures in case of missing gpiolib

[Uwe Kleine-König]

Hello,

On Fri, Aug 02, 2019 at 07:56:54AM +0000, Schrempf Frieder wrote:
> On 01.08.19 22:33, Uwe Kleine-König wrote:
> > On Thu, Aug 01, 2019 at 06:45:21PM +0000, Schrempf Frieder wrote:
> >> diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c
> >> index 2b400189be91..54c43e02e375 100644
> >> --- a/drivers/tty/serial/serial_mctrl_gpio.c
> >> +++ b/drivers/tty/serial/serial_mctrl_gpio.c
> >> @@ -61,6 +61,9 @@ EXPORT_SYMBOL_GPL(mctrl_gpio_set);
> >>   struct gpio_desc *mctrl_gpio_to_gpiod(struct mctrl_gpios *gpios,
> >>   				      enum mctrl_gpio_idx gidx)
> >>   {
> >> +	if (gpios == NULL)
> >> +		return NULL;
> >> +
> > 
> > I wonder why you need this. If GPIOLIB is off this code isn't active and
> > with GPIOLIB calling mctrl_gpio_to_gpiod with a gpios == NULL is a bug
> > that IMHO should not be silently ignored.
> > 
> > Am I missing something (again)?
> 
> No, you're right. My thoughts were, that if the mctrl_gpio functions are 
> allowed to be passed a NULL pointer in general, they all should have a 
> NULL check, even if in the current context (GPIOLIB disabled) this code 
> is not active. Apparently there are other cases when a NULL pointer is 
> passed, see [1]. So you can't really consider gpios == NULL to be a bug 
> as this seems to be allowed in general.

OK, then this is another separate commit, right? Preferably with a
comment pointing to drivers that use mctrl_gpio before being
initialized.

Best regards
Uwe

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