Re: [PATCH] auxdisplay: panel: replace struct with union for display configuration

[Willy TARREAU <wtarreau@haproxy.com>]

Hello!

On Wed, Dec 25, 2024 at 11:11:18PM +0530, Atharva Tiwari wrote:
> this patch replaces a struct with a union in the panel.c driver 
> to better represent display configuration as mentioned in TODO
> 
> Signed-off-by: Atharva Tiwari <evepolonium@gmail.com>
> ---
>  drivers/auxdisplay/panel.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/auxdisplay/panel.c b/drivers/auxdisplay/panel.c
> index a731f28455b4..4662f763dac7 100644
> --- a/drivers/auxdisplay/panel.c
> +++ b/drivers/auxdisplay/panel.c
> @@ -204,8 +204,7 @@ static struct {
>  	int charset;
>  	int proto;
>  
> -	/* TODO: use union here? */
> -	struct {
> +	union {
>  		int e;
>  		int rs;
>  		int rw;

Have you tested this patch ? I guess not. The TODO here is not just to
change a language keyword but to see if it would be better achieved
using a different construct and representation of the different signals.
Here what you've done is merge all the signals into a single one.

I think that a better patch would be to just remove the TODO comment
that has been there for about 20 years without making any progress, and
which is no longer relevant since that code will not change now.

regards,
Willy