| Message ID | 20240221183442.4124354-3-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | serial: Add a helper to parse device properties and more | expand |
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.
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.
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,
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?
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,
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.
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 --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 */
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(+)