Re: [PATCH] gpiolib: Add gpio_detect, gpio_debounce and gpio_alt_func features to GPIOLIB

[Ben Dooks <ben-linux@fluff.org>]

On Tue, Jun 16, 2009 at 04:51:35PM +0800, Alek Du wrote:
> On Tue, 16 Jun 2009 16:45:24 +0800
> Ben Dooks <ben-linux@fluff.org> wrote:
> 
> under set_type callback.
> > 
> > Somehow you are still missing my point, this is the way it can be done
> > at the moment, without any extension to the API!
> > 
> > int attach_my_gpio_irq(int gpio, void *irqpw)
> > {
> > 	int ret;
> > 	int irq;
> > 
> > 	irq = gpio_to_irq(gpio);
> > 	if (irq < 0) {
> > 		printk(KERN_ERR "%s: no gpio irq available\n", __func__);
> > 		return irq;
> > 	}
> > 
> > 	ret = request_irq(irq, my_irq_handler, IRQF_TRIGGER_FALLING,
> > 			  "mygpio", irqpw);
> > 	if (ret < 0) 
> > 		printk(KERN_ERR "%s: cannot request irq\n", __func__);
> > 
> > 	return ret;
> > }
> > 
> I mean, in the set_type of the irq_chip driver, you need to call something like rb532_gpio_set_ilevel.

The correct place is for the code to live in the set_type of the IRQ chip,
I see no reason for this extra complexity of adding stuff to the GPIOLIB
where there is already an interface for it.

You seem to be trying to export some internal to the driver function
implementing an extant interface for no gain to the system as a whole.

> The GPIO driver needs to export rb532_gpio_set_ilevel, and this is specific to each GPIO driver. Ideally,
> I should always call gpio_detect(gpio, ...) here.
> 
> static int rb532_set_type(unsigned int irq_nr, unsigned type)
> {
> 	int gpio = irq_nr - GPIO_MAPPED_IRQ_BASE;
> 	int group = irq_to_group(irq_nr);
> 
> 	if (group != GPIO_MAPPED_IRQ_GROUP || irq_nr > (GROUP4_IRQ_BASE + 13))
> 		return (type == IRQ_TYPE_LEVEL_HIGH) ? 0 : -EINVAL;
> 
> 	switch (type) {
> 	case IRQ_TYPE_LEVEL_HIGH:
> 		rb532_gpio_set_ilevel(1, gpio);
> 		break;
> 	case IRQ_TYPE_LEVEL_LOW:
> 		rb532_gpio_set_ilevel(0, gpio);
> 		break;
> 	default:
> 		return -EINVAL;
> 	}
> 	return 0;
> }

My point is that there is an already extant way of configuring
IRQ polarities and types. This interface is exported from something
you must already implement if you want IRQs from GPIOs.

so:

1) This interface exists in the irq_chip struct
2) The GPIO driver needs to implement irq_chip to provide interrupts
3) Therefore the functionality is already covered.

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.