Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753208AbcDVHST (ORCPT ); Fri, 22 Apr 2016 03:18:19 -0400 Received: from mx08-00178001.pphosted.com ([91.207.212.93]:47543 "EHLO mx07-00178001.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751707AbcDVHSR (ORCPT ); Fri, 22 Apr 2016 03:18:17 -0400 Subject: Re: [PATCH 7/8] gpio: stmpe: Add STMPE1600 support To: Linus Walleij References: <1461068317-28016-1-git-send-email-patrice.chotard@st.com> <1461068317-28016-8-git-send-email-patrice.chotard@st.com> CC: Lee Jones , Alexandre Courbot , "linux-gpio@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , Maxime Coquelin , , Shawn Guo , Sascha Hauer , Dinh Nguyen , Viresh Kumar , Shiraz Hashim , Stephen Warren , Thierry Reding From: Patrice Chotard Message-ID: <5719D00F.3020202@st.com> Date: Fri, 22 Apr 2016 09:17:35 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.48.1.66] X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2016-04-22_05:,, signatures=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3087 Lines: 87 On 04/20/2016 04:53 PM, Linus Walleij wrote: > On Tue, Apr 19, 2016 at 2:18 PM, wrote: > >> From: Patrice Chotard >> >> The particularities of this variant are: >> - GPIO_XXX_LSB and GPIO_XXX_MSB memory locations are inverted compared >> to other variants. >> - There is no Edge detection, Rising Edge and Falling Edge registers. >> - IRQ flags are cleared when read, no need to write in Status register. >> >> Signed-off-by: Amelie DELAUNAY >> Signed-off-by: Patrice Chotard >> - u8 reg = stmpe->regs[STMPE_IDX_GPMR_LSB] - (offset / 8); >> + u8 reg; >> u8 mask = 1 << (offset % 8); >> int ret; >> >> + if (stmpe->partnum == STMPE1600) >> + reg = stmpe->regs[STMPE_IDX_GPMR_LSB] + (offset / 8); >> + else >> + reg = stmpe->regs[STMPE_IDX_GPMR_LSB] - (offset / 8); > This construct is a bit hard to grasp. > > Can we think of something more intuitive? Maybe using more > code lines but easier to understand. > > Subtracting the offset is just totally unintuitive in the first place, > the STMPE1600 arrangement is much more intuitive. > > I would prefer if we address the LSB+MSB register explicitly > instead of adding or subtracting 1 to the LSB register to get > to the MSB register. > >> + if (stmpe->partnum == STMPE1600) >> + reg = stmpe->regs[which] + (offset / 8); >> + else >> + reg = stmpe->regs[which] - (offset / 8); > Same. > >> + if (stmpe->partnum == STMPE1600) >> + reg = stmpe->regs[STMPE_IDX_GPDR_LSB] + (offset / 8); >> + else >> + reg = stmpe->regs[STMPE_IDX_GPDR_LSB] - (offset / 8); > Same. > >> + if (stmpe->partnum == STMPE1600) >> + reg = stmpe->regs[STMPE_IDX_GPDR_LSB] + (offset / 8); >> + else >> + reg = stmpe->regs[STMPE_IDX_GPDR_LSB] - (offset / 8); > Same. > >> + stmpe_reg_write(stmpe, >> + stmpe->regs[regmap[i]] + j, >> + new); >> + else >> + stmpe_reg_write(stmpe, >> + stmpe->regs[regmap[i]] - j, >> + new); > This is also unintuitively backwards. > >> + if (stmpe->partnum == STMPE1600) >> + dir_reg = stmpe->regs[STMPE_IDX_GPDR_LSB] + (offset / 8); >> + else >> + dir_reg = stmpe->regs[STMPE_IDX_GPDR_LSB] - (offset / 8); > Same. > >> + if (stmpe->partnum == STMPE1600) >> + statmsbreg = stmpe->regs[STMPE_IDX_ISGPIOR_LSB]; >> + else >> + statmsbreg = stmpe->regs[STMPE_IDX_ISGPIOR_MSB]; > And this kind of points at the problem. > > Can we write this in some way that make it super-clear which register > we're using and why? Ok i will rework all these points Thanks Patrice > > Yours, > Linus Walleij