| Message ID | 1324229655-5538-1-git-send-email-dirk.behme@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Sun, Dec 18, 2011 at 06:34:14PM +0100, Dirk Behme wrote: > Factor out the uart save/restore functionality instead of > having the same code several times in the driver. > > Signed-off-by: Dirk Behme <dirk.behme@gmail.com> > CC: Saleem Abdulrasool <compnerd@compnerd.org> > CC: Sascha Hauer <s.hauer@pengutronix.de> > CC: Fabio Estevam <festevam@gmail.com> > CC: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > CC: linux-serial@vger.kernel.org > --- > drivers/tty/serial/imx.c | 38 +++++++++++++++++++++++++++++++------- > 1 files changed, 31 insertions(+), 7 deletions(-) > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c > index 163fc90..6a01c2a 100644 > --- a/drivers/tty/serial/imx.c > +++ b/drivers/tty/serial/imx.c > @@ -260,6 +260,31 @@ static inline int is_imx21_uart(struct imx_port *sport) > } > > /* > + * Save and restore functions for UCR1, UCR2 and UCR3 registers > + */ > +static void imx_console_mode(struct uart_port *port, Function name imx_console_mode seems not like a couple with imx_console_restore. And I guess something like imx_port_ucrs_save[restore] would be better? > + unsigned int *ucr1, > + unsigned int *ucr2, > + unsigned int *ucr3) Can we define something like 'struct imx_port_ucrs' to contains these? Regards, Shawn > +{ > + /* save control registers */ > + *ucr1 = readl(port->membase + UCR1); > + *ucr2 = readl(port->membase + UCR2); > + *ucr3 = readl(port->membase + UCR3); > +} > + > +static void imx_console_restore(struct uart_port *port, > + unsigned int ucr1, > + unsigned int ucr2, > + unsigned int ucr3) > +{ > + /* restore control registers */ > + writel(ucr1, port->membase + UCR1); > + writel(ucr2, port->membase + UCR2); > + writel(ucr3, port->membase + UCR3); > +} > + > +/* > * Handle any change of modem status signal since we were last called. > */ > static void imx_mctrl_check(struct imx_port *sport) > @@ -1118,13 +1143,13 @@ static void > imx_console_write(struct console *co, const char *s, unsigned int count) > { > struct imx_port *sport = imx_ports[co->index]; > - unsigned int old_ucr1, old_ucr2, ucr1; > + unsigned int old_ucr1, old_ucr2, old_ucr3, ucr1; > > /* > - * First, save UCR1/2 and then disable interrupts > + * First, save UCR1/2/3 and then disable interrupts > */ > - ucr1 = old_ucr1 = readl(sport->port.membase + UCR1); > - old_ucr2 = readl(sport->port.membase + UCR2); > + imx_console_mode(&sport->port, &old_ucr1, &old_ucr2, &old_ucr3); > + ucr1 = old_ucr1; > > if (is_imx1_uart(sport)) > ucr1 |= IMX1_UCR1_UARTCLKEN; > @@ -1139,12 +1164,11 @@ imx_console_write(struct console *co, const char *s, unsigned int count) > > /* > * Finally, wait for transmitter to become empty > - * and restore UCR1/2 > + * and restore UCR1/2/3 > */ > while (!(readl(sport->port.membase + USR2) & USR2_TXDC)); > > - writel(old_ucr1, sport->port.membase + UCR1); > - writel(old_ucr2, sport->port.membase + UCR2); > + imx_console_restore(&sport->port, old_ucr1, old_ucr2, old_ucr3); > } > > /* > -- > 1.7.7.4
On 19.12.2011 04:49, Shawn Guo wrote: > On Sun, Dec 18, 2011 at 06:34:14PM +0100, Dirk Behme wrote: >> Factor out the uart save/restore functionality instead of >> having the same code several times in the driver. >> >> Signed-off-by: Dirk Behme<dirk.behme@gmail.com> >> CC: Saleem Abdulrasool<compnerd@compnerd.org> >> CC: Sascha Hauer<s.hauer@pengutronix.de> >> CC: Fabio Estevam<festevam@gmail.com> >> CC: Uwe Kleine-König<u.kleine-koenig@pengutronix.de> >> CC: linux-serial@vger.kernel.org >> --- >> drivers/tty/serial/imx.c | 38 +++++++++++++++++++++++++++++++------- >> 1 files changed, 31 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c >> index 163fc90..6a01c2a 100644 >> --- a/drivers/tty/serial/imx.c >> +++ b/drivers/tty/serial/imx.c >> @@ -260,6 +260,31 @@ static inline int is_imx21_uart(struct imx_port *sport) >> } >> >> /* >> + * Save and restore functions for UCR1, UCR2 and UCR3 registers >> + */ >> +static void imx_console_mode(struct uart_port *port, > > Function name imx_console_mode seems not like a couple with > imx_console_restore. And I guess something like > imx_port_ucrs_save[restore] would be better? > >> + unsigned int *ucr1, >> + unsigned int *ucr2, >> + unsigned int *ucr3) > > Can we define something like 'struct imx_port_ucrs' to contains these? Yes, we could have different function names above and use a struct. I implemented what Sascha asked for in http://www.spinics.net/lists/arm-kernel/msg144960.html though: -- Sascha wrote: -- I'm thinking about: imx_console_mode(struct uart_port *port, u32 *ucr1, u32 *ucr2, u32 *ucr2); imx_console_restore(struct uart_port *port, u32 ucr1, u32 ucr2, u32 ucr3); -- Sascha end -- It seems Shawn proposes imx_port_ucrs_save(struct uart_port *port, struct *imx_port_ucrs); imx_port_ucrs_restore(struct uart_port *port, struct *imx_port_ucrs); (?) Sascha, Shawn: Could you agree on what we should use? Once you agreed, I will send an update of both two patches. Many thanks and best regards Dirk
On Mon, Dec 19, 2011 at 07:49:56AM +0100, Dirk Behme wrote: > On 19.12.2011 04:49, Shawn Guo wrote: > >On Sun, Dec 18, 2011 at 06:34:14PM +0100, Dirk Behme wrote: > >>Factor out the uart save/restore functionality instead of > >>having the same code several times in the driver. > >> > >>Signed-off-by: Dirk Behme<dirk.behme@gmail.com> > >>CC: Saleem Abdulrasool<compnerd@compnerd.org> > >>CC: Sascha Hauer<s.hauer@pengutronix.de> > >>CC: Fabio Estevam<festevam@gmail.com> > >>CC: Uwe Kleine-König<u.kleine-koenig@pengutronix.de> > >>CC: linux-serial@vger.kernel.org > >>--- > >> drivers/tty/serial/imx.c | 38 +++++++++++++++++++++++++++++++------- > >> 1 files changed, 31 insertions(+), 7 deletions(-) > >> > >>diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c > >>index 163fc90..6a01c2a 100644 > >>--- a/drivers/tty/serial/imx.c > >>+++ b/drivers/tty/serial/imx.c > >>@@ -260,6 +260,31 @@ static inline int is_imx21_uart(struct imx_port *sport) > >> } > >> > >> /* > >>+ * Save and restore functions for UCR1, UCR2 and UCR3 registers > >>+ */ > >>+static void imx_console_mode(struct uart_port *port, > > > >Function name imx_console_mode seems not like a couple with > >imx_console_restore. And I guess something like > >imx_port_ucrs_save[restore] would be better? > > > >>+ unsigned int *ucr1, > >>+ unsigned int *ucr2, > >>+ unsigned int *ucr3) > > > >Can we define something like 'struct imx_port_ucrs' to contains these? > > Yes, we could have different function names above and use a struct. > > I implemented what Sascha asked for in > > http://www.spinics.net/lists/arm-kernel/msg144960.html > > though: > > -- Sascha wrote: -- > I'm thinking about: > > imx_console_mode(struct uart_port *port, u32 *ucr1, u32 *ucr2, u32 *ucr2); > imx_console_restore(struct uart_port *port, u32 ucr1, u32 ucr2, u32 ucr3); > -- Sascha end -- > > It seems Shawn proposes > > imx_port_ucrs_save(struct uart_port *port, struct *imx_port_ucrs); > imx_port_ucrs_restore(struct uart_port *port, struct *imx_port_ucrs); > > (?) > > Sascha, Shawn: Could you agree on what we should use? > > Once you agreed, I will send an update of both two patches. > Please Cc Alan Cox <alan@linux.intel.com> for these patches. Though we could possibly send them through arm-soc tree, we need to ask for his ack.
On Mon, Dec 19, 2011 at 07:49:56AM +0100, Dirk Behme wrote: > On 19.12.2011 04:49, Shawn Guo wrote: > >On Sun, Dec 18, 2011 at 06:34:14PM +0100, Dirk Behme wrote: > >>Factor out the uart save/restore functionality instead of > >>having the same code several times in the driver. > >> > >>Signed-off-by: Dirk Behme<dirk.behme@gmail.com> > >>CC: Saleem Abdulrasool<compnerd@compnerd.org> > >>CC: Sascha Hauer<s.hauer@pengutronix.de> > >>CC: Fabio Estevam<festevam@gmail.com> > >>CC: Uwe Kleine-König<u.kleine-koenig@pengutronix.de> > >>CC: linux-serial@vger.kernel.org > >>--- > >> drivers/tty/serial/imx.c | 38 +++++++++++++++++++++++++++++++------- > >> 1 files changed, 31 insertions(+), 7 deletions(-) > >> > >>diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c > >>index 163fc90..6a01c2a 100644 > >>--- a/drivers/tty/serial/imx.c > >>+++ b/drivers/tty/serial/imx.c > >>@@ -260,6 +260,31 @@ static inline int is_imx21_uart(struct imx_port *sport) > >> } > >> > >> /* > >>+ * Save and restore functions for UCR1, UCR2 and UCR3 registers > >>+ */ > >>+static void imx_console_mode(struct uart_port *port, > > > >Function name imx_console_mode seems not like a couple with > >imx_console_restore. And I guess something like > >imx_port_ucrs_save[restore] would be better? > > > >>+ unsigned int *ucr1, > >>+ unsigned int *ucr2, > >>+ unsigned int *ucr3) > > > >Can we define something like 'struct imx_port_ucrs' to contains these? > > Yes, we could have different function names above and use a struct. > > I implemented what Sascha asked for in > > http://www.spinics.net/lists/arm-kernel/msg144960.html > > though: > > -- Sascha wrote: -- > I'm thinking about: > > imx_console_mode(struct uart_port *port, u32 *ucr1, u32 *ucr2, u32 *ucr2); > imx_console_restore(struct uart_port *port, u32 ucr1, u32 ucr2, u32 ucr3); > -- Sascha end -- > > It seems Shawn proposes > > imx_port_ucrs_save(struct uart_port *port, struct *imx_port_ucrs); > imx_port_ucrs_restore(struct uart_port *port, struct *imx_port_ucrs); > > (?) > > Sascha, Shawn: Could you agree on what we should use? I think save/restore are better names. Maybe I thought about a function which saves the current values *and* sets ucrx to other values. In that case imx_port_ucrs_save() would be the wrong name. > > Once you agreed, I will send an update of both two patches. +1 for save/restore Sascha
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index 163fc90..6a01c2a 100644 --- a/drivers/tty/serial/imx.c +++ b/drivers/tty/serial/imx.c @@ -260,6 +260,31 @@ static inline int is_imx21_uart(struct imx_port *sport) } /* + * Save and restore functions for UCR1, UCR2 and UCR3 registers + */ +static void imx_console_mode(struct uart_port *port, + unsigned int *ucr1, + unsigned int *ucr2, + unsigned int *ucr3) +{ + /* save control registers */ + *ucr1 = readl(port->membase + UCR1); + *ucr2 = readl(port->membase + UCR2); + *ucr3 = readl(port->membase + UCR3); +} + +static void imx_console_restore(struct uart_port *port, + unsigned int ucr1, + unsigned int ucr2, + unsigned int ucr3) +{ + /* restore control registers */ + writel(ucr1, port->membase + UCR1); + writel(ucr2, port->membase + UCR2); + writel(ucr3, port->membase + UCR3); +} + +/* * Handle any change of modem status signal since we were last called. */ static void imx_mctrl_check(struct imx_port *sport) @@ -1118,13 +1143,13 @@ static void imx_console_write(struct console *co, const char *s, unsigned int count) { struct imx_port *sport = imx_ports[co->index]; - unsigned int old_ucr1, old_ucr2, ucr1; + unsigned int old_ucr1, old_ucr2, old_ucr3, ucr1; /* - * First, save UCR1/2 and then disable interrupts + * First, save UCR1/2/3 and then disable interrupts */ - ucr1 = old_ucr1 = readl(sport->port.membase + UCR1); - old_ucr2 = readl(sport->port.membase + UCR2); + imx_console_mode(&sport->port, &old_ucr1, &old_ucr2, &old_ucr3); + ucr1 = old_ucr1; if (is_imx1_uart(sport)) ucr1 |= IMX1_UCR1_UARTCLKEN; @@ -1139,12 +1164,11 @@ imx_console_write(struct console *co, const char *s, unsigned int count) /* * Finally, wait for transmitter to become empty - * and restore UCR1/2 + * and restore UCR1/2/3 */ while (!(readl(sport->port.membase + USR2) & USR2_TXDC)); - writel(old_ucr1, sport->port.membase + UCR1); - writel(old_ucr2, sport->port.membase + UCR2); + imx_console_restore(&sport->port, old_ucr1, old_ucr2, old_ucr3); } /*
Factor out the uart save/restore functionality instead of having the same code several times in the driver. Signed-off-by: Dirk Behme <dirk.behme@gmail.com> CC: Saleem Abdulrasool <compnerd@compnerd.org> CC: Sascha Hauer <s.hauer@pengutronix.de> CC: Fabio Estevam <festevam@gmail.com> CC: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> CC: linux-serial@vger.kernel.org --- drivers/tty/serial/imx.c | 38 +++++++++++++++++++++++++++++++------- 1 files changed, 31 insertions(+), 7 deletions(-)