Hi Grant,
On Sat, 7 Aug 2010 09:12:50 -0600
Grant Likely <grant.lik...@secretlab.ca> wrote:
> Hi Anatolij,
>
> Looks pretty good, but some comments below...
>
> On Sat, Aug 7, 2010 at 7:28 AM, Anatolij Gustschin <ag...@denx.de> wrote:
> > Signed-off-by: Anatolij Gustschin <ag...@denx.de>
>
> 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.
...
> > @@ -226,6 +272,9 @@ static struct irq_chip mpc8xxx_irq_chip = {
> > static int mpc8xxx_gpio_irq_map(struct irq_host *h, unsigned int virq,
> > irq_hw_number_t hw)
> > {
> > + if (of_device_is_compatible(h->of_node, "fsl,mpc5121-gpio"))
> > + mpc8xxx_irq_chip.set_type = mpc512x_irq_set_type;
> > +
>
> 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.
...
> > @@ -330,6 +379,9 @@ static int __init mpc8xxx_add_gpiochips(void)
> > for_each_compatible_node(np, NULL, "fsl,mpc8610-gpio")
> > mpc8xxx_add_controller(np);
> >
> > + for_each_compatible_node(np, NULL, "fsl,mpc5121-gpio")
> > + mpc8xxx_add_controller(np);
> > +
>
> 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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
