* [PATCH v3 1/4] tty: serial_mctrl_gpio: Add irqs helpers for input lines
@ 2015-02-06 16:55 Janusz Uzycki
2015-02-06 16:55 ` [PATCH v3 2/4] serial: mxs-auart: Use helpers for gpio irqs Janusz Uzycki
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: Janusz Uzycki @ 2015-02-06 16:55 UTC (permalink / raw)
To: linux-arm-kernel
A driver using mctrl_gpio called gpiod_get_direction() to check if
gpio is input line. However .get_direction callback is not available
for all platforms. The patch allows to avoid the function.
The patch introduces the following helpers:
- mctrl_gpio_request_irqs
- mctrl_gpio_free_irqs
- mctrl_gpio_enable_ms
- mctrl_gpio_disable_ms
They allow to:
- simplify drivers which use mctrl_gpio
- hide irq table in mctrl_gpio
- use default irq handler for gpios
- better separate code for gpio modem lines from uart's code
In addition mctrl_gpio_init() has been renamed to mctrl_gpio_init_dt()
to focus DT usage. Also mctrl_gpio_init_dt() initializes irq table for
gpios now and passes struct uart_port into struct mctrl_gpios.
This resulted in changed mctrl_gpio_init_dt() parameter.
It also requires port->dev is set before the function is called.
There were also fixed:
- irq = 0 means invalid/unused (-EINVAL no more used)
- mctrl_gpio_request_irqs() doesn't use negative enum value
if request_irq() failed.
The mctrl_gpio_is_gpio() inline function is under discussion
and likely it can replace exported mctrl_gpio_to_gpiod() function.
Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl>
---
The patch requires to update the drivers which use mctrl_gpio:
- atmel_serial
- mxs-auart
- clps711x
Please test it again on the platforms above.
Compile tests (next-20150204) and a test on mxs (i.mx28)
hardware were done only.
Changes since RFC v2:
- fixed mctrl_gpio_request_irqs(): just calling mctrl_gpio_free_irqs()
on failure was wrong idea because free_irq() would be called for not
requested irqs yet and kernel/irq/manage.c: free_irq(): __free_irq():
WARN: "Trying to free already-free IRQ %d" would appear
- fixed mctrl_gpio_is_gpio() inline function:
1. allow to compile when CONFIG_GPIOLIB is not defined
(inline is not broken, reported by Richard Genoud),
2. now a serial driver also can be a module
(exported function used),
3. gpios can be NULL (protected in mctrl_gpio_to_gpiod())
It calls mctrl_gpio_to_gpiod() which likely can be removed and
replaced by simpler mctrl_gpio_is_gpio()
- IRQ_NOAUTOEN setting and request_irq() atomic issue commented in the
code (thanks to Uwe Kleine-K?nig) despite other drivers do the same
- removed (clean up) useless comment "return -ENOTSUP" from
mctrl_gpio_request_irqs() inline function, it shouldn't return any
error if serial_mctrl_gpio.h is just a stub, ie. CONFIG_GPIOLIB not defined
- fixed mctrl_gpio_request_irqs(), mctrl_gpio_free_irqs(),
mctrl_gpio_enable_ms(), mctrl_gpio_disable_ms:
if "gpios" is NULL just return without any error. Serial drivers don't fail
on probe(), just warns, if mctrl_gpio_init_dt() returns NULL or an error.
- updated the documentation on mctrl_ helpers: Documentation/serial/driver
---
Documentation/serial/driver | 22 +++++-
drivers/tty/serial/serial_mctrl_gpio.c | 132 ++++++++++++++++++++++++++++++++-
drivers/tty/serial/serial_mctrl_gpio.h | 61 ++++++++++++++-
3 files changed, 209 insertions(+), 6 deletions(-)
diff --git a/Documentation/serial/driver b/Documentation/serial/driver
index c415b0e..7a0811c 100644
--- a/Documentation/serial/driver
+++ b/Documentation/serial/driver
@@ -439,7 +439,7 @@ Modem control lines via GPIO
Some helpers are provided in order to set/get modem control lines via GPIO.
-mctrl_gpio_init(dev, idx):
+mctrl_gpio_init_dt(port, idx)
This will get the {cts,rts,...}-gpios from device tree if they are
present and request them, set direction etc, and return an
allocated structure. devm_* functions are used, so there's no need
@@ -453,8 +453,28 @@ mctrl_gpio_free(dev, gpios):
mctrl_gpio_to_gpiod(gpios, gidx)
This returns the gpio structure associated to the modem line index.
+mctrl_gpio_is_gpio(gpios, gidx)
+ This returns true if the given modem line index is initialized and
+ handled by mtrl_gpio, ie. the line is a gpio line.
+
mctrl_gpio_set(gpios, mctrl):
This will sets the gpios according to the mctrl state.
mctrl_gpio_get(gpios, mctrl):
This will update mctrl with the gpios values.
+
+mctrl_gpio_request_irqs(gpios)
+ This requests an interrupt of each input gpio line. The interupts
+ are set disabled and triggered on both edges.
+ It returns an error code or zero if success.
+
+mctrl_gpio_free_irqs(gpios)
+ This will free the requested interrupts of gpio lines.
+
+mctrl_gpio_enable_ms(gpios)
+ This enables modem status interrupts assigned to gpio lines.
+ It should be called by enable_ms() of an uart driver.
+
+mctrl_gpio_disable_ms(gpios)
+ This disables modem status interrupts assigned to gpio lines.
+ It should be called by disable_ms() of an uart driver.
diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c
index a38596c..7c43d15 100644
--- a/drivers/tty/serial/serial_mctrl_gpio.c
+++ b/drivers/tty/serial/serial_mctrl_gpio.c
@@ -19,11 +19,15 @@
#include <linux/device.h>
#include <linux/gpio/consumer.h>
#include <linux/termios.h>
+#include <linux/irq.h>
#include "serial_mctrl_gpio.h"
struct mctrl_gpios {
+ struct uart_port *port;
struct gpio_desc *gpio[UART_GPIO_MAX];
+ int irq[UART_GPIO_MAX];
+ unsigned int mctrl_prev;
};
static const struct {
@@ -96,8 +100,9 @@ unsigned int mctrl_gpio_get(struct mctrl_gpios *gpios, unsigned int *mctrl)
}
EXPORT_SYMBOL_GPL(mctrl_gpio_get);
-struct mctrl_gpios *mctrl_gpio_init(struct device *dev, unsigned int idx)
+struct mctrl_gpios *mctrl_gpio_init_dt(struct uart_port *port, unsigned int idx)
{
+ struct device *dev = port->dev;
struct mctrl_gpios *gpios;
enum mctrl_gpio_idx i;
int err;
@@ -106,6 +111,7 @@ struct mctrl_gpios *mctrl_gpio_init(struct device *dev, unsigned int idx)
if (!gpios)
return ERR_PTR(-ENOMEM);
+ gpios->port = port;
for (i = 0; i < UART_GPIO_MAX; i++) {
gpios->gpio[i] = devm_gpiod_get_index(dev,
mctrl_gpios_desc[i].name,
@@ -128,11 +134,13 @@ struct mctrl_gpios *mctrl_gpio_init(struct device *dev, unsigned int idx)
devm_gpiod_put(dev, gpios->gpio[i]);
gpios->gpio[i] = NULL;
}
+ if (gpios->gpio[i] && !mctrl_gpios_desc[i].dir_out)
+ gpios->irq[i] = gpiod_to_irq(gpios->gpio[i]);
}
return gpios;
}
-EXPORT_SYMBOL_GPL(mctrl_gpio_init);
+EXPORT_SYMBOL_GPL(mctrl_gpio_init_dt);
void mctrl_gpio_free(struct device *dev, struct mctrl_gpios *gpios)
{
@@ -147,3 +155,123 @@ void mctrl_gpio_free(struct device *dev, struct mctrl_gpios *gpios)
devm_kfree(dev, gpios);
}
EXPORT_SYMBOL_GPL(mctrl_gpio_free);
+
+/*
+ * Dealing with GPIO interrupt
+ */
+#define MCTRL_ANY_DELTA (TIOCM_RI | TIOCM_DSR | TIOCM_CD | TIOCM_CTS)
+static irqreturn_t mctrl_gpio_irq_handle(int irq, void *context)
+{
+ struct mctrl_gpios *gpios = context;
+ struct uart_port *port = gpios->port;
+ u32 mctrl = gpios->mctrl_prev;
+ u32 mctrl_diff;
+
+ mctrl_gpio_get(gpios, &mctrl);
+
+ mctrl_diff = mctrl ^ gpios->mctrl_prev;
+ gpios->mctrl_prev = mctrl;
+ if (mctrl_diff & MCTRL_ANY_DELTA && port->state != NULL) {
+ if (mctrl_diff & TIOCM_RI)
+ port->icount.rng++;
+ if (mctrl_diff & TIOCM_DSR)
+ port->icount.dsr++;
+ if (mctrl_diff & TIOCM_CD)
+ uart_handle_dcd_change(port, mctrl & TIOCM_CD);
+ if (mctrl_diff & TIOCM_CTS)
+ uart_handle_cts_change(port, mctrl & TIOCM_CTS);
+
+ wake_up_interruptible(&port->state->port.delta_msr_wait);
+ }
+
+ return IRQ_HANDLED;
+}
+
+int mctrl_gpio_request_irqs(struct mctrl_gpios *gpios)
+{
+ struct uart_port *port = gpios->port;
+ int *irq = gpios->irq;
+ enum mctrl_gpio_idx i;
+ int err = 0;
+
+ /* When gpios is NULL just gpio irqs are also never set
+ * so return without any error */
+ if (IS_ERR_OR_NULL(gpios))
+ return err;
+
+ for (i = 0; i < UART_GPIO_MAX; i++) {
+ if (irq[i] <= 0)
+ continue;
+
+ /*
+ * FIXME IRQ_NOAUTOEN:
+ * This is not undone in the error path. To be fixed
+ * properly this needs to be set atomically together with
+ * request_irq.
+ */
+ irq_set_status_flags(irq[i], IRQ_NOAUTOEN);
+ err = request_irq(irq[i], mctrl_gpio_irq_handle,
+ IRQ_TYPE_EDGE_BOTH,
+ dev_name(port->dev), gpios);
+ if (err) {
+ dev_err(port->dev, "%s: Can't get %d irq\n",
+ __func__, irq[i]);
+ break;
+ }
+ }
+
+ /*
+ * If something went wrong, rollback avoiding
+ * negative enum values.
+ */
+ while (err && i > 0) {
+ i--;
+ if (irq[i] > 0)
+ free_irq(irq[i], gpios);
+ }
+
+ return err;
+}
+EXPORT_SYMBOL_GPL(mctrl_gpio_request_irqs);
+
+void mctrl_gpio_free_irqs(struct mctrl_gpios *gpios)
+{
+ enum mctrl_gpio_idx i;
+
+ if (IS_ERR_OR_NULL(gpios))
+ return;
+
+ for (i = 0; i < UART_GPIO_MAX; i++)
+ if (gpios->irq[i] > 0)
+ free_irq(gpios->irq[i], gpios);
+}
+EXPORT_SYMBOL_GPL(mctrl_gpio_free_irqs);
+
+void mctrl_gpio_enable_ms(struct mctrl_gpios *gpios)
+{
+ enum mctrl_gpio_idx i;
+
+ if (IS_ERR_OR_NULL(gpios))
+ return;
+
+ /* get initial status of modem lines GPIOs */
+ mctrl_gpio_get(gpios, &gpios->mctrl_prev);
+
+ for (i = 0; i < UART_GPIO_MAX; i++)
+ if (gpios->irq[i] > 0)
+ enable_irq(gpios->irq[i]);
+}
+EXPORT_SYMBOL_GPL(mctrl_gpio_enable_ms);
+
+void mctrl_gpio_disable_ms(struct mctrl_gpios *gpios)
+{
+ enum mctrl_gpio_idx i;
+
+ if (IS_ERR_OR_NULL(gpios))
+ return;
+
+ for (i = 0; i < UART_GPIO_MAX; i++)
+ if (gpios->irq[i] > 0)
+ disable_irq(gpios->irq[i]);
+}
+EXPORT_SYMBOL_GPL(mctrl_gpio_disable_ms);
diff --git a/drivers/tty/serial/serial_mctrl_gpio.h b/drivers/tty/serial/serial_mctrl_gpio.h
index 400ba04..1f79be3 100644
--- a/drivers/tty/serial/serial_mctrl_gpio.h
+++ b/drivers/tty/serial/serial_mctrl_gpio.h
@@ -21,6 +21,7 @@
#include <linux/err.h>
#include <linux/device.h>
#include <linux/gpio/consumer.h>
+#include <linux/serial_core.h>
enum mctrl_gpio_idx {
UART_GPIO_CTS,
@@ -40,6 +41,7 @@ enum mctrl_gpio_idx {
*/
struct mctrl_gpios;
+
#ifdef CONFIG_GPIOLIB
/*
@@ -60,12 +62,13 @@ struct gpio_desc *mctrl_gpio_to_gpiod(struct mctrl_gpios *gpios,
enum mctrl_gpio_idx gidx);
/*
- * Request and set direction of modem control lines GPIOs.
+ * Request and set direction of modem control lines GPIOs. DT is used.
+ * Initialize irq table for GPIOs.
* devm_* functions are used, so there's no need to call mctrl_gpio_free().
* Returns a pointer to the allocated mctrl structure if ok, -ENOMEM on
* allocation error.
*/
-struct mctrl_gpios *mctrl_gpio_init(struct device *dev, unsigned int idx);
+struct mctrl_gpios *mctrl_gpio_init_dt(struct uart_port *port, unsigned int idx);
/*
* Free the mctrl_gpios structure.
@@ -74,6 +77,28 @@ struct mctrl_gpios *mctrl_gpio_init(struct device *dev, unsigned int idx);
*/
void mctrl_gpio_free(struct device *dev, struct mctrl_gpios *gpios);
+/*
+ * Request irqs for input lines GPIOs. The irqs are set disabled
+ * and triggered on both edges.
+ * Returns zero if OK, otherwise an error code.
+ */
+int mctrl_gpio_request_irqs(struct mctrl_gpios *gpios);
+
+/*
+ * Free irqs for input lines GPIOs.
+ */
+void mctrl_gpio_free_irqs(struct mctrl_gpios *gpios);
+
+/*
+ * Disable modem status interrupts assigned to GPIOs
+ */
+void mctrl_gpio_enable_ms(struct mctrl_gpios *gpios);
+
+/*
+ * Enable modem status interrupts assigned to GPIOs
+ */
+void mctrl_gpio_disable_ms(struct mctrl_gpios *gpios);
+
#else /* GPIOLIB */
static inline
@@ -95,7 +120,7 @@ struct gpio_desc *mctrl_gpio_to_gpiod(struct mctrl_gpios *gpios,
}
static inline
-struct mctrl_gpios *mctrl_gpio_init(struct device *dev, unsigned int idx)
+struct mctrl_gpios *mctrl_gpio_init_dt(struct uart_port *port, unsigned int idx)
{
return ERR_PTR(-ENOSYS);
}
@@ -105,6 +130,36 @@ void mctrl_gpio_free(struct device *dev, struct mctrl_gpios *gpios)
{
}
+static inline
+int mctrl_gpio_request_irqs(struct mctrl_gpios *gpios)
+{
+ return 0;
+}
+
+static inline
+void mctrl_gpio_free_irqs(struct mctrl_gpios *gpios)
+{
+}
+
+static inline
+void mctrl_gpio_enable_ms(struct mctrl_gpios *gpios)
+{
+}
+
+static inline
+void mctrl_gpio_disable_ms(struct mctrl_gpios *gpios)
+{
+}
+
#endif /* GPIOLIB */
+/*
+ * Return true if gidx is GPIO line, otherwise false.
+ */
+static inline
+bool mctrl_gpio_is_gpio(struct mctrl_gpios *gpios, enum mctrl_gpio_idx gidx)
+{
+ return !IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(gpios, gidx));
+}
+
#endif
--
1.7.11.3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v3 2/4] serial: mxs-auart: Use helpers for gpio irqs
2015-02-06 16:55 [PATCH v3 1/4] tty: serial_mctrl_gpio: Add irqs helpers for input lines Janusz Uzycki
@ 2015-02-06 16:55 ` Janusz Uzycki
2015-02-12 13:40 ` Uwe Kleine-König
2015-02-06 16:55 ` [PATCH v3 3/4] serial: at91: " Janusz Uzycki
` (2 subsequent siblings)
3 siblings, 1 reply; 7+ messages in thread
From: Janusz Uzycki @ 2015-02-06 16:55 UTC (permalink / raw)
To: linux-arm-kernel
The patch updates mxs-auart driver to use new mctrl_gpio helpers for
gpio irqs. The code is much simpler now.
Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl>
---
Changes since RFC v2:
- fixed comments in *_ms functions to match the coding style
(credits to Uwe Kleine-K?nig)
- rebased to next-20150204
---
drivers/tty/serial/mxs-auart.c | 137 +++++------------------------------------
1 file changed, 17 insertions(+), 120 deletions(-)
diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index d1298b6..5a8cf2c 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -149,7 +149,6 @@ struct mxs_auart_port {
#define MXS_AUART_DMA_RX_READY 3 /* bit 3 */
#define MXS_AUART_RTSCTS 4 /* bit 4 */
unsigned long flags;
- unsigned int mctrl_prev;
enum mxs_auart_type devtype;
struct clk *clk;
@@ -165,7 +164,6 @@ struct mxs_auart_port {
void *rx_dma_buf;
struct mctrl_gpios *gpios;
- int gpio_irq[UART_GPIO_MAX];
bool ms_irq_enabled;
};
@@ -431,29 +429,6 @@ static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
mctrl_gpio_set(s->gpios, mctrl);
}
-#define MCTRL_ANY_DELTA (TIOCM_RI | TIOCM_DSR | TIOCM_CD | TIOCM_CTS)
-static u32 mxs_auart_modem_status(struct mxs_auart_port *s, u32 mctrl)
-{
- u32 mctrl_diff;
-
- mctrl_diff = mctrl ^ s->mctrl_prev;
- s->mctrl_prev = mctrl;
- if (mctrl_diff & MCTRL_ANY_DELTA && s->ms_irq_enabled &&
- s->port.state != NULL) {
- if (mctrl_diff & TIOCM_RI)
- s->port.icount.rng++;
- if (mctrl_diff & TIOCM_DSR)
- s->port.icount.dsr++;
- if (mctrl_diff & TIOCM_CD)
- uart_handle_dcd_change(&s->port, mctrl & TIOCM_CD);
- if (mctrl_diff & TIOCM_CTS)
- uart_handle_cts_change(&s->port, mctrl & TIOCM_CTS);
-
- wake_up_interruptible(&s->port.state->port.delta_msr_wait);
- }
- return mctrl;
-}
-
static u32 mxs_auart_get_mctrl(struct uart_port *u)
{
struct mxs_auart_port *s = to_auart_port(u);
@@ -481,18 +456,11 @@ static void mxs_auart_enable_ms(struct uart_port *port)
s->ms_irq_enabled = true;
- if (s->gpio_irq[UART_GPIO_CTS] >= 0)
- enable_irq(s->gpio_irq[UART_GPIO_CTS]);
- /* TODO: enable AUART_INTR_CTSMIEN otherwise */
-
- if (s->gpio_irq[UART_GPIO_DSR] >= 0)
- enable_irq(s->gpio_irq[UART_GPIO_DSR]);
-
- if (s->gpio_irq[UART_GPIO_RI] >= 0)
- enable_irq(s->gpio_irq[UART_GPIO_RI]);
-
- if (s->gpio_irq[UART_GPIO_DCD] >= 0)
- enable_irq(s->gpio_irq[UART_GPIO_DCD]);
+ mctrl_gpio_enable_ms(s->gpios);
+ /*
+ * TODO: enable AUART_INTR_CTSMIEN if CTS isn't handled by
+ * mctrl_gpio.
+ */
}
/*
@@ -510,18 +478,11 @@ static void mxs_auart_disable_ms(struct uart_port *port)
s->ms_irq_enabled = false;
- if (s->gpio_irq[UART_GPIO_CTS] >= 0)
- disable_irq(s->gpio_irq[UART_GPIO_CTS]);
- /* TODO: disable AUART_INTR_CTSMIEN otherwise */
-
- if (s->gpio_irq[UART_GPIO_DSR] >= 0)
- disable_irq(s->gpio_irq[UART_GPIO_DSR]);
-
- if (s->gpio_irq[UART_GPIO_RI] >= 0)
- disable_irq(s->gpio_irq[UART_GPIO_RI]);
-
- if (s->gpio_irq[UART_GPIO_DCD] >= 0)
- disable_irq(s->gpio_irq[UART_GPIO_DCD]);
+ mctrl_gpio_disable_ms(s->gpios);
+ /*
+ * TODO: disable AUART_INTR_CTSMIEN if CTS isn't handled by
+ * mctrl_gpio.
+ */
}
static int mxs_auart_dma_prep_rx(struct mxs_auart_port *s);
@@ -797,7 +758,6 @@ static irqreturn_t mxs_auart_irq_handle(int irq, void *context)
{
u32 istat;
struct mxs_auart_port *s = context;
- u32 mctrl_temp = s->mctrl_prev;
u32 stat = readl(s->port.membase + AUART_STAT);
istat = readl(s->port.membase + AUART_INTR);
@@ -809,16 +769,6 @@ static irqreturn_t mxs_auart_irq_handle(int irq, void *context)
| AUART_INTR_CTSMIS),
s->port.membase + AUART_INTR_CLR);
- /*
- * Dealing with GPIO interrupt
- */
- if (irq == s->gpio_irq[UART_GPIO_CTS] ||
- irq == s->gpio_irq[UART_GPIO_DCD] ||
- irq == s->gpio_irq[UART_GPIO_DSR] ||
- irq == s->gpio_irq[UART_GPIO_RI])
- mxs_auart_modem_status(s,
- mctrl_gpio_get(s->gpios, &mctrl_temp));
-
if (istat & AUART_INTR_CTSMIS) {
if (CTS_AT_AUART() && s->ms_irq_enabled)
uart_handle_cts_change(&s->port,
@@ -883,9 +833,6 @@ static int mxs_auart_startup(struct uart_port *u)
*/
writel(AUART_LINECTRL_FEN, u->membase + AUART_LINECTRL_SET);
- /* get initial status of modem lines */
- mctrl_gpio_get(s->gpios, &s->mctrl_prev);
-
s->ms_irq_enabled = false;
return 0;
}
@@ -1155,71 +1102,23 @@ static int serial_mxs_probe_dt(struct mxs_auart_port *s,
return 0;
}
-static bool mxs_auart_init_gpios(struct mxs_auart_port *s, struct device *dev)
+static bool mxs_auart_init_gpios(struct mxs_auart_port *s)
{
- enum mctrl_gpio_idx i;
- struct gpio_desc *gpiod;
-
- s->gpios = mctrl_gpio_init(dev, 0);
+ s->gpios = mctrl_gpio_init_dt(&s->port, 0);
if (IS_ERR_OR_NULL(s->gpios))
return false;
/* Block (enabled before) DMA option if RTS or CTS is GPIO line */
if (!RTS_AT_AUART() || !CTS_AT_AUART()) {
if (test_bit(MXS_AUART_RTSCTS, &s->flags))
- dev_warn(dev,
+ dev_warn(s->dev,
"DMA and flow control via gpio may cause some problems. DMA disabled!\n");
clear_bit(MXS_AUART_RTSCTS, &s->flags);
}
- for (i = 0; i < UART_GPIO_MAX; i++) {
- gpiod = mctrl_gpio_to_gpiod(s->gpios, i);
- if (gpiod && (gpiod_get_direction(gpiod) == GPIOF_DIR_IN))
- s->gpio_irq[i] = gpiod_to_irq(gpiod);
- else
- s->gpio_irq[i] = -EINVAL;
- }
-
return true;
}
-static void mxs_auart_free_gpio_irq(struct mxs_auart_port *s)
-{
- enum mctrl_gpio_idx i;
-
- for (i = 0; i < UART_GPIO_MAX; i++)
- if (s->gpio_irq[i] >= 0)
- free_irq(s->gpio_irq[i], s);
-}
-
-static int mxs_auart_request_gpio_irq(struct mxs_auart_port *s)
-{
- int *irq = s->gpio_irq;
- enum mctrl_gpio_idx i;
- int err = 0;
-
- for (i = 0; (i < UART_GPIO_MAX) && !err; i++) {
- if (irq[i] < 0)
- continue;
-
- irq_set_status_flags(irq[i], IRQ_NOAUTOEN);
- err = request_irq(irq[i], mxs_auart_irq_handle,
- IRQ_TYPE_EDGE_BOTH, dev_name(s->dev), s);
- if (err)
- dev_err(s->dev, "%s - Can't get %d irq\n",
- __func__, irq[i]);
- }
-
- /*
- * If something went wrong, rollback.
- */
- while (err && (--i >= 0))
- if (irq[i] >= 0)
- free_irq(irq[i], s);
-
- return err;
-}
-
static int mxs_auart_probe(struct platform_device *pdev)
{
const struct of_device_id *of_id =
@@ -1262,8 +1161,6 @@ static int mxs_auart_probe(struct platform_device *pdev)
s->port.type = PORT_IMX;
s->port.dev = s->dev = &pdev->dev;
- s->mctrl_prev = 0;
-
irq = platform_get_irq(pdev, 0);
if (irq < 0)
return irq;
@@ -1276,14 +1173,14 @@ static int mxs_auart_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, s);
- if (!mxs_auart_init_gpios(s, &pdev->dev))
+ if (!mxs_auart_init_gpios(s))
dev_err(&pdev->dev,
"Failed to initialize GPIOs. The serial port may not work as expected\n");
/*
* Get the GPIO lines IRQ
*/
- ret = mxs_auart_request_gpio_irq(s);
+ ret = mctrl_gpio_request_irqs(s->gpios);
if (ret)
return ret;
@@ -1303,7 +1200,7 @@ static int mxs_auart_probe(struct platform_device *pdev)
return 0;
out_free_gpio_irq:
- mxs_auart_free_gpio_irq(s);
+ mctrl_gpio_free_irqs(s->gpios);
auart_port[pdev->id] = NULL;
return ret;
}
@@ -1314,7 +1211,7 @@ static int mxs_auart_remove(struct platform_device *pdev)
uart_remove_one_port(&auart_driver, &s->port);
auart_port[pdev->id] = NULL;
- mxs_auart_free_gpio_irq(s);
+ mctrl_gpio_free_irqs(s->gpios);
return 0;
}
--
1.7.11.3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v3 3/4] serial: at91: Use helpers for gpio irqs
2015-02-06 16:55 [PATCH v3 1/4] tty: serial_mctrl_gpio: Add irqs helpers for input lines Janusz Uzycki
2015-02-06 16:55 ` [PATCH v3 2/4] serial: mxs-auart: Use helpers for gpio irqs Janusz Uzycki
@ 2015-02-06 16:55 ` Janusz Uzycki
2015-02-06 16:55 ` [PATCH v3 4/4] serial: clps711x: Update to new mctrl_gpio_init_dt Janusz Uzycki
2015-02-12 13:29 ` [PATCH v3 1/4] tty: serial_mctrl_gpio: Add irqs helpers for input lines Uwe Kleine-König
3 siblings, 0 replies; 7+ messages in thread
From: Janusz Uzycki @ 2015-02-06 16:55 UTC (permalink / raw)
To: linux-arm-kernel
The patch updates atmel_serial driver to use new mctrl_gpio helpers for
gpio irqs. The code is simpler now.
Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl>
---
There is no changes since v2.
---
drivers/tty/serial/atmel_serial.c | 123 +++++++-------------------------------
1 file changed, 20 insertions(+), 103 deletions(-)
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 846552b..6bcb181 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -168,7 +168,6 @@ struct atmel_uart_port {
struct circ_buf rx_ring;
struct mctrl_gpios *gpios;
- int gpio_irq[UART_GPIO_MAX];
unsigned int tx_done_mask;
bool ms_irq_enabled;
bool is_usart; /* usart or uart */
@@ -512,24 +511,18 @@ static void atmel_enable_ms(struct uart_port *port)
atmel_port->ms_irq_enabled = true;
- if (atmel_port->gpio_irq[UART_GPIO_CTS] >= 0)
- enable_irq(atmel_port->gpio_irq[UART_GPIO_CTS]);
- else
+ mctrl_gpio_enable_ms(atmel_port->gpios);
+
+ if (!mctrl_gpio_is_gpio(atmel_port->gpios, UART_GPIO_CTS))
ier |= ATMEL_US_CTSIC;
- if (atmel_port->gpio_irq[UART_GPIO_DSR] >= 0)
- enable_irq(atmel_port->gpio_irq[UART_GPIO_DSR]);
- else
+ if (!mctrl_gpio_is_gpio(atmel_port->gpios, UART_GPIO_DSR))
ier |= ATMEL_US_DSRIC;
- if (atmel_port->gpio_irq[UART_GPIO_RI] >= 0)
- enable_irq(atmel_port->gpio_irq[UART_GPIO_RI]);
- else
+ if (!mctrl_gpio_is_gpio(atmel_port->gpios, UART_GPIO_RI))
ier |= ATMEL_US_RIIC;
- if (atmel_port->gpio_irq[UART_GPIO_DCD] >= 0)
- enable_irq(atmel_port->gpio_irq[UART_GPIO_DCD]);
- else
+ if (!mctrl_gpio_is_gpio(atmel_port->gpios, UART_GPIO_DCD))
ier |= ATMEL_US_DCDIC;
UART_PUT_IER(port, ier);
@@ -551,24 +544,18 @@ static void atmel_disable_ms(struct uart_port *port)
atmel_port->ms_irq_enabled = false;
- if (atmel_port->gpio_irq[UART_GPIO_CTS] >= 0)
- disable_irq(atmel_port->gpio_irq[UART_GPIO_CTS]);
- else
+ mctrl_gpio_disable_ms(atmel_port->gpios);
+
+ if (!mctrl_gpio_is_gpio(atmel_port->gpios, UART_GPIO_CTS))
idr |= ATMEL_US_CTSIC;
- if (atmel_port->gpio_irq[UART_GPIO_DSR] >= 0)
- disable_irq(atmel_port->gpio_irq[UART_GPIO_DSR]);
- else
+ if (!mctrl_gpio_is_gpio(atmel_port->gpios, UART_GPIO_DSR))
idr |= ATMEL_US_DSRIC;
- if (atmel_port->gpio_irq[UART_GPIO_RI] >= 0)
- disable_irq(atmel_port->gpio_irq[UART_GPIO_RI]);
- else
+ if (!mctrl_gpio_is_gpio(atmel_port->gpios, UART_GPIO_RI))
idr |= ATMEL_US_RIIC;
- if (atmel_port->gpio_irq[UART_GPIO_DCD] >= 0)
- disable_irq(atmel_port->gpio_irq[UART_GPIO_DCD]);
- else
+ if (!mctrl_gpio_is_gpio(atmel_port->gpios, UART_GPIO_DCD))
idr |= ATMEL_US_DCDIC;
UART_PUT_IDR(port, idr);
@@ -1178,31 +1165,11 @@ atmel_handle_status(struct uart_port *port, unsigned int pending,
static irqreturn_t atmel_interrupt(int irq, void *dev_id)
{
struct uart_port *port = dev_id;
- struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
unsigned int status, pending, pass_counter = 0;
- bool gpio_handled = false;
do {
status = atmel_get_lines_status(port);
pending = status & UART_GET_IMR(port);
- if (!gpio_handled) {
- /*
- * Dealing with GPIO interrupt
- */
- if (irq == atmel_port->gpio_irq[UART_GPIO_CTS])
- pending |= ATMEL_US_CTSIC;
-
- if (irq == atmel_port->gpio_irq[UART_GPIO_DSR])
- pending |= ATMEL_US_DSRIC;
-
- if (irq == atmel_port->gpio_irq[UART_GPIO_RI])
- pending |= ATMEL_US_RIIC;
-
- if (irq == atmel_port->gpio_irq[UART_GPIO_DCD])
- pending |= ATMEL_US_DCDIC;
-
- gpio_handled = true;
- }
if (!pending)
break;
@@ -1682,45 +1649,6 @@ static void atmel_get_ip_name(struct uart_port *port)
}
}
-static void atmel_free_gpio_irq(struct uart_port *port)
-{
- struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
- enum mctrl_gpio_idx i;
-
- for (i = 0; i < UART_GPIO_MAX; i++)
- if (atmel_port->gpio_irq[i] >= 0)
- free_irq(atmel_port->gpio_irq[i], port);
-}
-
-static int atmel_request_gpio_irq(struct uart_port *port)
-{
- struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
- int *irq = atmel_port->gpio_irq;
- enum mctrl_gpio_idx i;
- int err = 0;
-
- for (i = 0; (i < UART_GPIO_MAX) && !err; i++) {
- if (irq[i] < 0)
- continue;
-
- irq_set_status_flags(irq[i], IRQ_NOAUTOEN);
- err = request_irq(irq[i], atmel_interrupt, IRQ_TYPE_EDGE_BOTH,
- "atmel_serial", port);
- if (err)
- dev_err(port->dev, "atmel_startup - Can't get %d irq\n",
- irq[i]);
- }
-
- /*
- * If something went wrong, rollback.
- */
- while (err && (--i >= 0))
- if (irq[i] >= 0)
- free_irq(irq[i], port);
-
- return err;
-}
-
/*
* Perform initialization and enable port for reception
*/
@@ -1752,7 +1680,7 @@ static int atmel_startup(struct uart_port *port)
/*
* Get the GPIO lines IRQ
*/
- retval = atmel_request_gpio_irq(port);
+ retval = mctrl_gpio_request_irqs(atmel_port->gpios);
if (retval)
goto free_irq;
@@ -1889,7 +1817,7 @@ static void atmel_shutdown(struct uart_port *port)
* Free the interrupts
*/
free_irq(port->irq, port);
- atmel_free_gpio_irq(port);
+ mctrl_gpio_free_irqs(atmel_port->gpios);
atmel_port->ms_irq_enabled = false;
@@ -2536,24 +2464,13 @@ static int atmel_serial_resume(struct platform_device *pdev)
#define atmel_serial_resume NULL
#endif
-static int atmel_init_gpios(struct atmel_uart_port *p, struct device *dev)
+static bool atmel_init_gpios(struct atmel_uart_port *p)
{
- enum mctrl_gpio_idx i;
- struct gpio_desc *gpiod;
-
- p->gpios = mctrl_gpio_init(dev, 0);
+ p->gpios = mctrl_gpio_init_dt(&p->uart, 0);
if (IS_ERR_OR_NULL(p->gpios))
- return -1;
+ return false;
- for (i = 0; i < UART_GPIO_MAX; i++) {
- gpiod = mctrl_gpio_to_gpiod(p->gpios, i);
- if (gpiod && (gpiod_get_direction(gpiod) == GPIOF_DIR_IN))
- p->gpio_irq[i] = gpiod_to_irq(gpiod);
- else
- p->gpio_irq[i] = -EINVAL;
- }
-
- return 0;
+ return true;
}
static int atmel_serial_probe(struct platform_device *pdev)
@@ -2593,8 +2510,8 @@ static int atmel_serial_probe(struct platform_device *pdev)
port->backup_imr = 0;
port->uart.line = ret;
- ret = atmel_init_gpios(port, &pdev->dev);
- if (ret < 0)
+ port->uart.dev = &pdev->dev;
+ if (!atmel_init_gpios(port))
dev_err(&pdev->dev, "%s",
"Failed to initialize GPIOs. The serial port may not work as expected");
--
1.7.11.3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v3 4/4] serial: clps711x: Update to new mctrl_gpio_init_dt
2015-02-06 16:55 [PATCH v3 1/4] tty: serial_mctrl_gpio: Add irqs helpers for input lines Janusz Uzycki
2015-02-06 16:55 ` [PATCH v3 2/4] serial: mxs-auart: Use helpers for gpio irqs Janusz Uzycki
2015-02-06 16:55 ` [PATCH v3 3/4] serial: at91: " Janusz Uzycki
@ 2015-02-06 16:55 ` Janusz Uzycki
2015-02-12 13:29 ` [PATCH v3 1/4] tty: serial_mctrl_gpio: Add irqs helpers for input lines Uwe Kleine-König
3 siblings, 0 replies; 7+ messages in thread
From: Janusz Uzycki @ 2015-02-06 16:55 UTC (permalink / raw)
To: linux-arm-kernel
The patch updates clps711x serial driver to new mctrl_gpio_init_dt
which came with new helpers for gpio irqs.
Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl>
---
There is no changes since v2.
Compile-test only - please test it on real hardware.
---
drivers/tty/serial/clps711x.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/clps711x.c b/drivers/tty/serial/clps711x.c
index 6e11c27..4fa0210 100644
--- a/drivers/tty/serial/clps711x.c
+++ b/drivers/tty/serial/clps711x.c
@@ -500,7 +500,7 @@ static int uart_clps711x_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, s);
- s->gpios = mctrl_gpio_init(&pdev->dev, 0);
+ s->gpios = mctrl_gpio_init_dt(&s->port, 0);
ret = uart_add_one_port(&clps711x_uart, &s->port);
if (ret)
--
1.7.11.3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v3 1/4] tty: serial_mctrl_gpio: Add irqs helpers for input lines
2015-02-06 16:55 [PATCH v3 1/4] tty: serial_mctrl_gpio: Add irqs helpers for input lines Janusz Uzycki
` (2 preceding siblings ...)
2015-02-06 16:55 ` [PATCH v3 4/4] serial: clps711x: Update to new mctrl_gpio_init_dt Janusz Uzycki
@ 2015-02-12 13:29 ` Uwe Kleine-König
2015-02-12 13:45 ` Alexander Shiyan
3 siblings, 1 reply; 7+ messages in thread
From: Uwe Kleine-König @ 2015-02-12 13:29 UTC (permalink / raw)
To: linux-arm-kernel
Hello,
On Fri, Feb 06, 2015 at 05:55:32PM +0100, Janusz Uzycki wrote:
> A driver using mctrl_gpio called gpiod_get_direction() to check if
> gpio is input line. However .get_direction callback is not available
> for all platforms. The patch allows to avoid the function.
> The patch introduces the following helpers:
> - mctrl_gpio_request_irqs
> - mctrl_gpio_free_irqs
> - mctrl_gpio_enable_ms
> - mctrl_gpio_disable_ms
> They allow to:
> - simplify drivers which use mctrl_gpio
> - hide irq table in mctrl_gpio
> - use default irq handler for gpios
> - better separate code for gpio modem lines from uart's code
> In addition mctrl_gpio_init() has been renamed to mctrl_gpio_init_dt()
> to focus DT usage. Also mctrl_gpio_init_dt() initializes irq table for
> gpios now and passes struct uart_port into struct mctrl_gpios.
> This resulted in changed mctrl_gpio_init_dt() parameter.
> It also requires port->dev is set before the function is called.
>
> There were also fixed:
> - irq = 0 means invalid/unused (-EINVAL no more used)
> - mctrl_gpio_request_irqs() doesn't use negative enum value
> if request_irq() failed.
>
> The mctrl_gpio_is_gpio() inline function is under discussion
> and likely it can replace exported mctrl_gpio_to_gpiod() function.
>
> Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl>
For the git history this commit log is too verbose. The diff between v2
and v3 at least could be dropped.
I'd write something like:
serial: mctrl-gpio: Add irq handling
The actions required when modem status line inputs change don't need to
be duplicated in each driver using mctrl-gpio so add the respective
support to the mctrl-gpio core.
This adds a few new helper function and allows to remove some
boilerplate from the drivers currently using mctrl-gpio.
I think this patch still breaks bisectibilty, doesn't it? That means the
API changes and its users must be updated in a single patch.
> The patch requires to update the drivers which use mctrl_gpio:
> - atmel_serial
> - mxs-auart
> - clps711x
>
> Please test it again on the platforms above.
> Compile tests (next-20150204) and a test on mxs (i.mx28)
> hardware were done only.
>
> Changes since RFC v2:
> - fixed mctrl_gpio_request_irqs(): just calling mctrl_gpio_free_irqs()
> on failure was wrong idea because free_irq() would be called for not
> requested irqs yet and kernel/irq/manage.c: free_irq(): __free_irq():
> WARN: "Trying to free already-free IRQ %d" would appear
> - fixed mctrl_gpio_is_gpio() inline function:
> 1. allow to compile when CONFIG_GPIOLIB is not defined
> (inline is not broken, reported by Richard Genoud),
> 2. now a serial driver also can be a module
> (exported function used),
> 3. gpios can be NULL (protected in mctrl_gpio_to_gpiod())
> It calls mctrl_gpio_to_gpiod() which likely can be removed and
> replaced by simpler mctrl_gpio_is_gpio()
> - IRQ_NOAUTOEN setting and request_irq() atomic issue commented in the
> code (thanks to Uwe Kleine-K?nig) despite other drivers do the same
> - removed (clean up) useless comment "return -ENOTSUP" from
> mctrl_gpio_request_irqs() inline function, it shouldn't return any
> error if serial_mctrl_gpio.h is just a stub, ie. CONFIG_GPIOLIB not defined
> - fixed mctrl_gpio_request_irqs(), mctrl_gpio_free_irqs(),
> mctrl_gpio_enable_ms(), mctrl_gpio_disable_ms:
> if "gpios" is NULL just return without any error. Serial drivers don't fail
> on probe(), just warns, if mctrl_gpio_init_dt() returns NULL or an error.
> - updated the documentation on mctrl_ helpers: Documentation/serial/driver
>
> ---
> Documentation/serial/driver | 22 +++++-
> drivers/tty/serial/serial_mctrl_gpio.c | 132 ++++++++++++++++++++++++++++++++-
> drivers/tty/serial/serial_mctrl_gpio.h | 61 ++++++++++++++-
> 3 files changed, 209 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/serial/driver b/Documentation/serial/driver
> index c415b0e..7a0811c 100644
> --- a/Documentation/serial/driver
> +++ b/Documentation/serial/driver
> @@ -439,7 +439,7 @@ Modem control lines via GPIO
>
> Some helpers are provided in order to set/get modem control lines via GPIO.
>
> -mctrl_gpio_init(dev, idx):
> +mctrl_gpio_init_dt(port, idx)
> This will get the {cts,rts,...}-gpios from device tree if they are
> present and request them, set direction etc, and return an
> allocated structure. devm_* functions are used, so there's no need
> @@ -453,8 +453,28 @@ mctrl_gpio_free(dev, gpios):
> mctrl_gpio_to_gpiod(gpios, gidx)
> This returns the gpio structure associated to the modem line index.
>
> +mctrl_gpio_is_gpio(gpios, gidx)
> + This returns true if the given modem line index is initialized and
> + handled by mtrl_gpio, ie. the line is a gpio line.
> +
> mctrl_gpio_set(gpios, mctrl):
> This will sets the gpios according to the mctrl state.
>
> mctrl_gpio_get(gpios, mctrl):
> This will update mctrl with the gpios values.
> +
> +mctrl_gpio_request_irqs(gpios)
> + This requests an interrupt of each input gpio line. The interupts
> + are set disabled and triggered on both edges.
> + It returns an error code or zero if success.
> +
> +mctrl_gpio_free_irqs(gpios)
> + This will free the requested interrupts of gpio lines.
> +
> +mctrl_gpio_enable_ms(gpios)
> + This enables modem status interrupts assigned to gpio lines.
> + It should be called by enable_ms() of an uart driver.
> +
> +mctrl_gpio_disable_ms(gpios)
> + This disables modem status interrupts assigned to gpio lines.
> + It should be called by disable_ms() of an uart driver.
> diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c
> index a38596c..7c43d15 100644
> --- a/drivers/tty/serial/serial_mctrl_gpio.c
> +++ b/drivers/tty/serial/serial_mctrl_gpio.c
> @@ -19,11 +19,15 @@
> #include <linux/device.h>
> #include <linux/gpio/consumer.h>
> #include <linux/termios.h>
> +#include <linux/irq.h>
>
> #include "serial_mctrl_gpio.h"
>
> struct mctrl_gpios {
> + struct uart_port *port;
> struct gpio_desc *gpio[UART_GPIO_MAX];
> + int irq[UART_GPIO_MAX];
> + unsigned int mctrl_prev;
> };
>
> static const struct {
> @@ -96,8 +100,9 @@ unsigned int mctrl_gpio_get(struct mctrl_gpios *gpios, unsigned int *mctrl)
> }
> EXPORT_SYMBOL_GPL(mctrl_gpio_get);
>
> -struct mctrl_gpios *mctrl_gpio_init(struct device *dev, unsigned int idx)
> +struct mctrl_gpios *mctrl_gpio_init_dt(struct uart_port *port, unsigned int idx)
> {
> + struct device *dev = port->dev;
> struct mctrl_gpios *gpios;
> enum mctrl_gpio_idx i;
> int err;
> @@ -106,6 +111,7 @@ struct mctrl_gpios *mctrl_gpio_init(struct device *dev, unsigned int idx)
> if (!gpios)
> return ERR_PTR(-ENOMEM);
>
> + gpios->port = port;
> for (i = 0; i < UART_GPIO_MAX; i++) {
> gpios->gpio[i] = devm_gpiod_get_index(dev,
> mctrl_gpios_desc[i].name,
> @@ -128,11 +134,13 @@ struct mctrl_gpios *mctrl_gpio_init(struct device *dev, unsigned int idx)
> devm_gpiod_put(dev, gpios->gpio[i]);
> gpios->gpio[i] = NULL;
> }
> + if (gpios->gpio[i] && !mctrl_gpios_desc[i].dir_out)
> + gpios->irq[i] = gpiod_to_irq(gpios->gpio[i]);
> }
>
> return gpios;
> }
> -EXPORT_SYMBOL_GPL(mctrl_gpio_init);
> +EXPORT_SYMBOL_GPL(mctrl_gpio_init_dt);
>
> void mctrl_gpio_free(struct device *dev, struct mctrl_gpios *gpios)
> {
> @@ -147,3 +155,123 @@ void mctrl_gpio_free(struct device *dev, struct mctrl_gpios *gpios)
> devm_kfree(dev, gpios);
> }
> EXPORT_SYMBOL_GPL(mctrl_gpio_free);
> +
> +/*
> + * Dealing with GPIO interrupt
> + */
> +#define MCTRL_ANY_DELTA (TIOCM_RI | TIOCM_DSR | TIOCM_CD | TIOCM_CTS)
> +static irqreturn_t mctrl_gpio_irq_handle(int irq, void *context)
> +{
> + struct mctrl_gpios *gpios = context;
> + struct uart_port *port = gpios->port;
> + u32 mctrl = gpios->mctrl_prev;
> + u32 mctrl_diff;
> +
> + mctrl_gpio_get(gpios, &mctrl);
> +
> + mctrl_diff = mctrl ^ gpios->mctrl_prev;
> + gpios->mctrl_prev = mctrl;
This function needs to hold port->lock, right? This should be
documented.
> + if (mctrl_diff & MCTRL_ANY_DELTA && port->state != NULL) {
> + if (mctrl_diff & TIOCM_RI)
> + port->icount.rng++;
> + if (mctrl_diff & TIOCM_DSR)
> + port->icount.dsr++;
> + if (mctrl_diff & TIOCM_CD)
> + uart_handle_dcd_change(port, mctrl & TIOCM_CD);
> + if (mctrl_diff & TIOCM_CTS)
> + uart_handle_cts_change(port, mctrl & TIOCM_CTS);
> +
> + wake_up_interruptible(&port->state->port.delta_msr_wait);
> + }
> +
> + return IRQ_HANDLED;
This should return IRQ_NONE if mctrl_diff is 0, doesn't it?
> +}
> +
> +int mctrl_gpio_request_irqs(struct mctrl_gpios *gpios)
> +{
> + struct uart_port *port = gpios->port;
> + int *irq = gpios->irq;
> + enum mctrl_gpio_idx i;
> + int err = 0;
> +
> + /* When gpios is NULL just gpio irqs are also never set
> + * so return without any error */
> + if (IS_ERR_OR_NULL(gpios))
> + return err;
I'd expect drivers using mctrl-gpio to error out if mctrl_gpio_init_dt
fails. Also mctrl_gpio_init_dt never returns NULL, so IMHO this check
can be dropped. (There are more places in mctrl-gpio that should be
fixed accordingly.
> + for (i = 0; i < UART_GPIO_MAX; i++) {
> + if (irq[i] <= 0)
> + continue;
> +
> + /*
> + * FIXME IRQ_NOAUTOEN:
> + * This is not undone in the error path. To be fixed
> + * properly this needs to be set atomically together with
> + * request_irq.
> + */
> + irq_set_status_flags(irq[i], IRQ_NOAUTOEN);
> + err = request_irq(irq[i], mctrl_gpio_irq_handle,
> + IRQ_TYPE_EDGE_BOTH,
> + dev_name(port->dev), gpios);
> + if (err) {
> + dev_err(port->dev, "%s: Can't get %d irq\n",
> + __func__, irq[i]);
The function name isn't interesting here. Please drop it. Now that the
irqs are requested with IRQ_NOAUTOEN is there a reason not to use
devm_request_irq?
> + break;
> + }
> + }
> +
> + /*
> + * If something went wrong, rollback avoiding
> + * negative enum values.
> + */
> + while (err && i > 0) {
> + i--;
> + if (irq[i] > 0)
> + free_irq(irq[i], gpios);
> + }
> +
> + return err;
> +}
> +EXPORT_SYMBOL_GPL(mctrl_gpio_request_irqs);
> +
> +void mctrl_gpio_free_irqs(struct mctrl_gpios *gpios)
> +{
> + enum mctrl_gpio_idx i;
> +
> + if (IS_ERR_OR_NULL(gpios))
> + return;
see above
> +
> + for (i = 0; i < UART_GPIO_MAX; i++)
> + if (gpios->irq[i] > 0)
> + free_irq(gpios->irq[i], gpios);
> +}
> +EXPORT_SYMBOL_GPL(mctrl_gpio_free_irqs);
> +
> +void mctrl_gpio_enable_ms(struct mctrl_gpios *gpios)
> +{
> + enum mctrl_gpio_idx i;
> +
> + if (IS_ERR_OR_NULL(gpios))
> + return;
ditto
> +
> + /* get initial status of modem lines GPIOs */
> + mctrl_gpio_get(gpios, &gpios->mctrl_prev);
> +
> + for (i = 0; i < UART_GPIO_MAX; i++)
> + if (gpios->irq[i] > 0)
> + enable_irq(gpios->irq[i]);
> +}
> +EXPORT_SYMBOL_GPL(mctrl_gpio_enable_ms);
> +
> +void mctrl_gpio_disable_ms(struct mctrl_gpios *gpios)
> +{
> + enum mctrl_gpio_idx i;
> +
> + if (IS_ERR_OR_NULL(gpios))
> + return;
> +
> + for (i = 0; i < UART_GPIO_MAX; i++)
> + if (gpios->irq[i] > 0)
> + disable_irq(gpios->irq[i]);
> +}
> +EXPORT_SYMBOL_GPL(mctrl_gpio_disable_ms);
> diff --git a/drivers/tty/serial/serial_mctrl_gpio.h b/drivers/tty/serial/serial_mctrl_gpio.h
> index 400ba04..1f79be3 100644
> --- a/drivers/tty/serial/serial_mctrl_gpio.h
> +++ b/drivers/tty/serial/serial_mctrl_gpio.h
> @@ -21,6 +21,7 @@
> #include <linux/err.h>
> #include <linux/device.h>
> #include <linux/gpio/consumer.h>
> +#include <linux/serial_core.h>
>
> enum mctrl_gpio_idx {
> UART_GPIO_CTS,
> @@ -40,6 +41,7 @@ enum mctrl_gpio_idx {
> */
> struct mctrl_gpios;
>
> +
drop this
> #ifdef CONFIG_GPIOLIB
>
> /*
> @@ -60,12 +62,13 @@ struct gpio_desc *mctrl_gpio_to_gpiod(struct mctrl_gpios *gpios,
> enum mctrl_gpio_idx gidx);
>
> /*
> - * Request and set direction of modem control lines GPIOs.
> + * Request and set direction of modem control lines GPIOs. DT is used.
> + * Initialize irq table for GPIOs.
> * devm_* functions are used, so there's no need to call mctrl_gpio_free().
> * Returns a pointer to the allocated mctrl structure if ok, -ENOMEM on
> * allocation error.
> */
> -struct mctrl_gpios *mctrl_gpio_init(struct device *dev, unsigned int idx);
> +struct mctrl_gpios *mctrl_gpio_init_dt(struct uart_port *port, unsigned int idx);
>
> /*
> * Free the mctrl_gpios structure.
> @@ -74,6 +77,28 @@ struct mctrl_gpios *mctrl_gpio_init(struct device *dev, unsigned int idx);
> */
> void mctrl_gpio_free(struct device *dev, struct mctrl_gpios *gpios);
>
> +/*
> + * Request irqs for input lines GPIOs. The irqs are set disabled
> + * and triggered on both edges.
> + * Returns zero if OK, otherwise an error code.
> + */
> +int mctrl_gpio_request_irqs(struct mctrl_gpios *gpios);
> +
> +/*
> + * Free irqs for input lines GPIOs.
> + */
> +void mctrl_gpio_free_irqs(struct mctrl_gpios *gpios);
> +
> +/*
> + * Disable modem status interrupts assigned to GPIOs
> + */
> +void mctrl_gpio_enable_ms(struct mctrl_gpios *gpios);
> +
> +/*
> + * Enable modem status interrupts assigned to GPIOs
> + */
> +void mctrl_gpio_disable_ms(struct mctrl_gpios *gpios);
> +
> #else /* GPIOLIB */
>
> static inline
> @@ -95,7 +120,7 @@ struct gpio_desc *mctrl_gpio_to_gpiod(struct mctrl_gpios *gpios,
> }
>
> static inline
> -struct mctrl_gpios *mctrl_gpio_init(struct device *dev, unsigned int idx)
> +struct mctrl_gpios *mctrl_gpio_init_dt(struct uart_port *port, unsigned int idx)
> {
> return ERR_PTR(-ENOSYS);
> }
> @@ -105,6 +130,36 @@ void mctrl_gpio_free(struct device *dev, struct mctrl_gpios *gpios)
> {
> }
>
> +static inline
> +int mctrl_gpio_request_irqs(struct mctrl_gpios *gpios)
> +{
> + return 0;
The gpiolib stubs return -ENOSYS in this case. Maybe this should be done
here, too?
> +}
> +
> +static inline
> +void mctrl_gpio_free_irqs(struct mctrl_gpios *gpios)
> +{
> +}
> +
> +static inline
> +void mctrl_gpio_enable_ms(struct mctrl_gpios *gpios)
> +{
> +}
> +
> +static inline
> +void mctrl_gpio_disable_ms(struct mctrl_gpios *gpios)
> +{
> +}
> +
> #endif /* GPIOLIB */
>
> +/*
> + * Return true if gidx is GPIO line, otherwise false.
> + */
> +static inline
> +bool mctrl_gpio_is_gpio(struct mctrl_gpios *gpios, enum mctrl_gpio_idx gidx)
> +{
> + return !IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(gpios, gidx));
If gpiod_get_optional was used to get the handles, we could drop this
ugly IS_ERR_OR_NULL here, too.
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3 2/4] serial: mxs-auart: Use helpers for gpio irqs
2015-02-06 16:55 ` [PATCH v3 2/4] serial: mxs-auart: Use helpers for gpio irqs Janusz Uzycki
@ 2015-02-12 13:40 ` Uwe Kleine-König
0 siblings, 0 replies; 7+ messages in thread
From: Uwe Kleine-König @ 2015-02-12 13:40 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Feb 06, 2015 at 05:55:33PM +0100, Janusz Uzycki wrote:
> The patch updates mxs-auart driver to use new mctrl_gpio helpers for
> gpio irqs. The code is much simpler now.
>
> Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl>
> ---
>
> Changes since RFC v2:
> - fixed comments in *_ms functions to match the coding style
> (credits to Uwe Kleine-K?nig)
> - rebased to next-20150204
>
> ---
> drivers/tty/serial/mxs-auart.c | 137 +++++------------------------------------
> 1 file changed, 17 insertions(+), 120 deletions(-)
>
> diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
> index d1298b6..5a8cf2c 100644
> --- a/drivers/tty/serial/mxs-auart.c
> +++ b/drivers/tty/serial/mxs-auart.c
> @@ -149,7 +149,6 @@ struct mxs_auart_port {
> #define MXS_AUART_DMA_RX_READY 3 /* bit 3 */
> #define MXS_AUART_RTSCTS 4 /* bit 4 */
> unsigned long flags;
> - unsigned int mctrl_prev;
> enum mxs_auart_type devtype;
>
> struct clk *clk;
> @@ -165,7 +164,6 @@ struct mxs_auart_port {
> void *rx_dma_buf;
>
> struct mctrl_gpios *gpios;
> - int gpio_irq[UART_GPIO_MAX];
> bool ms_irq_enabled;
> };
>
> @@ -431,29 +429,6 @@ static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
> mctrl_gpio_set(s->gpios, mctrl);
> }
>
> -#define MCTRL_ANY_DELTA (TIOCM_RI | TIOCM_DSR | TIOCM_CD | TIOCM_CTS)
> -static u32 mxs_auart_modem_status(struct mxs_auart_port *s, u32 mctrl)
> -{
> - u32 mctrl_diff;
> -
> - mctrl_diff = mctrl ^ s->mctrl_prev;
> - s->mctrl_prev = mctrl;
> - if (mctrl_diff & MCTRL_ANY_DELTA && s->ms_irq_enabled &&
> - s->port.state != NULL) {
> - if (mctrl_diff & TIOCM_RI)
> - s->port.icount.rng++;
> - if (mctrl_diff & TIOCM_DSR)
> - s->port.icount.dsr++;
> - if (mctrl_diff & TIOCM_CD)
> - uart_handle_dcd_change(&s->port, mctrl & TIOCM_CD);
> - if (mctrl_diff & TIOCM_CTS)
> - uart_handle_cts_change(&s->port, mctrl & TIOCM_CTS);
> -
> - wake_up_interruptible(&s->port.state->port.delta_msr_wait);
> - }
> - return mctrl;
> -}
> -
> static u32 mxs_auart_get_mctrl(struct uart_port *u)
> {
> struct mxs_auart_port *s = to_auart_port(u);
> @@ -481,18 +456,11 @@ static void mxs_auart_enable_ms(struct uart_port *port)
>
> s->ms_irq_enabled = true;
>
> - if (s->gpio_irq[UART_GPIO_CTS] >= 0)
> - enable_irq(s->gpio_irq[UART_GPIO_CTS]);
> - /* TODO: enable AUART_INTR_CTSMIEN otherwise */
> -
> - if (s->gpio_irq[UART_GPIO_DSR] >= 0)
> - enable_irq(s->gpio_irq[UART_GPIO_DSR]);
> -
> - if (s->gpio_irq[UART_GPIO_RI] >= 0)
> - enable_irq(s->gpio_irq[UART_GPIO_RI]);
> -
> - if (s->gpio_irq[UART_GPIO_DCD] >= 0)
> - enable_irq(s->gpio_irq[UART_GPIO_DCD]);
> + mctrl_gpio_enable_ms(s->gpios);
> + /*
> + * TODO: enable AUART_INTR_CTSMIEN if CTS isn't handled by
> + * mctrl_gpio.
> + */
> }
>
> /*
> @@ -510,18 +478,11 @@ static void mxs_auart_disable_ms(struct uart_port *port)
>
> s->ms_irq_enabled = false;
>
> - if (s->gpio_irq[UART_GPIO_CTS] >= 0)
> - disable_irq(s->gpio_irq[UART_GPIO_CTS]);
> - /* TODO: disable AUART_INTR_CTSMIEN otherwise */
> -
> - if (s->gpio_irq[UART_GPIO_DSR] >= 0)
> - disable_irq(s->gpio_irq[UART_GPIO_DSR]);
> -
> - if (s->gpio_irq[UART_GPIO_RI] >= 0)
> - disable_irq(s->gpio_irq[UART_GPIO_RI]);
> -
> - if (s->gpio_irq[UART_GPIO_DCD] >= 0)
> - disable_irq(s->gpio_irq[UART_GPIO_DCD]);
> + mctrl_gpio_disable_ms(s->gpios);
> + /*
> + * TODO: disable AUART_INTR_CTSMIEN if CTS isn't handled by
> + * mctrl_gpio.
> + */
> }
>
> static int mxs_auart_dma_prep_rx(struct mxs_auart_port *s);
> @@ -797,7 +758,6 @@ static irqreturn_t mxs_auart_irq_handle(int irq, void *context)
> {
> u32 istat;
> struct mxs_auart_port *s = context;
> - u32 mctrl_temp = s->mctrl_prev;
> u32 stat = readl(s->port.membase + AUART_STAT);
>
> istat = readl(s->port.membase + AUART_INTR);
> @@ -809,16 +769,6 @@ static irqreturn_t mxs_auart_irq_handle(int irq, void *context)
> | AUART_INTR_CTSMIS),
> s->port.membase + AUART_INTR_CLR);
>
> - /*
> - * Dealing with GPIO interrupt
> - */
> - if (irq == s->gpio_irq[UART_GPIO_CTS] ||
> - irq == s->gpio_irq[UART_GPIO_DCD] ||
> - irq == s->gpio_irq[UART_GPIO_DSR] ||
> - irq == s->gpio_irq[UART_GPIO_RI])
> - mxs_auart_modem_status(s,
> - mctrl_gpio_get(s->gpios, &mctrl_temp));
> -
> if (istat & AUART_INTR_CTSMIS) {
> if (CTS_AT_AUART() && s->ms_irq_enabled)
> uart_handle_cts_change(&s->port,
> @@ -883,9 +833,6 @@ static int mxs_auart_startup(struct uart_port *u)
> */
> writel(AUART_LINECTRL_FEN, u->membase + AUART_LINECTRL_SET);
>
> - /* get initial status of modem lines */
> - mctrl_gpio_get(s->gpios, &s->mctrl_prev);
> -
> s->ms_irq_enabled = false;
> return 0;
> }
> @@ -1155,71 +1102,23 @@ static int serial_mxs_probe_dt(struct mxs_auart_port *s,
> return 0;
> }
>
> -static bool mxs_auart_init_gpios(struct mxs_auart_port *s, struct device *dev)
> +static bool mxs_auart_init_gpios(struct mxs_auart_port *s)
> {
> - enum mctrl_gpio_idx i;
> - struct gpio_desc *gpiod;
> -
> - s->gpios = mctrl_gpio_init(dev, 0);
> + s->gpios = mctrl_gpio_init_dt(&s->port, 0);
> if (IS_ERR_OR_NULL(s->gpios))
mctrl_gpio_init_dt never returns NULL.
IMHO mxs_auart_init_gpios should return PTR_ERR(s->gpios) in this case.
(So the return type has to change.)
> return false;
>
> /* Block (enabled before) DMA option if RTS or CTS is GPIO line */
> if (!RTS_AT_AUART() || !CTS_AT_AUART()) {
> if (test_bit(MXS_AUART_RTSCTS, &s->flags))
> - dev_warn(dev,
> + dev_warn(s->dev,
> "DMA and flow control via gpio may cause some problems. DMA disabled!\n");
> clear_bit(MXS_AUART_RTSCTS, &s->flags);
> }
>
> - for (i = 0; i < UART_GPIO_MAX; i++) {
> - gpiod = mctrl_gpio_to_gpiod(s->gpios, i);
> - if (gpiod && (gpiod_get_direction(gpiod) == GPIOF_DIR_IN))
> - s->gpio_irq[i] = gpiod_to_irq(gpiod);
> - else
> - s->gpio_irq[i] = -EINVAL;
> - }
> -
> return true;
> }
>
> -static void mxs_auart_free_gpio_irq(struct mxs_auart_port *s)
> -{
> - enum mctrl_gpio_idx i;
> -
> - for (i = 0; i < UART_GPIO_MAX; i++)
> - if (s->gpio_irq[i] >= 0)
> - free_irq(s->gpio_irq[i], s);
> -}
> -
> -static int mxs_auart_request_gpio_irq(struct mxs_auart_port *s)
> -{
> - int *irq = s->gpio_irq;
> - enum mctrl_gpio_idx i;
> - int err = 0;
> -
> - for (i = 0; (i < UART_GPIO_MAX) && !err; i++) {
> - if (irq[i] < 0)
> - continue;
> -
> - irq_set_status_flags(irq[i], IRQ_NOAUTOEN);
> - err = request_irq(irq[i], mxs_auart_irq_handle,
> - IRQ_TYPE_EDGE_BOTH, dev_name(s->dev), s);
> - if (err)
> - dev_err(s->dev, "%s - Can't get %d irq\n",
> - __func__, irq[i]);
> - }
> -
> - /*
> - * If something went wrong, rollback.
> - */
> - while (err && (--i >= 0))
> - if (irq[i] >= 0)
> - free_irq(irq[i], s);
> -
> - return err;
> -}
> -
> static int mxs_auart_probe(struct platform_device *pdev)
> {
> const struct of_device_id *of_id =
> @@ -1262,8 +1161,6 @@ static int mxs_auart_probe(struct platform_device *pdev)
> s->port.type = PORT_IMX;
> s->port.dev = s->dev = &pdev->dev;
>
> - s->mctrl_prev = 0;
> -
> irq = platform_get_irq(pdev, 0);
> if (irq < 0)
> return irq;
> @@ -1276,14 +1173,14 @@ static int mxs_auart_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, s);
>
> - if (!mxs_auart_init_gpios(s, &pdev->dev))
> + if (!mxs_auart_init_gpios(s))
> dev_err(&pdev->dev,
> "Failed to initialize GPIOs. The serial port may not work as expected\n");
Better return failure in this case.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/4] tty: serial_mctrl_gpio: Add irqs helpers for input lines
2015-02-12 13:29 ` [PATCH v3 1/4] tty: serial_mctrl_gpio: Add irqs helpers for input lines Uwe Kleine-König
@ 2015-02-12 13:45 ` Alexander Shiyan
0 siblings, 0 replies; 7+ messages in thread
From: Alexander Shiyan @ 2015-02-12 13:45 UTC (permalink / raw)
To: linux-arm-kernel
???????, 12 ??????? 2015, 14:29 +01:00 ?? Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>:
...
> > +int mctrl_gpio_request_irqs(struct mctrl_gpios *gpios)
>
> > +{
>
> > + struct uart_port *port = gpios->port;
>
> > + int *irq = gpios->irq;
>
> > + enum mctrl_gpio_idx i;
>
> > + int err = 0;
>
> > +
>
> > + /* When gpios is NULL just gpio irqs are also never set
>
> > + * so return without any error */
>
> > + if (IS_ERR_OR_NULL(gpios))
>
> > + return err;
>
> I'd expect drivers using mctrl-gpio to error out if mctrl_gpio_init_dt
>
> fails. Also mctrl_gpio_init_dt never returns NULL, so IMHO this check
>
> can be dropped. (There are more places in mctrl-gpio that should be
>
> fixed accordingly.
At least this check should be moved to start of this function due
future usage of gpios->port and gpios->irq.
---
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-02-12 13:45 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-02-06 16:55 [PATCH v3 1/4] tty: serial_mctrl_gpio: Add irqs helpers for input lines Janusz Uzycki
2015-02-06 16:55 ` [PATCH v3 2/4] serial: mxs-auart: Use helpers for gpio irqs Janusz Uzycki
2015-02-12 13:40 ` Uwe Kleine-König
2015-02-06 16:55 ` [PATCH v3 3/4] serial: at91: " Janusz Uzycki
2015-02-06 16:55 ` [PATCH v3 4/4] serial: clps711x: Update to new mctrl_gpio_init_dt Janusz Uzycki
2015-02-12 13:29 ` [PATCH v3 1/4] tty: serial_mctrl_gpio: Add irqs helpers for input lines Uwe Kleine-König
2015-02-12 13:45 ` Alexander Shiyan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).