Re: [PATCH] iio-trig-gpio:Remove redundant gpio_request

[Jonathan Cameron <jic23@cam.ac.uk>]

On 03/08/10 14:47, Hennerich, Michael wrote:
> Hennerich, Michael wrote on 2010-03-08:
>> Jonathan Cameron wrote on 2010-03-08:
>>> On 03/08/10 11:38, Hennerich, Michael wrote:
>>>> Hi Jonathan,
>>>> Jonathan Cameron wrote on 2010-03-08:
>>>>> Hi Michael,
>>>>>
>>>>> I'm wondering about this one.  Are you sure all platforms have the
>>>>> correct gpio setup when doing the request_irq? (those that need it
>>>>> obviously, basically the question is whether all platforms default to
>>>>> having gpio's as inputs).
>>>>>
>>>>> If the do there is certainly a lot of redundant code about based on a
>>>>> quick bit of grepping. (loads of it in arm/mach-pxa for starters). In
>>>>> that particular case all gpio's are configured as inputs, but I'm not
>>>>> sure other platforms are so well behaved.
>>>>>
>>>>>
>>>>> Unless I'm completely wrong that means we have run into a rather
>>>>> general issue.
>>>>
>>>> By no means - you have to first configure a GPIO_IRQ as GPIO-Input and
>>>> then as IRQ. irq_type takes care of it. In your case you gave the
>>>> IRQF_TRIGGER_RISING flag with request_irq(). In case such a
>>>> IRQF_TRIGGER_XXX flag is present the common IRQ infrastructure will
>>>> call the associated irq_chips irq_type() function. This function is
>>>> supposed to take care for all of it.(GPIO input, setting the level,
>>>> edge and polarity)
>>>>
>>>> As an alternative you can request_irq() without the
>>>> IRQF_TRIGGER_XXX
>>> flags and use set_irq_type() anytime later, or even in advance.
>>> Ah fair enough. Thanks for cleaning that up for me.
>>>
>>> Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
>>>
>>> Please send it on to Greg KH
>>
>> Shall I change the driver to loop on while(irq_res != NULL)?
>> And get the irqflags from the platform data struct resource?
> 
> What do you think about this one?
> 
> Example Platform Data:
> 
> #if defined(CONFIG_IIO_GPIO_TRIGGER) || \
>         defined(CONFIG_IIO_GPIO_TRIGGER_MODULE)
> 
> static struct resource iio_gpio_trigger_resources[] = {
>         [0] = {
>                 .start  = IRQ_PF5,
>                 .end    = IRQ_PF5,
>                 .flags  = IORESOURCE_IRQ | IORESOURCE_IRQ_HIGHEDGE,
>         },
>         [1] = {
>                 .start  = IRQ_PF2,
>                 .end    = IRQ_PF3,
>                 .flags  = IORESOURCE_IRQ | IORESOURCE_IRQ_LOWEDGE,
>         },
> };
> 
> static struct platform_device iio_gpio_trigger = {
>         .name = "iio_gpio_trigger",
>         .num_resources = ARRAY_SIZE(iio_gpio_trigger_resources),
>         .resource = iio_gpio_trigger_resources,
> };
> #endif
That seems sensible.
> 
> This way allows you to specify different GPIO Edge/Polarity behavior as well as GPIO-IRQ sequences.
> Patch below is an top of my previous one.
> I wonder I we should change all references to gpio completely (replace with irq), since this driver can also be used with any other system IRQ as well.
Hmm.. Could do I guess.  The intention was always to fill out the driver
so one could control the rising, falling choices from userspace, but
as we can do that with an interrupt that works just as well.

Would clean things up.

The patch below looks good to me.

Jonathan
> 
> 
> ===================================================================
> --- drivers/staging/iio/trigger/iio-trig-gpio.c (revision 8415)
> +++ drivers/staging/iio/trigger/iio-trig-gpio.c (working copy)
> @@ -57,64 +57,73 @@
>         .attrs = iio_gpio_trigger_attrs,
>  };
> 
> -static int iio_gpio_trigger_probe(struct platform_device *dev)
> +static int iio_gpio_trigger_probe(struct platform_device *pdev)
>  {
> -       int *pdata = dev->dev.platform_data;
> +       int *pdata = pdev->dev.platform_data;
>         struct iio_gpio_trigger_info *trig_info;
>         struct iio_trigger *trig, *trig2;
> -       int i, irq, ret = 0;
> -       if (!pdata) {
> -               printk(KERN_ERR "No IIO gpio trigger platform data found\n");
> -               goto error_ret;
> -       }
> -       for (i = 0;; i++) {
> -               if (!gpio_is_valid(pdata[i]))
> +       unsigned long irqflags;
> +       struct resource *irq_res;
> +       int irq, ret = 0, irq_res_cnt = 0;
> +       unsigned gpio;
> +
> +       do {
> +               irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, irq_res_cnt);
> +
> +               if (irq_res == NULL) {
> +                       if (irq_res_cnt == 0)
> +                               dev_err(&pdev->dev, "No GPIO IRQs specified");
>                         break;
> -               trig = iio_allocate_trigger();
> -               if (!trig) {
> -                       ret = -ENOMEM;
> -                       goto error_free_completed_registrations;
>                 }
> +               irqflags = irq_res->flags & IRQF_TRIGGER_MASK;
> 
> -               trig_info = kzalloc(sizeof(*trig_info), GFP_KERNEL);
> -               if (!trig_info) {
> -                       ret = -ENOMEM;
> -                       goto error_put_trigger;
> -               }
> -               trig->control_attrs = &iio_gpio_trigger_attr_group;
> -               trig->private_data = trig_info;
> -               trig_info->gpio = pdata[i];
> -               trig->owner = THIS_MODULE;
> -               trig->name = kmalloc(IIO_TRIGGER_NAME_LENGTH, GFP_KERNEL);
> -               if (!trig->name) {
> -                       ret = -ENOMEM;
> -                       goto error_free_trig_info;
> -               }
> -               snprintf((char *)trig->name,
> -                        IIO_TRIGGER_NAME_LENGTH,
> -                        "gpiotrig%d",
> -                        pdata[i]);
> +               for (irq = irq_res->start; irq <= irq_res->end; irq++) {
> +                       gpio = irq_to_gpio(irq);
> +                       if (!gpio_is_valid(gpio))
> +                               break;
> +                       trig = iio_allocate_trigger();
> +                       if (!trig) {
> +                               ret = -ENOMEM;
> +                               goto error_free_completed_registrations;
> +                       }
> 
> -               irq = gpio_to_irq(trig_info->gpio);
> -               if (irq < 0) {
> -                       ret = irq;
> -                       goto error_free_name;
> +                       trig_info = kzalloc(sizeof(*trig_info), GFP_KERNEL);
> +                       if (!trig_info) {
> +                               ret = -ENOMEM;
> +                               goto error_put_trigger;
> +                       }
> +                       trig->control_attrs = &iio_gpio_trigger_attr_group;
> +                       trig->private_data = trig_info;
> +                       trig_info->gpio = gpio;
> +                       trig->owner = THIS_MODULE;
> +                       trig->name = kmalloc(IIO_TRIGGER_NAME_LENGTH, GFP_KERNEL);
> +                       if (!trig->name) {
> +                               ret = -ENOMEM;
> +                               goto error_free_trig_info;
> +                       }
> +                       snprintf((char *)trig->name,
> +                                IIO_TRIGGER_NAME_LENGTH,
> +                                "gpiotrig%d",
> +                                gpio);
> +
> +                       ret = request_irq(irq, iio_gpio_trigger_poll,
> +                                         irqflags,
> +                                         trig->name,
> +                                         trig);
> +                       if (ret)
> +                               goto error_free_name;
> +
> +                       ret = iio_trigger_register(trig);
> +                       if (ret)
> +                               goto error_release_irq;
> +
> +                       list_add_tail(&trig->alloc_list, &iio_gpio_trigger_list);
>                 }
> 
> -               ret = request_irq(irq, iio_gpio_trigger_poll,
> -                                 IRQF_TRIGGER_RISING,
> -                                 trig->name,
> -                                 trig);
> -               if (ret)
> -                       goto error_free_name;
> +               irq_res_cnt++;
> +       } while (irq_res != NULL);
> 
> -               ret = iio_trigger_register(trig);
> -               if (ret)
> -                       goto error_release_irq;
> 
> -               list_add_tail(&trig->alloc_list, &iio_gpio_trigger_list);
> -
> -       }
>         return 0;
> 
>  /* First clean up the partly allocated trigger */
> @@ -143,7 +152,7 @@
>         return ret;
>  }
> 
> -static int iio_gpio_trigger_remove(struct platform_device *dev)
> +static int iio_gpio_trigger_remove(struct platform_device *pdev)
>  {
>         struct iio_trigger *trig, *trig2;
>         struct iio_gpio_trigger_info *trig_info;