Hi Srini, Thanks for review.
On Mon, Sep 29, 2014 at 8:16 PM, Srinivas Kandagatla <[email protected]> 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] >> <asn:2>*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 <[email protected]> >> Cc: Patrice Chotard <[email protected]> >> CC: Linus Walleij <[email protected]> >> Cc: Srinivas Kandagatla <[email protected]> >> Signed-off-by: Pramod Gurav <[email protected]> >> --- >> 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. > > Thanks for this nice explanation. > 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, But if I am not wrong in function st_pctl_probe_dt, This is already done: if (IS_ERR(info->irqmux_base)) return PTR_ERR(info->irqmux_base); That is the reason I thought there is no need to recheck the pointer info->irqmux_base. Am I misunderstanding something? > &st_gpio_irqchip, > 0, handle_simple_irq, > IRQ_TYPE_LEVEL_LOW); > > > Thanks, > srini > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to [email protected] > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- Thanks and Regards Pramod -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
