From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752639AbbCPJJ1 (ORCPT ); Mon, 16 Mar 2015 05:09:27 -0400 Received: from aserp1040.oracle.com ([141.146.126.69]:23639 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751450AbbCPJJY (ORCPT ); Mon, 16 Mar 2015 05:09:24 -0400 Date: Mon, 16 Mar 2015 12:08:49 +0300 From: Dan Carpenter To: Isaac Lleida Cc: willy@meta-x.org, devel@driverdev.osuosl.org, isaky , armand.bastien@laposte.net, gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org, marius.gorski@gmail.com, domdevlin@free.fr, sudipm.mukherjee@gmail.com Subject: Re: [PATCH] staging: panel: change struct bits to a bit array Message-ID: <20150316090848.GH10964@mwanda> References: <1426264923-12392-1-git-send-email-isakyllr@openmailbox.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1426264923-12392-1-git-send-email-isakyllr@openmailbox.org> User-Agent: Mutt/1.5.21 (2010-09-15) X-Source-IP: userv0022.oracle.com [156.151.31.74] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Mar 13, 2015 at 05:42:03PM +0100, Isaac Lleida wrote: > From: isaky 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 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