Received-SPF: pass (google.com: domain of linux-kernel+bounces-30927-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <76e1dc42-cabe-4925-8aa1-c8f733fb36a2@linaro.org>
Date: Fri, 19 Jan 2024 08:56:02 +0000
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 17/18] tty: serial: samsung: shrink port feature flags to
 u8
Content-Language: en-US
To: Sam Protsenko <semen.protsenko@linaro.org>
Cc: krzysztof.kozlowski@linaro.org, alim.akhtar@samsung.com,
 gregkh@linuxfoundation.org, jirislaby@kernel.org,
 linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org,
 linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org,
 andre.draszik@linaro.org, peter.griffin@linaro.org, kernel-team@android.com,
 willmcvicker@google.com
References: <20240110102102.61587-1-tudor.ambarus@linaro.org>
 <20240110102102.61587-18-tudor.ambarus@linaro.org>
 <CAPLW+4k091328krLB_KdHyobG-pR--Rt5WaN6c1ccpgdV8ry7Q@mail.gmail.com>
From: Tudor Ambarus <tudor.ambarus@linaro.org>
In-Reply-To: <CAPLW+4k091328krLB_KdHyobG-pR--Rt5WaN6c1ccpgdV8ry7Q@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit


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.