[patch 1/2] Add dt_compat field to struct gpio_chip

[patch 1/2] Add dt_compat field to struct gpio_chip

[Domenico Andreoli]

From: Domenico Andreoli <cavokz@gmail.com>

This new field allows easy creation of GPIO chips in base of struct arrays.

Signed-off-by: Domenico Andreoli <cavokz@gmail.com>

---
 drivers/of/gpio.c          |    3 +++
 include/asm-generic/gpio.h |    1 +
 2 files changed, 4 insertions(+)

Index: b/drivers/of/gpio.c
===================================================================
--- a/drivers/of/gpio.c	2011-04-07 18:19:20.000000000 +0200
+++ b/drivers/of/gpio.c	2011-04-07 18:20:31.000000000 +0200
@@ -212,6 +212,9 @@
 
 void of_gpiochip_add(struct gpio_chip *chip)
 {
+	if ((!chip->of_node) && (chip->dt_compat))
+		chip->of_node = of_find_compatible_node(NULL, NULL, chip->dt_compat);
+
 	if ((!chip->of_node) && (chip->dev))
 		chip->of_node = chip->dev->of_node;
 
Index: b/include/asm-generic/gpio.h
===================================================================
--- a/include/asm-generic/gpio.h	2011-04-07 18:19:20.000000000 +0200
+++ b/include/asm-generic/gpio.h	2011-04-07 18:19:30.000000000 +0200
@@ -129,6 +129,7 @@
 	int of_gpio_n_cells;
 	int (*of_xlate)(struct gpio_chip *gc, struct device_node *np,
 		        const void *gpio_spec, u32 *flags);
+	const char *dt_compat;
 #endif
 };
 

[patch 1/2] Add dt_compat field to struct gpio_chip

[Grant Likely]

