gpio: mxs: implement get_direction callback

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.

Hello,

Quoted 4 lines · 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.

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

W dniu 2014-11-15 o 00:26, Uwe Kleine-K?nig pisze:

Quoted 10 lines

Hello,

Quoted 4 lines · 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.

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

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

Hello Janusz,

Quoted 7 lines · Janusz Uzycki

Quoted 7 lines

Quoted 4 lines

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.

Quoted 13 lines

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

Quoted 6 lines

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.

[...]

Quoted 7 lines

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.

Quoted 8 lines

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.

Quoted 3 lines

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.

Quoted 1 line

d) implement get_direction callback in gpio-mxs.c

My suggestion isn't included in your list and IMHO is the best.

Quoted 1 line

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.

Quoted 4 lines

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

W dniu 2014-11-16 o 22:42, Uwe Kleine-König pisze:

Quoted 80 lines

Hello Janusz,

Quoted 7 lines · Janusz Uzycki

Quoted 7 lines

Quoted 4 lines

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.

Quoted 13 lines

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

Quoted 6 lines

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.

[...]

Quoted 7 lines

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.

Quoted 8 lines

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.

Quoted 3 lines

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.

Quoted 1 line

d) implement get_direction callback in gpio-mxs.c

My suggestion isn't included in your list and IMHO is the best.

Quoted 1 line

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.

Quoted 7 lines

Quoted 4 lines

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

Quoted 4 lines

Best regards
Uwe

W dniu 2014-11-17 o 00:59, Janusz U?ycki pisze:

Quoted 6 lines

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.

Quoted 5 lines

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?

Quoted 9 lines

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

Quoted 4 lines

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

Quoted 1 line

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)

Quoted 4 lines

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

Quoted 2 lines

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?

Hello.

...

Quoted 4 lines

Quoted 2 lines

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.

Hello Alexander,

Quoted 9 lines · Alexander Shiyan

Hello.

...

Quoted 4 lines

Quoted 2 lines

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.

Hello Janusz,

Quoted 6 lines · Janusz Użycki

W dniu 2014-11-17 o 00:59, Janusz Użycki pisze:

Quoted 5 lines

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.

Quoted 5 lines

Quoted 5 lines

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.

Quoted 10 lines

Quoted 10 lines

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.

Quoted 5 lines

Quoted 5 lines

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.

Quoted 3 lines

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

Quoted 8 lines

Quoted 2 lines

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.

Quoted 4 lines

Quoted 4 lines

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

Quoted 10 lines · Janusz U?ycki

Quoted 6 lines

W dniu 2014-11-17 o 00:59, Janusz U?ycki pisze:

Quoted 5 lines

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.

Quoted 7 lines

Quoted 5 lines

Quoted 5 lines

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.

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

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

Quoted 1 line

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?

Quoted 6 lines

Quoted 4 lines

Quoted 4 lines

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.

Quoted 4 lines

Quoted 3 lines

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.

Quoted 9 lines

Quoted 7 lines

Quoted 1 line

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

Quoted 13 lines

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.

Quoted 4 lines

Quoted 4 lines

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:

Quoted 7 lines

	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?

Quoted 7 lines

	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.

Quoted 1 line

Not sure there is a corresponding request_irq variant for that.

What would you propose?

Quoted 5 lines

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

Quoted 4 lines

Best regards
Uwe

W dniu 2014-11-17 o 09:28, Uwe Kleine-K?nig pisze:

Quoted 21 lines

Hello Janusz,

Quoted 8 lines · Janusz U?ycki

W dniu 2014-11-17 o 00:59, Janusz U?ycki pisze:

Quoted 6 lines

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.

Quoted 5 lines

Quoted 5 lines

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.

Quoted 4 lines

Quoted 1 line

Quoted 1 line

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.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

And, I or Richard should submit a patch for
mctrl_gpio/atmel_serial/mxs-auart
to introduce the irq helper, yes?

Quoted 7 lines

Quoted 5 lines

Quoted 5 lines

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?

Quoted 6 lines

Quoted 5 lines

Quoted 5 lines

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

Quoted 7 lines

yes.

Quoted 3 lines

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.

Quoted 10 lines

Quoted 9 lines

Quoted 2 lines

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)

Quoted 2 lines

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

Quoted 19 lines

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.

Quoted 4 lines

Quoted 4 lines

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;

Quoted 2 lines

                } mctrl_line[UART_GPIO_MAX];
        };

