[PATCH v3 07/15] ARM: mxs: Add gpio support

Hi Uwe,

2010/12/10 Uwe Kleine-K?nig [off-list ref]:

Hello Shawn,

On Fri, Dec 10, 2010 at 03:23:13PM +0800, Shawn Guo wrote:

quoted

On Fri, Dec 10, 2010 at 3:06 PM, Shawn Guo [off-list ref] wrote:

quoted

On Fri, Dec 10, 2010 at 12:47 AM, Lothar Wa?mann [off-list ref] wrote:

quoted

Shawn Guo writes:

quoted

+static void _set_gpio_irqenable(struct mxs_gpio_port *port, u32 index,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? int enable)
+{
+ ? ? if (enable == 0) {
+ ? ? ? ? ? ? __raw_writel(1 << index,
+ ? ? ? ? ? ? ? ? ? ? port->base + PINCTRL_PIN2IRQ(port->id) + MXS_CLR_ADDR);
+ ? ? ? ? ? ? __raw_writel(1 << index,
+ ? ? ? ? ? ? ? ? ? ? port->base + PINCTRL_IRQEN(port->id) + MXS_CLR_ADDR);

This will loose interrupt pulses that happen while IRQs are disabled.
IMO PIN2IRQ should only be cleared when an IRQ is freed, not when it
is disabled.

What about leaving PIN2IRQ always be 1 and only using IRQEN to
enable/disable IRQ? ?I consulted the designer it's no problem no
matter what the pin function is.

The code will be like:

static void _set_gpio_irqenable(struct mxs_gpio_port *port, u32 index,
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? int enable)
{
? ? ? ? if (enable) {
? ? ? ? ? ? ? ? __raw_writel(1 << index,
? ? ? ? ? ? ? ? ? ? ? ? port->base + PINCTRL_IRQEN(port->id) + MXS_SET_ADDR);
? ? ? ? ? ? ? ? __raw_writel(1 << index,
? ? ? ? ? ? ? ? ? ? ? ? port->base + PINCTRL_PIN2IRQ(port->id) + MXS_SET_ADDR);
? ? ? ? } else {
? ? ? ? ? ? ? ? __raw_writel(1 << index,
? ? ? ? ? ? ? ? ? ? ? ? port->base + PINCTRL_IRQEN(port->id) + MXS_CLR_ADDR);
? ? ? ? }
}

If I understood Lothar correctly the best way is to set and clear
PINCTRL_PIN2IRQ in the irqchips irq_startup and irq_shutdown routine.

Is there any problem with the current code to address Lothar's
concern?  If no, I would not introduce two more irq_chip routines into
the file.

-- 
Regards,
Shawn