On Mon, May 23, 2016 at 11:09 AM, Uwe Kleine-König
<u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
> 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.
Let's keep everything as before and only check for (gpios == NULL),
that is then indicator, that MCTRL_GPIO API wasn't initialized.
>> 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, ...
OK
>> @@ -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.
OK. What do think about such workaround in
drivers/tty/serial/8250/8250_core.c for not breaking the UART
initialization, when GPIOLIB is disabled?
if (IS_ERR(uart->gpios) && IS_ENABLED(CONFIG_GPIOLIB))
return real error;
Yegor
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
