[PATCH] gpio: mxs: implement get_direction callback

[PATCH] gpio: mxs: implement get_direction callback

[Janusz Uzycki]

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.
The patch implements the missing callback.

Inspired from arch/arm/mach-at91/gpio.c
Required to get the patch "serial: mxs-auart: enable PPS support" working.

Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl>
---

Two patches were missed during movements between our internal repos.
The 2/2 patch is required against commits:
  f9e42397d79b ("serial: mxs-auart: add interrupts for modem control lines")
  36a262782b04 ("serial: mxs-auart: enable PPS support")
I've done build-test for the next only before.
I should have done hardware test also for the next, sorry.
Now it is tested for the next on real hardware too.

---
 drivers/gpio/gpio-mxs.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpio/gpio-mxs.c b/drivers/gpio/gpio-mxs.c
index 8ffdd7d..56052c2 100644
--- a/drivers/gpio/gpio-mxs.c
+++ b/drivers/gpio/gpio-mxs.c
@@ -227,6 +227,18 @@ static int mxs_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
 	return irq_find_mapping(port->domain, offset);
 }
 
+static int mxs_gpio_get_direction(struct gpio_chip *gc, unsigned offset)
+{
+	struct bgpio_chip *bgc = to_bgpio_chip(gc);
+	struct mxs_gpio_port *port =
+		container_of(bgc, struct mxs_gpio_port, bgc);
+	u32 mask = 1 << offset;
+	u32 dir;
+
+	dir = readl(port->base + PINCTRL_DOE(port));
+	return !(dir & mask);
+}
+
 static struct platform_device_id mxs_gpio_ids[] = {
 	{
 		.name = "imx23-gpio",
@@ -320,6 +332,7 @@ static int mxs_gpio_probe(struct platform_device *pdev)
 		goto out_irqdesc_free;
 
 	port->bgc.gc.to_irq = mxs_gpio_to_irq;
+	port->bgc.gc.get_direction = mxs_gpio_get_direction;
 	port->bgc.gc.base = port->id * 32;
 
 	err = gpiochip_add(&port->bgc.gc);
-- 
1.7.11.3

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

[Uwe Kleine-König]

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.

Also the grammar of you sentence isn't German enough to sound correct.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
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

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

[Janusz Użycki]

Hello Uwe,

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.

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

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)
c) modify drivers/gpio/gpio-generic.c:
    bgpio_init() could assign to get_direction default callback
    if BGPIOF_UNREADABLE_REG_DIR is not set
d) implement get_direction callback in gpio-mxs.c

Solution d) was selected because it is safe for other drivers. 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".

Maybe Richard will be interested in a).

> Also the grammar of you sentence isn't German enough to sound correct.
Right, I was tired, sorry.
What about the following description for the patch?

gpiolib's function gpiod_get_direction() returns the EINVAL error
if get_direction() callback is not defined.
The patch implements the callback for mxs chip as commit
f9e42397d79b ("serial: mxs-auart: add interrupts for modem control lines")
uses the gpiod_get_direction() function.

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

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

[Uwe Kleine-König]

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.

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.

Also note that the atmel_get_lines_status function can be simplified to
just:

	status = UART_GET_CSR(port);
	return mctrl_gpio_get(atmel_port->gpios, &status);

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

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Uwe Kleine-König]

Hello again,

On Sun, Nov 16, 2014 at 10:42:39PM +0100, Uwe Kleine-König wrote:
> Also note that the atmel_get_lines_status function can be simplified to
> just:
> 
> 	status = UART_GET_CSR(port);
> 	return mctrl_gpio_get(atmel_port->gpios, &status);
thinking again, this is wrong because it uses the Atmel specific
register bits for the different flags, but with the generic solution
outlined in the previous mail this function wouldn't need to handle the
gpio parts at all.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
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

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

[Janusz Użycki]

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().
gpio_irq table initialized in mctrl_gpio_request_irqs().

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*);

Richard, what do you think about?

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

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

[Janusz Użycki]

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

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Uwe Kleine-König]

Hello Janusz,

On Mon, Nov 17, 2014 at 02:58:44AM +0100, Janusz Użycki wrote:
> 
> 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:
> >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.
That's something you have to live with and that's why there is a merge
window.

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

> >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
You don't need irqhandler. struct mctrl_gpios is needed of course.

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

