Re: [PATCH 1/2] GPIO: Add driver for Zynq GPIO controller

[Harini Katakam <harinikatakamlinux@xxxxxxxxx>]

Hi Linus,

On Sat, Mar 29, 2014 at 3:20 AM, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
> On Thu, Mar 27, 2014 at 4:25 PM, Harini Katakam <harinik@xxxxxxxxxx> wrote:
>
>> Add support for GPIO controller used by Xilinx Zynq
>>
>> Signed-off-by: Harini Katakam <harinik@xxxxxxxxxx>
>
> This will not be integrated before v3.16 so we have some time to
> think things over.
>
>> +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>
>> +/* 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>
>> +                       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>
>> +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.

>> +       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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html