diff mbox series

[v6,1/2] tty: serial: Cleanup the bit shift with macro

Message ID 20231201071554.258607-2-crescentcy.hsieh@moxa.com
State New
Headers show
Series tty: serial: Add RS422 serial interface | expand

Commit Message

Crescent CY Hsieh Dec. 1, 2023, 7:15 a.m. UTC
This patch replaces the bit shift code with "_BITUL()" macro inside
"serial_rs485" struct.

Signed-off-by: Crescent CY Hsieh <crescentcy.hsieh@moxa.com>
---
 include/uapi/linux/serial.h | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Comments

Christoph Niedermaier Jan. 17, 2024, 2:56 p.m. UTC | #1
Hi everyone,

> This patch replaces the bit shift code with "_BITUL()" macro inside
> "serial_rs485" struct.
> 
> Signed-off-by: Crescent CY Hsieh <crescentcy.hsieh@moxa.com>
> ---
>  include/uapi/linux/serial.h | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h
> index 53bc1af67..6c75ebdd7 100644
> --- a/include/uapi/linux/serial.h
> +++ b/include/uapi/linux/serial.h
> @@ -11,6 +11,7 @@
>  #ifndef _UAPI_LINUX_SERIAL_H
>  #define _UAPI_LINUX_SERIAL_H
>  
> +#include <linux/const.h>
>  #include <linux/types.h>
>  
>  #include <linux/tty_flags.h>
> @@ -140,14 +141,14 @@ struct serial_icounter_struct {
>   */
>  struct serial_rs485 {
>  	__u32	flags;
> -#define SER_RS485_ENABLED		(1 << 0)
> -#define SER_RS485_RTS_ON_SEND		(1 << 1)
> -#define SER_RS485_RTS_AFTER_SEND	(1 << 2)

In the old definition (1 << 3) wasn't used.

> -#define SER_RS485_RX_DURING_TX		(1 << 4)
> -#define SER_RS485_TERMINATE_BUS		(1 << 5)
> -#define SER_RS485_ADDRB			(1 << 6)
> -#define SER_RS485_ADDR_RECV		(1 << 7)
> -#define SER_RS485_ADDR_DEST		(1 << 8)
> +#define SER_RS485_ENABLED		_BITUL(0)
> +#define SER_RS485_RTS_ON_SEND		_BITUL(1)
> +#define SER_RS485_RTS_AFTER_SEND	_BITUL(2)
> +#define SER_RS485_RX_DURING_TX		_BITUL(3)

Isn't it a break if number 3 isn't skipped here as well?

Regards
Christoph
Greg Kroah-Hartman Jan. 18, 2024, 7:01 a.m. UTC | #2
On Wed, Jan 17, 2024 at 03:56:23PM +0100, Christoph Niedermaier wrote:
> Hi everyone,
> 
> > This patch replaces the bit shift code with "_BITUL()" macro inside
> > "serial_rs485" struct.
> > 
> > Signed-off-by: Crescent CY Hsieh <crescentcy.hsieh@moxa.com>
> > ---
> >  include/uapi/linux/serial.h | 17 +++++++++--------
> >  1 file changed, 9 insertions(+), 8 deletions(-)
> > 
> > diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h
> > index 53bc1af67..6c75ebdd7 100644
> > --- a/include/uapi/linux/serial.h
> > +++ b/include/uapi/linux/serial.h
> > @@ -11,6 +11,7 @@
> >  #ifndef _UAPI_LINUX_SERIAL_H
> >  #define _UAPI_LINUX_SERIAL_H
> >  
> > +#include <linux/const.h>
> >  #include <linux/types.h>
> >  
> >  #include <linux/tty_flags.h>
> > @@ -140,14 +141,14 @@ struct serial_icounter_struct {
> >   */
> >  struct serial_rs485 {
> >  	__u32	flags;
> > -#define SER_RS485_ENABLED		(1 << 0)
> > -#define SER_RS485_RTS_ON_SEND		(1 << 1)
> > -#define SER_RS485_RTS_AFTER_SEND	(1 << 2)
> 
> In the old definition (1 << 3) wasn't used.
> 
> > -#define SER_RS485_RX_DURING_TX		(1 << 4)
> > -#define SER_RS485_TERMINATE_BUS		(1 << 5)
> > -#define SER_RS485_ADDRB			(1 << 6)
> > -#define SER_RS485_ADDR_RECV		(1 << 7)
> > -#define SER_RS485_ADDR_DEST		(1 << 8)
> > +#define SER_RS485_ENABLED		_BITUL(0)
> > +#define SER_RS485_RTS_ON_SEND		_BITUL(1)
> > +#define SER_RS485_RTS_AFTER_SEND	_BITUL(2)
> > +#define SER_RS485_RX_DURING_TX		_BITUL(3)
> 
> Isn't it a break if number 3 isn't skipped here as well?

Ugh, yes it is, good catch!

Care to send a patch to fix this up?

thanks,

greg k-h
Crescent CY Hsieh Jan. 18, 2024, 8:14 a.m. UTC | #3
On Thu, Jan 18, 2024 at 08:01:58AM +0100, Greg KH wrote:
> On Wed, Jan 17, 2024 at 03:56:23PM +0100, Christoph Niedermaier wrote:
> > >  struct serial_rs485 {
> > >  	__u32	flags;
> > > -#define SER_RS485_ENABLED		(1 << 0)
> > > -#define SER_RS485_RTS_ON_SEND		(1 << 1)
> > > -#define SER_RS485_RTS_AFTER_SEND	(1 << 2)
> > 
> > In the old definition (1 << 3) wasn't used.
> > 
> > > -#define SER_RS485_RX_DURING_TX		(1 << 4)
> > > -#define SER_RS485_TERMINATE_BUS		(1 << 5)
> > > -#define SER_RS485_ADDRB			(1 << 6)
> > > -#define SER_RS485_ADDR_RECV		(1 << 7)
> > > -#define SER_RS485_ADDR_DEST		(1 << 8)
> > > +#define SER_RS485_ENABLED		_BITUL(0)
> > > +#define SER_RS485_RTS_ON_SEND		_BITUL(1)
> > > +#define SER_RS485_RTS_AFTER_SEND	_BITUL(2)
> > > +#define SER_RS485_RX_DURING_TX		_BITUL(3)
> > 
> > Isn't it a break if number 3 isn't skipped here as well?

Sorry I might have misunderstood the meaning of "broke userspace".

In this case, does it imply splitting the "cleanup" patch and the "add
feature" patch, or leaving the third bit unused? Or perhaps both?

---
Sincerely,
Crescent Hsieh
Greg Kroah-Hartman Jan. 18, 2024, 8:44 a.m. UTC | #4
On Thu, Jan 18, 2024 at 04:14:41PM +0800, Crescent CY Hsieh wrote:
> On Thu, Jan 18, 2024 at 08:01:58AM +0100, Greg KH wrote:
> > On Wed, Jan 17, 2024 at 03:56:23PM +0100, Christoph Niedermaier wrote:
> > > >  struct serial_rs485 {
> > > >  	__u32	flags;
> > > > -#define SER_RS485_ENABLED		(1 << 0)
> > > > -#define SER_RS485_RTS_ON_SEND		(1 << 1)
> > > > -#define SER_RS485_RTS_AFTER_SEND	(1 << 2)
> > > 
> > > In the old definition (1 << 3) wasn't used.
> > > 
> > > > -#define SER_RS485_RX_DURING_TX		(1 << 4)
> > > > -#define SER_RS485_TERMINATE_BUS		(1 << 5)
> > > > -#define SER_RS485_ADDRB			(1 << 6)
> > > > -#define SER_RS485_ADDR_RECV		(1 << 7)
> > > > -#define SER_RS485_ADDR_DEST		(1 << 8)
> > > > +#define SER_RS485_ENABLED		_BITUL(0)
> > > > +#define SER_RS485_RTS_ON_SEND		_BITUL(1)
> > > > +#define SER_RS485_RTS_AFTER_SEND	_BITUL(2)
> > > > +#define SER_RS485_RX_DURING_TX		_BITUL(3)
> > > 
> > > Isn't it a break if number 3 isn't skipped here as well?
> 
> Sorry I might have misunderstood the meaning of "broke userspace".
> 
> In this case, does it imply splitting the "cleanup" patch and the "add
> feature" patch, or leaving the third bit unused? Or perhaps both?

You can not redefine existing defines to different values, like you did
here, that changes the user/kernel api of the system.

Luckily this isn't in any released kernel, I'll either revert this after
-rc1 is out, or take a patch from anyone else to do so if they want to
send it now.

thanks,

greg k-h
Christoph Niedermaier Jan. 18, 2024, 9:29 a.m. UTC | #5
From: Greg KH <gregkh@linuxfoundation.org>
Sent: Thursday, January 18, 2024 8:02 AM
> On Wed, Jan 17, 2024 at 03:56:23PM +0100, Christoph Niedermaier wrote:
>> Hi everyone,
>>
>>> This patch replaces the bit shift code with "_BITUL()" macro inside
>>> "serial_rs485" struct.
>>>
>>> Signed-off-by: Crescent CY Hsieh <crescentcy.hsieh@moxa.com>
>>> ---
>>>  include/uapi/linux/serial.h | 17 +++++++++--------
>>>  1 file changed, 9 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h
>>> index 53bc1af67..6c75ebdd7 100644
>>> --- a/include/uapi/linux/serial.h
>>> +++ b/include/uapi/linux/serial.h
>>> @@ -11,6 +11,7 @@
>>>  #ifndef _UAPI_LINUX_SERIAL_H
>>>  #define _UAPI_LINUX_SERIAL_H
>>>
>>> +#include <linux/const.h>
>>>  #include <linux/types.h>
>>>
>>>  #include <linux/tty_flags.h>
>>> @@ -140,14 +141,14 @@ struct serial_icounter_struct {
>>>   */
>>>  struct serial_rs485 {
>>>     __u32   flags;
>>> -#define SER_RS485_ENABLED          (1 << 0)
>>> -#define SER_RS485_RTS_ON_SEND              (1 << 1)
>>> -#define SER_RS485_RTS_AFTER_SEND   (1 << 2)
>>
>> In the old definition (1 << 3) wasn't used.
>>
>>> -#define SER_RS485_RX_DURING_TX             (1 << 4)
>>> -#define SER_RS485_TERMINATE_BUS            (1 << 5)
>>> -#define SER_RS485_ADDRB                    (1 << 6)
>>> -#define SER_RS485_ADDR_RECV                (1 << 7)
>>> -#define SER_RS485_ADDR_DEST                (1 << 8)
>>> +#define SER_RS485_ENABLED          _BITUL(0)
>>> +#define SER_RS485_RTS_ON_SEND              _BITUL(1)
>>> +#define SER_RS485_RTS_AFTER_SEND   _BITUL(2)
>>> +#define SER_RS485_RX_DURING_TX             _BITUL(3)
>>
>> Isn't it a break if number 3 isn't skipped here as well?
> 
> Ugh, yes it is, good catch!
> 
> Care to send a patch to fix this up?

OK, I'll make a patch.

Regards
Christoph
diff mbox series

Patch

diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h
index 53bc1af67..6c75ebdd7 100644
--- a/include/uapi/linux/serial.h
+++ b/include/uapi/linux/serial.h
@@ -11,6 +11,7 @@ 
 #ifndef _UAPI_LINUX_SERIAL_H
 #define _UAPI_LINUX_SERIAL_H
 
+#include <linux/const.h>
 #include <linux/types.h>
 
 #include <linux/tty_flags.h>
@@ -140,14 +141,14 @@  struct serial_icounter_struct {
  */
 struct serial_rs485 {
 	__u32	flags;
-#define SER_RS485_ENABLED		(1 << 0)
-#define SER_RS485_RTS_ON_SEND		(1 << 1)
-#define SER_RS485_RTS_AFTER_SEND	(1 << 2)
-#define SER_RS485_RX_DURING_TX		(1 << 4)
-#define SER_RS485_TERMINATE_BUS		(1 << 5)
-#define SER_RS485_ADDRB			(1 << 6)
-#define SER_RS485_ADDR_RECV		(1 << 7)
-#define SER_RS485_ADDR_DEST		(1 << 8)
+#define SER_RS485_ENABLED		_BITUL(0)
+#define SER_RS485_RTS_ON_SEND		_BITUL(1)
+#define SER_RS485_RTS_AFTER_SEND	_BITUL(2)
+#define SER_RS485_RX_DURING_TX		_BITUL(3)
+#define SER_RS485_TERMINATE_BUS		_BITUL(4)
+#define SER_RS485_ADDRB			_BITUL(5)
+#define SER_RS485_ADDR_RECV		_BITUL(6)
+#define SER_RS485_ADDR_DEST		_BITUL(7)
 
 	__u32	delay_rts_before_send;
 	__u32	delay_rts_after_send;