Re: [PATCH] gpio: mxs: implement get_direction callback

["Janusz Użycki" <j.uzycki@elproma.com.pl>]

W dniu 2014-11-17 o 10:46, Richard Genoud pisze:
> 2014-11-17 10:11 GMT+01:00 Janusz Użycki <j.uzycki@elproma.com.pl>:
>> W dniu 2014-11-17 o 09:28, Uwe Kleine-König pisze:
>>
>>>>> I don't understand why gpiod_get_direction() always requires the
>>>>> callback
>>>>> and b) would be broken (I'm not so familiar with gpiolib) but I
>>>>> don't need it now.
>>>>>
>>>>> So, it looks we can drop the gpio-mxs patch, yes?
>>> That patch is not wrong, just its motivation. IMHO the only valid
>>> usecase for .get_direction is debugging.
>>
>> OK, I will submit v2.
>>
>> [...]
>>> You don't need irqhandler. struct mctrl_gpios is needed of course.
>>
>> request_irq() needs a irqhandler. Do you thing about a mctrl_ handler for
>> gpios?
>>
>>>>> mctrl_gpio_request_irqs() helper.
>>>>> The gpio_irq table could be hidden and moved into struct
>>>>> mctrl_gpios. Then
>>>>> a second helper function is required: mctrl_gpio_free_irqs().
>>> yes.
>>>
>>>> After some coding...
>>>> gpio_irq cannot be hidden - it is used by disable/enable_ms() and
>>>> not only :/
>>> mctrl_gpio_enable_ms(struct mctrl_gpios *gpios);
>>
>> This makes unable to combine gpio's and chip's lines but it could be
>> advantage
>> to separate them.
>>
>> [...]
>>> There is no need to pass an output parameter for irqs. Just save them in
>>> struct mctrl_gpios.
>>>
>>> I'd go and change all struct device * parameters of the mctrl_gpio API
>>> to struct uart_port for consistency or add struct uart_port to struct
>>> mctrl_gpios.
>>>
>>>>> 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
>>>                  } mctrl_line[UART_GPIO_MAX];
>>>          };
>>
>> Looks good. Richard, do you agree?
> yes, seems ok too me !

What do you prefer?

struct mctrl_gpios {
         struct uart_port *port;
         struct {
                 struct gpio_desc *gpio;
                 unsigned int irq;
         } mline[UART_GPIO_MAX];
};

or

struct mctrl_gpios {
         struct uart_port *port;
         struct gpio_desc *gpio[UART_GPIO_MAX];
         unsigned int irq[UART_GPIO_MAX];
};

The second one allows to save a lot of current code and make the code
shorter.

best regards
Janusz

>
>>>          struct mctrl_gpios *mctrl_gpio_init(struct uart_port *port,
>>> unsigned int idx_if_needed);
>>>          int mctrl_gpio_enable_ms(struct mctrl_gpios *gpios);
>>>          int mctrl_gpio_disable_ms(struct mctrl_gpios *gpios);
>>>          void mctrl_gpio_free(struct mctrl_gpios *gpios);
>>>
>>> I think mctrl_gpio_init should request the needed irqs, but not enable
>>> them.
>>
>> 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.
>
>>> Another open issue is how mctrl_gpio_init should find out about which
>>> gpios to use if there is no device tree. This doesn't necessarily needs
>>> to be solved now, but maybe rename mctrl_gpio_init to
>>> mctrl_gpio_init_dt?
>>
>> Right
>>
>>
>> best regards
>> Janusz
>>
>>> Best regards
>>> Uwe
>>>
>
>

--
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