1. Patchew
  2. linux
  3. [PATCH 00/15] serial: sc16is7xx: register access fixes and improvements
  4. View patch

[PATCH 12/15] serial: sc16is7xx: add missing space between macro args (checkpatch)

Hugo Villeneuve posted 15 patches 2 months, 3 weeks ago
There is a newer version of this series
[PATCH 12/15] serial: sc16is7xx: add missing space between macro args (checkpatch)
Posted by Hugo Villeneuve 2 months, 3 weeks ago 
From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Fix the following checkpatch error:

    ERROR: space required after that ',' (ctx:VxV)
    +#define to_sc16is7xx_one(p,e)...

Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 drivers/tty/serial/sc16is7xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index a05be92f7e776..9d04d665ec9ab 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -365,7 +365,7 @@ static struct uart_driver sc16is7xx_uart = {
 	.nr		= SC16IS7XX_MAX_DEVS,
 };
 
-#define to_sc16is7xx_one(p,e)	((container_of((p), struct sc16is7xx_one, e)))
+#define to_sc16is7xx_one(p, e)	((container_of((p), struct sc16is7xx_one, e)))
 
 static u8 sc16is7xx_port_read(struct uart_port *port, u8 reg)
 {
-- 
2.39.5
Re: [PATCH 12/15] serial: sc16is7xx: add missing space between macro args (checkpatch)
Posted by Jiri Slaby 2 months, 2 weeks ago 
On 24. 09. 25, 17:37, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Fix the following checkpatch error:
> 
>      ERROR: space required after that ',' (ctx:VxV)
>      +#define to_sc16is7xx_one(p,e)...
> 
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> ---
>   drivers/tty/serial/sc16is7xx.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index a05be92f7e776..9d04d665ec9ab 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -365,7 +365,7 @@ static struct uart_driver sc16is7xx_uart = {
>   	.nr		= SC16IS7XX_MAX_DEVS,
>   };
>   
> -#define to_sc16is7xx_one(p,e)	((container_of((p), struct sc16is7xx_one, e)))
> +#define to_sc16is7xx_one(p, e)	((container_of((p), struct sc16is7xx_one, e)))

Or perhaps make it type-safe and more obvious by switching it to an inline?

-- 
js
suse labs
Re: [PATCH 12/15] serial: sc16is7xx: add missing space between macro args (checkpatch)
Posted by Hugo Villeneuve 2 months, 2 weeks ago 
Hi Jiri,

On Mon, 29 Sep 2025 08:15:56 +0200
Jiri Slaby <jirislaby@kernel.org> wrote:

> On 24. 09. 25, 17:37, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > Fix the following checkpatch error:
> > 
> >      ERROR: space required after that ',' (ctx:VxV)
> >      +#define to_sc16is7xx_one(p,e)...
> > 
> > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > ---
> >   drivers/tty/serial/sc16is7xx.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> > index a05be92f7e776..9d04d665ec9ab 100644
> > --- a/drivers/tty/serial/sc16is7xx.c
> > +++ b/drivers/tty/serial/sc16is7xx.c
> > @@ -365,7 +365,7 @@ static struct uart_driver sc16is7xx_uart = {
> >   	.nr		= SC16IS7XX_MAX_DEVS,
> >   };
> >   
> > -#define to_sc16is7xx_one(p,e)	((container_of((p), struct sc16is7xx_one, e)))
> > +#define to_sc16is7xx_one(p, e)	((container_of((p), struct sc16is7xx_one, e)))
> 
> Or perhaps make it type-safe and more obvious by switching it to an inline?

Not easy to do, because this macro is also used with the second
argument "e" not always being used with the same member name. At the
same time, this is what makes this macro more complex than it should
be. I will convert it to an inline and simplify it by removing the
second argument (and of course adapt the code where the new, simpler,
inline function no longer works).

The end result improves readability a lot.

Thank you,
Hugo.

-- 
Hugo Villeneuve
Re: [PATCH 12/15] serial: sc16is7xx: add missing space between macro args (checkpatch)
Posted by Greg KH 2 months, 2 weeks ago 
On Tue, Sep 30, 2025 at 04:08:28PM -0400, Hugo Villeneuve wrote:
> Hi Jiri,
> 
> On Mon, 29 Sep 2025 08:15:56 +0200
> Jiri Slaby <jirislaby@kernel.org> wrote:
> 
> > On 24. 09. 25, 17:37, Hugo Villeneuve wrote:
> > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > 
> > > Fix the following checkpatch error:
> > > 
> > >      ERROR: space required after that ',' (ctx:VxV)
> > >      +#define to_sc16is7xx_one(p,e)...
> > > 
> > > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > ---
> > >   drivers/tty/serial/sc16is7xx.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> > > index a05be92f7e776..9d04d665ec9ab 100644
> > > --- a/drivers/tty/serial/sc16is7xx.c
> > > +++ b/drivers/tty/serial/sc16is7xx.c
> > > @@ -365,7 +365,7 @@ static struct uart_driver sc16is7xx_uart = {
> > >   	.nr		= SC16IS7XX_MAX_DEVS,
> > >   };
> > >   
> > > -#define to_sc16is7xx_one(p,e)	((container_of((p), struct sc16is7xx_one, e)))
> > > +#define to_sc16is7xx_one(p, e)	((container_of((p), struct sc16is7xx_one, e)))
> > 
> > Or perhaps make it type-safe and more obvious by switching it to an inline?
> 
> Not easy to do, because this macro is also used with the second
> argument "e" not always being used with the same member name. At the
> same time, this is what makes this macro more complex than it should
> be. I will convert it to an inline and simplify it by removing the
> second argument (and of course adapt the code where the new, simpler,
> inline function no longer works).

Please don't use an inline fuction for container_of() as you will just
need to change it later in the future when you really want to use
container_of_const() instead :)

