diff mbox series

[v1,02/14] serial: core: Add UPIO_UNSET constant for unset port type

Message ID 20240221183442.4124354-3-andriy.shevchenko@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series serial: Add a helper to parse device properties and more | expand

Commit Message

Andy Shevchenko Feb. 21, 2024, 6:31 p.m. UTC
In some APIs we would like to assign the special value to iotype
and compare against it in another places. Introduce UPIO_UNSET
for this purpose.

Note, we can't use 0, because it's a valid value for IO port access.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/serial_core.h | 1 +
 1 file changed, 1 insertion(+)

Comments

Florian Fainelli Feb. 21, 2024, 6:47 p.m. UTC | #1
On 2/21/24 10:31, Andy Shevchenko wrote:
> In some APIs we would like to assign the special value to iotype
> and compare against it in another places. Introduce UPIO_UNSET
> for this purpose.
> 
> Note, we can't use 0, because it's a valid value for IO port access.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   include/linux/serial_core.h | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 2d2ec99eca93..2b0526ae1fac 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -470,6 +470,7 @@ struct uart_port {
>   
>   	unsigned char		iotype;			/* io access style */
>   
> +#define UPIO_UNSET		((unsigned char)~0U)	/* UCHAR_MAX */

Nit: I would name this UPIO_UNKNOWN, or UPIO_NOTSET, unset means to me 
that it was previously set and we undid that action, whereas unknown or 
not set means we never did.
Andy Shevchenko Feb. 21, 2024, 6:53 p.m. UTC | #2
On Wed, Feb 21, 2024 at 10:47:13AM -0800, Florian Fainelli wrote:
> On 2/21/24 10:31, Andy Shevchenko wrote:

...

> >   	unsigned char		iotype;			/* io access style */
> > +#define UPIO_UNSET		((unsigned char)~0U)	/* UCHAR_MAX */
> 
> Nit: I would name this UPIO_UNKNOWN, or UPIO_NOTSET, unset means to me that
> it was previously set and we undid that action, whereas unknown or not set
> means we never did.

Works for me. I will wait for a few days / week to have more reviews and
likely testings to be collected. Would be nice if you be able to test on
(some of) the hardware in the list of modified drivers.
Jiri Slaby Feb. 22, 2024, 6:58 a.m. UTC | #3
On 21. 02. 24, 19:31, Andy Shevchenko wrote:
> In some APIs we would like to assign the special value to iotype
> and compare against it in another places. Introduce UPIO_UNSET
> for this purpose.
> 
> Note, we can't use 0, because it's a valid value for IO port access.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   include/linux/serial_core.h | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 2d2ec99eca93..2b0526ae1fac 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -470,6 +470,7 @@ struct uart_port {
>   
>   	unsigned char		iotype;			/* io access style */
>   
> +#define UPIO_UNSET		((unsigned char)~0U)	/* UCHAR_MAX */

Perhaps making the var u8 and this U8_MAX then? It would make more sense 
to me.

>   #define UPIO_PORT		(SERIAL_IO_PORT)	/* 8b I/O port access */
>   #define UPIO_HUB6		(SERIAL_IO_HUB6)	/* Hub6 ISA card */
>   #define UPIO_MEM		(SERIAL_IO_MEM)		/* driver-specific */

thanks,
Andy Shevchenko Feb. 22, 2024, 1:21 p.m. UTC | #4
On Thu, Feb 22, 2024 at 07:58:32AM +0100, Jiri Slaby wrote:
> On 21. 02. 24, 19:31, Andy Shevchenko wrote:

...

> >   	unsigned char		iotype;			/* io access style */
> > +#define UPIO_UNSET		((unsigned char)~0U)	/* UCHAR_MAX */
> 
> Perhaps making the var u8 and this U8_MAX then? It would make more sense to
> me.

WFM, should it be a separate change? Btw, how can I justify it?
Jiri Slaby Feb. 23, 2024, 5:42 a.m. UTC | #5
On 22. 02. 24, 14:21, Andy Shevchenko wrote:
> On Thu, Feb 22, 2024 at 07:58:32AM +0100, Jiri Slaby wrote:
>> On 21. 02. 24, 19:31, Andy Shevchenko wrote:
> 
> ...
> 
>>>    	unsigned char		iotype;			/* io access style */
>>> +#define UPIO_UNSET		((unsigned char)~0U)	/* UCHAR_MAX */
>>
>> Perhaps making the var u8 and this U8_MAX then? It would make more sense to
>> me.
> 
> WFM, should it be a separate change?

Likely.

> Btw, how can I justify it?

Hmm, thinking about it, why is it not an enum?

But it could be also an u8 because you want it be exactly 8 bits as you 
want to be sure values up to 255 fit.

thanks,
Andy Shevchenko Feb. 23, 2024, 2:59 p.m. UTC | #6
On Fri, Feb 23, 2024 at 06:42:15AM +0100, Jiri Slaby wrote:
> On 22. 02. 24, 14:21, Andy Shevchenko wrote:
> > On Thu, Feb 22, 2024 at 07:58:32AM +0100, Jiri Slaby wrote:
> > > On 21. 02. 24, 19:31, Andy Shevchenko wrote:

...

> > > >    	unsigned char		iotype;			/* io access style */
> > > > +#define UPIO_UNSET		((unsigned char)~0U)	/* UCHAR_MAX */
> > > 
> > > Perhaps making the var u8 and this U8_MAX then? It would make more sense to
> > > me.
> > 
> > WFM, should it be a separate change?
> 
> Likely.

Then I need a commit message, because I'm unable to justify this change myself.

> > Btw, how can I justify it?
> 
> Hmm, thinking about it, why is it not an enum?

Maybe, but it is a replica of UAPI definitions, do we want to see it as a enum?
To me it will be a bit ugly looking.

> But it could be also an u8 because you want it be exactly 8 bits as you want
> to be sure values up to 255 fit.

Depends on what we assume UAPI does with those flags. It maybe even less than
8 bits, or great than, currently 8 bits is enough...

TL;DR: I would rather take a patch from you and incorporate into the series
than trying hard to invent a justification and proper type.
Andy Shevchenko Feb. 26, 2024, 2:17 p.m. UTC | #7
On Fri, Feb 23, 2024 at 04:59:25PM +0200, Andy Shevchenko wrote:
> On Fri, Feb 23, 2024 at 06:42:15AM +0100, Jiri Slaby wrote:
> > On 22. 02. 24, 14:21, Andy Shevchenko wrote:
> > > On Thu, Feb 22, 2024 at 07:58:32AM +0100, Jiri Slaby wrote:
> > > > On 21. 02. 24, 19:31, Andy Shevchenko wrote:

...

> > > > >    	unsigned char		iotype;			/* io access style */
> > > > > +#define UPIO_UNSET		((unsigned char)~0U)	/* UCHAR_MAX */
> > > > 
> > > > Perhaps making the var u8 and this U8_MAX then? It would make more sense to
> > > > me.
> > > 
> > > WFM, should it be a separate change?
> > 
> > Likely.
> 
> Then I need a commit message, because I'm unable to justify this change myself.
> 
> > > Btw, how can I justify it?
> > 
> > Hmm, thinking about it, why is it not an enum?
> 
> Maybe, but it is a replica of UAPI definitions, do we want to see it as a enum?
> To me it will be a bit ugly looking.
> 
> > But it could be also an u8 because you want it be exactly 8 bits as you want
> > to be sure values up to 255 fit.
> 
> Depends on what we assume UAPI does with those flags. It maybe even less than
> 8 bits, or great than, currently 8 bits is enough...
> 
> TL;DR: I would rather take a patch from you and incorporate into the series
> than trying hard to invent a justification and proper type.

Okay, I want to send a new version, for now I leave the type change for
the next time. It looks that quirks as well will benefit from type clarifying.
diff mbox series

Patch

diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 2d2ec99eca93..2b0526ae1fac 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -470,6 +470,7 @@  struct uart_port {
 
 	unsigned char		iotype;			/* io access style */
 
+#define UPIO_UNSET		((unsigned char)~0U)	/* UCHAR_MAX */
 #define UPIO_PORT		(SERIAL_IO_PORT)	/* 8b I/O port access */
 #define UPIO_HUB6		(SERIAL_IO_HUB6)	/* Hub6 ISA card */
 #define UPIO_MEM		(SERIAL_IO_MEM)		/* driver-specific */