On Thu, Apr 07, 2011 at 06:39:30PM +0200, Domenico Andreoli wrote:
> From: Domenico Andreoli <cavokz@gmail.com>
> 
> This new field allows easy creation of GPIO chips in base of struct arrays.
> 
> Signed-off-by: Domenico Andreoli <cavokz@gmail.com>
> 
> ---
>  drivers/of/gpio.c          |    3 +++
>  include/asm-generic/gpio.h |    1 +
>  2 files changed, 4 insertions(+)
> 
> Index: b/drivers/of/gpio.c
> ===================================================================
> --- a/drivers/of/gpio.c	2011-04-07 18:19:20.000000000 +0200
> +++ b/drivers/of/gpio.c	2011-04-07 18:20:31.000000000 +0200
> @@ -212,6 +212,9 @@
>  
>  void of_gpiochip_add(struct gpio_chip *chip)
>  {
> +	if ((!chip->of_node) && (chip->dt_compat))
> +		chip->of_node = of_find_compatible_node(NULL, NULL, chip->dt_compat);
> +

Hi Domenico,

Thanks for looking at this.

However, I think there is a better way to solve this problem,

[patch 1/2] Add dt_compat field to struct gpio_chip

[Domenico Andreoli]

On Thu, Apr 7, 2011 at 7:17 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
>
> On Thu, Apr 07, 2011 at 06:39:30PM +0200, Domenico Andreoli wrote:
> > From: Domenico Andreoli <cavokz@gmail.com>
> >
> > This new field allows easy creation of GPIO chips in base of struct arrays.
> >
> > Signed-off-by: Domenico Andreoli <cavokz@gmail.com>
> >
> > ---
> > ?drivers/of/gpio.c ? ? ? ? ?| ? ?3 +++
> > ?include/asm-generic/gpio.h | ? ?1 +
> > ?2 files changed, 4 insertions(+)
> >
> > Index: b/drivers/of/gpio.c
> > ===================================================================
> > --- a/drivers/of/gpio.c ? ? ? 2011-04-07 18:19:20.000000000 +0200
> > +++ b/drivers/of/gpio.c ? ? ? 2011-04-07 18:20:31.000000000 +0200
> > @@ -212,6 +212,9 @@
> >
> > ?void of_gpiochip_add(struct gpio_chip *chip)
> > ?{
> > + ? ? if ((!chip->of_node) && (chip->dt_compat))
> > + ? ? ? ? ? ? chip->of_node = of_find_compatible_node(NULL, NULL, chip->dt_compat);
> > +
>
> Hi Domenico,

Hi Grant,

> From what I can tell, this patch attempts to adapt to the way that
> arch/arm/plat-samsung/gpio.c registers gpios with the kernel. ?Each
> platform seems to have its own static table of gpio banks which are
> registered without any regard to the Linux device model. ?It works,
> but it isn't really the way things should be done.

About the independent models of the ARM platforms, I can't say
anything. I only wanted to explore the DT concepts and import them on
the s3c24xx. While this could look only as the latest individual
attempt, the DT interface is not and cannot be of any harm if ARM
deploys it a little more.

> The design of gpiolib right now is such that the of_node pointer
> should be known *before* of_gpiochip_add() gets called. ?This is very
> important because the code that registers the gpio is the only place
> that can truly know which node is actually associated with the gpio.

This explains why a so simple patch has never been proposed before.

> To solve your problem, the best solution would be to rework
> arch/arm/plat-samsung-gpio.c to properly use the driver model and
> register platform_devices which can be attached to dt nodes. ?This
> change should probably be done anyway, even ignoring the dt needs, but
> I'm not going to force you to make this change to get dt support added
> for samsung gpios.

Indeed my plan is to not rework it, not in the next future, not before
I switched all its users to DT ;)

I'd like to follow the path towards a pure dts platform definition so
the next step would be to DT-fy something like the s3c
sdi/sdhci/whatever, which already uses some GPIO. Then something else
like audio or network and so on. I would be very glad to arrive sooner
or later to a s3c24xx FDT.

> Alternately, what you should do is make sure that the chip->of_node
> pointer is correctly populated before calling gpiochip_add(), possibly
> in s3c_gpiolib_add().

I already restored it to the previous state, with .dt_compat in struct
s3c_gpio_chip and the the call of of_find_compatible_node() in
s3c_gpiolib_add(), this way it stays out of gpiolib.

> Also, be careful about the way that 'compatible' is being used.
> Remember that compatible describes the /interface/ to a device, but
> not the /instance/. ?Many systems have multiple instances of the same
> device, and compatible doesn't provide any help for differentiating
> between them.

I needed some clarification here, I already suspected that my way to
encode banks in the compatible property was somehow fishy. And I
needed a working patch to get some attention before digging further..
;)

> Typically, when trying to find a specific instance of a
> device, it should be resolved with a property in the /aliases node, or
> it should be matched up against the resolved base address of the
> device.

This is what I'll try next. This alone seems to imply the rework you
said above, let's see.

> Cheers,
> g.

Thank you.

cheers,
Domenico

