All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Bugfix RTS line config in RS485 mode is overwritten in pl011_set_mctrl() function.
@ 2021-12-31 17:15 jmades
  2022-01-02 10:07 ` Lukas Wunner
  0 siblings, 1 reply; 10+ messages in thread
From: jmades @ 2021-12-31 17:15 UTC (permalink / raw)
  To: gregkh; +Cc: jochen, Russell King, Jiri Slaby, linux-serial, linux-kernel

Based on the "0001-serial-amba-pl011-add-RS485-support.patch" this change is necesarry otherwise the RTS-line will be pulled up in SER_RS485_RTS_BEFORE_SEND mode before sending data. This hinders the driver to receive data, f.ex. when the device is an RS485 slave device.

Signed-off-by: jmades <jochen@mades.net>
---
 drivers/tty/serial/amba-pl011.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 537f37ac4..1749c1498 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1646,8 +1646,12 @@ static void pl011_set_mctrl(struct uart_port *port, unsigned int mctrl)
 	    container_of(port, struct uart_amba_port, port);
 	unsigned int cr;
 
-	if (port->rs485.flags & SER_RS485_ENABLED)
-		mctrl &= ~TIOCM_RTS;
+	if (port->rs485.flags & SER_RS485_ENABLED) {
+		if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
+			mctrl &= ~TIOCM_RTS;
+		else
+			mctrl |= TIOCM_RTS;
+	}
 
 	cr = pl011_read(uap, REG_CR);
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] Bugfix RTS line config in RS485 mode is overwritten in pl011_set_mctrl() function.
  2021-12-31 17:15 [PATCH] Bugfix RTS line config in RS485 mode is overwritten in pl011_set_mctrl() function jmades
@ 2022-01-02 10:07 ` Lukas Wunner
  2022-01-02 15:06   ` Lino Sanfilippo
  2022-01-13 10:12   ` Jochen Mades
  0 siblings, 2 replies; 10+ messages in thread
From: Lukas Wunner @ 2022-01-02 10:07 UTC (permalink / raw)
  To: jmades
  Cc: gregkh, Russell King, Jiri Slaby, linux-serial, linux-kernel,
	Lino Sanfilippo, Philipp Rosenberger

On Fri, Dec 31, 2021 at 05:15:14PM +0000, jmades wrote:
> Based on the "0001-serial-amba-pl011-add-RS485-support.patch" this change is necesarry otherwise the RTS-line will be pulled up in SER_RS485_RTS_BEFORE_SEND mode before sending data. This hinders the driver to receive data, f.ex. when the device is an RS485 slave device.
> 
> Signed-off-by: jmades <jochen@mades.net>

Patch is correct, but commit message could be improved:

* Subject should be in imperative mood (by convention), it should be
  prepended by "serial: pl011: " (in line with previous commits touching
  this driver, use "git log --oneline amba-pl011.c") and the trailing dot
  is unnecessary, e.g.:

  "serial: pl011: Fix incorrect rs485 RTS polarity on set_mctrl"

* Commit message should be wrapped at 72 characters (so that it appears
  centered when displayed with "git log" on an 80 chars terminal).
  The reference to "0001-serial-amba-pl011-add-RS485-support.patch"
  should be replaced with a reference to the offending commit, e.g.:

  "Commit 8d479237727c ("serial: amba-pl011: add RS485 support") sought
  to keep RTS deasserted on set_mctrl if rs485 is enabled.  However it
  did so only if deasserted RTS polarity is high.  Fix it in case it's
  low."

  Feel free to copy this to a v2 of your patch and amend as you see fit.

* Add tags for the offending commit:

  Fixes: 8d479237727c ("serial: amba-pl011: add RS485 support")
  Cc: stable@vger.kernel.org # v5.15+

* Be sure to cc the author of the offending commit.

Thanks,

Lukas

