On Wed, Jun 18, 2014 at 1:39 PM, Harini Katakam <harinik@xxxxxxxxxx> wrote: > From: Harini Katakam <harini.katakam@xxxxxxxxxx> > > Add support for GPIO controller used by Xilinx Zynq. > > Signed-off-by: Harini Katakam <harinik@xxxxxxxxxx> > Signed-off-by: Soren Brinkmann <soren.brinkmann@xxxxxxxxxx> > --- > > v2 changes: > - convert to pm_runtime_force_(suspend|resume) > - add pm_runtime_set_active in probe() > - also (un)prepare clocks when they are dis-/enabled > - add some missing calls to pm_runtime_get() > - use pm_runtime_put() instead of sync variant > - remove gpio chip in driver remove() > - remove redundant type casts > - directly use IO helpers > - use BIT macro to set/clear bits > - migrate to GPIOLIB_IRQCHIP This is a great improvement! Only small stuff remains. > +#include <linux/bitops.h> > +#include <linux/clk.h> > +#include <linux/gpio.h> This should be: #include <linux/gpio/driver.h> If that doesn't work ... why? > +/** > + * struct zynq_gpio - gpio device private data structure > + * @chip: instance of the gpio_chip > + * @base_addr: base address of the GPIO device > + * @irq: irq associated with the controller > + * @clk: clock resource for this controller > + */ > +struct zynq_gpio { > + struct gpio_chip chip; > + void __iomem *base_addr; > + int irq; Why is irq kept around in this struct? It looks like it could just be a local variable in probe()? > + struct clk *clk; > +}; Apart from that this nitpicking the driver looks very nice. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html