[PATCH] gpio: mxs: implement get_direction callback

[Richard Genoud]

2014-11-17 16:53 GMT+01:00 Uwe Kleine-König <u.kleine-koenig at pengutronix.de>:
> Hello,
>
> On Mon, Nov 17, 2014 at 11:05:51AM +0100, Richard Genoud wrote:
>> 2014-11-17 10:59 GMT+01:00 Uwe Kleine-König <u.kleine-koenig at pengutronix.de>:
>> > Hello Richard,
>> >
>> >> >>>> So finally the prototypes would be:
>> >> >>>> int mctrl_gpio_request_irqs(struct mctrl_gpios*, struct
>> >> >>>> uart_port*, irqhandler_t);
>> >> >>>> void mctrl_gpio_free_irqs(struct mctrl_gpios*);
>> >> >>
>> >> >> I think:
>> >> >>
>> >> >>         struct mctrl_gpios {
>> >> >>                 struct uart_port *port;
>> >> >>                 struct {
>> >> >>                         gpio_desc *gpio;
>> >> >>                         unsigned int irq;
>> >> I think it's just "int irq;" there
>> > irqs are unsigned. Some functions returning an irq use "int", but
>> > depending on who you ask this only for error reporting or a relict.
>> > Use 0 for invalid/unused in mctrl_gpio*.
>> >
>> >> > Yes. I tried to assign irq value in mctrl_gpio_init() only.
>> >> > There was another issue if CONFIG_GPIOLIB is not defined but it looks mctrl_
>> >> > disable/enable_ms()
>> >> > and mctrl_ irq handler solve the problem.
>> >> >
>> >> >>   Not sure there is a corresponding request_irq variant for that.
>> >> >
>> >> >
>> >> > What would you propose?
>> >> In atmel_request_gpio_irq(), the function irq_set_status_flags(irq,
>> >> IRQ_NOAUTOEN); is used before request_irq to prevent the irq from
>> >> being enabled when requested.
>> > I'm not sure this is allowed. How do you handle request_irq failing? (I
>> > just checked: you don't.) Consider another thread just doing
>> > request_irq($yourirq, ...) between
>> >
>> >         irq_set_status_flags(irq[i], IRQ_NOAUTOEN);
>> >
>> > and
>> >
>> >         err = request_irq(irq[i], ...
>>
>> well, in this case, request_irq() will fail and all the previously
>> requested irqs will be freed:
>>     /*
>>      * If something went wrong, rollback.
>>      */
>>     while (err && (--i >= 0))
>>         if (irq[i] >= 0)
>>             free_irq(irq[i], port);
> Just in case you didn't notice: Your statement is right, but for the
> other caller to request_irq there is something fishy. He gets
> IRQ_NOAUTOEN without being able to notice ...

Yes indeed. I didn't think about that !