thanks,

greg k-h
Re: [PATCH 12/15] serial: sc16is7xx: add missing space between macro args (checkpatch)
Posted by Hugo Villeneuve 2 months, 2 weeks ago 
Hi Greg,

On Wed, 1 Oct 2025 07:16:06 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Tue, Sep 30, 2025 at 04:08:28PM -0400, Hugo Villeneuve wrote:
> > Hi Jiri,
> > 
> > On Mon, 29 Sep 2025 08:15:56 +0200
> > Jiri Slaby <jirislaby@kernel.org> wrote:
> > 
> > > On 24. 09. 25, 17:37, Hugo Villeneuve wrote:
> > > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > > 
> > > > Fix the following checkpatch error:
> > > > 
> > > >      ERROR: space required after that ',' (ctx:VxV)
> > > >      +#define to_sc16is7xx_one(p,e)...
> > > > 
> > > > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > > ---
> > > >   drivers/tty/serial/sc16is7xx.c | 2 +-
> > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> > > > index a05be92f7e776..9d04d665ec9ab 100644
> > > > --- a/drivers/tty/serial/sc16is7xx.c
> > > > +++ b/drivers/tty/serial/sc16is7xx.c
> > > > @@ -365,7 +365,7 @@ static struct uart_driver sc16is7xx_uart = {
> > > >   	.nr		= SC16IS7XX_MAX_DEVS,
> > > >   };
> > > >   
> > > > -#define to_sc16is7xx_one(p,e)	((container_of((p), struct sc16is7xx_one, e)))
> > > > +#define to_sc16is7xx_one(p, e)	((container_of((p), struct sc16is7xx_one, e)))
> > > 
> > > Or perhaps make it type-safe and more obvious by switching it to an inline?
> > 
> > Not easy to do, because this macro is also used with the second
> > argument "e" not always being used with the same member name. At the
> > same time, this is what makes this macro more complex than it should
> > be. I will convert it to an inline and simplify it by removing the
> > second argument (and of course adapt the code where the new, simpler,
> > inline function no longer works).
> 
> Please don't use an inline fuction for container_of() as you will just
> need to change it later in the future when you really want to use
> container_of_const() instead :)

Noted. I will simplify the macro and leave it as a macro then.

Would you suggest to also convert container_of to
container_of_const in this patch series?

Hugo.
Re: [PATCH 12/15] serial: sc16is7xx: add missing space between macro args (checkpatch)
Posted by Greg KH 2 months, 2 weeks ago 
On Wed, Oct 01, 2025 at 09:29:20AM -0400, Hugo Villeneuve wrote:
> Hi Greg,
> 
> On Wed, 1 Oct 2025 07:16:06 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Tue, Sep 30, 2025 at 04:08:28PM -0400, Hugo Villeneuve wrote:
> > > Hi Jiri,
> > > 
> > > On Mon, 29 Sep 2025 08:15:56 +0200
> > > Jiri Slaby <jirislaby@kernel.org> wrote:
> > > 
> > > > On 24. 09. 25, 17:37, Hugo Villeneuve wrote:
> > > > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > > > 
> > > > > Fix the following checkpatch error:
> > > > > 
> > > > >      ERROR: space required after that ',' (ctx:VxV)
> > > > >      +#define to_sc16is7xx_one(p,e)...
> > > > > 
> > > > > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > > > ---
> > > > >   drivers/tty/serial/sc16is7xx.c | 2 +-
> > > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> > > > > index a05be92f7e776..9d04d665ec9ab 100644
> > > > > --- a/drivers/tty/serial/sc16is7xx.c
> > > > > +++ b/drivers/tty/serial/sc16is7xx.c
> > > > > @@ -365,7 +365,7 @@ static struct uart_driver sc16is7xx_uart = {
> > > > >   	.nr		= SC16IS7XX_MAX_DEVS,
> > > > >   };
> > > > >   
> > > > > -#define to_sc16is7xx_one(p,e)	((container_of((p), struct sc16is7xx_one, e)))
> > > > > +#define to_sc16is7xx_one(p, e)	((container_of((p), struct sc16is7xx_one, e)))
> > > > 
> > > > Or perhaps make it type-safe and more obvious by switching it to an inline?
> > > 
> > > Not easy to do, because this macro is also used with the second
> > > argument "e" not always being used with the same member name. At the
> > > same time, this is what makes this macro more complex than it should
> > > be. I will convert it to an inline and simplify it by removing the
> > > second argument (and of course adapt the code where the new, simpler,
> > > inline function no longer works).
> > 
> > Please don't use an inline fuction for container_of() as you will just
> > need to change it later in the future when you really want to use
> > container_of_const() instead :)
> 
> Noted. I will simplify the macro and leave it as a macro then.
> 
> Would you suggest to also convert container_of to
> container_of_const in this patch series?

For now, no, it's not needed as I do not think this structure is ever
marked as const, is it?

I have a long-term plan to move container_of() to be
container_of_const(), but that's a kernel-wide thing, and not relevant
here.  I was just trying to point out that when inlining container_of(),
it can actually cause problems.

thanks,

greg k-h

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.