Looks good. Richard, do you agree?

Quoted 8 lines

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.

Quoted 1 line

Not sure there is a corresponding request_irq variant for that.

What would you propose?

Quoted 4 lines

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

Quoted 4 lines

Best regards
Uwe

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Quoted 4 lines

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

Quoted 7 lines

Quoted 7 lines

I think:

        struct mctrl_gpios {
                struct uart_port *port;
                struct {
                        gpio_desc *gpio;
                        unsigned int irq;

I think it's just "int irq;" there

Quoted 5 lines

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.

Quoted 4 lines

Quoted 1 line

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.

Hello Richard,

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Quoted 4 lines

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

Quoted 8 lines

Quoted 7 lines

Quoted 7 lines

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

Quoted 5 lines

Quoted 5 lines

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.

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

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

2014-11-17 10:59 GMT+01:00 Uwe Kleine-König <[email protected]>:

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

Hi Richard,

W dniu 2014-11-17 o 11:05, Richard Genoud pisze:

Quoted 4 lines

2014-11-17 10:59 GMT+01:00 Uwe Kleine-K?nig
<[email protected]>:

Quoted 8 lines

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.

2014-11-17 10:59 GMT+01:00 Uwe Kleine-K?nig <[email protected]>:

Quoted 35 lines

Hello Richard,

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Quoted 4 lines

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

Quoted 8 lines

Quoted 7 lines

Quoted 7 lines

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

Quoted 5 lines

Quoted 5 lines

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.

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

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

Quoted 5 lines

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

Hello,

Quoted 39 lines · Richard Genoud

2014-11-17 10:59 GMT+01:00 Uwe Kleine-König <[email protected]>:

Quoted 33 lines

Hello Richard,

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

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

Quoted 8 lines

Quoted 7 lines

Quoted 7 lines

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

Quoted 11 lines

Quoted 8 lines

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.

Quoted 1 line

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

Quoted 5 lines

         irq_set_status_flags(irq[i], IRQ_NOAUTOEN);

and

         err = request_irq(irq[i], ...

Quoted 8 lines

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

W dniu 2014-11-17 o 16:53, Uwe Kleine-K?nig pisze:

Quoted 50 lines

Hello,

Quoted 47 lines · Richard Genoud

2014-11-17 10:59 GMT+01:00 Uwe Kleine-K?nig <[email protected]>:

Quoted 33 lines

Hello Richard,

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

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

Quoted 8 lines

Quoted 7 lines

Quoted 7 lines

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

Quoted 11 lines

Quoted 8 lines

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.

Quoted 1 line

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

Quoted 5 lines

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

Quoted 3 lines

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.

Hello,

Quoted 42 lines · Richard Genoud

2014-11-17 10:59 GMT+01:00 Uwe Kleine-K?nig <[email protected]>:

Quoted 35 lines

Hello Richard,

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Quoted 4 lines

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

Quoted 8 lines

Quoted 7 lines

Quoted 7 lines

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

Quoted 5 lines

Quoted 5 lines

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.

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

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

Quoted 5 lines

        irq_set_status_flags(irq[i], IRQ_NOAUTOEN);

and

        err = request_irq(irq[i], ...

Quoted 8 lines

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

2014-11-17 10:59 GMT+01:00 Uwe Kleine-König <[email protected]>:

Quoted 33 lines

Hello Richard,

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

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

Quoted 8 lines

Quoted 7 lines

Quoted 7 lines

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

Quoted 11 lines

Quoted 8 lines

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.

Quoted 1 line

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

Quoted 5 lines

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

Hello Richard,

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 4 lines

Quoted 4 lines

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;

Quoted 1 line

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

Mon, 17 Nov 2014 10:59:09 +0100 ?? Uwe Kleine-K?nig <[email protected]>:

Quoted 14 lines

Hello Richard,

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 4 lines

Quoted 4 lines

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;

Quoted 4 lines

Quoted 1 line

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.

Mon, 17 Nov 2014 10:59:09 +0100 ?? Uwe Kleine-K?nig <[email protected]>:

Quoted 14 lines

Hello Richard,

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 4 lines

Quoted 4 lines

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;

Quoted 4 lines

Quoted 1 line

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.

2014-11-17 11:05 GMT+01:00 Alexander Shiyan <[email protected]>:

Quoted 15 lines

Mon, 17 Nov 2014 10:59:09 +0100 ?? Uwe Kleine-K?nig <[email protected]>:

Quoted 14 lines

Hello Richard,

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 4 lines

Quoted 4 lines

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;

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

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.

2014-11-17 10:11 GMT+01:00 Janusz Użycki <[email protected]>:

Quoted 28 lines

W dniu 2014-11-17 o 09:28, Uwe Kleine-König pisze:

Quoted 8 lines

Quoted 6 lines

Quoted 6 lines

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.

[...]

Quoted 1 line

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?

Quoted 9 lines

Quoted 4 lines

Quoted 4 lines

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.

Quoted 3 lines

After some coding...
gpio_irq cannot be hidden - it is used by disable/enable_ms() and not only :/

Quoted 25 lines

Quoted 1 line

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.

[...]

Quoted 18 lines

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.

Quoted 4 lines

Quoted 4 lines

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

Quoted 4 lines

Quoted 2 lines

                 } mctrl_line[UART_GPIO_MAX];
         };

Looks good. Richard, do you agree?

yes, seems ok too me !

Quoted 15 lines

Quoted 9 lines

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.

Quoted 3 lines

Quoted 1 line

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.

Quoted 14 lines

Quoted 4 lines

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

Quoted 3 lines

Best regards
Uwe

W dniu 2014-11-17 o 09:28, Uwe Kleine-K?nig pisze:

Quoted 2 lines

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?

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.

Quoted 5 lines · Janusz U?ycki

W dniu 2014-11-17 o 09:28, Uwe Kleine-K?nig pisze:

Quoted 2 lines

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?

Hello Janusz,

Quoted 6 lines · Janusz Użycki

W dniu 2014-11-17 o 00:59, Janusz Użycki pisze:

Quoted 5 lines

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.

Quoted 5 lines

Quoted 5 lines

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.

Quoted 9 lines

Quoted 9 lines

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.

Quoted 4 lines

Quoted 4 lines

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.

Quoted 3 lines

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

Quoted 7 lines

Quoted 1 line

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.

Quoted 4 lines

Quoted 4 lines

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

W dniu 2014-11-17 o 09:28, Uwe Kleine-König pisze:

Quoted 15 lines

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?

Quoted 8 lines

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

Quoted 5 lines

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

Hello Janusz,

Quoted 13 lines · Janusz Użycki

Quoted 13 lines

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?

Quoted 5 lines

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

W dniu 2014-11-17 o 19:42, Uwe Kleine-K?nig pisze:

Quoted 14 lines · Janusz U?ycki

Quoted 13 lines

Quoted 13 lines

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?

Quoted 6 lines

Quoted 5 lines

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

Hello Janusz,

Quoted 20 lines · Janusz Użycki

W dniu 2014-11-17 o 19:42, Uwe Kleine-König pisze:

Quoted 19 lines · Janusz Użycki

Quoted 18 lines

Quoted 8 lines

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

Quoted 5 lines

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

Quoted 8 lines

Quoted 1 line

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

W dniu 2014-11-16 o 22:42, Uwe Kleine-K?nig pisze:

Quoted 93 lines

Hello Janusz,

Quoted 10 lines · Janusz Uzycki

Quoted 9 lines

Quoted 5 lines

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.

Quoted 14 lines

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

Quoted 6 lines

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.

[...]

Quoted 7 lines

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.

Quoted 8 lines

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.

Quoted 3 lines

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.

Quoted 1 line

d) implement get_direction callback in gpio-mxs.c

My suggestion isn't included in your list and IMHO is the best.

Quoted 1 line

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.

Quoted 8 lines

Quoted 4 lines

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?

2014-11-17 0:59 GMT+01:00 Janusz Użycki <[email protected]>:

Quoted 10 lines

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.

Quoted 4 lines

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 !

Quoted 4 lines

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.

Seems ok !

Richard.

W dniu 2014-11-17 o 10:26, Richard Genoud pisze:

Quoted 3 lines

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

Hello Janusz,

Quoted 12 lines · Janusz U?ycki

W dniu 2014-11-15 o 00:26, Uwe Kleine-K?nig pisze:

Quoted 10 lines

Hello,

Quoted 4 lines · 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.

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.

Quoted 13 lines

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

Quoted 6 lines

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.

2014-11-16 22:42 GMT+01:00 Uwe Kleine-K?nig <[email protected]>:

Quoted 10 lines · Janusz U?ycki

Quoted 6 lines

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