From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail.perches.com ([173.55.12.10]:1286 "EHLO mail.perches.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754989Ab0KOQaV (ORCPT ); Mon, 15 Nov 2010 11:30:21 -0500 Subject: RE: [PATCH 01/10] MCDE: Add hardware abstraction layer From: Joe Perches To: Jimmy RUBIN Cc: "linux-fbdev@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-media@vger.kernel.org" , Dan JOHANSSON , Linus WALLEIJ In-Reply-To: References: <1289390653-6111-1-git-send-email-jimmy.rubin@stericsson.com> <1289390653-6111-2-git-send-email-jimmy.rubin@stericsson.com> <1289409276.15905.65.camel@Joe-Laptop> Content-Type: text/plain; charset="UTF-8" Date: Mon, 15 Nov 2010 08:30:18 -0800 Message-ID: <1289838618.16461.132.camel@Joe-Laptop> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit List-ID: Sender: On Mon, 2010-11-15 at 10:52 +0100, Jimmy RUBIN wrote: > > Just trivia: [] > > It'd be nice to change to continuous_running > Continous_running [...] It was just a spelling comment. continous continuous > > > > +int mcde_dsi_dcs_write(struct mcde_chnl_state *chnl, u8 cmd, u8* > > data, int len) > > > +{ > > > + int i; > > > + u32 wrdat[4] = { 0, 0, 0, 0 }; > > > + u32 settings; > > > + u8 link = chnl->port.link; > > > + u8 virt_id = chnl->port.phy.dsi.virt_id; > > > + > > > + /* REVIEW: One command at a time */ > > > + /* REVIEW: Allow read/write on unreserved ports */ > > > + if (len > MCDE_MAX_DCS_WRITE || chnl->port.type != > > MCDE_PORTTYPE_DSI) > > > + return -EINVAL; > > > + > > > + wrdat[0] = cmd; > > > + for (i = 1; i <= len; i++) > > > + wrdat[i>>2] |= ((u32)data[i-1] << ((i & 3) * 8)); > > > > Ever overrun wrdat? > > Maybe WARN_ON(len > 16, "oops?") > > > MCDE_MAX_DCS_WRITE is 15 so it will be an early return in that case. Perhaps it'd be better to use DECLARE_BITMAP(wrdat, MCDE_MAX_DCS_WRITE); or some other mechanism to link the array size to the #define. > /Jimmy From mboxrd@z Thu Jan 1 00:00:00 1970 From: Joe Perches Date: Mon, 15 Nov 2010 16:30:18 +0000 Subject: RE: [PATCH 01/10] MCDE: Add hardware abstraction layer Message-Id: <1289838618.16461.132.camel@Joe-Laptop> List-Id: References: <1289390653-6111-1-git-send-email-jimmy.rubin@stericsson.com> <1289390653-6111-2-git-send-email-jimmy.rubin@stericsson.com> <1289409276.15905.65.camel@Joe-Laptop> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: linux-arm-kernel@lists.infradead.org On Mon, 2010-11-15 at 10:52 +0100, Jimmy RUBIN wrote: > > Just trivia: [] > > It'd be nice to change to continuous_running > Continous_running [...] It was just a spelling comment. continous continuous > > > > +int mcde_dsi_dcs_write(struct mcde_chnl_state *chnl, u8 cmd, u8* > > data, int len) > > > +{ > > > + int i; > > > + u32 wrdat[4] = { 0, 0, 0, 0 }; > > > + u32 settings; > > > + u8 link = chnl->port.link; > > > + u8 virt_id = chnl->port.phy.dsi.virt_id; > > > + > > > + /* REVIEW: One command at a time */ > > > + /* REVIEW: Allow read/write on unreserved ports */ > > > + if (len > MCDE_MAX_DCS_WRITE || chnl->port.type !> > MCDE_PORTTYPE_DSI) > > > + return -EINVAL; > > > + > > > + wrdat[0] = cmd; > > > + for (i = 1; i <= len; i++) > > > + wrdat[i>>2] |= ((u32)data[i-1] << ((i & 3) * 8)); > > > > Ever overrun wrdat? > > Maybe WARN_ON(len > 16, "oops?") > > > MCDE_MAX_DCS_WRITE is 15 so it will be an early return in that case. Perhaps it'd be better to use DECLARE_BITMAP(wrdat, MCDE_MAX_DCS_WRITE); or some other mechanism to link the array size to the #define. > /Jimmy From mboxrd@z Thu Jan 1 00:00:00 1970 From: joe@perches.com (Joe Perches) Date: Mon, 15 Nov 2010 08:30:18 -0800 Subject: [PATCH 01/10] MCDE: Add hardware abstraction layer In-Reply-To: References: <1289390653-6111-1-git-send-email-jimmy.rubin@stericsson.com> <1289390653-6111-2-git-send-email-jimmy.rubin@stericsson.com> <1289409276.15905.65.camel@Joe-Laptop> Message-ID: <1289838618.16461.132.camel@Joe-Laptop> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Mon, 2010-11-15 at 10:52 +0100, Jimmy RUBIN wrote: > > Just trivia: [] > > It'd be nice to change to continuous_running > Continous_running [...] It was just a spelling comment. continous continuous > > > > +int mcde_dsi_dcs_write(struct mcde_chnl_state *chnl, u8 cmd, u8* > > data, int len) > > > +{ > > > + int i; > > > + u32 wrdat[4] = { 0, 0, 0, 0 }; > > > + u32 settings; > > > + u8 link = chnl->port.link; > > > + u8 virt_id = chnl->port.phy.dsi.virt_id; > > > + > > > + /* REVIEW: One command at a time */ > > > + /* REVIEW: Allow read/write on unreserved ports */ > > > + if (len > MCDE_MAX_DCS_WRITE || chnl->port.type != > > MCDE_PORTTYPE_DSI) > > > + return -EINVAL; > > > + > > > + wrdat[0] = cmd; > > > + for (i = 1; i <= len; i++) > > > + wrdat[i>>2] |= ((u32)data[i-1] << ((i & 3) * 8)); > > > > Ever overrun wrdat? > > Maybe WARN_ON(len > 16, "oops?") > > > MCDE_MAX_DCS_WRITE is 15 so it will be an early return in that case. Perhaps it'd be better to use DECLARE_BITMAP(wrdat, MCDE_MAX_DCS_WRITE); or some other mechanism to link the array size to the #define. > /Jimmy