From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754646AbaI2PJz (ORCPT ); Mon, 29 Sep 2014 11:09:55 -0400 Received: from mail-wg0-f48.google.com ([74.125.82.48]:51726 "EHLO mail-wg0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753247AbaI2PJy (ORCPT ); Mon, 29 Sep 2014 11:09:54 -0400 Message-ID: <54297645.7030208@linaro.org> Date: Mon, 29 Sep 2014 16:09:57 +0100 From: Daniel Thompson User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.1.1 MIME-Version: 1.0 To: Srinivas Kandagatla , Pramod Gurav , linux-kernel@vger.kernel.org CC: Maxime Coquelin , Patrice Chotard , Linus Walleij Subject: Re: [PATCH] pinctrl: st: Fix Sparse error References: <1411998499-20300-1-git-send-email-pramod.gurav@smartplayin.com> <542970CF.6040405@linaro.org> In-Reply-To: <542970CF.6040405@linaro.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 29/09/14 15:46, Srinivas Kandagatla wrote: > On 29/09/14 14:48, Pramod Gurav wrote: >> This change fixes below sparse error, >> drivers/pinctrl/pinctrl-st.c:1515:31: error: incompatible types for >> operation (>) >> drivers/pinctrl/pinctrl-st.c:1515:31: left side has type void >> [noderef] *irqmux_base >> drivers/pinctrl/pinctrl-st.c:1515:31: right side has type int >> >> The fix is done by removing a check on info->irqmux_base as >> info->irqmux_base has already been checked for error when allocating it. >> Hence there is no need to redo the check. >> >> Cc: Maxime Coquelin >> Cc: Patrice Chotard >> CC: Linus Walleij >> Cc: Srinivas Kandagatla >> Signed-off-by: Pramod Gurav >> --- >> drivers/pinctrl/pinctrl-st.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/pinctrl/pinctrl-st.c b/drivers/pinctrl/pinctrl-st.c >> index 5475374..ddeaeda 100644 >> --- a/drivers/pinctrl/pinctrl-st.c >> +++ b/drivers/pinctrl/pinctrl-st.c >> @@ -1512,7 +1512,7 @@ static int st_gpiolib_register_bank(struct >> st_pinctrl *info, >> gpio_irq, st_gpio_irq_handler); >> } >> >> - if (info->irqmux_base > 0 || gpio_irq > 0) { >> + if (gpio_irq > 0) { >> err = gpiochip_irqchip_add(&bank->gpio_chip, &st_gpio_irqchip, >> 0, handle_simple_irq, >> IRQ_TYPE_LEVEL_LOW); > > This is not the correct fix. Please see why irqmux_base and gpio_irq are > used in the driver. > You are breaking the logic here... > > please read the below comment from the code. > > /** > * GPIO bank can have one of the two possible types of > * interrupt-wirings. > * > * First type is via irqmux, single interrupt is used by multiple > * gpio banks. This reduces number of overall interrupts numbers > * required. All these banks belong to a single pincontroller. > * _________ > * | |----> [gpio-bank (n) ] > * | |----> [gpio-bank (n + 1)] > * [irqN]-- | irq-mux |----> [gpio-bank (n + 2)] > * | |----> [gpio-bank (... )] > * |_________|----> [gpio-bank (n + 7)] > * > * Second type has a dedicated interrupt per each gpio bank. > * > * [irqN]----> [gpio-bank (n)] > */ > > > so irqmux_base is first type and gpio_irq is second type. > if you remove check for irqmux_base here you would end up NOT adding > irqchip the gpiochip in first type so you break the existing logic here. > > > I think the correct fix is: > > diff --git a/drivers/pinctrl/pinctrl-st.c b/drivers/pinctrl/pinctrl-st.c > index 5475374..4060c30 100644 > --- a/drivers/pinctrl/pinctrl-st.c > +++ b/drivers/pinctrl/pinctrl-st.c > @@ -1512,7 +1512,7 @@ static int st_gpiolib_register_bank(struct > st_pinctrl *info, > gpio_irq, > st_gpio_irq_handler); > } > > - if (info->irqmux_base > 0 || gpio_irq > 0) { > + if (!IS_ERR(info->irqmux_base) || gpio_irq > 0) { > err = gpiochip_irqchip_add(&bank->gpio_chip, > &st_gpio_irqchip, > 0, handle_simple_irq, > IRQ_TYPE_LEVEL_LOW); IS_ERR() should be irrelavent because the allocation code bombs out on error. Shouldn't this just be a NULL pointer check? if(info->irqmux_base || gpio_irq > 0) Daniel.