> >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)
What is idx about? I see it already in the mctrl_gpio API, but there is
no documentation about how it's used. Is it always 0?

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;
		} mctrl_line[UART_GPIO_MAX];
	};

	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. Not sure there is a corresponding request_irq variant for that.
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?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Alexander Shiyan]

Hello.

...
> > struct mctrl_gpios *mctrl_gpio_init(struct device *dev, unsigned int
> > idx, int *irqs)
> What is idx about? I see it already in the mctrl_gpio API, but there is
> no documentation about how it's used. Is it always 0?

This could be used for drivers with several UARTs, which registered at once.

---

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

[Uwe Kleine-König]

Hello Alexander,

On Mon, Nov 17, 2014 at 11:38:54AM +0300, Alexander Shiyan wrote:
> Hello.
> 
> ...
> > > struct mctrl_gpios *mctrl_gpio_init(struct device *dev, unsigned int
> > > idx, int *irqs)
> > What is idx about? I see it already in the mctrl_gpio API, but there is
> > no documentation about how it's used. Is it always 0?
> 
> This could be used for drivers with several UARTs, which registered at once.
Then what about:

	/**
	 * Document what idx is about here.
	 */
	struct mctrl_gpios *mctrl_gpio_init_index(struct uart_port *port, unsigned int idx);
	static inline struct mctrl_gpios *mctrl_gpio_init(struct uart_port *port)
	{
		return mctrl_gpio_init_index(port, 0);
	}

? That matches how gpio and of functions use the indexed variants.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
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

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

[Alexander Shiyan]

Mon, 17 Nov 2014 09:44:10 +0100 от Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> Hello Alexander,
> 
> On Mon, Nov 17, 2014 at 11:38:54AM +0300, Alexander Shiyan wrote:
> > Hello.
> > 
> > ...
> > > > struct mctrl_gpios *mctrl_gpio_init(struct device *dev, unsigned int
> > > > idx, int *irqs)
> > > What is idx about? I see it already in the mctrl_gpio API, but there is
> > > no documentation about how it's used. Is it always 0?
> > 
> > This could be used for drivers with several UARTs, which registered at once.
> Then what about:
> 
> 	/**
> 	 * Document what idx is about here.
> 	 */
> 	struct mctrl_gpios *mctrl_gpio_init_index(struct uart_port *port, unsigned int idx);
> 	static inline struct mctrl_gpios *mctrl_gpio_init(struct uart_port *port)
> 	{
> 		return mctrl_gpio_init_index(port, 0);
> 	}
> 
> ? That matches how gpio and of functions use the indexed variants.

This is planned by me for "sccnxp" driver at least.

---

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

[Janusz Użycki]

W dniu 2014-11-17 o 09:28, Uwe Kleine-König pisze:
> Hello Janusz,
>
> On Mon, Nov 17, 2014 at 02:58:44AM +0100, Janusz Użycki wrote:
>> 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:
>>> 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.
> That's something you have to live with and that's why there is a merge
> window.
>
>>> 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.

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

>
>>> 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)
> What is idx about? I see it already in the mctrl_gpio API, but there is
> no documentation about how it's used. Is it always 0?

dt index

> 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;
> 		} mctrl_line[UART_GPIO_MAX];
> 	};

Looks good. Richard, do you agree?

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

> 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

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

[Uwe Kleine-König]

Hello,

On Mon, Nov 17, 2014 at 10:11:25AM +0100, Janusz Użycki wrote:
> >On Mon, Nov 17, 2014 at 02:58:44AM +0100, Janusz Użycki wrote:
> >>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:
> >>>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.
> >That's something you have to live with and that's why there is a merge
> >window.
> >
> >>>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.
> 
> >
> >>>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
> >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?
Right, there shouldn't be anything driver specific in the irq handler.
Using the same irq handler as for the uart irq is just non-optimal (to
say it nicely). Look at the atmel driver. It sets the full irq handler
atmel_interrupt for the gpio lines irq. For this to work this handler
has added complexity (several "if (irq == atmel_port->gpio_irq[...])")
instead of only handling the in-chip stuff and for the gpio lines just
do the necessary statistic update and
wake_up_interruptible(&port->state->port.delta_msr_wait).

Moreover atmel_interrupt is more complex as needed, which makes
following it harder than necessary. (atmel_handle_status could just test
for status != atmel_port->irq_status instead of determining the pending
mask.)


