On Fri, Mar 13, 2015 at 05:42:03PM +0100, Isaac Lleida wrote:
> From: isaky <illeida@xxxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxx>
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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
