From: j.uzycki@elproma•com.pl (Janusz Użycki)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH] gpio: mxs: implement get_direction callback
Date: Mon, 17 Nov 2014 02:58:44 +0100 [thread overview]
Message-ID: <54695654.3070209@elproma.com.pl> (raw)
In-Reply-To: <54693A51.5080907@elproma.com.pl>
W dniu 2014-11-17 o 00:59, Janusz U?ycki pisze:
>
> W dniu 2014-11-16 o 22:42, Uwe Kleine-K?nig pisze:
>> Hello Janusz,
>>
>> 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.
>>
>> For that another mctrl_ helper function would be nice that does the
>> required request_irqs for a given struct mctrl_gpios pointer. That would
>> be more valuable than adding the same boilerplate to another driver.
>> Also note that there is nothing special required in the irq handler in
>> this case, just passing your struct uart_port is enough. That also has
>> the nice side effect that not the complete driver's logic to dissect the
>> irq reason is needed and so probably all drivers using that mctrl_gpio
>> stuff could be simplified.
>>
>> [...]
>>
>>> at91 had defined get_direction() callback, mxs platform not.
>>> The condition helps to find gpio-inputs to set them as interrupts.
>>>
>>> Likely gpiod_get_direction() was used because
>>> drivers/tty/serial/serial_mctrl_gpio.c
>>> has no function to read back "dir_out" from mctrl_gpios_desc table.
>>>
>>> There were the ways to fix it:
>>> a) add to serial_mctrl_gpio.c function to read the "dir_out" and use
>>> it instead of
>>> gpiod_get_direction()
>>> if gpiod_get_direction() still used in the condition:
>>> b) modify gpiod_get_direction() implementation in gpiolib.c:
>>> if get_direction() callback is not defined use shadow flag
>>> (FLAG_IS_OUT)
>> That would be broken.
>>
>>> c) modify drivers/gpio/gpio-generic.c:
>>> bgpio_init() could assign to get_direction default callback
>>> if BGPIOF_UNREADABLE_REG_DIR is not set
>> Not sure this would be correct. I pass the question to Linus.
>>
>>> d) implement get_direction callback in gpio-mxs.c
>> My suggestion isn't included in your list and IMHO is the best.
>>
>>> Solution d) was selected because it is safe for other drivers.
>> That's a poor argument. Sure, implementing a generic solution that is
>> used on several platforms is not "safe" as you probably cannot test them
>> all. But the result is better: More generic code, more people using it
>> and so find the bugs in it.
>>
>>> Although a) and b)
>>> are also nice the patch doesn't modify neither serial_mctrl_gpio.c and
>>> atmel_serial.c nor gpiolib.c. It focuses on mxs platform as needed for
>>> the commit "serial: mxs-auart: add interrupts for modem control lines".
>> I wouldn't call a) nice because in the presence of my suggested solution
>> there is no need for a driver to use this function. b) is broken and so
>> cannot be nice.
>
> Thanks Uwe. I fully agree with you.
> a) was just a starter to your suggestion. My options were too
> conservative - I just
> wanted to avoid tests on hardware I don't have.
> 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?
> And, I or Richard should submit a patch for
> mctrl_gpio/atmel_serial/mxs-auart
> to introduce the irq helper, yes?
>
> You wrote passing uart_port is enough. Argument "name" for
> request_irq() can be
> recovered from dev_name(dev) or dev_driver_string(dev) where dev =
> port_uart->dev.
> But irqhandler and mctrl_gpios must be passed to
> 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().
After some coding...
gpio_irq cannot be hidden - it is used by disable/enable_ms() and not
only :/
> gpio_irq table initialized in mctrl_gpio_request_irqs().
or it could be nicely done in mctrl_gpio_init() but the problem is next
argument
for the function :/
eg.:
struct mctrl_gpios *mctrl_gpio_init(struct device *dev, unsigned int
idx, int *irqs)
Is it ok?
>
> 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*);
int mctrl_gpio_request_irqs(int *irqs, struct uart_port *port,
irq_handler_t handler);
void mctrl_gpio_free_irqs(struct mctrl_gpios *gpios, struct uart_port
*port);
Janusz
>
> Richard, what do you think about?
>
> best regards
> Janusz
>
>>
>> Best regards
>> Uwe
>>
>
next prev parent reply other threads:[~2014-11-17 1:58 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-11-14 22:27 [PATCH] gpio: mxs: implement get_direction callback Janusz Uzycki
2014-11-14 23:26 ` Uwe Kleine-König
2014-11-15 19:29 ` Janusz Użycki
2014-11-16 21:42 ` Uwe Kleine-König
2014-11-16 21:48 ` Uwe Kleine-König
2014-11-16 23:59 ` Janusz Użycki
2014-11-17 1:58 ` Janusz Użycki [this message]
2014-11-17 8:28 ` Uwe Kleine-König
2014-11-17 8:38 ` Alexander Shiyan
2014-11-17 8:44 ` Uwe Kleine-König
2014-11-17 8:53 ` Alexander Shiyan
2014-11-17 9:11 ` Janusz Użycki
2014-11-17 9:39 ` Uwe Kleine-König
2014-11-17 9:46 ` Richard Genoud
2014-11-17 9:59 ` Uwe Kleine-König
2014-11-17 10:05 ` Richard Genoud
2014-11-17 14:29 ` Janusz Użycki
2014-11-17 16:14 ` Richard Genoud
2014-11-17 15:53 ` Uwe Kleine-König
2014-11-17 15:58 ` Janusz Użycki
2014-11-17 16:02 ` Uwe Kleine-König
2014-11-17 16:04 ` Richard Genoud
2014-11-17 17:26 ` Janusz Użycki
2014-11-17 10:05 ` Alexander Shiyan
2014-11-17 10:09 ` Russell King - ARM Linux
2014-11-17 10:10 ` Richard Genoud
2014-11-17 10:17 ` Russell King - ARM Linux
2014-11-17 12:40 ` Janusz Użycki
2014-11-17 9:51 ` request an irq without enabling? [Was: Re: [PATCH] gpio: mxs: implement get_direction callback] Uwe Kleine-König
2014-11-17 9:57 ` Richard Genoud
2014-11-17 17:00 ` [PATCH] gpio: mxs: implement get_direction callback Janusz Użycki
2014-11-17 17:07 ` Janusz Użycki
2014-11-17 18:42 ` Uwe Kleine-König
2014-11-17 19:02 ` Janusz Użycki
2014-11-17 22:21 ` Uwe Kleine-König
2014-11-18 9:59 ` Janusz Użycki
2014-11-17 9:26 ` Richard Genoud
2014-11-17 14:45 ` Janusz Użycki
2014-11-17 15:59 ` Uwe Kleine-König
2014-11-17 8:31 ` Richard Genoud
2014-11-17 8:39 ` Uwe Kleine-König
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=54695654.3070209@elproma.com.pl \
--to=j.uzycki@elproma$(echo .)com.pl \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox