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:

quoted

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

quoted

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

quoted

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

quoted

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?

quoted

quoted

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

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

quoted

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.

quoted

quoted

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