[PATCH] gpio: mxs: implement get_direction callback

[Richard Genoud]

Hi all,

2014-11-16 22:42 GMT+01:00 Uwe Kleine-König <u.kleine-koenig at pengutronix.de>:
> Hello Janusz,
>
> On Sat, Nov 15, 2014 at 08:29:04PM +0100, Janusz Użycki wrote:
>> W dniu 2014-11-15 o 00:26, Uwe Kleine-König pisze:
>> >Hello,
>> >
>> >On Fri, Nov 14, 2014 at 11:27:06PM +0100, Janusz Uzycki wrote:
>> >>Function gpiod_get_direction() of gpiolib calls get_direction()
>> >>callback. If chip doesn't implement it EINVAL error is returned.
>> >>The function doesn't use for returned value shadowed FLAG_IS_OUT
>> >>bit of gpio_desc.flags field so the callback is required.
>> >This sounds like a bugfix but "required" is wrong as AFAIR this call is
>> >optional and hardly used. Or did that change? The only drawback is that
>> >the debug output for (by Linux) uninitialized gpios is wrong.
>>
>> Yes, the callback is optional but gpiod_get_direction() doesn't work
>> without it.
>> gpiod_get_direction() doesn't seem optional,
>> Documentation/gpio/consumer.txt:
>> "A driver can also query the current direction of a GPIO:
>>          int gpiod_get_direction(const struct gpio_desc *desc)
>> This function will return either GPIOF_DIR_IN or GPIOF_DIR_OUT.
>> Be aware that there is no default direction for GPIOs. Therefore,
>> **using a GPIO
>> without setting its direction first is illegal and will result in undefined
>> behavior!**"
>> There is nothing about EINVAL error. It happens despite direction was
>> set before. The reason is undefined get_direction callback.
> I still think you must not rely on gpiod_get_direction working. Some
> controllers might not be able to provide this info and if you know that
> the direction was set before, there is no need to ask the framework.
> (Although I admit it might be comfortable at times.)
>
>> The commit f9e42397d79b6e810437ba1130b0b4b594f5e56c
>> ("serial: mxs-auart: add interrupts for modem control lines")
>> is based on Richard's commit ab5e4e4108ca5d8326cb6b4b3a21b096a002f68f
>> ("tty/serial: at91: add interrupts for modem control lines").
>> Both patches use the condition:
>> "if (gpiod && (gpiod_get_direction(gpiod) == GPIOF_DIR_IN))"
> This is broken. Actually you want to loop only over the functions in
> mctrl_gpios_desc that are inputs (i.e. CTS, DSR, DCD and RNG) and don't
> depend on the hardware state and/or a working gpiod_get_direction.
Yes, it seemed a convenient test to locate inputs in atmel_serial's
probe function.
But you're right, looping on a
enum mctrl_gpio_idx [] = {
        UART_GPIO_CTS, UART_GPIO_DSR,
        UART_GPIO_DCD, UART_GPIO_RNG, UART_GPIO_RI
};
Would better describe what is done, without using gpiod_get_direction().