[4/7] pinctrl: sh-pfc: Stop calling gpiochip_add_pin_range() on DT platforms
diff mbox

Message ID 1435650327-2542-5-git-send-email-geert+renesas@glider.be
State New
Headers show

Commit Message

Geert Uytterhoeven June 30, 2015, 7:45 a.m. UTC
On platforms where the PFC/GPIO controller is instantiated from DT, the
mapping between GPIOs and pins is set up using the "gpio-ranges"
property in DT.

Hence stop setting up the mapping from C code on DT platforms.
This code is still used for SH or ARM-legacy platforms.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/pinctrl/sh-pfc/gpio.c | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

Comments

Laurent Pinchart June 30, 2015, 9:30 a.m. UTC | #1
Hi Geert,

Thank you for the patch.

On Tuesday 30 June 2015 09:45:24 Geert Uytterhoeven wrote:
> On platforms where the PFC/GPIO controller is instantiated from DT, the
> mapping between GPIOs and pins is set up using the "gpio-ranges"
> property in DT.
> 
> Hence stop setting up the mapping from C code on DT platforms.
> This code is still used for SH or ARM-legacy platforms.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/pinctrl/sh-pfc/gpio.c | 36 ++++++++++++++++++++----------------
>  1 file changed, 20 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/pinctrl/sh-pfc/gpio.c b/drivers/pinctrl/sh-pfc/gpio.c
> index ba353735ecf2be9a..1be118e4865fd3f8 100644
> --- a/drivers/pinctrl/sh-pfc/gpio.c
> +++ b/drivers/pinctrl/sh-pfc/gpio.c
> @@ -379,22 +379,26 @@ int sh_pfc_register_gpiochip(struct sh_pfc *pfc)
> 
>  	pfc->gpio = chip;
> 
> -	/* Register the GPIO to pin mappings. As pins with GPIO ports must come
> -	 * first in the ranges, skip the pins without GPIO ports by stopping at
> -	 * the first range that contains such a pin.
> -	 */
> -	for (i = 0; i < pfc->nr_ranges; ++i) {
> -		const struct sh_pfc_pin_range *range = &pfc->ranges[i];
> -
> -		if (range->start >= pfc->nr_gpio_pins)
> -			break;
> -
> -		ret = gpiochip_add_pin_range(&chip->gpio_chip,
> -					     dev_name(pfc->dev),
> -					     range->start, range->start,
> -					     range->end - range->start + 1);
> -		if (ret < 0)
> -			return ret;
> +	if (IS_ENABLED(CONFIG_SUPERH) ||
> +	    IS_ENABLED(CONFIG_ARCH_SHMOBILE_LEGACY)) {

I'd prefer checking IS_ENABLED(CONFIG_OF) && pfc->dev->of_node as that doesn't 
explicitly depend on the platform type.

As the code after this if block also don't need to run on non-DT platforms, 
how about just using

	if (IS_ENABLED(CONFIG_OF) && pfc->dev->of_node)
		return 0;

?

> +		/*
> +		 * Register the GPIO to pin mappings. As pins with GPIO ports
> +		 * must come first in the ranges, skip the pins without GPIO
> +		 * ports by stopping at the first range that contains such a
> +		 * pin.
> +		 */
> +		for (i = 0; i < pfc->nr_ranges; ++i) {
> +			const struct sh_pfc_pin_range *range = &pfc->ranges[i];
> +
> +			if (range->start >= pfc->nr_gpio_pins)
> +				break;
> +
> +			ret = gpiochip_add_pin_range(&chip->gpio_chip,
> +				dev_name(pfc->dev), range->start, range->start,
> +				range->end - range->start + 1);
> +			if (ret < 0)
> +				return ret;
> +		}
>  	}
> 
>  	/* Register the function GPIOs chip. */
Geert Uytterhoeven June 30, 2015, 9:44 a.m. UTC | #2
Hi Laurent,

On Tue, Jun 30, 2015 at 11:30 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Tuesday 30 June 2015 09:45:24 Geert Uytterhoeven wrote:
>> On platforms where the PFC/GPIO controller is instantiated from DT, the
>> mapping between GPIOs and pins is set up using the "gpio-ranges"
>> property in DT.
>>
>> Hence stop setting up the mapping from C code on DT platforms.
>> This code is still used for SH or ARM-legacy platforms.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>>  drivers/pinctrl/sh-pfc/gpio.c | 36 ++++++++++++++++++++----------------
>>  1 file changed, 20 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/pinctrl/sh-pfc/gpio.c b/drivers/pinctrl/sh-pfc/gpio.c
>> index ba353735ecf2be9a..1be118e4865fd3f8 100644
>> --- a/drivers/pinctrl/sh-pfc/gpio.c
>> +++ b/drivers/pinctrl/sh-pfc/gpio.c
>> @@ -379,22 +379,26 @@ int sh_pfc_register_gpiochip(struct sh_pfc *pfc)
>>
>>       pfc->gpio = chip;
>>
>> -     /* Register the GPIO to pin mappings. As pins with GPIO ports must come
>> -      * first in the ranges, skip the pins without GPIO ports by stopping at
>> -      * the first range that contains such a pin.
>> -      */
>> -     for (i = 0; i < pfc->nr_ranges; ++i) {
>> -             const struct sh_pfc_pin_range *range = &pfc->ranges[i];
>> -
>> -             if (range->start >= pfc->nr_gpio_pins)
>> -                     break;
>> -
>> -             ret = gpiochip_add_pin_range(&chip->gpio_chip,
>> -                                          dev_name(pfc->dev),
>> -                                          range->start, range->start,
>> -                                          range->end - range->start + 1);
>> -             if (ret < 0)
>> -                     return ret;
>> +     if (IS_ENABLED(CONFIG_SUPERH) ||
>> +         IS_ENABLED(CONFIG_ARCH_SHMOBILE_LEGACY)) {
>
> I'd prefer checking IS_ENABLED(CONFIG_OF) && pfc->dev->of_node as that doesn't
> explicitly depend on the platform type.

Note that CONFIG_OF is also set for ARM-legacy (and soon for SH). But that's
a minor problem. But due to the runtime check on pfc->dev->of_node, the
unused code below won't be left out by the compiler, and I want to get rid
of that code.

The platform check make it clear when the code can be removed.

> As the code after this if block also don't need to run on non-DT platforms,
> how about just using
>
>         if (IS_ENABLED(CONFIG_OF) && pfc->dev->of_node)
>                 return 0;
>
> ?

Early return is indeed an option, as we don't need the function GPIOs on
DT platforms.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Laurent Pinchart June 30, 2015, 9:54 a.m. UTC | #3
Hi Geert,

On Tuesday 30 June 2015 11:44:24 Geert Uytterhoeven wrote:
> On Tue, Jun 30, 2015 at 11:30 AM, Laurent Pinchart wrote:
> > On Tuesday 30 June 2015 09:45:24 Geert Uytterhoeven wrote:
> >> On platforms where the PFC/GPIO controller is instantiated from DT, the
> >> mapping between GPIOs and pins is set up using the "gpio-ranges"
> >> property in DT.
> >> 
> >> Hence stop setting up the mapping from C code on DT platforms.
> >> This code is still used for SH or ARM-legacy platforms.
> >> 
> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >> ---
> >> 
> >>  drivers/pinctrl/sh-pfc/gpio.c | 36 ++++++++++++++++++++----------------
> >>  1 file changed, 20 insertions(+), 16 deletions(-)
> >> 
> >> diff --git a/drivers/pinctrl/sh-pfc/gpio.c
> >> b/drivers/pinctrl/sh-pfc/gpio.c
> >> index ba353735ecf2be9a..1be118e4865fd3f8 100644
> >> --- a/drivers/pinctrl/sh-pfc/gpio.c
> >> +++ b/drivers/pinctrl/sh-pfc/gpio.c
> >> @@ -379,22 +379,26 @@ int sh_pfc_register_gpiochip(struct sh_pfc *pfc)
> >> 
> >>       pfc->gpio = chip;
> >> 
> >> -     /* Register the GPIO to pin mappings. As pins with GPIO ports must
> >> come
> >> -      * first in the ranges, skip the pins without GPIO ports by
> >> stopping at
> >> -      * the first range that contains such a pin.
> >> -      */
> >> -     for (i = 0; i < pfc->nr_ranges; ++i) {
> >> -             const struct sh_pfc_pin_range *range = &pfc->ranges[i];
> >> -
> >> -             if (range->start >= pfc->nr_gpio_pins)
> >> -                     break;
> >> -
> >> -             ret = gpiochip_add_pin_range(&chip->gpio_chip,
> >> -                                          dev_name(pfc->dev),
> >> -                                          range->start, range->start,
> >> -                                          range->end - range->start +
> >> 1);
> >> -             if (ret < 0)
> >> -                     return ret;
> >> +     if (IS_ENABLED(CONFIG_SUPERH) ||
> >> +         IS_ENABLED(CONFIG_ARCH_SHMOBILE_LEGACY)) {
> > 
> > I'd prefer checking IS_ENABLED(CONFIG_OF) && pfc->dev->of_node as that
> > doesn't explicitly depend on the platform type.
> 
> Note that CONFIG_OF is also set for ARM-legacy (and soon for SH). But that's
> a minor problem. But due to the runtime check on pfc->dev->of_node, the
> unused code below won't be left out by the compiler, and I want to get rid
> of that code.
> 
> The platform check make it clear when the code can be removed.

Not any time soon I'm afraid as arch/sh won't fully move to DT in the near 
future, but ARCH_SHMOBILE_LEGACY should go away soon, so I'm fine with 
compile-time optimization.

> > As the code after this if block also don't need to run on non-DT
> > platforms,
> > how about just using
> > 
> >         if (IS_ENABLED(CONFIG_OF) && pfc->dev->of_node)
> >         
> >                 return 0;
> > 
> > ?
> 
> Early return is indeed an option, as we don't need the function GPIOs on
> DT platforms.

Patch
diff mbox

diff --git a/drivers/pinctrl/sh-pfc/gpio.c b/drivers/pinctrl/sh-pfc/gpio.c
index ba353735ecf2be9a..1be118e4865fd3f8 100644
--- a/drivers/pinctrl/sh-pfc/gpio.c
+++ b/drivers/pinctrl/sh-pfc/gpio.c
@@ -379,22 +379,26 @@  int sh_pfc_register_gpiochip(struct sh_pfc *pfc)
 
 	pfc->gpio = chip;
 
-	/* Register the GPIO to pin mappings. As pins with GPIO ports must come
-	 * first in the ranges, skip the pins without GPIO ports by stopping at
-	 * the first range that contains such a pin.
-	 */
-	for (i = 0; i < pfc->nr_ranges; ++i) {
-		const struct sh_pfc_pin_range *range = &pfc->ranges[i];
-
-		if (range->start >= pfc->nr_gpio_pins)
-			break;
-
-		ret = gpiochip_add_pin_range(&chip->gpio_chip,
-					     dev_name(pfc->dev),
-					     range->start, range->start,
-					     range->end - range->start + 1);
-		if (ret < 0)
-			return ret;
+	if (IS_ENABLED(CONFIG_SUPERH) ||
+	    IS_ENABLED(CONFIG_ARCH_SHMOBILE_LEGACY)) {
+		/*
+		 * Register the GPIO to pin mappings. As pins with GPIO ports
+		 * must come first in the ranges, skip the pins without GPIO
+		 * ports by stopping at the first range that contains such a
+		 * pin.
+		 */
+		for (i = 0; i < pfc->nr_ranges; ++i) {
+			const struct sh_pfc_pin_range *range = &pfc->ranges[i];
+
+			if (range->start >= pfc->nr_gpio_pins)
+				break;
+
+			ret = gpiochip_add_pin_range(&chip->gpio_chip,
+				dev_name(pfc->dev), range->start, range->start,
+				range->end - range->start + 1);
+			if (ret < 0)
+				return ret;
+		}
 	}
 
 	/* Register the function GPIOs chip. */