> ---
>  drivers/tty/serial/amba-pl011.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 537f37ac4..1749c1498 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -1646,8 +1646,12 @@ static void pl011_set_mctrl(struct uart_port *port, unsigned int mctrl)
>  	    container_of(port, struct uart_amba_port, port);
>  	unsigned int cr;
>  
> -	if (port->rs485.flags & SER_RS485_ENABLED)
> -		mctrl &= ~TIOCM_RTS;
> +	if (port->rs485.flags & SER_RS485_ENABLED) {
> +		if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
> +			mctrl &= ~TIOCM_RTS;
> +		else
> +			mctrl |= TIOCM_RTS;
> +	}
>  
>  	cr = pl011_read(uap, REG_CR);

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Bugfix RTS line config in RS485 mode is overwritten in pl011_set_mctrl() function.
  2022-01-02 10:07 ` Lukas Wunner
@ 2022-01-02 15:06   ` Lino Sanfilippo
  2022-01-02 18:28     ` Lukas Wunner
  2022-01-13 10:12   ` Jochen Mades
  1 sibling, 1 reply; 10+ messages in thread
From: Lino Sanfilippo @ 2022-01-02 15:06 UTC (permalink / raw)
  To: Lukas Wunner, jmades
  Cc: gregkh, Russell King, Jiri Slaby, linux-serial, linux-kernel,
	Philipp Rosenberger


Hi,

On 02.01.22 at 11:07, Lukas Wunner wrote:
> On Fri, Dec 31, 2021 at 05:15:14PM +0000, jmades wrote:
>> Based on the "0001-serial-amba-pl011-add-RS485-support.patch" this change is necesarry otherwise the RTS-line will be pulled up in SER_RS485_RTS_BEFORE_SEND mode before sending data. This hinders the driver to receive data, f.ex. when the device is an RS485 slave device.
>>
>> Signed-off-by: jmades <jochen@mades.net>
>
> Patch is correct, but commit message could be improved:
>
> * Subject should be in imperative mood (by convention), it should be
>   prepended by "serial: pl011: " (in line with previous commits touching
>   this driver, use "git log --oneline amba-pl011.c") and the trailing dot
>   is unnecessary, e.g.:
>
>   "serial: pl011: Fix incorrect rs485 RTS polarity on set_mctrl"
>
> * Commit message should be wrapped at 72 characters (so that it appears
>   centered when displayed with "git log" on an 80 chars terminal).
>   The reference to "0001-serial-amba-pl011-add-RS485-support.patch"
>   should be replaced with a reference to the offending commit, e.g.:
>
>   "Commit 8d479237727c ("serial: amba-pl011: add RS485 support") sought
>   to keep RTS deasserted on set_mctrl if rs485 is enabled.  However it
>   did so only if deasserted RTS polarity is high.  Fix it in case it's
>   low."
>
>   Feel free to copy this to a v2 of your patch and amend as you see fit.
>
> * Add tags for the offending commit:
>
>   Fixes: 8d479237727c ("serial: amba-pl011: add RS485 support")
>   Cc: stable@vger.kernel.org # v5.15+
>
> * Be sure to cc the author of the offending commit.
>
> Thanks,
>
> Lukas
>
>> ---
>>  drivers/tty/serial/amba-pl011.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
>> index 537f37ac4..1749c1498 100644
>> --- a/drivers/tty/serial/amba-pl011.c
>> +++ b/drivers/tty/serial/amba-pl011.c
>> @@ -1646,8 +1646,12 @@ static void pl011_set_mctrl(struct uart_port *port, unsigned int mctrl)
>>  	    container_of(port, struct uart_amba_port, port);
>>  	unsigned int cr;
>>
>> -	if (port->rs485.flags & SER_RS485_ENABLED)
>> -		mctrl &= ~TIOCM_RTS;
>> +	if (port->rs485.flags & SER_RS485_ENABLED) {
>> +		if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
>> +			mctrl &= ~TIOCM_RTS;
>> +		else
>> +			mctrl |= TIOCM_RTS;
>> +	}
>>
>>  	cr = pl011_read(uap, REG_CR);

