Re: [PATCH] staging: panel: change struct bits to a bit array

[Dan Carpenter <dan.carpenter@oracle.com>]

On Fri, Mar 13, 2015 at 05:42:03PM +0100, Isaac Lleida wrote:
> From: isaky <illeida@openaliasbox.org>

Not your name.  Leave this out unless you are passing on someone else's
patch.

> 
> This path implements a bit array representing the LCD signal states instead of the old "struct bits", which used char to represent a single bit. This will reduce the memory usage.
> 

Line wraps the changelog.

> Signed-off-by: Isaac Lleida <illeida@openaliasbox.org>

This should match the email address you used to send the patch.


> ---
>  drivers/staging/panel/panel.c | 86 ++++++++++++++++++++++++-------------------
>  1 file changed, 49 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
> index 3339633..7deb092 100644
> --- a/drivers/staging/panel/panel.c
> +++ b/drivers/staging/panel/panel.c
> @@ -310,6 +310,25 @@ static int selected_lcd_type = NOT_SET;
>  #define LCD_BITS	6
>  
>  /*
> + * LCD signal states
> + */
> +#define LCD_BIT_E_MASK		0x1	/* E (data latch on falling edge) */
> +#define LCD_BIT_RS_MASK		0x2	/* RS  (0 = cmd, 1 = data) */
> +#define LCD_BIT_RW_MASK		0x4	/* R/W (0 = W, 1 = R) */
> +#define LCD_BIT_BL_MASK		0x8	/* backlight (0 = off, 1 = on) */
> +#define LCD_BIT_CL_MASK		0x10	/* clock (latch on rising edge) */
> +#define LCD_BIT_DA_MASK		0x20	/* data */
> +
> +/*
> + * Bit array operations
> + */
> +#define BIT_ON(b, m)	(b |= m)
> +#define BIT_OFF(b, m)	(b &= ~m)
> +#define BIT_CHK(b, m)	(b & m)
> +#define BIT_MOD(b, m, v)			\
> +	((v) ? BIT_ON(b, m) : BIT_OFF(b, m))	\
> +


Ugh.  No.  These are yuck macros.  Just do it directly.  Also the
BIT_MOD() has end in a '\' char.

> +/*
>   * each bit can be either connected to a DATA or CTRL port
>   */
>  #define LCD_PORT_C	0
> @@ -653,15 +672,8 @@ static const char nexcom_keypad_profile[][4][9] = {
>  
>  static const char (*keypad_profile)[4][9] = old_keypad_profile;
>  
> -/* FIXME: this should be converted to a bit array containing signals states */
> -static struct {
> -	unsigned char e;  /* parallel LCD E (data latch on falling edge) */
> -	unsigned char rs; /* parallel LCD RS  (0 = cmd, 1 = data) */
> -	unsigned char rw; /* parallel LCD R/W (0 = W, 1 = R) */
> -	unsigned char bl; /* parallel LCD backlight (0 = off, 1 = on) */
> -	unsigned char cl; /* serial LCD clock (latch on rising edge) */
> -	unsigned char da; /* serial LCD data */
> -} bits;
> +/* Bit array containing signals states */
> +static char bits;

I don't think this is what the comment means.  I think it means that
we do something like:

static struct {
	unsigned long e:1;
	unsigned long rs:1;
	...
} bits;

1) This space saving are very small.  On 64 bit systems then we don't
save any space at all because everything is aligned at 8 bytes.
2) The name "e" is not self documenting.  None of them are.

Christophe had a bunch more comments on this patch so I'm not going to
read any further.

regards,
dan carpenter