-----[ Domenico Andreoli, aka cavok
?--[ http://www.dandreoli.com/gpgkey.asc
?? ---[ 3A0F 2F80 F79C 678A 8936? 4FEE 0677 9033 A20E BC50

[patch 1/2] Add dt_compat field to struct gpio_chip

[Grant Likely]

On Thu, Apr 7, 2011 at 1:41 PM, Domenico Andreoli <cavokz@gmail.com> wrote:
> On Thu, Apr 7, 2011 at 7:17 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
>>
>> On Thu, Apr 07, 2011 at 06:39:30PM +0200, Domenico Andreoli wrote:
>> > From: Domenico Andreoli <cavokz@gmail.com>
>> >
>> > This new field allows easy creation of GPIO chips in base of struct arrays.
>> >
>> > Signed-off-by: Domenico Andreoli <cavokz@gmail.com>
>> >
>> > ---
>> > ?drivers/of/gpio.c ? ? ? ? ?| ? ?3 +++
>> > ?include/asm-generic/gpio.h | ? ?1 +
>> > ?2 files changed, 4 insertions(+)
>> >
>> > Index: b/drivers/of/gpio.c
>> > ===================================================================
>> > --- a/drivers/of/gpio.c ? ? ? 2011-04-07 18:19:20.000000000 +0200
>> > +++ b/drivers/of/gpio.c ? ? ? 2011-04-07 18:20:31.000000000 +0200
>> > @@ -212,6 +212,9 @@
>> >
>> > ?void of_gpiochip_add(struct gpio_chip *chip)
>> > ?{
>> > + ? ? if ((!chip->of_node) && (chip->dt_compat))
>> > + ? ? ? ? ? ? chip->of_node = of_find_compatible_node(NULL, NULL, chip->dt_compat);
>> > +
>>
>> Hi Domenico,
>
> Hi Grant,
>
>> From what I can tell, this patch attempts to adapt to the way that
>> arch/arm/plat-samsung/gpio.c registers gpios with the kernel. ?Each
>> platform seems to have its own static table of gpio banks which are
>> registered without any regard to the Linux device model. ?It works,
>> but it isn't really the way things should be done.
>
> About the independent models of the ARM platforms, I can't say
> anything. I only wanted to explore the DT concepts and import them on
> the s3c24xx. While this could look only as the latest individual
> attempt, the DT interface is not and cannot be of any harm if ARM
> deploys it a little more.
>
>> The design of gpiolib right now is such that the of_node pointer
>> should be known *before* of_gpiochip_add() gets called. ?This is very
>> important because the code that registers the gpio is the only place
>> that can truly know which node is actually associated with the gpio.
>
> This explains why a so simple patch has never been proposed before.
>
>> To solve your problem, the best solution would be to rework
>> arch/arm/plat-samsung-gpio.c to properly use the driver model and
>> register platform_devices which can be attached to dt nodes. ?This
>> change should probably be done anyway, even ignoring the dt needs, but
>> I'm not going to force you to make this change to get dt support added
>> for samsung gpios.
>
> Indeed my plan is to not rework it, not in the next future, not before
> I switched all its users to DT ;)
>
> I'd like to follow the path towards a pure dts platform definition so
> the next step would be to DT-fy something like the s3c
> sdi/sdhci/whatever, which already uses some GPIO. Then something else
> like audio or network and so on. I would be very glad to arrive sooner
> or later to a s3c24xx FDT.
>
>> Alternately, what you should do is make sure that the chip->of_node
>> pointer is correctly populated before calling gpiochip_add(), possibly
>> in s3c_gpiolib_add().
>
> I already restored it to the previous state, with .dt_compat in struct
> s3c_gpio_chip and the the call of of_find_compatible_node() in
> s3c_gpiolib_add(), this way it stays out of gpiolib.
>
>> Also, be careful about the way that 'compatible' is being used.
>> Remember that compatible describes the /interface/ to a device, but
>> not the /instance/. ?Many systems have multiple instances of the same
>> device, and compatible doesn't provide any help for differentiating
>> between them.
>
> I needed some clarification here, I already suspected that my way to
> encode banks in the compatible property was somehow fishy. And I
> needed a working patch to get some attention before digging further..
> ;)
>
>> Typically, when trying to find a specific instance of a
>> device, it should be resolved with a property in the /aliases node, or
>> it should be matched up against the resolved base address of the
>> device.
>
> This is what I'll try next. This alone seems to imply the rework you
> said above, let's see.

It doesn't actually depend on the rework.  You already have the
register information in s3c gpio.  You could use the same compatible
value for all, and then add another comparison against the base
address to figure out exactly which one is the correct one.

g.

[patch 2/2] Add GPIO DT support to s3c24xx

[Domenico Andreoli]

From: Domenico Andreoli <cavokz@gmail.com>

Add DT compat strings to the GPIO chips registerd by s3c24xx SOCs.

Signed-off-by: Domenico Andreoli <cavokz@gmail.com>

---
 arch/arm/plat-s3c24xx/gpiolib.c |   36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

Index: b/arch/arm/plat-s3c24xx/gpiolib.c
===================================================================
--- a/arch/arm/plat-s3c24xx/gpiolib.c	2011-04-07 18:16:09.000000000 +0200
+++ b/arch/arm/plat-s3c24xx/gpiolib.c	2011-04-07 18:20:41.000000000 +0200
@@ -94,6 +94,9 @@
 			.owner			= THIS_MODULE,
 			.label			= "GPIOA",
 			.ngpio			= 24,
+#if defined(CONFIG_OF_GPIO)
+			.dt_compat		= "samsung,s3c2410-gpio-a",
+#endif
 			.direction_input	= s3c24xx_gpiolib_banka_input,
 			.direction_output	= s3c24xx_gpiolib_banka_output,
 		},
@@ -106,6 +109,9 @@
 			.owner			= THIS_MODULE,
 			.label			= "GPIOB",
 			.ngpio			= 16,
+#if defined(CONFIG_OF_GPIO)
+			.dt_compat		= "samsung,s3c2410-gpio-b",
+#endif
 		},
 	},
 	[2] = {
@@ -116,6 +122,9 @@
 			.owner			= THIS_MODULE,
 			.label			= "GPIOC",
 			.ngpio			= 16,
+#if defined(CONFIG_OF_GPIO)
+			.dt_compat		= "samsung,s3c2410-gpio-c",
+#endif
 		},
 	},
 	[3] = {
@@ -126,6 +135,9 @@
 			.owner			= THIS_MODULE,
 			.label			= "GPIOD",
 			.ngpio			= 16,
+#if defined(CONFIG_OF_GPIO)
+			.dt_compat		= "samsung,s3c2410-gpio-d",
+#endif
 		},
 	},
 	[4] = {
@@ -136,6 +148,9 @@
 			.label			= "GPIOE",
 			.owner			= THIS_MODULE,
 			.ngpio			= 16,
+#if defined(CONFIG_OF_GPIO)
+			.dt_compat		= "samsung,s3c2410-gpio-e",
+#endif
 		},
 	},
 	[5] = {
@@ -147,6 +162,9 @@
 			.label			= "GPIOF",
 			.ngpio			= 8,
 			.to_irq			= s3c24xx_gpiolib_bankf_toirq,
+#if defined(CONFIG_OF_GPIO)
+			.dt_compat		= "samsung,s3c2410-gpio-f",
+#endif
 		},
 	},
 	[6] = {
@@ -159,6 +177,9 @@
 			.label			= "GPIOG",
 			.ngpio			= 16,
 			.to_irq			= samsung_gpiolib_to_irq,
+#if defined(CONFIG_OF_GPIO)
+			.dt_compat		= "samsung,s3c2410-gpio-g",
+#endif
 		},
 	}, {
 		.base	= S3C2410_GPHCON,
@@ -168,6 +189,9 @@
 			.owner			= THIS_MODULE,
 			.label			= "GPIOH",
 			.ngpio			= 11,
+#if defined(CONFIG_OF_GPIO)
+			.dt_compat		= "samsung,s3c2410-gpio-h",
+#endif
 		},
 	},
 		/* GPIOS for the S3C2443 and later devices. */