Does this logic really have to be implemented in the driver? It looks as if the serial core already takes
RS485 into account before calling set_mctrls(). At least I get the impression when looking
at uart_tiocmset() and uart_port_dtr_rts(). Also other drivers like imx simply seem to ignore RTS in case
of RS485.

Regards,
Lino

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Bugfix RTS line config in RS485 mode is overwritten in pl011_set_mctrl() function.
  2022-01-02 15:06   ` Lino Sanfilippo
@ 2022-01-02 18:28     ` Lukas Wunner
  2022-01-03 13:44       ` Lino Sanfilippo
  2022-01-11 10:00       ` Jochen Mades
  0 siblings, 2 replies; 10+ messages in thread
From: Lukas Wunner @ 2022-01-02 18:28 UTC (permalink / raw)
  To: Lino Sanfilippo
  Cc: jmades, gregkh, Russell King, Jiri Slaby, linux-serial,
	linux-kernel, Philipp Rosenberger

On Sun, Jan 02, 2022 at 04:06:53PM +0100, Lino Sanfilippo wrote:
> On 02.01.22 at 11:07, Lukas Wunner wrote:
> > On Fri, Dec 31, 2021 at 05:15:14PM +0000, jmades wrote:
> > > --- a/drivers/tty/serial/amba-pl011.c
> > > +++ b/drivers/tty/serial/amba-pl011.c
> > > @@ -1646,8 +1646,12 @@ static void pl011_set_mctrl(struct uart_port *port, unsigned int mctrl)
> > >  	    container_of(port, struct uart_amba_port, port);
> > >  	unsigned int cr;
> > >
> > > -	if (port->rs485.flags & SER_RS485_ENABLED)
> > > -		mctrl &= ~TIOCM_RTS;
> > > +	if (port->rs485.flags & SER_RS485_ENABLED) {
> > > +		if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
> > > +			mctrl &= ~TIOCM_RTS;
> > > +		else
> > > +			mctrl |= TIOCM_RTS;
> > > +	}
> > >
> > >  	cr = pl011_read(uap, REG_CR);
> 
> Does this logic really have to be implemented in the driver?

No, it doesn't have to be and indeed I'm working towards consolidating
it in the serial core with this collection of patches:

https://git.kernel.org/gregkh/tty/c/d3b3404df318
https://lore.kernel.org/all/f49f945375f5ccb979893c49f1129f51651ac738.1641129062.git.lukas@wunner.de
https://lore.kernel.org/all/e22089ab49e6e78822c50c8c4db46bf3ee885623.1641129328.git.lukas@wunner.de
https://lore.kernel.org/all/bceeaba030b028ed810272d55d5fc6f3656ddddb.1641129752.git.lukas@wunner.de
https://github.com/l1k/linux/commit/532ef2ad757f

The last of these removes the rs485 logic from pl011_set_mctrl().
I'll post it once the others (and Jochen Mades' patch) have landed.

Even though the logic is eventually removed from pl011_set_mctrl(),
Jochen's patch makes sense as a backportable fix for v5.15.


> It looks as if the serial core already takes RS485 into account before
> calling set_mctrls(). At least I get the impression when looking
> at uart_tiocmset() and uart_port_dtr_rts(). Also other drivers like imx
> simply seem to ignore RTS in case of RS485.

The logic in uart_port_dtr_rts() is broken.  That's fixed by d3b3404df318,
which is queued up in tty-next for v5.17.

The pl011 driver papered over it with its own rs485-specific logic in
pl011_set_mctrl().  But as Jochen Mades correctly pointed out, that
only worked correctly if RTS is driven high on idle.


The logic in uart_tiocmset() is correct, but not sufficient because
uart_throttle(), uart_unthrottle and uart_set_termios() need to become
rs485-aware as well.  That's also addressed by the above-linked
GitHub commit.

Thanks,

