Hello Yegor,
On Mon, May 23, 2016 at 09:30:57AM +0200, yegorslists@xxxxxxxxxxxxxx wrote:
> From: Yegor Yefremov <yegorslists@xxxxxxxxxxxxxx>
>
> This workaround is needed for the cases, where mctrl_gpio API is used
> before mctrl_gpio_init() was invoked. This happens in 8250 during
> console initialization, as the driver sets DTR signal.
>
> Return NULL in case of initialization errors or absence of GPIOLIB.
> This way the above workaround can be safely used.
My sentiments for this patch didn't change: I don't like it. Better make
sure that you already called mctrl_gpio_init before you call any other
mctrl_gpio_* function. Otherwise you prepare bad surprises.
> Signed-off-by: Yegor Yefremov <yegorslists@xxxxxxxxxxxxxx>
> ---
> drivers/tty/serial/serial_mctrl_gpio.c | 24 +++++++++++++++++++++---
> drivers/tty/serial/serial_mctrl_gpio.h | 2 +-
> 2 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c
> index a868595..35588c5 100644
> --- a/drivers/tty/serial/serial_mctrl_gpio.c
> +++ b/drivers/tty/serial/serial_mctrl_gpio.c
> @@ -52,6 +52,9 @@ void mctrl_gpio_set(struct mctrl_gpios *gpios, unsigned int mctrl)
> int value_array[UART_GPIO_MAX];
> unsigned int count = 0;
>
> + if (gpios == NULL)
> + return;
> +
> for (i = 0; i < UART_GPIO_MAX; i++)
> if (gpios->gpio[i] && mctrl_gpios_desc[i].dir_out) {
> desc_array[count] = gpios->gpio[i];
> @@ -73,6 +76,9 @@ unsigned int mctrl_gpio_get(struct mctrl_gpios *gpios, unsigned int *mctrl)
> {
> enum mctrl_gpio_idx i;
>
> + if (gpios == NULL)
> + return *mctrl;
> +
> for (i = 0; i < UART_GPIO_MAX; i++) {
> if (gpios->gpio[i] && !mctrl_gpios_desc[i].dir_out) {
> if (gpiod_get_value(gpios->gpio[i]))
> @@ -91,6 +97,9 @@ mctrl_gpio_get_outputs(struct mctrl_gpios *gpios, unsigned int *mctrl)
> {
> enum mctrl_gpio_idx i;
>
> + if (gpios == NULL)
> + return *mctrl;
> +
> for (i = 0; i < UART_GPIO_MAX; i++) {
> if (gpios->gpio[i] && mctrl_gpios_desc[i].dir_out) {
> if (gpiod_get_value(gpios->gpio[i]))
I could live with the changes above, ...
> @@ -178,7 +187,7 @@ struct mctrl_gpios *mctrl_gpio_init(struct uart_port *port, unsigned int idx)
>
> gpios = mctrl_gpio_init_noauto(port->dev, idx);
> if (IS_ERR(gpios))
> - return gpios;
> + return NULL;
..., but this one is definitely bogus. If the UART is probed before the
GPIO driver, you want to pass -EPROBE_DEFER. If the GPIO is already
requested, you also want to forward the error.
> gpios->port = port;
>
> @@ -193,7 +202,7 @@ struct mctrl_gpios *mctrl_gpio_init(struct uart_port *port, unsigned int idx)
> dev_err(port->dev,
> "failed to find corresponding irq for %s (idx=%d, err=%d)\n",
> mctrl_gpios_desc[i].name, idx, ret);
> - return ERR_PTR(ret);
> + return NULL;
Ditto, if a GPIO is used as handshake line and it doesn't have a
corresponding irq, polling must be set up instead of simply ignoring the
missing irq.
> }
> gpios->irq[i] = ret;
>
> @@ -209,7 +218,7 @@ struct mctrl_gpios *mctrl_gpio_init(struct uart_port *port, unsigned int idx)
> dev_err(port->dev,
> "failed to request irq for %s (idx=%d, err=%d)\n",
> mctrl_gpios_desc[i].name, idx, ret);
> - return ERR_PTR(ret);
> + return NULL;
ditto.
> diff --git a/drivers/tty/serial/serial_mctrl_gpio.h b/drivers/tty/serial/serial_mctrl_gpio.h
> index fa000bc..82c5fbf 100644
> --- a/drivers/tty/serial/serial_mctrl_gpio.h
> +++ b/drivers/tty/serial/serial_mctrl_gpio.h
> @@ -130,7 +130,7 @@ struct gpio_desc *mctrl_gpio_to_gpiod(struct mctrl_gpios *gpios,
> static inline
> struct mctrl_gpios *mctrl_gpio_init(struct uart_port *port, unsigned int idx)
> {
> - return ERR_PTR(-ENOSYS);
> + return NULL;
With the same reasoning that gpiod_get_optional returns -ENOSYS without
GPIOLIB, this also should return -ENOSYS.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