> >>>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.
This is still possible because mctrl_gpio_enable_ms only handles gpios
it is responsible for.

> >>>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)
> >What is idx about? I see it already in the mctrl_gpio API, but there is
> >no documentation about how it's used. Is it always 0?
> 
> dt index
> 
> >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;
> >		} mctrl_line[UART_GPIO_MAX];
> >	};
> 
> Looks good. Richard, do you agree?
> 
> >	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?
Hmm
> 
> >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
> >
> 
> 

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Richard Genoud]

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:
>>
>> Hello Janusz,
>>
>> On Mon, Nov 17, 2014 at 02:58:44AM +0100, Janusz Użycki wrote:
>>>
>>> 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:
>>>> 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.
>>
>> That's something you have to live with and that's why there is a merge
>> window.
>>
>>>> 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.
>
>>
>>>> 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
>>
>> 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.
>
>>
>>>> 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)
>>
>> What is idx about? I see it already in the mctrl_gpio API, but there is
>> no documentation about how it's used. Is it always 0?
>
>
> dt index
>
>> 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 !

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



-- 
for me, ck means con kolivas and not calvin klein... does it mean I'm a geek ?
--
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

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

[Uwe Kleine-König]

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], ...

.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
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

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

[Richard Genoud]

2014-11-17 10:59 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@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);
--
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

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

[Janusz Użycki]

Hi Richard,

W dniu 2014-11-17 o 11:05, Richard Genoud pisze:
> 2014-11-17 10:59 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>
> 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);

I hesitate if use the rollback or just call mctrl_gpio_free_irqs().
Let's note that "i" is enum and the loop ends on i < 0.

best regards
Janusz

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

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

[Richard Genoud]

2014-11-17 15:29 GMT+01:00 Janusz Użycki <j.uzycki@elproma.com.pl>:
> Hi Richard,
>
> W dniu 2014-11-17 o 11:05, Richard Genoud pisze:
>>
>> 2014-11-17 10:59 GMT+01:00 Uwe Kleine-König
>> <u.kleine-koenig@pengutronix.de>:
>>
>> 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);
>
>
> I hesitate if use the rollback or just call mctrl_gpio_free_irqs().
> Let's note that "i" is enum and the loop ends on i < 0.

That's right, it's a bug.
I thought that enum was signed, but actually, it could be unsigned on
some platforms.
--
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

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

[Uwe Kleine-König]

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

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
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

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

[Janusz Użycki]

W dniu 2014-11-17 o 16:53, Uwe Kleine-König pisze:
> 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@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 ...

Likely the gpio interrupts will never shared. We can say mctrl_gpio is 
the only owner
of a gpio after a request. So should we worry that IRQ_NOAUTOEN is hidden?

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

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

[Uwe Kleine-König]

Hello,

On Mon, Nov 17, 2014 at 04:58:15PM +0100, Janusz Użycki wrote:
> 
> W dniu 2014-11-17 o 16:53, Uwe Kleine-König pisze:
> >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@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 ...
> 
> Likely the gpio interrupts will never shared. We can say mctrl_gpio
> is the only owner
> of a gpio after a request.
Right. The important part of your sentence is: "after a request". So at
the time irq_set_status_flags(..., IRQ_NOAUTOEN) is called, you're not
yet owning it. At a minimum you must clear the flag in the error path.

I'd like to have a statement from Thomas here if this is considered OK.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
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

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

[Richard Genoud]

2014-11-17 16:53 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@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@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 !
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Janusz Użycki]

W dniu 2014-11-17 o 11:05, Richard Genoud pisze:
> 2014-11-17 10:59 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@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);

I finally returned "unsigned int irq" into "int irq" because 
gpiod_to_irq() returns int also.
It makes code longer, no more.
I've changed (irq >= 0) to (irq > 0) only.

best regards
Janusz

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

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

[Alexander Shiyan]

Mon, 17 Nov 2014 10:59:09 +0100 от Uwe Kleine-König <u.kleine-koenig@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*.

afaik, IRQ 0 is valid irq number.

---

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Russell King - ARM Linux]

On Mon, Nov 17, 2014 at 01:05:53PM +0300, Alexander Shiyan wrote:
> Mon, 17 Nov 2014 10:59:09 +0100 от Uwe Kleine-König <u.kleine-koenig@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*.
> 
> afaik, IRQ 0 is valid irq number.