Lukas

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Bugfix RTS line config in RS485 mode is overwritten in pl011_set_mctrl() function.
  2022-01-02 18:28     ` Lukas Wunner
@ 2022-01-03 13:44       ` Lino Sanfilippo
  2022-01-11 10:00       ` Jochen Mades
  1 sibling, 0 replies; 10+ messages in thread
From: Lino Sanfilippo @ 2022-01-03 13:44 UTC (permalink / raw)
  To: Lukas Wunner
  Cc: jmades, gregkh, Russell King, Jiri Slaby, linux-serial,
	linux-kernel, Philipp Rosenberger

On 02.01.22 at 19:28, Lukas Wunner wrote:
> On Sun, Jan 02, 2022 at 04:06:53PM +0100, Lino Sanfilippo wrote:
>> On 02.01.22 at 11:07, Lukas Wunner wrote:
>>> On Fri, Dec 31, 2021 at 05:15:14PM +0000, jmades wrote:
>>>> --- a/drivers/tty/serial/amba-pl011.c
>>>> +++ b/drivers/tty/serial/amba-pl011.c
>>>> @@ -1646,8 +1646,12 @@ static void pl011_set_mctrl(struct uart_port *port, unsigned int mctrl)
>>>>  	    container_of(port, struct uart_amba_port, port);
>>>>  	unsigned int cr;
>>>>
>>>> -	if (port->rs485.flags & SER_RS485_ENABLED)
>>>> -		mctrl &= ~TIOCM_RTS;
>>>> +	if (port->rs485.flags & SER_RS485_ENABLED) {
>>>> +		if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
>>>> +			mctrl &= ~TIOCM_RTS;
>>>> +		else
>>>> +			mctrl |= TIOCM_RTS;
>>>> +	}
>>>>
>>>>  	cr = pl011_read(uap, REG_CR);
>>
>> Does this logic really have to be implemented in the driver?
>
> No, it doesn't have to be and indeed I'm working towards consolidating
> it in the serial core with this collection of patches:
>
> https://git.kernel.org/gregkh/tty/c/d3b3404df318
> https://lore.kernel.org/all/f49f945375f5ccb979893c49f1129f51651ac738.1641129062.git.lukas@wunner.de
> https://lore.kernel.org/all/e22089ab49e6e78822c50c8c4db46bf3ee885623.1641129328.git.lukas@wunner.de
> https://lore.kernel.org/all/bceeaba030b028ed810272d55d5fc6f3656ddddb.1641129752.git.lukas@wunner.de
> https://github.com/l1k/linux/commit/532ef2ad757f
>
> The last of these removes the rs485 logic from pl011_set_mctrl().
> I'll post it once the others (and Jochen Mades' patch) have landed.
>
> Even though the logic is eventually removed from pl011_set_mctrl(),
> Jochen's patch makes sense as a backportable fix for v5.15.
>
>
>> It looks as if the serial core already takes RS485 into account before
>> calling set_mctrls(). At least I get the impression when looking
>> at uart_tiocmset() and uart_port_dtr_rts(). Also other drivers like imx
>> simply seem to ignore RTS in case of RS485.
>
> The logic in uart_port_dtr_rts() is broken.  That's fixed by d3b3404df318,
> which is queued up in tty-next for v5.17.
>
> The pl011 driver papered over it with its own rs485-specific logic in
> pl011_set_mctrl().  But as Jochen Mades correctly pointed out, that
> only worked correctly if RTS is driven high on idle.
>
>
> The logic in uart_tiocmset() is correct, but not sufficient because
> uart_throttle(), uart_unthrottle and uart_set_termios() need to become
> rs485-aware as well.  That's also addressed by the above-linked
> GitHub commit.
>


Thanks for this information. I have one or two patches prepared a while ago that
aim into a similar direction (move RS485 related code out of the drivers into
the serial core). I will send them in the next days.

Best regards,
Lino




^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Bugfix RTS line config in RS485 mode is overwritten in pl011_set_mctrl() function.
  2022-01-02 18:28     ` Lukas Wunner
  2022-01-03 13:44       ` Lino Sanfilippo
@ 2022-01-11 10:00       ` Jochen Mades
  2022-01-12  1:41         ` Lino Sanfilippo
  1 sibling, 1 reply; 10+ messages in thread
From: Jochen Mades @ 2022-01-11 10:00 UTC (permalink / raw)
  To: Lukas Wunner, Lino Sanfilippo
  Cc: gregkh, Russell King, Jiri Slaby, linux-serial, linux-kernel,
	Philipp Rosenberger

Hi Lukas, Lino,

please let me know when I could test again with an "official" linux kernel, instead of using my local patch.

Bests
Jochen

> On 02/01/2022 19:28 Lukas Wunner <lukas@wunner.de> wrote:
> 
>  
> On Sun, Jan 02, 2022 at 04:06:53PM +0100, Lino Sanfilippo wrote:
> > On 02.01.22 at 11:07, Lukas Wunner wrote:
> > > On Fri, Dec 31, 2021 at 05:15:14PM +0000, jmades wrote:
> > > > --- a/drivers/tty/serial/amba-pl011.c
> > > > +++ b/drivers/tty/serial/amba-pl011.c
> > > > @@ -1646,8 +1646,12 @@ static void pl011_set_mctrl(struct uart_port *port, unsigned int mctrl)
> > > >  	    container_of(port, struct uart_amba_port, port);
> > > >  	unsigned int cr;
> > > >
> > > > -	if (port->rs485.flags & SER_RS485_ENABLED)
> > > > -		mctrl &= ~TIOCM_RTS;
> > > > +	if (port->rs485.flags & SER_RS485_ENABLED) {
> > > > +		if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
> > > > +			mctrl &= ~TIOCM_RTS;
> > > > +		else
> > > > +			mctrl |= TIOCM_RTS;
> > > > +	}
> > > >
> > > >  	cr = pl011_read(uap, REG_CR);
> > 
> > Does this logic really have to be implemented in the driver?
> 
> No, it doesn't have to be and indeed I'm working towards consolidating
> it in the serial core with this collection of patches:
> 
> https://git.kernel.org/gregkh/tty/c/d3b3404df318
> https://lore.kernel.org/all/f49f945375f5ccb979893c49f1129f51651ac738.1641129062.git.lukas@wunner.de
> https://lore.kernel.org/all/e22089ab49e6e78822c50c8c4db46bf3ee885623.1641129328.git.lukas@wunner.de
> https://lore.kernel.org/all/bceeaba030b028ed810272d55d5fc6f3656ddddb.1641129752.git.lukas@wunner.de
> https://github.com/l1k/linux/commit/532ef2ad757f
> 
> The last of these removes the rs485 logic from pl011_set_mctrl().
> I'll post it once the others (and Jochen Mades' patch) have landed.
> 
> Even though the logic is eventually removed from pl011_set_mctrl(),
> Jochen's patch makes sense as a backportable fix for v5.15.
> 
> 
> > It looks as if the serial core already takes RS485 into account before
> > calling set_mctrls(). At least I get the impression when looking
> > at uart_tiocmset() and uart_port_dtr_rts(). Also other drivers like imx
> > simply seem to ignore RTS in case of RS485.
> 
> The logic in uart_port_dtr_rts() is broken.  That's fixed by d3b3404df318,
> which is queued up in tty-next for v5.17.
> 
> The pl011 driver papered over it with its own rs485-specific logic in
> pl011_set_mctrl().  But as Jochen Mades correctly pointed out, that
> only worked correctly if RTS is driven high on idle.
> 
> 
> The logic in uart_tiocmset() is correct, but not sufficient because
> uart_throttle(), uart_unthrottle and uart_set_termios() need to become
> rs485-aware as well.  That's also addressed by the above-linked
> GitHub commit.
> 
> Thanks,
> 
> Lukas

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Bugfix RTS line config in RS485 mode is overwritten in pl011_set_mctrl() function.
  2022-01-11 10:00       ` Jochen Mades
