[PATCH] gpio: mxs: implement get_direction callback

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

2014-11-17 10:11 GMT+01:00 Janusz U?ycki [off-list ref]:

quoted

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

quoted

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

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

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

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;

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

quoted

quoted

                 } 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

quoted

quoted

         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

   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

quoted

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

Best regards
Uwe