Re: [PATCH 1/2] gpiolib: Add possibility to mask which GPIOs are added to IRQ domain
On Thu, Sep 15, 2016 at 5:52 PM, Mika Westerbergwrote: > When using GPIO irqchip helpers to setup irqchip for a gpiolib based > driver, it is not possible to select which GPIOs to add to the IRQ domain. > Instead it just adds all GPIOs which is not always desired. For example > there might be GPIOs that for some reason just cannot be used as interrupts > at all. > > To make this possible we add valid_mask to each gpio_chip and by default > assume all GPIOs can be used as interrupts. Drivers can then tune this > using clear_bit() or similar before they call gpiochip_irqchip_add(). > > Suggested-by: Linus Walleij > Signed-off-by: Mika Westerberg Awesome, nice infrastructure. Will apply once tglx and Marc's comments are fixed. Yours, Linus Walleij
Re: [PATCH 1/2] gpiolib: Add possibility to mask which GPIOs are added to IRQ domain
On Thu, Sep 15, 2016 at 5:52 PM, Mika Westerberg wrote: > When using GPIO irqchip helpers to setup irqchip for a gpiolib based > driver, it is not possible to select which GPIOs to add to the IRQ domain. > Instead it just adds all GPIOs which is not always desired. For example > there might be GPIOs that for some reason just cannot be used as interrupts > at all. > > To make this possible we add valid_mask to each gpio_chip and by default > assume all GPIOs can be used as interrupts. Drivers can then tune this > using clear_bit() or similar before they call gpiochip_irqchip_add(). > > Suggested-by: Linus Walleij > Signed-off-by: Mika Westerberg Awesome, nice infrastructure. Will apply once tglx and Marc's comments are fixed. Yours, Linus Walleij
Re: [PATCH 1/2] gpiolib: Add possibility to mask which GPIOs are added to IRQ domain
On Thu, 15 Sep 2016, Mika Westerberg wrote: > On Thu, Sep 15, 2016 at 05:07:58PM +0100, Marc Zyngier wrote: > > Mika, > > > > On 15/09/16 16:52, Mika Westerberg wrote: > > > When using GPIO irqchip helpers to setup irqchip for a gpiolib based > > > driver, it is not possible to select which GPIOs to add to the IRQ domain. > > > Instead it just adds all GPIOs which is not always desired. For example > > > there might be GPIOs that for some reason just cannot be used as > > > interrupts > > > at all. > > > > > > To make this possible we add valid_mask to each gpio_chip and by default > > > assume all GPIOs can be used as interrupts. Drivers can then tune this > > > using clear_bit() or similar before they call gpiochip_irqchip_add(). > > > > > > Suggested-by: Linus Walleij> > > Signed-off-by: Mika Westerberg > > > --- > > > drivers/gpio/gpiolib.c | 32 +--- > > > include/linux/gpio/driver.h | 1 + > > > 2 files changed, 30 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > > > index 53ff25ac66d8..d84c23b47f44 100644 > > > --- a/drivers/gpio/gpiolib.c > > > +++ b/drivers/gpio/gpiolib.c > > > @@ -1186,6 +1186,18 @@ int gpiochip_add_data(struct gpio_chip *chip, void > > > *data) > > > if (status) > > > goto err_remove_chip; > > > } > > > + > > > +#ifdef CONFIG_GPIOLIB_IRQCHIP > > > + chip->valid_mask = kcalloc(BITS_TO_LONGS(chip->ngpio), sizeof(long), > > > +GFP_KERNEL); > > > > Do we really want to make this a mandatory thing? In most cases, I'd > > expect this valid_mask to have all bits set, so you might as well not > > allocate it at all in that case (and only allocate it if you actually > > need it). > > We can make drivers to allocate it and only if set use it in core. Make it a flag in gpio_chip which allocates it here when requested so we don't end up with driver writers getting the alloc/free/error handling wrong over and over. Thanks, tglx
Re: [PATCH 1/2] gpiolib: Add possibility to mask which GPIOs are added to IRQ domain
On Thu, 15 Sep 2016, Mika Westerberg wrote: > On Thu, Sep 15, 2016 at 05:07:58PM +0100, Marc Zyngier wrote: > > Mika, > > > > On 15/09/16 16:52, Mika Westerberg wrote: > > > When using GPIO irqchip helpers to setup irqchip for a gpiolib based > > > driver, it is not possible to select which GPIOs to add to the IRQ domain. > > > Instead it just adds all GPIOs which is not always desired. For example > > > there might be GPIOs that for some reason just cannot be used as > > > interrupts > > > at all. > > > > > > To make this possible we add valid_mask to each gpio_chip and by default > > > assume all GPIOs can be used as interrupts. Drivers can then tune this > > > using clear_bit() or similar before they call gpiochip_irqchip_add(). > > > > > > Suggested-by: Linus Walleij > > > Signed-off-by: Mika Westerberg > > > --- > > > drivers/gpio/gpiolib.c | 32 +--- > > > include/linux/gpio/driver.h | 1 + > > > 2 files changed, 30 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > > > index 53ff25ac66d8..d84c23b47f44 100644 > > > --- a/drivers/gpio/gpiolib.c > > > +++ b/drivers/gpio/gpiolib.c > > > @@ -1186,6 +1186,18 @@ int gpiochip_add_data(struct gpio_chip *chip, void > > > *data) > > > if (status) > > > goto err_remove_chip; > > > } > > > + > > > +#ifdef CONFIG_GPIOLIB_IRQCHIP > > > + chip->valid_mask = kcalloc(BITS_TO_LONGS(chip->ngpio), sizeof(long), > > > +GFP_KERNEL); > > > > Do we really want to make this a mandatory thing? In most cases, I'd > > expect this valid_mask to have all bits set, so you might as well not > > allocate it at all in that case (and only allocate it if you actually > > need it). > > We can make drivers to allocate it and only if set use it in core. Make it a flag in gpio_chip which allocates it here when requested so we don't end up with driver writers getting the alloc/free/error handling wrong over and over. Thanks, tglx
Re: [PATCH 1/2] gpiolib: Add possibility to mask which GPIOs are added to IRQ domain
On Thu, Sep 15, 2016 at 05:07:58PM +0100, Marc Zyngier wrote: > Mika, > > On 15/09/16 16:52, Mika Westerberg wrote: > > When using GPIO irqchip helpers to setup irqchip for a gpiolib based > > driver, it is not possible to select which GPIOs to add to the IRQ domain. > > Instead it just adds all GPIOs which is not always desired. For example > > there might be GPIOs that for some reason just cannot be used as interrupts > > at all. > > > > To make this possible we add valid_mask to each gpio_chip and by default > > assume all GPIOs can be used as interrupts. Drivers can then tune this > > using clear_bit() or similar before they call gpiochip_irqchip_add(). > > > > Suggested-by: Linus Walleij> > Signed-off-by: Mika Westerberg > > --- > > drivers/gpio/gpiolib.c | 32 +--- > > include/linux/gpio/driver.h | 1 + > > 2 files changed, 30 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > > index 53ff25ac66d8..d84c23b47f44 100644 > > --- a/drivers/gpio/gpiolib.c > > +++ b/drivers/gpio/gpiolib.c > > @@ -1186,6 +1186,18 @@ int gpiochip_add_data(struct gpio_chip *chip, void > > *data) > > if (status) > > goto err_remove_chip; > > } > > + > > +#ifdef CONFIG_GPIOLIB_IRQCHIP > > + chip->valid_mask = kcalloc(BITS_TO_LONGS(chip->ngpio), sizeof(long), > > + GFP_KERNEL); > > Do we really want to make this a mandatory thing? In most cases, I'd > expect this valid_mask to have all bits set, so you might as well not > allocate it at all in that case (and only allocate it if you actually > need it). We can make drivers to allocate it and only if set use it in core. > > > + if (!chip->valid_mask) > > + return -ENOMEM; > > You really want to revise your error handling here. Oops, right. It should be: if (!chip->valid_mask) { status = -ENOMEM; goto err_remove_chip; }
Re: [PATCH 1/2] gpiolib: Add possibility to mask which GPIOs are added to IRQ domain
On Thu, Sep 15, 2016 at 05:07:58PM +0100, Marc Zyngier wrote: > Mika, > > On 15/09/16 16:52, Mika Westerberg wrote: > > When using GPIO irqchip helpers to setup irqchip for a gpiolib based > > driver, it is not possible to select which GPIOs to add to the IRQ domain. > > Instead it just adds all GPIOs which is not always desired. For example > > there might be GPIOs that for some reason just cannot be used as interrupts > > at all. > > > > To make this possible we add valid_mask to each gpio_chip and by default > > assume all GPIOs can be used as interrupts. Drivers can then tune this > > using clear_bit() or similar before they call gpiochip_irqchip_add(). > > > > Suggested-by: Linus Walleij > > Signed-off-by: Mika Westerberg > > --- > > drivers/gpio/gpiolib.c | 32 +--- > > include/linux/gpio/driver.h | 1 + > > 2 files changed, 30 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > > index 53ff25ac66d8..d84c23b47f44 100644 > > --- a/drivers/gpio/gpiolib.c > > +++ b/drivers/gpio/gpiolib.c > > @@ -1186,6 +1186,18 @@ int gpiochip_add_data(struct gpio_chip *chip, void > > *data) > > if (status) > > goto err_remove_chip; > > } > > + > > +#ifdef CONFIG_GPIOLIB_IRQCHIP > > + chip->valid_mask = kcalloc(BITS_TO_LONGS(chip->ngpio), sizeof(long), > > + GFP_KERNEL); > > Do we really want to make this a mandatory thing? In most cases, I'd > expect this valid_mask to have all bits set, so you might as well not > allocate it at all in that case (and only allocate it if you actually > need it). We can make drivers to allocate it and only if set use it in core. > > > + if (!chip->valid_mask) > > + return -ENOMEM; > > You really want to revise your error handling here. Oops, right. It should be: if (!chip->valid_mask) { status = -ENOMEM; goto err_remove_chip; }
Re: [PATCH 1/2] gpiolib: Add possibility to mask which GPIOs are added to IRQ domain
Mika, On 15/09/16 16:52, Mika Westerberg wrote: > When using GPIO irqchip helpers to setup irqchip for a gpiolib based > driver, it is not possible to select which GPIOs to add to the IRQ domain. > Instead it just adds all GPIOs which is not always desired. For example > there might be GPIOs that for some reason just cannot be used as interrupts > at all. > > To make this possible we add valid_mask to each gpio_chip and by default > assume all GPIOs can be used as interrupts. Drivers can then tune this > using clear_bit() or similar before they call gpiochip_irqchip_add(). > > Suggested-by: Linus Walleij> Signed-off-by: Mika Westerberg > --- > drivers/gpio/gpiolib.c | 32 +--- > include/linux/gpio/driver.h | 1 + > 2 files changed, 30 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 53ff25ac66d8..d84c23b47f44 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -1186,6 +1186,18 @@ int gpiochip_add_data(struct gpio_chip *chip, void > *data) > if (status) > goto err_remove_chip; > } > + > +#ifdef CONFIG_GPIOLIB_IRQCHIP > + chip->valid_mask = kcalloc(BITS_TO_LONGS(chip->ngpio), sizeof(long), > +GFP_KERNEL); Do we really want to make this a mandatory thing? In most cases, I'd expect this valid_mask to have all bits set, so you might as well not allocate it at all in that case (and only allocate it if you actually need it). > + if (!chip->valid_mask) > + return -ENOMEM; You really want to revise your error handling here. Thanks, M. -- Jazz is not dead. It just smells funny...
Re: [PATCH 1/2] gpiolib: Add possibility to mask which GPIOs are added to IRQ domain
Mika, On 15/09/16 16:52, Mika Westerberg wrote: > When using GPIO irqchip helpers to setup irqchip for a gpiolib based > driver, it is not possible to select which GPIOs to add to the IRQ domain. > Instead it just adds all GPIOs which is not always desired. For example > there might be GPIOs that for some reason just cannot be used as interrupts > at all. > > To make this possible we add valid_mask to each gpio_chip and by default > assume all GPIOs can be used as interrupts. Drivers can then tune this > using clear_bit() or similar before they call gpiochip_irqchip_add(). > > Suggested-by: Linus Walleij > Signed-off-by: Mika Westerberg > --- > drivers/gpio/gpiolib.c | 32 +--- > include/linux/gpio/driver.h | 1 + > 2 files changed, 30 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 53ff25ac66d8..d84c23b47f44 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -1186,6 +1186,18 @@ int gpiochip_add_data(struct gpio_chip *chip, void > *data) > if (status) > goto err_remove_chip; > } > + > +#ifdef CONFIG_GPIOLIB_IRQCHIP > + chip->valid_mask = kcalloc(BITS_TO_LONGS(chip->ngpio), sizeof(long), > +GFP_KERNEL); Do we really want to make this a mandatory thing? In most cases, I'd expect this valid_mask to have all bits set, so you might as well not allocate it at all in that case (and only allocate it if you actually need it). > + if (!chip->valid_mask) > + return -ENOMEM; You really want to revise your error handling here. Thanks, M. -- Jazz is not dead. It just smells funny...