No.  IRQ0 is not a valid IRQ number, it's a historical mistake to think it
is (and it is something which needs correcting.)

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
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

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

[Richard Genoud]

2014-11-17 11:05 GMT+01:00 Alexander Shiyan <shc_work@mail.ru>:
> Mon, 17 Nov 2014 10:59:09 +0100 от Uwe Kleine-König <u.kleine-koenig@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*.
>
> afaik, IRQ 0 is valid irq number.
Unfortunately, that's right. For instance on atmel processors, irq 0
is often used.
--
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

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

[Russell King - ARM Linux]

On Mon, Nov 17, 2014 at 11:10:53AM +0100, Richard Genoud wrote:
> 2014-11-17 11:05 GMT+01:00 Alexander Shiyan <shc_work@mail.ru>:
> > Mon, 17 Nov 2014 10:59:09 +0100 от Uwe Kleine-König <u.kleine-koenig@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*.
> >
> > afaik, IRQ 0 is valid irq number.
> Unfortunately, that's right. For instance on atmel processors, irq 0
> is often used.

... which means Atmel SoCs need fixing not to use IRQ0 as a valid
interrupt.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Janusz Użycki]

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

request an irq without enabling? [Was: Re: [PATCH] gpio: mxs: implement get_direction callback]

[Uwe Kleine-König]

Hello,

Sorry for the just sent unfinished mail. I pressed "send" instead of
"reedit" after adding tglx to the recipients. Apart from the topic
handled in this mail it's complete though.

On Mon, Nov 17, 2014 at 10:11:25AM +0100, Janusz Użycki wrote:
> W dniu 2014-11-17 o 09:28, Uwe Kleine-König pisze:
> >I think mctrl_gpio_init should request the needed irqs, but not enable
> >them. Not sure there is a corresponding request_irq variant for that.
> 
> What would you propose?
Thomas: Is there something available among the irq functions for that.
The use case is that when probing an uart driver the modem control irqs
should only be enabled when the enable_ms callback is called by the
tty/serial framework. So we'd like to have something like
request_irq_noenable. This way the enable_ms callback would just need
to call enable_irq without first checking if the handler is already
setup.

The less nice alternatives are:

 - use request_irq(..); disable_irq(); and handle the short window where
   irqs are on but not yet desired.
 - only request_irq in the enable_ms callback, and free_irq in
   disable_ms.

Any ideas?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
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

Re: request an irq without enabling? [Was: Re: [PATCH] gpio: mxs: implement get_direction callback]

[Richard Genoud]

2014-11-17 10:51 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> Hello,
>
> Sorry for the just sent unfinished mail. I pressed "send" instead of
> "reedit" after adding tglx to the recipients. Apart from the topic
> handled in this mail it's complete though.
>
> On Mon, Nov 17, 2014 at 10:11:25AM +0100, Janusz Użycki wrote:
>> W dniu 2014-11-17 o 09:28, Uwe Kleine-König pisze:
>> >I think mctrl_gpio_init should request the needed irqs, but not enable
>> >them. Not sure there is a corresponding request_irq variant for that.
>>
>> What would you propose?
> Thomas: Is there something available among the irq functions for that.
> The use case is that when probing an uart driver the modem control irqs
> should only be enabled when the enable_ms callback is called by the
> tty/serial framework. So we'd like to have something like
> request_irq_noenable. This way the enable_ms callback would just need
> to call enable_irq without first checking if the handler is already
> setup.
>
> The less nice alternatives are:
>
>  - use request_irq(..); disable_irq(); and handle the short window where
>    irqs are on but not yet desired.
>  - only request_irq in the enable_ms callback, and free_irq in
>    disable_ms.
>
> Any ideas?
In atmel_request_gpio_irq(), I used:
irq_set_status_flags(irq,IRQ_NOAUTOEN);
request_irq(...)

[ sorry for repeating myself, but our emails crossed themselves :) ]

Richard.
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Janusz Użycki]

W dniu 2014-11-17 o 09:28, Uwe Kleine-König pisze:
> Hello Janusz,
>
> On Mon, Nov 17, 2014 at 02:58:44AM +0100, Janusz Użycki wrote:
>> 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:
>>> 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.
> That's something you have to live with and that's why there is a merge
> window.
>
>>> 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.
>
>>> 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
> You don't need irqhandler. struct mctrl_gpios is needed of course.
>
>>> 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);
>
>>> 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)
> What is idx about? I see it already in the mctrl_gpio API, but there is
> no documentation about how it's used. Is it always 0?
>
> 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;
> 		} mctrl_line[UART_GPIO_MAX];
> 	};
>
> 	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);

