Hi Grant,

On Sat, 7 Aug 2010 09:12:50 -0600
Grant Likely [off-list ref] wrote:

Hi Anatolij,
=20
Looks pretty good, but some comments below...
=20
On Sat, Aug 7, 2010 at 7:28 AM, Anatolij Gustschin [off-list ref] wrote:

quoted

Signed-off-by: Anatolij Gustschin <agust@denx.de>

=20
You haven't written a patch description.  Give some details about how
the 512x gpio controller is different from the 8xxx one.

Ok, will fix.

...

quoted

@@ -226,6 +272,9 @@ static struct irq_chip mpc8xxx_irq_chip =3D {

=A0static int mpc8xxx_gpio_irq_map(struct irq_host *h, unsigned int vir=

q,

quoted

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0irq_hw_n=

umber_t hw)

quoted

=A0{
+ =A0 =A0 =A0 if (of_device_is_compatible(h->of_node, "fsl,mpc5121-gpio=

"))

quoted

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 mpc8xxx_irq_chip.set_type =3D mpc512x_irq=

_set_type;

quoted

+

=20
You can put the set type hook into the of_match_table data which you
will need for of_find_matching_node() (see below).

How can I get this match table data reference in mpc8xxx_gpio_irq_map() ?
Is it okay to set data field of struct device_node to the set type
hook? I could do it in mpc8xxx_add_gpiochips() but I'm not sure whether
the data field will be used for other purposes somewhere else.

...

quoted

@@ -330,6 +379,9 @@ static int __init mpc8xxx_add_gpiochips(void)

=A0 =A0 =A0 =A0for_each_compatible_node(np, NULL, "fsl,mpc8610-gpio")
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0mpc8xxx_add_controller(np);

+ =A0 =A0 =A0 for_each_compatible_node(np, NULL, "fsl,mpc5121-gpio")
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 mpc8xxx_add_controller(np);
+

=20
This list is getting too long.  Refactor this function to use
for_each_matching_node().  Also doing it this way is dangerous because
if say a 5121 gpio node claims compatibility with a 8610 gpio node,
then the gpio controller will get registered twice.

Fixed.

Thanks,
Anatolij