[PATCH/RFC 06/12] sh-pfc: Skip gpiochip registration when no GPIO resource is found

[PATCH/RFC 06/12] sh-pfc: Skip gpiochip registration when no GPIO resource is found

[Laurent Pinchart]

Boards/platforms that register dedicated GPIO devices will not supply a
memory resource for GPIOs. Try to locate the GPIO memory resource at
initialization time, and skip registration of the gpiochip if the
resource can't be found.

This is a temporary modification to ease the transition to separate GPIO
drivers. It should be reverted when all boards and platforms will have
been moved.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/pinctrl/sh-pfc/gpio.c | 39 +++++++++++++++++++++------------------
 1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/pinctrl/sh-pfc/gpio.c b/drivers/pinctrl/sh-pfc/gpio.c
index 317cebb..d37efa7 100644
--- a/drivers/pinctrl/sh-pfc/gpio.c
+++ b/drivers/pinctrl/sh-pfc/gpio.c
@@ -101,24 +101,9 @@ static void gpio_setup_data_reg(struct sh_pfc_chip *chip, unsigned gpio)
 static int gpio_setup_data_regs(struct sh_pfc_chip *chip)
 {
 	struct sh_pfc *pfc = chip->pfc;
-	unsigned long addr = pfc->info->data_regs[0].reg;
 	const struct pinmux_data_reg *dreg;
 	unsigned int i;
 
-	/* Find the window that contain the GPIO registers. */
-	for (i = 0; i < pfc->num_windows; ++i) {
-		struct sh_pfc_window *window = &pfc->window[i];
-
-		if (addr >= window->phys && addr < window->phys + window->size)
-			break;
-	}
-
-	if (i = pfc->num_windows)
-		return -EINVAL;
-
-	/* GPIO data registers must be in the first memory resource. */
-	chip->mem = &pfc->window[i];
-
 	/* Count the number of data registers, allocate memory and initialize
 	 * them.
 	 */
@@ -319,7 +304,8 @@ static int gpio_function_setup(struct sh_pfc_chip *chip)
  */
 
 static struct sh_pfc_chip *
-sh_pfc_add_gpiochip(struct sh_pfc *pfc, int(*setup)(struct sh_pfc_chip *))
+sh_pfc_add_gpiochip(struct sh_pfc *pfc, int(*setup)(struct sh_pfc_chip *),
+		    struct sh_pfc_window *mem)
 {
 	struct sh_pfc_chip *chip;
 	int ret;
@@ -328,6 +314,7 @@ sh_pfc_add_gpiochip(struct sh_pfc *pfc, int(*setup)(struct sh_pfc_chip *))
 	if (unlikely(!chip))
 		return ERR_PTR(-ENOMEM);
 
+	chip->mem = mem;
 	chip->pfc = pfc;
 
 	ret = setup(chip);
@@ -357,8 +344,24 @@ int sh_pfc_register_gpiochip(struct sh_pfc *pfc)
 	if (pfc->info->data_regs = NULL)
 		return 0;
 
+	/* Find the memory window that contain the GPIO registers. Boards that
+	 * register a separate GPIO device will not supply a memory resource
+	 * that covers the data registers. In that case don't try to handle
+	 * GPIOs.
+	 */
+	for (i = 0; i < pfc->num_windows; ++i) {
+		struct sh_pfc_window *window = &pfc->window[i];
+
+		if (pfc->info->data_regs[0].reg >= window->phys &&
+		    pfc->info->data_regs[0].reg < window->phys + window->size)
+			break;
+	}
+
+	if (i = pfc->num_windows)
+		return 0;
+
 	/* Register the real GPIOs chip. */
-	chip = sh_pfc_add_gpiochip(pfc, gpio_pin_setup);
+	chip = sh_pfc_add_gpiochip(pfc, gpio_pin_setup, &pfc->window[i]);
 	if (IS_ERR(chip))
 		return PTR_ERR(chip);
 
@@ -390,7 +393,7 @@ int sh_pfc_register_gpiochip(struct sh_pfc *pfc)
 	if (pfc->info->nr_func_gpios = 0)
 		return 0;
 
-	chip = sh_pfc_add_gpiochip(pfc, gpio_function_setup);
+	chip = sh_pfc_add_gpiochip(pfc, gpio_function_setup, NULL);
 	if (IS_ERR(chip))
 		return PTR_ERR(chip);
 
-- 
1.8.1.5

Re: [PATCH/RFC 06/12] sh-pfc: Skip gpiochip registration when no GPIO resource is found

[Linus Walleij]

On Sun, Mar 10, 2013 at 7:58 PM, Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:

> Boards/platforms that register dedicated GPIO devices will not supply a
> memory resource for GPIOs. Try to locate the GPIO memory resource at
> initialization time, and skip registration of the gpiochip if the
> resource can't be found.

Isn't this a bit misleading? By memory resource we usually understand
something that will pass a struct resource connected to e.g. a
platform_device, whereas...

(...)
> @@ -328,6 +314,7 @@ sh_pfc_add_gpiochip(struct sh_pfc *pfc, int(*setup)(struct sh_pfc_chip *))
>         if (unlikely(!chip))
>                 return ERR_PTR(-ENOMEM);
>
> +       chip->mem = mem;
>         chip->pfc = pfc;
>
>         ret = setup(chip);
> @@ -357,8 +344,24 @@ int sh_pfc_register_gpiochip(struct sh_pfc *pfc)
>         if (pfc->info->data_regs = NULL)
>                 return 0;
>
> +       /* Find the memory window that contain the GPIO registers. Boards that
> +        * register a separate GPIO device will not supply a memory resource
> +        * that covers the data registers. In that case don't try to handle
> +        * GPIOs.
> +        */
> +       for (i = 0; i < pfc->num_windows; ++i) {
> +               struct sh_pfc_window *window = &pfc->window[i];
> +
> +               if (pfc->info->data_regs[0].reg >= window->phys &&
> +                   pfc->info->data_regs[0].reg < window->phys + window->size)
> +                       break;
> +       }

This "pfc" isn't quite a "resource". I would call that platform data or
something.

But maybe this could be refactored to actually be a number of
IOMEM resources?

> +       if (i = pfc->num_windows)
> +               return 0;

The ++i and this thing make the code pretty hard for me to
grasp. But maybe I'm just dumb? Could you explain here what
is happening, like with some small comment or so....

Yours,
Linus Walleij

Re: [PATCH/RFC 06/12] sh-pfc: Skip gpiochip registration when no GPIO resource is found

[Laurent Pinchart]

Hi Linus,

On Wednesday 13 March 2013 18:37:03 Linus Walleij wrote:
> On Sun, Mar 10, 2013 at 7:58 PM, Laurent Pinchart wrote:
> > Boards/platforms that register dedicated GPIO devices will not supply a
> > memory resource for GPIOs. Try to locate the GPIO memory resource at
> > initialization time, and skip registration of the gpiochip if the
> > resource can't be found.
> 
> Isn't this a bit misleading? By memory resource we usually understand
> something that will pass a struct resource connected to e.g. a
> platform_device,

Yes, that's exactly what this patch is about.

> whereas...
> 
> (...)
> 
> > @@ -328,6 +314,7 @@ sh_pfc_add_gpiochip(struct sh_pfc *pfc,
> > int(*setup)(struct sh_pfc_chip *))> 
> >         if (unlikely(!chip))
> >                 return ERR_PTR(-ENOMEM);
> > 
> > +       chip->mem = mem;
> >         chip->pfc = pfc;
> >         
> >         ret = setup(chip);
> > @@ -357,8 +344,24 @@ int sh_pfc_register_gpiochip(struct sh_pfc *pfc)
> >         if (pfc->info->data_regs = NULL)
> >                 return 0;
> > 
> > +   /* Find the memory window that contain the GPIO registers. Boards that
> > +    * register a separate GPIO device will not supply a memory resource
> > +    * that covers the data registers. In that case don't try to handle
> > +    * GPIOs.
> > +    */
> > +       for (i = 0; i < pfc->num_windows; ++i) {
> > +               struct sh_pfc_window *window = &pfc->window[i];
> > +
> > +               if (pfc->info->data_regs[0].reg >= window->phys &&
> > +                   pfc->info->data_regs[0].reg < window->phys +
> > window->size)
> > +                       break;
> > +       }
> 
> This "pfc" isn't quite a "resource". I would call that platform data or
> something.
> 
> But maybe this could be refactored to actually be a number of
> IOMEM resources?

pfc->info->data_regs is a list of SoC-specific information about GPIO 
registers. It contains, among other information, the register physical address 
in the reg field. pfc->window is a list of ioremapped IOMEM resources.

The above code thus tries to locate the IOMEM resource that contains the first 
GPIO data register. If no such resource is found then this patch assumes that 
GPIO will be handled externally.

The main purpose of this code is to allow an atomic transition from GPIO 
handling inside the PFC driver to GPIO handling by a separate driver without 
requiring a patch to touch both arch/arm/mach-shmobile/ and 
drivers/pinctrl/sh-pfc/. Such a transition only requires adding the GPIO 
platform device and removing the GPIO IOMEM resource from the PFC device. A 
later patch can then remove the pfc->info->data_regs completely from SoC data 
in drivers/pinctrl/sh-pfc/pfc-*.c.

> > +       if (i = pfc->num_windows)
> > +               return 0;
> 
> The ++i and this thing make the code pretty hard for me to grasp. But maybe
> I'm just dumb? Could you explain here what is happening, like with some
> small comment or so....

++i and i++ would be totally equivalent in the for loop above. I took the 
habit of using the pre-increment style in for loops from my C++ programming 
days (http://forums.codeguru.com/showthread.php?231052-C-Operator-Why-should-I-use-i-instead-of-i).

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 06/12] sh-pfc: Skip gpiochip registration when no GPIO resource is found

[Linus Walleij]

On Wed, Mar 13, 2013 at 9:51 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:

>> But maybe this could be refactored to actually be a number of
>> IOMEM resources?
>
> pfc->info->data_regs is a list of SoC-specific information about GPIO
> registers. It contains, among other information, the register physical address
> in the reg field. pfc->window is a list of ioremapped IOMEM resources.

So the thing is that this array of resources are handled elsewhere, where
they are ioremapped and stashed into pfc->window, OK following now.

> The main purpose of this code is to allow an atomic transition from GPIO
> handling inside the PFC driver to GPIO handling by a separate driver without
> requiring a patch to touch both arch/arm/mach-shmobile/ and
> drivers/pinctrl/sh-pfc/. Such a transition only requires adding the GPIO
> platform device and removing the GPIO IOMEM resource from the PFC device. A
> later patch can then remove the pfc->info->data_regs completely from SoC data
> in drivers/pinctrl/sh-pfc/pfc-*.c.

OK I buy this argument!

Acked-by: Linus Walleij <linus.walleij@linaro.org>

>> The ++i and this thing make the code pretty hard for me to grasp. But maybe
>> I'm just dumb? Could you explain here what is happening, like with some
>> small comment or so....
>
> ++i and i++ would be totally equivalent in the for loop above. I took the
> habit of using the pre-increment style in for loops from my C++ programming
> days (http://forums.codeguru.com/showthread.php?231052-C-Operator-Why-should-I-use-i-instead-of-i).

Interesting, that will be very useful the day we rewrite the kernel in
C++ ;-)

Yours,
Linus Walleij