@ 2022-01-12  1:41         ` Lino Sanfilippo
  0 siblings, 0 replies; 10+ messages in thread
From: Lino Sanfilippo @ 2022-01-12  1:41 UTC (permalink / raw)
  To: Jochen Mades, Lukas Wunner
  Cc: gregkh, Russell King, Jiri Slaby, linux-serial, linux-kernel,
	Philipp Rosenberger

Hi Jochen,

On 11.01.22 at 11:00, Jochen Mades wrote:
> Hi Lukas, Lino,
>
> please let me know when I could test again with an "official" linux kernel, instead of using my local patch.
>
> Bests
> Jochen

While the fix itself looks good so far there are still some style issue as Lukas pointed out. These issues
have to be fixed before the patch can be accepted for mainline integration. Please have a look at
Documentation/process/submitting-patches.rst for the correct patch format. After these issues are fixed
please send the patch as a v2, so we can review the new version.

Best regards,
Lino


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Bugfix RTS line config in RS485 mode is overwritten in pl011_set_mctrl() function.
  2022-01-02 10:07 ` Lukas Wunner
  2022-01-02 15:06   ` Lino Sanfilippo
@ 2022-01-13 10:12   ` Jochen Mades
  2022-01-13 10:20     ` Greg KH
  2022-01-23  4:32     ` Lukas Wunner
  1 sibling, 2 replies; 10+ messages in thread