It looks there could be one more helper useful.
Both atmel_serial.c and mxs-auart.c checks if the line is supported by 
mctrl_gpio.
One time it is eg.:
(s->gpio_irq[UART_GPIO_DCD] > 0)
another time it is eg.:
IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(s->gpios, UART_GPIO_RTS))

The first one is no possible now. The second seems rude.
bool mctrl_gpio_is_gpio((struct mctrl_gpios *gpios, enum mctrl_gpio_idx 
gidx);
The name is hard. Moreover the implementation could be very similar
to mctrl_gpio_to_gpiod(). Any ideas?

best regards
Janusz

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

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

[Janusz Użycki]

W dniu 2014-11-17 o 18:00, Janusz Użycki pisze:
>
> W dniu 2014-11-17 o 09:28, Uwe Kleine-König pisze:
>> I think:
>>
>>     struct mctrl_gpios {
>>         struct uart_port *port;
>>         struct {
>>             gpio_desc *gpio;
>>             unsigned int irq;
>>         } mctrl_line[UART_GPIO_MAX];
>>     };
>>
>>     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);
>
> It looks there could be one more helper useful.
> Both atmel_serial.c and mxs-auart.c checks if the line is supported by 
> mctrl_gpio.
> One time it is eg.:
> (s->gpio_irq[UART_GPIO_DCD] > 0)
> another time it is eg.:
> IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(s->gpios, UART_GPIO_RTS))
>
> The first one is no possible now. The second seems rude.
> bool mctrl_gpio_is_gpio((struct mctrl_gpios *gpios, enum 
> mctrl_gpio_idx gidx);
> The name is hard. Moreover the implementation could be very similar
> to mctrl_gpio_to_gpiod(). Any ideas?

The differences:
- faster
- not exported
- used mainly in uart's interrupt
- assumes that struct mctrl_gpios *gpios exists

Just inline function?

best regards
Janusz
--
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

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

[Uwe Kleine-König]

Hello Janusz,

On Mon, Nov 17, 2014 at 06:07:53PM +0100, Janusz Użycki wrote:
> >It looks there could be one more helper useful.
> >Both atmel_serial.c and mxs-auart.c checks if the line is
> >supported by mctrl_gpio.
> >One time it is eg.:
> >(s->gpio_irq[UART_GPIO_DCD] > 0)
> >another time it is eg.:
> >IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(s->gpios, UART_GPIO_RTS))
> >
> >The first one is no possible now. The second seems rude.
> >bool mctrl_gpio_is_gpio((struct mctrl_gpios *gpios, enum
> >mctrl_gpio_idx gidx);
> >The name is hard. Moreover the implementation could be very similar
> >to mctrl_gpio_to_gpiod(). Any ideas?
> 
> The differences:
> - faster
> - not exported
> - used mainly in uart's interrupt
> - assumes that struct mctrl_gpios *gpios exists
I wonder why you need it at all?!

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
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

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

[Janusz Użycki]

Hello Uwe,

W dniu 2014-11-17 o 19:42, Uwe Kleine-König pisze:
> Hello Janusz,
>
> On Mon, Nov 17, 2014 at 06:07:53PM +0100, Janusz Użycki wrote:
>>> It looks there could be one more helper useful.
>>> Both atmel_serial.c and mxs-auart.c checks if the line is
>>> supported by mctrl_gpio.
>>> One time it is eg.:
>>> (s->gpio_irq[UART_GPIO_DCD] > 0)
>>> another time it is eg.:
>>> IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(s->gpios, UART_GPIO_RTS))
>>>
>>> The first one is no possible now. The second seems rude.
>>> bool mctrl_gpio_is_gpio((struct mctrl_gpios *gpios, enum
>>> mctrl_gpio_idx gidx);
>>> The name is hard. Moreover the implementation could be very similar
>>> to mctrl_gpio_to_gpiod(). Any ideas?
>> The differences:
>> - faster
>> - not exported
>> - used mainly in uart's interrupt
>> - assumes that struct mctrl_gpios *gpios exists
> I wonder why you need it at all?!

If the line is not supported by gpio it could be supported by native 
uart's hardware
if possible. There are different configurations. One port has the lines, 
other
which uses the same driver doesn't have.
Let's look at disable/enable_ms() in atmet_serial.c.
In mxs-auart DMA is not used if CTS or RTS is gpio line (timming).

best regards
Janusz

>
> Best regards
> Uwe
>

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Uwe Kleine-König]

