| Message ID | 1420900366-9169-2-git-send-email-j.uzycki@elproma.com.pl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@elproma.com.pl> > --- > > 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. Other than that, this patch looks good. Uwe
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@elproma.com.pl> >> --- >> >> 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)) */ 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. > > Other than that, this patch looks good. > > Uwe > Thanks Janusz
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@elproma.com.pl> > >>--- > >> > >>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
W dniu 2015-01-13 o 10:35, Uwe Kleine-König pisze: > 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@elproma.com.pl> >>>> --- >>>> >>>> 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. > */ exactly, thanks > >> 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? Yes, my CTS is a gpio. CTSMIEN control CTS signal of auart block. There is a choice in DT: - use auart block's CTS: hardware flow control works for all baud rates, DMA can be used - use gpio as CTS: hardware flow control is limited, DMA disabled but CTS line is not limited by hardware pinmux Both options can't be set at once. I workarounded auart block's CTS irq handler in condition: "if (CTS_AT_AUART() && s->ms_irq_enabled)". Support by _ms would be more elegance but as I wrote I couldn't test all cases. Therefore the code for auart block's CTS is preserved. best regards Janusz > > Best regards > Uwe >
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 @@ -149,7 +149,6 @@ struct mxs_auart_port { #define MXS_AUART_DMA_RX_READY 3 /* bit 3 */ #define MXS_AUART_RTSCTS 4 /* bit 4 */ unsigned long flags; - unsigned int mctrl_prev; enum mxs_auart_type devtype; unsigned int irq; @@ -167,7 +166,6 @@ struct mxs_auart_port { void *rx_dma_buf; struct mctrl_gpios *gpios; - int gpio_irq[UART_GPIO_MAX]; bool ms_irq_enabled; }; @@ -433,29 +431,6 @@ static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl) mctrl_gpio_set(s->gpios, mctrl); } -#define MCTRL_ANY_DELTA (TIOCM_RI | TIOCM_DSR | TIOCM_CD | TIOCM_CTS) -static u32 mxs_auart_modem_status(struct mxs_auart_port *s, u32 mctrl) -{ - u32 mctrl_diff; - - mctrl_diff = mctrl ^ s->mctrl_prev; - s->mctrl_prev = mctrl; - if (mctrl_diff & MCTRL_ANY_DELTA && s->ms_irq_enabled && - s->port.state != NULL) { - if (mctrl_diff & TIOCM_RI) - s->port.icount.rng++; - if (mctrl_diff & TIOCM_DSR) - s->port.icount.dsr++; - if (mctrl_diff & TIOCM_CD) - uart_handle_dcd_change(&s->port, mctrl & TIOCM_CD); - if (mctrl_diff & TIOCM_CTS) - uart_handle_cts_change(&s->port, mctrl & TIOCM_CTS); - - wake_up_interruptible(&s->port.state->port.delta_msr_wait); - } - return mctrl; -} - static u32 mxs_auart_get_mctrl(struct uart_port *u) { struct mxs_auart_port *s = to_auart_port(u); @@ -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 */ } /* @@ -512,18 +478,9 @@ static void mxs_auart_disable_ms(struct uart_port *port) s->ms_irq_enabled = false; - if (s->gpio_irq[UART_GPIO_CTS] >= 0) - disable_irq(s->gpio_irq[UART_GPIO_CTS]); - /* TODO: disable AUART_INTR_CTSMIEN otherwise */ - - if (s->gpio_irq[UART_GPIO_DSR] >= 0) - disable_irq(s->gpio_irq[UART_GPIO_DSR]); - - if (s->gpio_irq[UART_GPIO_RI] >= 0) - disable_irq(s->gpio_irq[UART_GPIO_RI]); - - if (s->gpio_irq[UART_GPIO_DCD] >= 0) - disable_irq(s->gpio_irq[UART_GPIO_DCD]); + mctrl_gpio_disable_ms(s->gpios); + /* TODO: disable AUART_INTR_CTSMIEN + * if s->gpios->irq[UART_GPIO_CTS] == 0 */ } static int mxs_auart_dma_prep_rx(struct mxs_auart_port *s); @@ -799,7 +756,6 @@ static irqreturn_t mxs_auart_irq_handle(int irq, void *context) { u32 istat; struct mxs_auart_port *s = context; - u32 mctrl_temp = s->mctrl_prev; u32 stat = readl(s->port.membase + AUART_STAT); istat = readl(s->port.membase + AUART_INTR); @@ -811,16 +767,6 @@ static irqreturn_t mxs_auart_irq_handle(int irq, void *context) | AUART_INTR_CTSMIS), s->port.membase + AUART_INTR_CLR); - /* - * Dealing with GPIO interrupt - */ - if (irq == s->gpio_irq[UART_GPIO_CTS] || - irq == s->gpio_irq[UART_GPIO_DCD] || - irq == s->gpio_irq[UART_GPIO_DSR] || - irq == s->gpio_irq[UART_GPIO_RI]) - mxs_auart_modem_status(s, - mctrl_gpio_get(s->gpios, &mctrl_temp)); - if (istat & AUART_INTR_CTSMIS) { if (CTS_AT_AUART() && s->ms_irq_enabled) uart_handle_cts_change(&s->port, @@ -885,9 +831,6 @@ static int mxs_auart_startup(struct uart_port *u) */ writel(AUART_LINECTRL_FEN, u->membase + AUART_LINECTRL_SET); - /* get initial status of modem lines */ - mctrl_gpio_get(s->gpios, &s->mctrl_prev); - s->ms_irq_enabled = false; return 0; } @@ -1157,71 +1100,23 @@ static int serial_mxs_probe_dt(struct mxs_auart_port *s, return 0; } -static bool mxs_auart_init_gpios(struct mxs_auart_port *s, struct device *dev) +static bool mxs_auart_init_gpios(struct mxs_auart_port *s) { - enum mctrl_gpio_idx i; - struct gpio_desc *gpiod; - - s->gpios = mctrl_gpio_init(dev, 0); + s->gpios = mctrl_gpio_init_dt(&s->port, 0); if (IS_ERR_OR_NULL(s->gpios)) return false; /* Block (enabled before) DMA option if RTS or CTS is GPIO line */ if (!RTS_AT_AUART() || !CTS_AT_AUART()) { if (test_bit(MXS_AUART_RTSCTS, &s->flags)) - dev_warn(dev, + dev_warn(s->dev, "DMA and flow control via gpio may cause some problems. DMA disabled!\n"); clear_bit(MXS_AUART_RTSCTS, &s->flags); } - for (i = 0; i < UART_GPIO_MAX; i++) { - gpiod = mctrl_gpio_to_gpiod(s->gpios, i); - if (gpiod && (gpiod_get_direction(gpiod) == GPIOF_DIR_IN)) - s->gpio_irq[i] = gpiod_to_irq(gpiod); - else - s->gpio_irq[i] = -EINVAL; - } - return true; } -static void mxs_auart_free_gpio_irq(struct mxs_auart_port *s) -{ - enum mctrl_gpio_idx i; - - for (i = 0; i < UART_GPIO_MAX; i++) - if (s->gpio_irq[i] >= 0) - free_irq(s->gpio_irq[i], s); -} - -static int mxs_auart_request_gpio_irq(struct mxs_auart_port *s) -{ - int *irq = s->gpio_irq; - enum mctrl_gpio_idx i; - int err = 0; - - for (i = 0; (i < UART_GPIO_MAX) && !err; i++) { - if (irq[i] < 0) - continue; - - irq_set_status_flags(irq[i], IRQ_NOAUTOEN); - err = request_irq(irq[i], mxs_auart_irq_handle, - IRQ_TYPE_EDGE_BOTH, dev_name(s->dev), s); - if (err) - dev_err(s->dev, "%s - Can't get %d irq\n", - __func__, irq[i]); - } - - /* - * If something went wrong, rollback. - */ - while (err && (--i >= 0)) - if (irq[i] >= 0) - free_irq(irq[i], s); - - return err; -} - static int mxs_auart_probe(struct platform_device *pdev) { const struct of_device_id *of_id = @@ -1269,8 +1164,6 @@ static int mxs_auart_probe(struct platform_device *pdev) s->port.type = PORT_IMX; s->port.dev = s->dev = &pdev->dev; - s->mctrl_prev = 0; - s->irq = platform_get_irq(pdev, 0); s->port.irq = s->irq; ret = request_irq(s->irq, mxs_auart_irq_handle, 0, dev_name(&pdev->dev), s); @@ -1279,14 +1172,14 @@ static int mxs_auart_probe(struct platform_device *pdev) platform_set_drvdata(pdev, s); - if (!mxs_auart_init_gpios(s, &pdev->dev)) + if (!mxs_auart_init_gpios(s)) dev_err(&pdev->dev, "Failed to initialize GPIOs. The serial port may not work as expected\n"); /* * Get the GPIO lines IRQ */ - ret = mxs_auart_request_gpio_irq(s); + ret = mctrl_gpio_request_irqs(s->gpios); if (ret) goto out_free_irq; @@ -1306,7 +1199,7 @@ static int mxs_auart_probe(struct platform_device *pdev) return 0; out_free_gpio_irq: - mxs_auart_free_gpio_irq(s); + mctrl_gpio_free_irqs(s->gpios); out_free_irq: auart_port[pdev->id] = NULL; free_irq(s->irq, s); @@ -1326,7 +1219,7 @@ static int mxs_auart_remove(struct platform_device *pdev) auart_port[pdev->id] = NULL; - mxs_auart_free_gpio_irq(s); + mctrl_gpio_free_irqs(s->gpios); clk_put(s->clk); free_irq(s->irq, s); kfree(s);
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@elproma.com.pl> --- There is no changes since v1 (rebased only). --- drivers/tty/serial/mxs-auart.c | 133 ++++------------------------------------- 1 file changed, 13 insertions(+), 120 deletions(-)