From: Jochen Mades @ 2022-01-13 10:12 UTC (permalink / raw)
  To: Lukas Wunner
  Cc: gregkh, Russell King, Jiri Slaby, linux-serial, linux-kernel,
	Lino Sanfilippo, Philipp Rosenberger

[-- Attachment #1: Type: text/plain, Size: 2190 bytes --]

Hi Lukas,

> Patch is correct, but commit message could be improved:
> 
> * Subject should be in imperative mood (by convention), it should be
>   prepended by "serial: pl011: " (in line with previous commits touching
>   this driver, use "git log --oneline amba-pl011.c") and the trailing dot
>   is unnecessary, e.g.:
> 
>   "serial: pl011: Fix incorrect rs485 RTS polarity on set_mctrl"
> 
> * Commit message should be wrapped at 72 characters (so that it appears
>   centered when displayed with "git log" on an 80 chars terminal).
>   The reference to "0001-serial-amba-pl011-add-RS485-support.patch"
>   should be replaced with a reference to the offending commit, e.g.:
>
>   "Commit 8d479237727c ("serial: amba-pl011: add RS485 support") sought
>   to keep RTS deasserted on set_mctrl if rs485 is enabled.  However it
>   did so only if deasserted RTS polarity is high.  Fix it in case it's
>   low."
>
>   Feel free to copy this to a v2 of your patch and amend as you see fit.
> 

Find attached the patch with the new subject and corretced commit message.

> * Add tags for the offending commit:
> 
>   Fixes: 8d479237727c ("serial: amba-pl011: add RS485 support")
>   Cc: stable@vger.kernel.org # v5.15+
> 
> * Be sure to cc the author of the offending commit.

Sorry I don't know how to do that correctly. Can you please give support/hints?

 
> Thanks,
> 
> Lukas
> 
> > ---
> >  drivers/tty/serial/amba-pl011.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> > index 537f37ac4..1749c1498 100644
> > --- a/drivers/tty/serial/amba-pl011.c
> > +++ b/drivers/tty/serial/amba-pl011.c
> > @@ -1646,8 +1646,12 @@ static void pl011_set_mctrl(struct uart_port *port, unsigned int mctrl)
> >  	    container_of(port, struct uart_amba_port, port);
> >  	unsigned int cr;
> >  
> > -	if (port->rs485.flags & SER_RS485_ENABLED)
> > -		mctrl &= ~TIOCM_RTS;
> > +	if (port->rs485.flags & SER_RS485_ENABLED) {
> > +		if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
> > +			mctrl &= ~TIOCM_RTS;
> > +		else
> > +			mctrl |= TIOCM_RTS;
> > +	}
> >  
> >  	cr = pl011_read(uap, REG_CR);

[-- Attachment #2: 0001-serial-pl011-Fix-incorrect-rs485-RTS-polarity-on-set.patch --]
[-- Type: application/octet-stream, Size: 1210 bytes --]

From 90690b3e8afbdfbbb5ceb0f6cef40ed282b1c004 Mon Sep 17 00:00:00 2001
From: jmades <jochen@mades.net>
Date: Mon, 27 Dec 2021 08:37:26 +0000
Subject: [PATCH] serial: pl011: Fix incorrect rs485 RTS polarity on set_mctrl

Commit 8d479237727c ("serial: amba-pl011: add RS485 support") sought
to keep RTS deasserted on set_mctrl if rs485 is enabled. However it did
so only if deasserted RTS polarity is high. Fix it in case it's low.

Signed-off-by: jmades <jochen@mades.net>
---
 drivers/tty/serial/amba-pl011.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 537f37ac4..1749c1498 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1646,8 +1646,12 @@ static void pl011_set_mctrl(struct uart_port *port, unsigned int mctrl)
 	    container_of(port, struct uart_amba_port, port);
 	unsigned int cr;
 
-	if (port->rs485.flags & SER_RS485_ENABLED)
-		mctrl &= ~TIOCM_RTS;
+	if (port->rs485.flags & SER_RS485_ENABLED) {
+		if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
+			mctrl &= ~TIOCM_RTS;
+		else
+			mctrl |= TIOCM_RTS;
+	}
 
 	cr = pl011_read(uap, REG_CR);
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] Bugfix RTS line config in RS485 mode is overwritten in pl011_set_mctrl() function.
  2022-01-13 10:12   ` Jochen Mades
@ 2022-01-13 10:20     ` Greg KH
  2022-01-23  4:32     ` Lukas Wunner
  1 sibling, 0 replies; 10+ messages in thread
From: Greg KH @ 2022-01-13 10:20 UTC (permalink / raw)
  To: Jochen Mades
  Cc: Lukas Wunner, Russell King, Jiri Slaby, linux-serial,
	linux-kernel, Lino Sanfilippo, Philipp Rosenberger

On Thu, Jan 13, 2022 at 11:12:12AM +0100, Jochen Mades wrote:
> Hi Lukas,
> 
> > Patch is correct, but commit message could be improved:
> > 
> > * Subject should be in imperative mood (by convention), it should be
> >   prepended by "serial: pl011: " (in line with previous commits touching
> >   this driver, use "git log --oneline amba-pl011.c") and the trailing dot
> >   is unnecessary, e.g.:
> > 
> >   "serial: pl011: Fix incorrect rs485 RTS polarity on set_mctrl"
> > 
> > * Commit message should be wrapped at 72 characters (so that it appears
> >   centered when displayed with "git log" on an 80 chars terminal).
> >   The reference to "0001-serial-amba-pl011-add-RS485-support.patch"
> >   should be replaced with a reference to the offending commit, e.g.:
> >
> >   "Commit 8d479237727c ("serial: amba-pl011: add RS485 support") sought
> >   to keep RTS deasserted on set_mctrl if rs485 is enabled.  However it
> >   did so only if deasserted RTS polarity is high.  Fix it in case it's
> >   low."
> >
> >   Feel free to copy this to a v2 of your patch and amend as you see fit.
> > 
> 
> Find attached the patch with the new subject and corretced commit message.
> 
> > * Add tags for the offending commit:
> > 
> >   Fixes: 8d479237727c ("serial: amba-pl011: add RS485 support")
> >   Cc: stable@vger.kernel.org # v5.15+
> > 
> > * Be sure to cc the author of the offending commit.
> 
> Sorry I don't know how to do that correctly. Can you please give support/hints?
> 
>  
> > Thanks,
> > 
> > Lukas
> > 
> > > ---
> > >  drivers/tty/serial/amba-pl011.c | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> > > index 537f37ac4..1749c1498 100644
> > > --- a/drivers/tty/serial/amba-pl011.c
> > > +++ b/drivers/tty/serial/amba-pl011.c
> > > @@ -1646,8 +1646,12 @@ static void pl011_set_mctrl(struct uart_port *port, unsigned int mctrl)
> > >  	    container_of(port, struct uart_amba_port, port);
> > >  	unsigned int cr;
> > >  
> > > -	if (port->rs485.flags & SER_RS485_ENABLED)
> > > -		mctrl &= ~TIOCM_RTS;
> > > +	if (port->rs485.flags & SER_RS485_ENABLED) {
> > > +		if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
> > > +			mctrl &= ~TIOCM_RTS;
> > > +		else
> > > +			mctrl |= TIOCM_RTS;
> > > +	}
> > >  
> > >  	cr = pl011_read(uap, REG_CR);


Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- Your patch was attached, please place it inline so that it can be
  applied directly from the email message itself.

- It looks like you did not use your "real" name for the patch on either
  the Signed-off-by: line, or the From: line (both of which have to
  match).  Please read the kernel file, Documentation/SubmittingPatches
  for how to do this correctly.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Bugfix RTS line config in RS485 mode is overwritten in pl011_set_mctrl() function.
  2022-01-13 10:12   ` Jochen Mades
  2022-01-13 10:20     ` Greg KH
@ 2022-01-23  4:32     ` Lukas Wunner
  1 sibling, 0 replies; 10+ messages in thread
From: Lukas Wunner @ 2022-01-23  4:32 UTC (permalink / raw)
  To: Jochen Mades
  Cc: gregkh, Russell King, Jiri Slaby, linux-serial, linux-kernel,
	Lino Sanfilippo, Philipp Rosenberger

On Thu, Jan 13, 2022 at 11:12:12AM +0100, Jochen Mades wrote:
> Find attached the patch with the new subject and corretced commit message.

Greg doesn't accept patches as attachments and during the merge window
(which ends today).  I've just resubmitted your patch as part of a
2-patch series, together with a patch of mine which goes on top of yours:

https://lore.kernel.org/linux-serial/85fa3323ba8c307943969b7343e23f34c3e652ba.1642909284.git.lukas@wunner.de/

(I sent your patch twice because I botched the Date header the first time
around, sorry about that.)

Note that you still get patch authorship (and thus credit for the patch)
even though I submitted it, due to the From: line at the top of the
message body.


> > * Add tags for the offending commit:
> > 
> >   Fixes: 8d479237727c ("serial: amba-pl011: add RS485 support")
> >   Cc: stable@vger.kernel.org # v5.15+
> > 
> > * Be sure to cc the author of the offending commit.
> 
> Sorry I don't know how to do that correctly. Can you please give support/hints?

It just means that you need to add the two above-quoted tags to the
commit message.  The maintainers of stable kernels use them to
determine which patches need to be backported to which kernel
releases.  I've fixed up the commit message of your patch with
the missing tags, no problem.

And "cc the author of the offending commit" just means that when
posting the patch, it is customary to send a copy of the e-mail
to the author of the commit you're fixing, which is Lino in this
case, as he authored 8d479237727c.

Thanks,

Lukas

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-01-23  4:33 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-31 17:15 [PATCH] Bugfix RTS line config in RS485 mode is overwritten in pl011_set_mctrl() function jmades
2022-01-02 10:07 ` Lukas Wunner
2022-01-02 15:06   ` Lino Sanfilippo
2022-01-02 18:28     ` Lukas Wunner
2022-01-03 13:44       ` Lino Sanfilippo
2022-01-11 10:00       ` Jochen Mades
2022-01-12  1:41         ` Lino Sanfilippo
2022-01-13 10:12   ` Jochen Mades
2022-01-13 10:20     ` Greg KH
2022-01-23  4:32     ` Lukas Wunner

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.