gpiolib:change to use irq_alloc_descs_from to alloc virqs

Hi Wang,

On 09/23/2014 03:03 PM, Wang, Yalin wrote:
> hi
> 
> this is because , here:
> 
> gpiochip->irqdomain = irq_domain_add_simple(of_node,
> 					gpiochip->ngpio, first_irq,
> 					&gpiochip_domain_ops, gpiochip);
> 
> 
>   irq_domain_add_simple() in this function,
>   	if (first_irq > 0) {
> 		if (IS_ENABLED(CONFIG_SPARSE_IRQ)) {
> 			/* attempt to allocated irq_descs */
> 			int rc = irq_alloc_descs(first_irq, first_irq, size,
> 					of_node_to_nid(of_node));
> 			if (rc < 0)
> 				pr_info("Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n",
> 						first_irq);
> 		}
> 		irq_domain_associate_many(domain, first_irq, 0, size);
> 	}
> 
> if first_irq  > 0 , it will allocate it ,
> and make sure the return virq is equal to first_irq  .
> so we don't need allocate it again .

Could provide a little bit more information about issue you've observed, pls?

As for me, you patch will completely disable Sparse IRQ feature :( 

Also, I'm sure that struct gpio_chip->irq_base field can
be removed from gpiolib irqchip code - GPIO drivers shouldn't use it also,
because otherwise they will be incompatible with Sparse IRQ feature. 

Now the "irq_base" is used only twice in gpiolib code and below diff should
allow to drop it completely from gpiolib code.

not tested.



> ________________________________________
> From: Linus Walleij [linus.walleij@linaro.org]
> Sent: Tuesday, September 23, 2014 6:21 PM
> To: Wang, Yalin
> Cc: gnurou@gmail.com; linux-gpio@vger.kernel.org; akpm@linux-foundation.org
> Subject: Re: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs
> 
> On Tue, Sep 9, 2014 at 9:12 AM, Wang, Yalin <Yalin.Wang@sonymobile.com> wrote:
> 
>> this patch change use from irq_create_mapping to irq_alloc_descs_from,
>> use irq_create_mapping to alloc virq one by one is not safe,
>> it can't promise the allcated virqs are continuous,
>> in stead, we use irq_alloc_descs_from() to alloc virqs in one time,
>> so that the allocated virqs are in continuous bitmaps.
>>
>> Signed-off-by: Yalin Wang <yalin.wang@sonymobile.com>
> 
> (...)
> 
>> -       for (offset = 0; offset < gpiochip->ngpio; offset++) {
>> -               irq_base = irq_create_mapping(gpiochip->irqdomain, offset);
>> -               if (offset == 0)
>> -                       /*
>> -                        * Store the base into the gpiochip to be used when
>> -                        * unmapping the irqs.
>> -                        */
>> -                       gpiochip->irq_base = irq_base;
>> +       if (first_irq > 0) {
>> +               gpiochip->irq_base = first_irq;
> 
> Wait is this safe? Now you assume all descriptors are pre-allocated
> and associated in this case, atleast explain what is going on.
> 
>> +       } else {
>> +               gpiochip->irq_base = irq_alloc_descs_from(1, gpiochip->ngpio,
>> +                               of_node_to_nid(of_node));
>> +               irq_domain_associate_many(gpiochip->irqdomain,
>> +                               gpiochip->irq_base, 0, gpiochip->ngpio);
> 
> This part looks OK.
> 
> I'm holding this patch back until the above is clarified.
> 
> Yours,
> Linus Walleij--
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Best regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Sep 23, 2014 at 2:59 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> Now the "irq_base" is used only twice in gpiolib code and below diff should
> allow to drop it completely from gpiolib code.
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 15cc0bb..81762ed 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -524,7 +524,7 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip)
>         /* Remove all IRQ mappings and delete the domain */
>         if (gpiochip->irqdomain) {
>                 for (offset = 0; offset < gpiochip->ngpio; offset++)
> -                       irq_dispose_mapping(gpiochip->irq_base + offset);
> +                       irq_dispose_mapping(irq_find_mapping(gpiochip->irqdomain, offset));
>                 irq_domain_remove(gpiochip->irqdomain);
>         }
>
> not tested.

I like the looks of this.

Grygorii, can you send a proper, tested patch for this? Thansk!

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> From: Linus Walleij [mailto:linus.walleij@linaro.org]

> 

> On Tue, Sep 23, 2014 at 2:59 PM, Grygorii Strashko

> <grygorii.strashko@ti.com> wrote:

> 

> > Now the "irq_base" is used only twice in gpiolib code and below diff

> > should allow to drop it completely from gpiolib code.

> >

> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index

> > 15cc0bb..81762ed 100644

> > --- a/drivers/gpio/gpiolib.c

> > +++ b/drivers/gpio/gpiolib.c

> > @@ -524,7 +524,7 @@ static void gpiochip_irqchip_remove(struct gpio_chip

> *gpiochip)

> >         /* Remove all IRQ mappings and delete the domain */

> >         if (gpiochip->irqdomain) {

> >                 for (offset = 0; offset < gpiochip->ngpio; offset++)

> > -                       irq_dispose_mapping(gpiochip->irq_base + offset);

> > +

> > + irq_dispose_mapping(irq_find_mapping(gpiochip->irqdomain, offset));

> >                 irq_domain_remove(gpiochip->irqdomain);

> >         }

> >

> > not tested.

> 

> I like the looks of this.

> 

> Grygorii, can you send a proper, tested patch for this? Thansk!


Could we also remove gpiochip->irq_base if use this solution?
It seems gpiochip->irq_base is useless .

Thanks

On Thu, Sep 25, 2014 at 9:47 AM, Wang, Yalin <Yalin.Wang@sonymobile.com> wrote:
>> From: Linus Walleij [mailto:linus.walleij@linaro.org]
>>
>> On Tue, Sep 23, 2014 at 2:59 PM, Grygorii Strashko
>> <grygorii.strashko@ti.com> wrote:
>>
>> > Now the "irq_base" is used only twice in gpiolib code and below diff
>> > should allow to drop it completely from gpiolib code.
>> >
>> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index
>> > 15cc0bb..81762ed 100644
>> > --- a/drivers/gpio/gpiolib.c
>> > +++ b/drivers/gpio/gpiolib.c
>> > @@ -524,7 +524,7 @@ static void gpiochip_irqchip_remove(struct gpio_chip
>> *gpiochip)
>> >         /* Remove all IRQ mappings and delete the domain */
>> >         if (gpiochip->irqdomain) {
>> >                 for (offset = 0; offset < gpiochip->ngpio; offset++)
>> > -                       irq_dispose_mapping(gpiochip->irq_base + offset);
>> > +
>> > + irq_dispose_mapping(irq_find_mapping(gpiochip->irqdomain, offset));
>> >                 irq_domain_remove(gpiochip->irqdomain);
>> >         }
>> >
>> > not tested.
>>
>> I like the looks of this.
>>
>> Grygorii, can you send a proper, tested patch for this? Thansk!
>
> Could we also remove gpiochip->irq_base if use this solution?
> It seems gpiochip->irq_base is useless .

I think it is used in some drivers and especially for pin ranges
visavis the pin control subsystem, so that could be a complex
operation.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html