From: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com> To: Lukas Wunner <lukas@wunner.de> Cc: Lino Sanfilippo <LinoSanfilippo@gmx.de>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Jiri Slaby <jirislaby@kernel.org>, robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org, Andy Shevchenko <andriy.shevchenko@linux.intel.com>, vz@mleia.com, linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org, linux-serial <linux-serial@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>, p.rosenberger@kunbus.com, Lino Sanfilippo <l.sanfilippo@kunbus.com> Subject: Re: [PATCH 1/8] serial: core: only get RS485 termination gpio if supported Date: Mon, 27 Jun 2022 12:05:50 +0300 (EEST) [thread overview] Message-ID: <fbce8d7e-86e-d47e-bcd8-5b99754d1d2e@linux.intel.com> (raw) In-Reply-To: <20220625194951.GA2879@wunner.de> On Sat, 25 Jun 2022, Lukas Wunner wrote: > On Wed, Jun 22, 2022 at 05:46:52PM +0200, Lino Sanfilippo wrote: > > In uart_get_rs485_mode() only try to get a termination GPIO if RS485 bus > > termination is supported by the driver. > [...] > > --- a/drivers/tty/serial/serial_core.c > > +++ b/drivers/tty/serial/serial_core.c > > @@ -3384,17 +3384,20 @@ int uart_get_rs485_mode(struct uart_port *port) > > rs485conf->flags |= SER_RS485_RTS_AFTER_SEND; > > } > > > > - /* > > - * Disabling termination by default is the safe choice: Else if many > > - * bus participants enable it, no communication is possible at all. > > - * Works fine for short cables and users may enable for longer cables. > > - */ > > - port->rs485_term_gpio = devm_gpiod_get_optional(dev, "rs485-term", > > - GPIOD_OUT_LOW); > > - if (IS_ERR(port->rs485_term_gpio)) { > > - ret = PTR_ERR(port->rs485_term_gpio); > > - port->rs485_term_gpio = NULL; > > - return dev_err_probe(dev, ret, "Cannot get rs485-term-gpios\n"); > > + if (port->rs485_supported->flags & SER_RS485_TERMINATE_BUS) { > > So I think linux-next commit be2e2cb1d281 ("serial: Sanitize rs485_struct") > contains a mistake in that it forces drivers to set SER_RS485_TERMINATE_BUS > in their rs485_supported->flags to allow enabling bus termination. > > That's wrong because *every* rs485-capable driver can enable bus > termination if a GPIO has been defined for that in the DT. Do you mean every em485 using driver? Otherwise I don't see this "forces drivers to set" happening anywhere in the code? You're partially right because there are other bugs in this area such as the one you propose a fix below. While I was making the sanitization series, I entirely missed some parts related to termination because SER_RS485_TERMINATE_BUS is seemingly not set/handled correctly by the core. Another thing that looks a bug is that on subsequent call to TIOCSRS485, w/o SER_RS485_TERMINATE_BUS nothing happens (for non-em485 driver, that is)? It seems to be taken care by 2/8 of this series though, I think. But it should be properly marked as Fixes: ... in that case although nobody has complained about it so likely not a huge issue to anyone. > In fact, another commit which was applied as part of the same series, > ebe2cf736a04 ("serial: pl011: Fill in rs485_supported") does not set > SER_RS485_TERMINATE_BUS in amba-pl011.c's flags and thus forbids the > driver from enabling bus termination, even though we know there are > products out there which support bus termination on the pl011 through > a GPIO (Revolution Pi RevPi Compact, Revpi Flat). > > I think what you want to do is amend uart_get_rs485_mode() to set > SER_RS485_TERMINATE_BUS in port->rs485_supported_flags if a GPIO > was found in the DT. Instead of the change proposed above. That seems appropriate (and is a fix). What makes it a bit complicated though is that it's a pointer currently and what it points to is shared per driver (besides being const): const struct serial_rs485 *rs485_supported; While it could be embedded into uart_port, there's the .padding which we might not want to bloat uart_port with. Perhaps create non-uapi struct kserial_rs485 w/o .padding and add static_assert()s to ensure the layout is identical to serial_rs485? -- i.
WARNING: multiple messages have this Message-ID (diff)
From: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com> To: Lukas Wunner <lukas@wunner.de> Cc: Lino Sanfilippo <LinoSanfilippo@gmx.de>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Jiri Slaby <jirislaby@kernel.org>, robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org, Andy Shevchenko <andriy.shevchenko@linux.intel.com>, vz@mleia.com, linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org, linux-serial <linux-serial@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>, p.rosenberger@kunbus.com, Lino Sanfilippo <l.sanfilippo@kunbus.com> Subject: Re: [PATCH 1/8] serial: core: only get RS485 termination gpio if supported Date: Mon, 27 Jun 2022 12:05:50 +0300 (EEST) [thread overview] Message-ID: <fbce8d7e-86e-d47e-bcd8-5b99754d1d2e@linux.intel.com> (raw) In-Reply-To: <20220625194951.GA2879@wunner.de> On Sat, 25 Jun 2022, Lukas Wunner wrote: > On Wed, Jun 22, 2022 at 05:46:52PM +0200, Lino Sanfilippo wrote: > > In uart_get_rs485_mode() only try to get a termination GPIO if RS485 bus > > termination is supported by the driver. > [...] > > --- a/drivers/tty/serial/serial_core.c > > +++ b/drivers/tty/serial/serial_core.c > > @@ -3384,17 +3384,20 @@ int uart_get_rs485_mode(struct uart_port *port) > > rs485conf->flags |= SER_RS485_RTS_AFTER_SEND; > > } > > > > - /* > > - * Disabling termination by default is the safe choice: Else if many > > - * bus participants enable it, no communication is possible at all. > > - * Works fine for short cables and users may enable for longer cables. > > - */ > > - port->rs485_term_gpio = devm_gpiod_get_optional(dev, "rs485-term", > > - GPIOD_OUT_LOW); > > - if (IS_ERR(port->rs485_term_gpio)) { > > - ret = PTR_ERR(port->rs485_term_gpio); > > - port->rs485_term_gpio = NULL; > > - return dev_err_probe(dev, ret, "Cannot get rs485-term-gpios\n"); > > + if (port->rs485_supported->flags & SER_RS485_TERMINATE_BUS) { > > So I think linux-next commit be2e2cb1d281 ("serial: Sanitize rs485_struct") > contains a mistake in that it forces drivers to set SER_RS485_TERMINATE_BUS > in their rs485_supported->flags to allow enabling bus termination. > > That's wrong because *every* rs485-capable driver can enable bus > termination if a GPIO has been defined for that in the DT. Do you mean every em485 using driver? Otherwise I don't see this "forces drivers to set" happening anywhere in the code? You're partially right because there are other bugs in this area such as the one you propose a fix below. While I was making the sanitization series, I entirely missed some parts related to termination because SER_RS485_TERMINATE_BUS is seemingly not set/handled correctly by the core. Another thing that looks a bug is that on subsequent call to TIOCSRS485, w/o SER_RS485_TERMINATE_BUS nothing happens (for non-em485 driver, that is)? It seems to be taken care by 2/8 of this series though, I think. But it should be properly marked as Fixes: ... in that case although nobody has complained about it so likely not a huge issue to anyone. > In fact, another commit which was applied as part of the same series, > ebe2cf736a04 ("serial: pl011: Fill in rs485_supported") does not set > SER_RS485_TERMINATE_BUS in amba-pl011.c's flags and thus forbids the > driver from enabling bus termination, even though we know there are > products out there which support bus termination on the pl011 through > a GPIO (Revolution Pi RevPi Compact, Revpi Flat). > > I think what you want to do is amend uart_get_rs485_mode() to set > SER_RS485_TERMINATE_BUS in port->rs485_supported_flags if a GPIO > was found in the DT. Instead of the change proposed above. That seems appropriate (and is a fix). What makes it a bit complicated though is that it's a pointer currently and what it points to is shared per driver (besides being const): const struct serial_rs485 *rs485_supported; While it could be embedded into uart_port, there's the .padding which we might not want to bloat uart_port with. Perhaps create non-uapi struct kserial_rs485 w/o .padding and add static_assert()s to ensure the layout is identical to serial_rs485? -- i. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-06-27 9:05 UTC|newest] Thread overview: 82+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-06-22 15:46 [PATCH 0/8] Fixes and cleanup for RS485 Lino Sanfilippo 2022-06-22 15:46 ` Lino Sanfilippo 2022-06-22 15:46 ` [PATCH 1/8] serial: core: only get RS485 termination gpio if supported Lino Sanfilippo 2022-06-22 15:46 ` Lino Sanfilippo 2022-06-22 17:04 ` Andy Shevchenko 2022-06-22 17:04 ` Andy Shevchenko 2022-06-23 1:59 ` Lino Sanfilippo 2022-06-23 1:59 ` Lino Sanfilippo 2022-06-23 9:45 ` Andy Shevchenko 2022-06-23 9:45 ` Andy Shevchenko 2022-06-23 16:08 ` Lino Sanfilippo 2022-06-23 16:08 ` Lino Sanfilippo 2022-06-23 16:32 ` Andy Shevchenko 2022-06-23 16:32 ` Andy Shevchenko 2022-06-23 20:19 ` Lino Sanfilippo 2022-06-23 20:19 ` Lino Sanfilippo 2022-06-25 19:49 ` Lukas Wunner 2022-06-27 9:05 ` Ilpo Järvinen [this message] 2022-06-27 9:05 ` Ilpo Järvinen 2022-07-02 16:50 ` Lino Sanfilippo 2022-07-02 16:50 ` Lino Sanfilippo 2022-06-22 15:46 ` [PATCH 2/8] serial: core, 8250: set RS485 termination gpio in serial core Lino Sanfilippo 2022-06-22 15:46 ` Lino Sanfilippo 2022-06-22 17:06 ` Andy Shevchenko 2022-06-22 17:06 ` Andy Shevchenko 2022-06-23 2:03 ` Lino Sanfilippo 2022-06-23 2:03 ` Lino Sanfilippo 2022-06-25 10:40 ` Ilpo Järvinen 2022-06-25 10:40 ` Ilpo Järvinen 2022-06-26 15:41 ` Lino Sanfilippo 2022-06-26 15:41 ` Lino Sanfilippo 2022-06-25 19:58 ` Lukas Wunner 2022-06-26 13:36 ` Lino Sanfilippo 2022-06-26 13:36 ` Lino Sanfilippo 2022-06-28 8:31 ` Ilpo Järvinen 2022-06-28 8:31 ` Ilpo Järvinen 2022-06-22 15:46 ` [PATCH 3/8] serial: core: move sanitizing of RS485 delays into own function Lino Sanfilippo 2022-06-22 15:46 ` Lino Sanfilippo 2022-06-23 16:25 ` Andy Shevchenko 2022-06-23 16:25 ` Andy Shevchenko 2022-06-23 20:17 ` Lino Sanfilippo 2022-06-23 20:17 ` Lino Sanfilippo 2022-06-25 9:37 ` Ilpo Järvinen 2022-06-25 9:37 ` Ilpo Järvinen 2022-06-22 15:46 ` [PATCH 4/8] serial: core: sanitize RS485 delays read from device tree Lino Sanfilippo 2022-06-22 15:46 ` Lino Sanfilippo 2022-06-25 10:05 ` Ilpo Järvinen 2022-06-25 10:05 ` Ilpo Järvinen 2022-06-26 14:25 ` Lino Sanfilippo 2022-06-26 14:25 ` Lino Sanfilippo 2022-06-22 15:46 ` [PATCH 5/8] dt_bindings: rs485: Correct delay values Lino Sanfilippo 2022-06-22 15:46 ` Lino Sanfilippo 2022-06-23 16:29 ` Andy Shevchenko 2022-06-23 16:29 ` Andy Shevchenko 2022-06-23 20:17 ` Lino Sanfilippo 2022-06-23 20:17 ` Lino Sanfilippo 2022-06-25 9:54 ` Ilpo Järvinen 2022-06-25 9:54 ` Ilpo Järvinen 2022-06-27 9:23 ` Ilpo Järvinen 2022-06-27 9:23 ` Ilpo Järvinen 2022-06-28 10:03 ` Andy Shevchenko 2022-06-28 10:03 ` Andy Shevchenko 2022-06-29 23:50 ` Lino Sanfilippo 2022-06-29 23:50 ` Lino Sanfilippo 2022-06-22 15:46 ` [PATCH 6/8] serial: 8250_dwlib: remove redundant sanity check for RS485 flags Lino Sanfilippo 2022-06-22 15:46 ` Lino Sanfilippo 2022-06-25 10:21 ` Ilpo Järvinen 2022-06-25 10:21 ` Ilpo Järvinen 2022-06-22 15:46 ` [PATCH 7/8] serial: ar933x: Remove redundant assignment in rs485_config Lino Sanfilippo 2022-06-22 15:46 ` Lino Sanfilippo 2022-06-25 10:14 ` Ilpo Järvinen 2022-06-25 10:14 ` Ilpo Järvinen 2022-06-26 14:09 ` Lino Sanfilippo 2022-06-26 14:09 ` Lino Sanfilippo 2022-06-27 8:14 ` Ilpo Järvinen 2022-06-27 8:14 ` Ilpo Järvinen 2022-06-30 0:33 ` Lino Sanfilippo 2022-06-30 0:33 ` Lino Sanfilippo 2022-06-22 15:46 ` [PATCH 8/8] serial: 8250: lpc18xx: Remove redundant sanity check for RS485 flags Lino Sanfilippo 2022-06-22 15:46 ` Lino Sanfilippo 2022-06-25 10:18 ` Ilpo Järvinen 2022-06-25 10:18 ` Ilpo Järvinen
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=fbce8d7e-86e-d47e-bcd8-5b99754d1d2e@linux.intel.com \ --to=ilpo.jarvinen@linux.intel.com \ --cc=LinoSanfilippo@gmx.de \ --cc=andriy.shevchenko@linux.intel.com \ --cc=devicetree@vger.kernel.org \ --cc=gregkh@linuxfoundation.org \ --cc=jirislaby@kernel.org \ --cc=krzysztof.kozlowski+dt@linaro.org \ --cc=l.sanfilippo@kunbus.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-serial@vger.kernel.org \ --cc=lukas@wunner.de \ --cc=p.rosenberger@kunbus.com \ --cc=robh+dt@kernel.org \ --cc=vz@mleia.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.