W dniu 2014-11-17 o 10:46, Richard Genoud pisze:
2014-11-17 10:11 GMT+01:00 Janusz Użycki <j.uzycki@xxxxxxxxxxxxxx>:
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-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
