Hi Linus,

On Sat, Mar 29, 2014 at 3:20 AM, Linus Walleij [off-list ref] wrote:

On Thu, Mar 27, 2014 at 4:25 PM, Harini Katakam [off-list ref] wrote:

quoted

Add support for GPIO controller used by Xilinx Zynq

Signed-off-by: Harini Katakam <redacted>

This will not be integrated before v3.16 so we have some time to
think things over.

quoted

+config GPIO_ZYNQ
+       bool "Xilinx ZYNQ GPIO support"
+       depends on ARCH_ZYNQ
+       select GENERIC_IRQ_CHIP

I think that rather than selecting GENERIC_IRQ_CHIP,
select my new GPIOLIB_IRQCHIP and use the new helpers
gpiochip_irqchip_add() and gpiochip_set_chained_irqchip().

Look in drivers/gpio/gpio-pl061.c for example usage.

This is available in the "devel" or "for-next" branch of my
gpio tree at
https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-gpio.git/

OK.

<snip>

quoted

+/* Read/Write access to the GPIO PS registers */
+static inline u32 zynq_gpio_readreg(void __iomem *offset)
+{
+       return readl_relaxed(offset);
+}
+
+static inline void zynq_gpio_writereg(void __iomem *offset, u32 val)
+{
+       writel_relaxed(val, offset);
+}

I think this is unnecessary and confusing indirection.
Just use the readl_relaxed/writel_relaxed functions directly in
the code.

This is just to be flexible.

<snip>

quoted

+                       if (!(int_sts & 1))
+                               continue;
+                       gpio_irq_desc = irq_to_desc(gpio_irq);
+                       BUG_ON(!gpio_irq_desc);
+                       chip = irq_desc_get_chip(gpio_irq_desc);
+                       BUG_ON(!chip);
+                       chip->irq_ack(&gpio_irq_desc->irq_data);
+
+                       /* call the pin specific handler */
+                       generic_handle_irq(gpio_irq);
+               }
+               /* shift to first virtual irq of next bank */
+               gpio_irq = gpio->irq_base + zynq_gpio_pin_table[bank_num] + 1;

This is also pretty convoluted. Are you sure you don't want to
implement one gpiochip per bank instead? I guess the final "+1"
means there is actually one IRQ per bank even?

There is only one IRQ for all four banks.
I will try to make this logic and other places in the driver you pointed out,
less convoluted.
The handling is (very) slightly different for the upper two banks and
lower two banks.

<snip>

quoted

+static const struct dev_pm_ops zynq_gpio_dev_pm_ops = {
+       SET_SYSTEM_SLEEP_PM_OPS(zynq_gpio_suspend, zynq_gpio_resume)
+       SET_RUNTIME_PM_OPS(zynq_gpio_runtime_suspend, zynq_gpio_runtime_resume,
+                          zynq_gpio_idle)
+};

Is this runtime PM implementation aligned with Ulf Hansson's recent
new helpers to simplify suspend+runtime PM coexistance?

I'm sorry i just looked at them -
pm_runtime_force_suspend/resume are not used here.

quoted

+       chip->dbg_show = NULL;
+       chip->base = 0;         /* default pin base */

The GPIO numberspace is not the same as the pin number space.
This is only going to work if you have no other GPIO controllers
on your system. Use -1 as base so you get dynamic allocation
of a GPIO number range instead.

Make sure to use the new descriptor API for defining and accessing
GPIOs in all drivers and it will not matter which base you get.
(Good eh? :-)

As it appears you're using device tree, it is all transparent and
you need not worry.

OK.

<snip>

Thanks for the review!

Regards,
Harini