| Message ID | 20240110102102.61587-18-tudor.ambarus@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | serial: samsung: gs101 updates and winter cleanup | expand |
On 1/16/24 19:03, Sam Protsenko wrote: > On Wed, Jan 10, 2024 at 4:25 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: >> >> There's a single flag defined as of now. Shrink the feature flags to u8 >> and aim for a better memory footprint for ``struct s3c24xx_uart_info``. >> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> >> --- >> drivers/tty/serial/samsung_tty.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c >> index 5df2bcebf9fb..598d9fe7a492 100644 >> --- a/drivers/tty/serial/samsung_tty.c >> +++ b/drivers/tty/serial/samsung_tty.c >> @@ -90,7 +90,7 @@ struct s3c24xx_uart_info { >> >> /* uart port features */ >> >> - unsigned int has_divslot:1; >> + u8 has_divslot:1; > > But that's already a bit field. Why does it matter which type it is? > If using unsigned int the bitfied is combined with the previous u8 fields, whereas if using u8 the bitfield will be independently defined. So no benefit in terms of memory footprint, it's just a cosmetic change to align the bitfield with the previous u8 fields. Allowing u32 for just a bit can be misleading as one would ask itself where are the other bits. Between a u32 bitfield and a bool a u8 bitfield seems like a good compromise. I'll update the commit message with this explanation in v2 because I'd keep the patch, it makes the struct look cleaner. At the same time I don't have a strong opinion, so if you'd like to see the patch dropped, tell there, I'll be fine with it.
On 19. 01. 24, 9:56, Tudor Ambarus wrote: > > > On 1/16/24 19:03, Sam Protsenko wrote: >> On Wed, Jan 10, 2024 at 4:25 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: >>> >>> There's a single flag defined as of now. Shrink the feature flags to u8 >>> and aim for a better memory footprint for ``struct s3c24xx_uart_info``. >>> >>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> >>> --- >>> drivers/tty/serial/samsung_tty.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c >>> index 5df2bcebf9fb..598d9fe7a492 100644 >>> --- a/drivers/tty/serial/samsung_tty.c >>> +++ b/drivers/tty/serial/samsung_tty.c >>> @@ -90,7 +90,7 @@ struct s3c24xx_uart_info { >>> >>> /* uart port features */ >>> >>> - unsigned int has_divslot:1; >>> + u8 has_divslot:1; >> >> But that's already a bit field. Why does it matter which type it is? >> > > If using unsigned int the bitfied is combined with the previous u8 > fields, whereas if using u8 the bitfield will be independently defined. > So no benefit in terms of memory footprint, it's just a cosmetic change > to align the bitfield with the previous u8 fields. Allowing u32 for just > a bit can be misleading as one would ask itself where are the other > bits. Between a u32 bitfield and a bool a u8 bitfield seems like a good > compromise. Why? What's wrong with bool? bitfields have terrible semantics wrt atomic writes for example.
On 1/19/24 09:07, Jiri Slaby wrote: Hi, Jiri! > On 19. 01. 24, 9:56, Tudor Ambarus wrote: >> >> >> On 1/16/24 19:03, Sam Protsenko wrote: >>> On Wed, Jan 10, 2024 at 4:25 AM Tudor Ambarus >>> <tudor.ambarus@linaro.org> wrote: >>>> >>>> There's a single flag defined as of now. Shrink the feature flags to u8 >>>> and aim for a better memory footprint for ``struct s3c24xx_uart_info``. >>>> >>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> >>>> --- >>>> drivers/tty/serial/samsung_tty.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/tty/serial/samsung_tty.c >>>> b/drivers/tty/serial/samsung_tty.c >>>> index 5df2bcebf9fb..598d9fe7a492 100644 >>>> --- a/drivers/tty/serial/samsung_tty.c >>>> +++ b/drivers/tty/serial/samsung_tty.c >>>> @@ -90,7 +90,7 @@ struct s3c24xx_uart_info { >>>> >>>> /* uart port features */ >>>> >>>> - unsigned int has_divslot:1; >>>> + u8 has_divslot:1; >>> >>> But that's already a bit field. Why does it matter which type it is? >>> >> >> If using unsigned int the bitfied is combined with the previous u8 >> fields, whereas if using u8 the bitfield will be independently defined. >> So no benefit in terms of memory footprint, it's just a cosmetic change >> to align the bitfield with the previous u8 fields. Allowing u32 for just >> a bit can be misleading as one would ask itself where are the other >> bits. Between a u32 bitfield and a bool a u8 bitfield seems like a good >> compromise. > > Why? What's wrong with bool? bitfields have terrible semantics wrt > atomic writes for example. > Bool occupies a byte and if more port features will ever be added we'll occupy more bytes. Here's how the structure will look like with a bool: struct s3c24xx_uart_info { const char * name; /* 0 8 */ enum s3c24xx_port_type type; /* 8 4 */ unsigned int port_type; /* 12 4 */ unsigned int fifosize; /* 16 4 */ u32 rx_fifomask; /* 20 4 */ u32 rx_fifoshift; /* 24 4 */ u32 rx_fifofull; /* 28 4 */ u32 tx_fifomask; /* 32 4 */ u32 tx_fifoshift; /* 36 4 */ u32 tx_fifofull; /* 40 4 */ u32 clksel_mask; /* 44 4 */ u32 clksel_shift; /* 48 4 */ u32 ucon_mask; /* 52 4 */ u8 def_clk_sel; /* 56 1 */ u8 num_clks; /* 57 1 */ u8 iotype; /* 58 1 */ bool has_divslot; /* 59 1 */ /* size: 64, cachelines: 1, members: 17 */ /* padding: 4 */ }; What's your preference? Thanks, ta
Hi, On 19. 01. 24, 10:43, Tudor Ambarus wrote: >>> If using unsigned int the bitfied is combined with the previous u8 >>> fields, whereas if using u8 the bitfield will be independently defined. >>> So no benefit in terms of memory footprint, it's just a cosmetic change >>> to align the bitfield with the previous u8 fields. Allowing u32 for just >>> a bit can be misleading as one would ask itself where are the other >>> bits. Between a u32 bitfield and a bool a u8 bitfield seems like a good >>> compromise. >> >> Why? What's wrong with bool? bitfields have terrible semantics wrt >> atomic writes for example. >> > > Bool occupies a byte and if more port features will ever be added we'll > occupy more bytes. Here's how the structure will look like with a bool: > > struct s3c24xx_uart_info { > const char * name; /* 0 8 */ > enum s3c24xx_port_type type; /* 8 4 */ > unsigned int port_type; /* 12 4 */ > unsigned int fifosize; /* 16 4 */ > u32 rx_fifomask; /* 20 4 */ > u32 rx_fifoshift; /* 24 4 */ > u32 rx_fifofull; /* 28 4 */ > u32 tx_fifomask; /* 32 4 */ > u32 tx_fifoshift; /* 36 4 */ > u32 tx_fifofull; /* 40 4 */ > u32 clksel_mask; /* 44 4 */ > u32 clksel_shift; /* 48 4 */ > u32 ucon_mask; /* 52 4 */ > u8 def_clk_sel; /* 56 1 */ > u8 num_clks; /* 57 1 */ > u8 iotype; /* 58 1 */ > bool has_divslot; /* 59 1 */ > > /* size: 64, cachelines: 1, members: 17 */ > /* padding: 4 */ > }; > > What's your preference? bool :).
On 1/19/24 09:54, Jiri Slaby wrote: > Hi, > > On 19. 01. 24, 10:43, Tudor Ambarus wrote: >>>> If using unsigned int the bitfied is combined with the previous u8 >>>> fields, whereas if using u8 the bitfield will be independently defined. >>>> So no benefit in terms of memory footprint, it's just a cosmetic change >>>> to align the bitfield with the previous u8 fields. Allowing u32 for >>>> just >>>> a bit can be misleading as one would ask itself where are the other >>>> bits. Between a u32 bitfield and a bool a u8 bitfield seems like a good >>>> compromise. >>> >>> Why? What's wrong with bool? bitfields have terrible semantics wrt >>> atomic writes for example. >>> >> >> Bool occupies a byte and if more port features will ever be added we'll >> occupy more bytes. Here's how the structure will look like with a bool: >> >> struct s3c24xx_uart_info { >> const char * name; /* 0 8 */ >> enum s3c24xx_port_type type; /* 8 4 */ >> unsigned int port_type; /* 12 4 */ >> unsigned int fifosize; /* 16 4 */ >> u32 rx_fifomask; /* 20 4 */ >> u32 rx_fifoshift; /* 24 4 */ >> u32 rx_fifofull; /* 28 4 */ >> u32 tx_fifomask; /* 32 4 */ >> u32 tx_fifoshift; /* 36 4 */ >> u32 tx_fifofull; /* 40 4 */ >> u32 clksel_mask; /* 44 4 */ >> u32 clksel_shift; /* 48 4 */ >> u32 ucon_mask; /* 52 4 */ >> u8 def_clk_sel; /* 56 1 */ >> u8 num_clks; /* 57 1 */ >> u8 iotype; /* 58 1 */ >> bool has_divslot; /* 59 1 */ >> >> /* size: 64, cachelines: 1, members: 17 */ >> /* padding: 4 */ >> }; >> >> What's your preference? > > bool :). > I'm fine with a bool too as since the introduction of this driver we have just this flag, it's unlikey to have 4 more soon to bypass the first cacheline. Will change to bool. Cheers, ta
diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c index 5df2bcebf9fb..598d9fe7a492 100644 --- a/drivers/tty/serial/samsung_tty.c +++ b/drivers/tty/serial/samsung_tty.c @@ -90,7 +90,7 @@ struct s3c24xx_uart_info { /* uart port features */ - unsigned int has_divslot:1; + u8 has_divslot:1; }; struct s3c24xx_serial_drv_data {
There's a single flag defined as of now. Shrink the feature flags to u8 and aim for a better memory footprint for ``struct s3c24xx_uart_info``. Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> --- drivers/tty/serial/samsung_tty.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)