@@ -179,6 +203,9 @@
 			.owner			= THIS_MODULE,
 			.label			= "GPIOJ",
 			.ngpio			= 16,
+#if defined(CONFIG_OF_GPIO)
+			.dt_compat		= "samsung,s3c2440-gpio-j",
+#endif
 		},
 	}, {
 		.base	= S3C2443_GPKCON,
@@ -188,6 +215,9 @@
 			.owner			= THIS_MODULE,
 			.label			= "GPIOK",
 			.ngpio			= 16,
+#if defined(CONFIG_OF_GPIO)
+			.dt_compat		= "samsung,s3c2443-gpio-k",
+#endif
 		},
 	}, {
 		.base	= S3C2443_GPLCON,
@@ -197,6 +227,9 @@
 			.owner			= THIS_MODULE,
 			.label			= "GPIOL",
 			.ngpio			= 15,
+#if defined(CONFIG_OF_GPIO)
+			.dt_compat		= "samsung,s3c2443-gpio-l",
+#endif
 		},
 	}, {
 		.base	= S3C2443_GPMCON,
@@ -206,6 +239,9 @@
 			.owner			= THIS_MODULE,
 			.label			= "GPIOM",
 			.ngpio			= 2,
+#if defined(CONFIG_OF_GPIO)
+			.dt_compat		= "samsung,s3c2443-gpio-m",
+#endif
 		},
 	},
 };