Hello Janusz,

On Mon, Nov 17, 2014 at 08:02:54PM +0100, Janusz Użycki wrote:
> W dniu 2014-11-17 o 19:42, Uwe Kleine-König pisze:
> >On Mon, Nov 17, 2014 at 06:07:53PM +0100, Janusz Użycki wrote:
> >>>It looks there could be one more helper useful.
> >>>Both atmel_serial.c and mxs-auart.c checks if the line is
> >>>supported by mctrl_gpio.
> >>>One time it is eg.:
> >>>(s->gpio_irq[UART_GPIO_DCD] > 0)
> >>>another time it is eg.:
> >>>IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(s->gpios, UART_GPIO_RTS))
> >>>
> >>>The first one is no possible now. The second seems rude.
> >>>bool mctrl_gpio_is_gpio((struct mctrl_gpios *gpios, enum
> >>>mctrl_gpio_idx gidx);
> >>>The name is hard. Moreover the implementation could be very similar
> >>>to mctrl_gpio_to_gpiod(). Any ideas?
> >>The differences:
> >>- faster
> >>- not exported
> >>- used mainly in uart's interrupt
> >>- assumes that struct mctrl_gpios *gpios exists
> >I wonder why you need it at all?!
> 
> If the line is not supported by gpio it could be supported by native
> uart's hardware
> if possible. There are different configurations. One port has the
> lines, other
> which uses the same driver doesn't have.
> Let's look at disable/enable_ms() in atmet_serial.c.
> In mxs-auart DMA is not used if CTS or RTS is gpio line (timming).
For the atmel driver I would expect that it doesn't hurt to set
ATMEL_US_CTSIC if CTS is realized using a gpio. But maybe I'm too
optimistic here. So atmel_enable_ms could look as follows:

	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);

	/*
	 * Interrupt should not be enabled twice
	 * ukl: is this check really needed?
	 */
	if (atmel_port->ms_irq_enabled)
		return;

	atmel_port->ms_irq_enabled = true;

	mctrl_gpio_enable_ms(...);
	UART_PUT_IER(port, ATMEL_US_CTSIC | ATMEL_US_DSRIC | ATMEL_US_RIIC | ATMEL_US_DCDIC);

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Janusz Użycki]

Hello Uwe,

