From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Petazzoni Subject: Re: [PATCH 06/17] gpio: mvebu: add suspend/resume support Date: Fri, 31 Oct 2014 09:14:35 +0100 Message-ID: <20141031091435.2149346d@free-electrons.com> References: <1414151970-6626-1-git-send-email-thomas.petazzoni@free-electrons.com> <1414151970-6626-7-git-send-email-thomas.petazzoni@free-electrons.com> <54533FAF.7010505@free-electrons.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Return-path: Received: from down.free-electrons.com ([37.187.137.238]:40547 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750912AbaJaIOk (ORCPT ); Fri, 31 Oct 2014 04:14:40 -0400 In-Reply-To: <54533FAF.7010505@free-electrons.com> Sender: linux-gpio-owner@vger.kernel.org List-Id: linux-gpio@vger.kernel.org To: Gregory CLEMENT Cc: Linus Walleij , Jason Cooper , Andrew Lunn , Sebastian Hesselbarth , "linux-arm-kernel@lists.infradead.org" , Tawfik Bayouk , Nadav Haklai , Lior Amsalem , Ezequiel Garcia , "devicetree@vger.kernel.org" , Alexandre Courbot , "linux-gpio@vger.kernel.org" Hello, On Fri, 31 Oct 2014 08:52:15 +0100, Gregory CLEMENT wrote: > >> + switch (mvchip->soc_variant) { > >> + case MVEBU_GPIO_SOC_VARIANT_ORION: > >> + mvchip->edge_mask_regs[0] = > >> + readl(mvchip->membase + GPIO_EDGE_MASK_OFF); > >> + mvchip->level_mask_regs[0] = > >> + readl(mvchip->membase + GPIO_LEVEL_MASK_OFF); > >> + break; > > > > You are assigning index [0] twice, why? There must be some > > reason, and it should be stated in a comment. If the first read > > is necessary for hardware reasons, don't assign it but > > discard the result. > > Maybe I missed something but for me these 2 registers are different: > one is the _EDGE_ mask and the other the _LEVEL_ mask. Good that you looked at it Greg because I must admit I did not understand the comment from Linus :-) I read things again and both the variable *and* the register offset are different. I guess it's probably due to a -ENOTENOUGHCOFFEE :-) Thanks! Thomas -- Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com From mboxrd@z Thu Jan 1 00:00:00 1970 From: thomas.petazzoni@free-electrons.com (Thomas Petazzoni) Date: Fri, 31 Oct 2014 09:14:35 +0100 Subject: [PATCH 06/17] gpio: mvebu: add suspend/resume support In-Reply-To: <54533FAF.7010505@free-electrons.com> References: <1414151970-6626-1-git-send-email-thomas.petazzoni@free-electrons.com> <1414151970-6626-7-git-send-email-thomas.petazzoni@free-electrons.com> <54533FAF.7010505@free-electrons.com> Message-ID: <20141031091435.2149346d@free-electrons.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hello, On Fri, 31 Oct 2014 08:52:15 +0100, Gregory CLEMENT wrote: > >> + switch (mvchip->soc_variant) { > >> + case MVEBU_GPIO_SOC_VARIANT_ORION: > >> + mvchip->edge_mask_regs[0] = > >> + readl(mvchip->membase + GPIO_EDGE_MASK_OFF); > >> + mvchip->level_mask_regs[0] = > >> + readl(mvchip->membase + GPIO_LEVEL_MASK_OFF); > >> + break; > > > > You are assigning index [0] twice, why? There must be some > > reason, and it should be stated in a comment. If the first read > > is necessary for hardware reasons, don't assign it but > > discard the result. > > Maybe I missed something but for me these 2 registers are different: > one is the _EDGE_ mask and the other the _LEVEL_ mask. Good that you looked at it Greg because I must admit I did not understand the comment from Linus :-) I read things again and both the variable *and* the register offset are different. I guess it's probably due to a -ENOTENOUGHCOFFEE :-) Thanks! Thomas -- Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com