From: Andy Shevchenko <andy.shevchenko@gmail.com> To: Vladimir Murzin <vladimir.murzin@arm.com> Cc: "Arnd Bergmann" <arnd@arndb.de>, "Russell King" <linux@arm.linux.org.uk>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>, "Daniel Lezcano" <daniel.lezcano@linaro.org>, "Thomas Gleixner" <tglx@linutronix.de>, "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>, "Andreas Färber" <afaerber@suse.de>, "Maxime Coquelin" <mcoquelin.stm32@gmail.com>, "Mark Rutland" <Mark.Rutland@arm.com>, "Pawel Moll" <Pawel.Moll@arm.com>, ijc+devicetree <ijc+devicetree@hellion.org.uk>, "Kumar Gala" <galak@codeaurora.org>, "Jiri Slaby" <jslaby@suse.cz>, "Rob Herring" <robh+dt@kernel.org>, devicetree <devicetree@vger.kernel.org>, "linux-serial@vger.kernel.org" <linux-serial@vger.kernel.org>, "linux-api@vger.kernel.org" <linux-api@vger.kernel.org>, "linux-arm Mailing List" <linux-arm-kernel@lists.infradead.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org> Subject: Re: [PATCH v1 04/10] serial: mps2-uart: add MPS2 UART driver Date: Thu, 17 Dec 2015 15:15:12 +0200 [thread overview] Message-ID: <CAHp75Vd3eg5EQhx_36CKNO_N=2yVmsTyCx++6zTvAR2cbp-RYQ@mail.gmail.com> (raw) In-Reply-To: <56700A45.3070109@arm.com> On Tue, Dec 15, 2015 at 2:40 PM, Vladimir Murzin <vladimir.murzin@arm.com> wrote: > On 12/12/15 23:39, Andy Shevchenko wrote: >> On Wed, Dec 2, 2015 at 11:33 AM, Vladimir Murzin >> <vladimir.murzin@arm.com> wrote: >>> This driver adds support to the UART controller found on ARM MPS2 >>> platform. >> >> Just few comments (have neither time not big desire to do full review). >> > > Still better than nothing ;) I'm mostly agree on points you had, so I've > just left some I'm doubt about... > >>> + >>> +static void mps2_uart_enable_ms(struct uart_port *port) >>> +{ >>> +} >>> + >>> +static void mps2_uart_break_ctl(struct uart_port *port, int ctl) >>> +{ >>> +} >> >> Are those required to be present? If not, remove them until you have >> alive code there. > > A quick grep shows that core calls mps2_uart_break_ctl() > unconditionally, but, yes, it checks for presence of > mps2_uart_enable_ms() before jumping there, so it is safe to remove latter. OK. >>> +static irqreturn_t mps2_uart_oerrirq(int irq, void *data) >>> +{ >>> + irqreturn_t handled = IRQ_NONE; >>> + struct uart_port *port = data; >>> + u8 irqflag = mps2_uart_read8(port, UARTn_INT); >>> + >>> + spin_lock(&port->lock); >>> + >>> + if (irqflag & UARTn_INT_RX_OVERRUN) { >>> + struct tty_port *tport = &port->state->port; >>> + >>> + mps2_uart_write8(port, UARTn_INT_RX_OVERRUN, UARTn_INT); >>> + tty_insert_flip_char(tport, 0, TTY_OVERRUN); >>> + port->icount.overrun++; >>> + handled = IRQ_HANDLED; >>> + } >>> + >>> + /* XXX: this shouldn't happen? */ >> >> If shouldn't why it's there? Otherwise better to explain which >> conditions may lead to this. >> > > In practice I've never seen that happened and I think it never *should* > happen since we check if there is room in TX buffer. However, I could be > wrong here, so it is why that statement has question mark. So, worth to have a proper comment then. >>> +static int __init mps2_uart_init(void) >>> +{ >>> + int ret; >>> + >>> + ret = uart_register_driver(&mps2_uart_driver); >>> + if (ret) >>> + return ret; >>> + >>> + ret = platform_driver_register(&mps2_serial_driver); >>> + if (ret) >>> + uart_unregister_driver(&mps2_uart_driver); >>> + >>> + pr_info("MPS2 UART driver initialized\n"); >>> + >>> + return ret; >>> +} >>> +module_init(mps2_uart_init); >>> + >>> +static void __exit mps2_uart_exit(void) >>> +{ >>> + platform_driver_unregister(&mps2_serial_driver); >>> + uart_unregister_driver(&mps2_uart_driver); >>> +} >>> +module_exit(mps2_uart_exit); >> >> module_platform_driver(); >> And move uart_*register calls to probe/remove. >> > > With this move we'll get uart_*register for every device probed, no? Don't see a problem, maybe someone else could share an authoritive opinion. Some of the drivers do that, though most do in __init stage. So, see above. -- With Best Regards, Andy Shevchenko
WARNING: multiple messages have this Message-ID (diff)
From: andy.shevchenko@gmail.com (Andy Shevchenko) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v1 04/10] serial: mps2-uart: add MPS2 UART driver Date: Thu, 17 Dec 2015 15:15:12 +0200 [thread overview] Message-ID: <CAHp75Vd3eg5EQhx_36CKNO_N=2yVmsTyCx++6zTvAR2cbp-RYQ@mail.gmail.com> (raw) In-Reply-To: <56700A45.3070109@arm.com> On Tue, Dec 15, 2015 at 2:40 PM, Vladimir Murzin <vladimir.murzin@arm.com> wrote: > On 12/12/15 23:39, Andy Shevchenko wrote: >> On Wed, Dec 2, 2015 at 11:33 AM, Vladimir Murzin >> <vladimir.murzin@arm.com> wrote: >>> This driver adds support to the UART controller found on ARM MPS2 >>> platform. >> >> Just few comments (have neither time not big desire to do full review). >> > > Still better than nothing ;) I'm mostly agree on points you had, so I've > just left some I'm doubt about... > >>> + >>> +static void mps2_uart_enable_ms(struct uart_port *port) >>> +{ >>> +} >>> + >>> +static void mps2_uart_break_ctl(struct uart_port *port, int ctl) >>> +{ >>> +} >> >> Are those required to be present? If not, remove them until you have >> alive code there. > > A quick grep shows that core calls mps2_uart_break_ctl() > unconditionally, but, yes, it checks for presence of > mps2_uart_enable_ms() before jumping there, so it is safe to remove latter. OK. >>> +static irqreturn_t mps2_uart_oerrirq(int irq, void *data) >>> +{ >>> + irqreturn_t handled = IRQ_NONE; >>> + struct uart_port *port = data; >>> + u8 irqflag = mps2_uart_read8(port, UARTn_INT); >>> + >>> + spin_lock(&port->lock); >>> + >>> + if (irqflag & UARTn_INT_RX_OVERRUN) { >>> + struct tty_port *tport = &port->state->port; >>> + >>> + mps2_uart_write8(port, UARTn_INT_RX_OVERRUN, UARTn_INT); >>> + tty_insert_flip_char(tport, 0, TTY_OVERRUN); >>> + port->icount.overrun++; >>> + handled = IRQ_HANDLED; >>> + } >>> + >>> + /* XXX: this shouldn't happen? */ >> >> If shouldn't why it's there? Otherwise better to explain which >> conditions may lead to this. >> > > In practice I've never seen that happened and I think it never *should* > happen since we check if there is room in TX buffer. However, I could be > wrong here, so it is why that statement has question mark. So, worth to have a proper comment then. >>> +static int __init mps2_uart_init(void) >>> +{ >>> + int ret; >>> + >>> + ret = uart_register_driver(&mps2_uart_driver); >>> + if (ret) >>> + return ret; >>> + >>> + ret = platform_driver_register(&mps2_serial_driver); >>> + if (ret) >>> + uart_unregister_driver(&mps2_uart_driver); >>> + >>> + pr_info("MPS2 UART driver initialized\n"); >>> + >>> + return ret; >>> +} >>> +module_init(mps2_uart_init); >>> + >>> +static void __exit mps2_uart_exit(void) >>> +{ >>> + platform_driver_unregister(&mps2_serial_driver); >>> + uart_unregister_driver(&mps2_uart_driver); >>> +} >>> +module_exit(mps2_uart_exit); >> >> module_platform_driver(); >> And move uart_*register calls to probe/remove. >> > > With this move we'll get uart_*register for every device probed, no? Don't see a problem, maybe someone else could share an authoritive opinion. Some of the drivers do that, though most do in __init stage. So, see above. -- With Best Regards, Andy Shevchenko
next prev parent reply other threads:[~2015-12-17 13:15 UTC|newest] Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-12-02 9:33 [PATCH v1 00/10] Support for Cortex-M Prototyping System Vladimir Murzin 2015-12-02 9:33 ` Vladimir Murzin 2015-12-02 9:33 ` Vladimir Murzin 2015-12-02 9:33 ` [PATCH v1 01/10] dt-bindings: document the MPS2 timer bindings Vladimir Murzin 2015-12-02 9:33 ` Vladimir Murzin 2015-12-02 9:33 ` [PATCH v1 02/10] clockevents/drivers: add MPS2 Timer driver Vladimir Murzin 2015-12-02 9:33 ` Vladimir Murzin 2015-12-02 9:33 ` Vladimir Murzin 2015-12-07 9:25 ` Vladimir Murzin 2015-12-07 9:25 ` Vladimir Murzin 2015-12-07 9:25 ` Vladimir Murzin 2015-12-14 13:36 ` Daniel Lezcano 2015-12-14 13:36 ` Daniel Lezcano 2015-12-14 13:36 ` Daniel Lezcano 2015-12-15 12:47 ` Vladimir Murzin 2015-12-15 12:47 ` Vladimir Murzin 2015-12-15 12:47 ` Vladimir Murzin 2015-12-14 13:56 ` Rob Herring 2015-12-14 13:56 ` Rob Herring 2015-12-14 13:56 ` Rob Herring 2015-12-15 13:16 ` Vladimir Murzin 2015-12-15 13:16 ` Vladimir Murzin 2015-12-15 13:16 ` Vladimir Murzin 2015-12-02 9:33 ` [PATCH v1 03/10] dt-bindings: document the MPS2 UART bindings Vladimir Murzin 2015-12-02 9:33 ` Vladimir Murzin 2015-12-02 9:33 ` Vladimir Murzin 2015-12-02 9:33 ` [PATCH v1 04/10] serial: mps2-uart: add MPS2 UART driver Vladimir Murzin 2015-12-02 9:33 ` Vladimir Murzin 2015-12-02 9:33 ` Vladimir Murzin 2015-12-07 9:26 ` Vladimir Murzin 2015-12-07 9:26 ` Vladimir Murzin 2015-12-07 9:26 ` Vladimir Murzin 2015-12-12 23:39 ` Andy Shevchenko 2015-12-12 23:39 ` Andy Shevchenko 2015-12-12 23:39 ` Andy Shevchenko 2015-12-13 7:00 ` Greg Kroah-Hartman 2015-12-13 7:00 ` Greg Kroah-Hartman 2015-12-15 12:40 ` Vladimir Murzin 2015-12-15 12:40 ` Vladimir Murzin 2015-12-17 13:15 ` Andy Shevchenko [this message] 2015-12-17 13:15 ` Andy Shevchenko 2015-12-02 9:33 ` [PATCH v1 05/10] serial: mps2-uart: add support for early console Vladimir Murzin 2015-12-02 9:33 ` Vladimir Murzin 2015-12-02 9:33 ` [PATCH v1 06/10] ARM: mps2: introduce MPS2 platform Vladimir Murzin 2015-12-02 9:33 ` Vladimir Murzin 2015-12-02 9:33 ` [PATCH v1 07/10] ARM: mps2: add low-level debug support Vladimir Murzin 2015-12-02 9:33 ` Vladimir Murzin 2015-12-02 9:33 ` Vladimir Murzin 2015-12-02 9:33 ` [PATCH v1 08/10] ARM: configs: add MPS2 defconfig Vladimir Murzin 2015-12-02 9:33 ` Vladimir Murzin 2015-12-02 9:33 ` [PATCH v1 09/10] ARM: dts: introduce MPS2 AN385/AN386 Vladimir Murzin 2015-12-02 9:33 ` Vladimir Murzin 2015-12-23 9:33 ` Vladimir Murzin 2015-12-23 9:33 ` Vladimir Murzin 2015-12-23 9:33 ` Vladimir Murzin 2015-12-02 9:33 ` [PATCH v1 10/10] ARM: dts: introduce MPS2 AN399/AN400 Vladimir Murzin 2015-12-02 9:33 ` Vladimir Murzin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAHp75Vd3eg5EQhx_36CKNO_N=2yVmsTyCx++6zTvAR2cbp-RYQ@mail.gmail.com' \ --to=andy.shevchenko@gmail.com \ --cc=Mark.Rutland@arm.com \ --cc=Pawel.Moll@arm.com \ --cc=afaerber@suse.de \ --cc=arnd@arndb.de \ --cc=daniel.lezcano@linaro.org \ --cc=devicetree@vger.kernel.org \ --cc=galak@codeaurora.org \ --cc=gregkh@linuxfoundation.org \ --cc=ijc+devicetree@hellion.org.uk \ --cc=jslaby@suse.cz \ --cc=linux-api@vger.kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-serial@vger.kernel.org \ --cc=linux@arm.linux.org.uk \ --cc=mcoquelin.stm32@gmail.com \ --cc=robh+dt@kernel.org \ --cc=tglx@linutronix.de \ --cc=u.kleine-koenig@pengutronix.de \ --cc=vladimir.murzin@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.