| Message ID | 1416004026-9667-1-git-send-email-j.uzycki@elproma.com.pl |
|---|---|
| State | Rejected |
| Headers | show |
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
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
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
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
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
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-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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
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. ---
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
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
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. ---
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
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
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 > > > >
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 >> >
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
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-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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
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. ---
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.)
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
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.
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
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
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
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
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
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
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
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-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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
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
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
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
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-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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
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);
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(+)