Re: [RFC PATCH v2 2/4] serial: mxs-auart: Use helpers for gpio irqs

[Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>]

Hello,

On Tue, Jan 13, 2015 at 10:29:44AM +0100, Janusz Użycki wrote:
> 
> W dniu 2015-01-13 o 09:08, Uwe Kleine-König pisze:
> >Hello,
> >
> >On Sat, Jan 10, 2015 at 03:32:44PM +0100, Janusz Uzycki wrote:
> >>The patch updates mxs-auart driver to use new mctrl_gpio helpers for
> >>gpio irqs. The code is much simpler now.
> >>
> >>Signed-off-by: Janusz Uzycki <j.uzycki@xxxxxxxxxxxxxx>
> >>---
> >>
> >>There is no changes since v1 (rebased only).
> >>
> >>---
> >>  drivers/tty/serial/mxs-auart.c | 133 ++++-------------------------------------
> >>  1 file changed, 13 insertions(+), 120 deletions(-)
> >Very nice!
> >
> >>diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
> >>index ec553f8..2ddba69 100644
> >>--- a/drivers/tty/serial/mxs-auart.c
> >>+++ b/drivers/tty/serial/mxs-auart.c
> >>[...]
> >>@@ -483,18 +458,9 @@ static void mxs_auart_enable_ms(struct uart_port *port)
> >>  	s->ms_irq_enabled = true;
> >>-	if (s->gpio_irq[UART_GPIO_CTS] >= 0)
> >>-		enable_irq(s->gpio_irq[UART_GPIO_CTS]);
> >>-	/* TODO: enable AUART_INTR_CTSMIEN otherwise */
> >>-
> >>-	if (s->gpio_irq[UART_GPIO_DSR] >= 0)
> >>-		enable_irq(s->gpio_irq[UART_GPIO_DSR]);
> >>-
> >>-	if (s->gpio_irq[UART_GPIO_RI] >= 0)
> >>-		enable_irq(s->gpio_irq[UART_GPIO_RI]);
> >>-
> >>-	if (s->gpio_irq[UART_GPIO_DCD] >= 0)
> >>-		enable_irq(s->gpio_irq[UART_GPIO_DCD]);
> >>+	mctrl_gpio_enable_ms(s->gpios);
> >>+	/* TODO: enable AUART_INTR_CTSMIEN
> >>+	 * if s->gpios->irq[UART_GPIO_CTS] == 0 */
> >What is the problem here? For the other lines nothing needs to be done?
> >This comment doesn't match the coding style.
> 
> Right, the comment should be rather:
> /* TODO: enable AUART_INTR_CTSMIEN
>  * if (!mctrl_gpio_is_gpio(atmel_port->gpios, UART_GPIO_CTS)) */
I'd say:

	/*
	 * TODO: enable AUART_INTR_CTSMIEN if CTS isn't handled by
	 * mctrl_gpio.
	 */

> In this place I marked that CTSMIEN should be switched on
> enable/disable_ms if CTS
> is not a gpio. The driver enables CTSMIEN forever what is wrong but
> I can't test it
> and I don't need it so I've just marked the fact in the comment.
That's what I thought. You're not affected because CTS is a gpio for
you (or not?)?  What would be the effect otherwise?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
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