W dniu 2014-11-17 o 23:21, Uwe Kleine-König pisze:
> Hello Janusz,
>
> On Mon, Nov 17, 2014 at 08:02:54PM +0100, Janusz Użycki wrote:
>> W dniu 2014-11-17 o 19:42, Uwe Kleine-König pisze:
>>> On Mon, Nov 17, 2014 at 06:07:53PM +0100, Janusz Użycki wrote:
>>>>> It looks there could be one more helper useful.
>>>>> Both atmel_serial.c and mxs-auart.c checks if the line is
>>>>> supported by mctrl_gpio.
>>>>> One time it is eg.:
>>>>> (s->gpio_irq[UART_GPIO_DCD] > 0)
>>>>> another time it is eg.:
>>>>> IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(s->gpios, UART_GPIO_RTS))
>>>>>
>>>>> The first one is no possible now. The second seems rude.
>>>>> bool mctrl_gpio_is_gpio((struct mctrl_gpios *gpios, enum
>>>>> mctrl_gpio_idx gidx);
>>>>> The name is hard. Moreover the implementation could be very similar
>>>>> to mctrl_gpio_to_gpiod(). Any ideas?
>>>> The differences:
>>>> - faster
>>>> - not exported
>>>> - used mainly in uart's interrupt
>>>> - assumes that struct mctrl_gpios *gpios exists
>>> I wonder why you need it at all?!
>> If the line is not supported by gpio it could be supported by native
>> uart's hardware
>> if possible. There are different configurations. One port has the
>> lines, other
>> which uses the same driver doesn't have.
>> Let's look at disable/enable_ms() in atmet_serial.c.
>> In mxs-auart DMA is not used if CTS or RTS is gpio line (timming).
> For the atmel driver I would expect that it doesn't hurt to set
> ATMEL_US_CTSIC if CTS is realized using a gpio. But maybe I'm too
> optimistic here. So atmel_enable_ms could look as follows:
>
> 	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
>
> 	/*
> 	 * Interrupt should not be enabled twice
> 	 * ukl: is this check really needed?
> 	 */
> 	if (atmel_port->ms_irq_enabled)
> 		return;
>
> 	atmel_port->ms_irq_enabled = true;
>
> 	mctrl_gpio_enable_ms(...);
> 	UART_PUT_IER(port, ATMEL_US_CTSIC | ATMEL_US_DSRIC | ATMEL_US_RIIC | ATMEL_US_DCDIC);

It is too optimistic I think. We shouldn't assume that internal lines 
are biased if not selected by pinmux.
They should be but it is not ensured in most datasheets. Moreover today 
time to market makes erratas longer
and too often not complete. Sadly but true.

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

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

[Richard Genoud]

2014-11-17 0:59 GMT+01:00 Janusz Użycki <j.uzycki@elproma.com.pl>:
>
> 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're welcome to do it !
At the time the mctrl_helpers were introduced, there was only one user
(atmel_serial), so the line between specific code and factorizable
code was not so clear.
But clearly, the more we factorize, the better !

> 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.
And, honestly, I'm not sure dev_name(dev) is a good name.
Having something like dev_name(dev)_port_id_CTS may be better.

> 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().
> gpio_irq table initialized in mctrl_gpio_request_irqs().
>
> 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*);
>
> Richard, what do you think about?
Seems ok !

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

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

[Janusz Użycki]

W dniu 2014-11-17 o 10:26, Richard Genoud pisze:
> 2014-11-17 0:59 GMT+01:00 Janusz Użycki <j.uzycki@elproma.com.pl>:
>> W dniu 2014-11-16 o 22:42, Uwe Kleine-König pisze:
>>
>> 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're welcome to do it !
> At the time the mctrl_helpers were introduced, there was only one user
> (atmel_serial), so the line between specific code and factorizable
> code was not so clear.
> But clearly, the more we factorize, the better !
>
>> 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.
> And, honestly, I'm not sure dev_name(dev) is a good name.
> Having something like dev_name(dev)_port_id_CTS may be better.
>

For names other than device's or driver's name I would need to allocate
string. Is it so important? You can simple check the interrupt and 
corresponding port:
cat /proc/interrupts
  50:          0  gpio-mxs  21  80072000.serial
  80:          0  gpio-mxs  18  8006c000.serial
  81:          0  gpio-mxs  19  8006c000.serial
  83:          0  gpio-mxs  21  8006c000.serial
  84:          0  gpio-mxs  22  8006c000.serial

best regards
Janusz

> Seems ok !
>
> Richard.

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

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

[Uwe Kleine-König]

On Mon, Nov 17, 2014 at 03:45:00PM +0100, Janusz Użycki wrote:
> W dniu 2014-11-17 o 10:26, Richard Genoud pisze:
> >And, honestly, I'm not sure dev_name(dev) is a good name.
> >Having something like dev_name(dev)_port_id_CTS may be better.
> >
> 
> For names other than device's or driver's name I would need to allocate
> string. Is it so important? You can simple check the interrupt and
> corresponding port:
> cat /proc/interrupts
>  50:          0  gpio-mxs  21  80072000.serial
>  80:          0  gpio-mxs  18  8006c000.serial
>  81:          0  gpio-mxs  19  8006c000.serial
>  83:          0  gpio-mxs  21  8006c000.serial
>  84:          0  gpio-mxs  22  8006c000.serial
I'd say this is good enough.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
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

Re: [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@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().
--
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

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

[Uwe Kleine-König]

Hello Richard,

On Mon, Nov 17, 2014 at 09:31:21AM +0100, Richard Genoud wrote:
> 2014-11-16 22:42 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> > On Sat, Nov 15, 2014 at 08:29:04PM +0100, Janusz Użycki wrote:
> >> 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().
Note that UART_GPIO_RNG == UART_GPIO_RI. And this is also something that
could be handled be generic code, so the respective mctrl_gpio function
can just loop over mctrl_gpios_desc and skip the entries with
dir_out=true.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
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