[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-ID: <cb09a49a-37f8-9e3f-168c-4c5dd62e2c07@foss.st.com>
Date: Fri, 4 Feb 2022 16:41:58 +0100
From: Erwan LE RAY <erwan.leray@...s.st.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Jiri Slaby <jirislaby@...nel.org>,
Maxime Coquelin <mcoquelin.stm32@...il.com>,
Alexandre Torgue <alexandre.torgue@...s.st.com>,
Gerald Baeza <gerald.baeza@...com>,
Valentin Caron <valentin.caron@...s.st.com>,
"linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
"linux-stm32@...md-mailman.stormreply.com"
<linux-stm32@...md-mailman.stormreply.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] serial: mctrl_gpio: add a new API to enable / disable
wake_irq
Hi Andy,
On 2/4/22 10:07 AM, Andy Shevchenko wrote:
>
>
> On Thursday, February 3, 2022, Erwan Le Ray <erwan.leray@...s.st.com
> <mailto:erwan.leray@...s.st.com>> wrote:
>
> Add a new API to enable / disable wake_irq in order to enable gpio
> irqs as
> wakeup irqs for the uart port.
>
> Signed-off-by: Erwan Le Ray <erwan.leray@...s.st.com
> <mailto:erwan.leray@...s.st.com>>
>
> diff --git a/drivers/tty/serial/serial_mctrl_gpio.c
> b/drivers/tty/serial/serial_mctrl_gpio.c
> index c41d8911ce95..1663b3afc3a0 100644
> --- a/drivers/tty/serial/serial_mctrl_gpio.c
> +++ b/drivers/tty/serial/serial_mctrl_gpio.c
> @@ -299,4 +299,42 @@ void mctrl_gpio_disable_ms(struct mctrl_gpios
> *gpios)
> }
> EXPORT_SYMBOL_GPL(mctrl_gpio_disable_ms);
>
> +void mctrl_gpio_enable_irq_wake(struct mctrl_gpios *gpios)
> +{
> + enum mctrl_gpio_idx i;
> +
> + if (!gpios)
> + return;
> +
> + if (!gpios->mctrl_on)
> + return;
> +
> + for (i = 0; i < UART_GPIO_MAX; ++i) {
> + if (!gpios->irq[i])
> + continue;
>
>
>
> Why not simply
>
> if (gpios[])
> enable_irq_...
>
> ?
>
> And same for disabling.
>
> +
> + enable_irq_wake(gpios->irq[i]);
> + }
> +}
> +EXPORT_SYMBOL_GPL(mctrl_gpio_enable_irq_wake);
> +
> +void mctrl_gpio_disable_irq_wake(struct mctrl_gpios *gpios)
> +{
> + enum mctrl_gpio_idx i;
> +
> + if (!gpios)
> + return;
> +
> + if (!gpios->mctrl_on)
> + return;
> +
> + for (i = 0; i < UART_GPIO_MAX; ++i) {
> + if (!gpios->irq[i])
> + continue;
> +
> + disable_irq_wake(gpios->irq[i]);
> + }
> +}
> +EXPORT_SYMBOL_GPL(mctrl_gpio_disable_irq_wake);
> +
> MODULE_LICENSE("GPL");
> diff --git a/drivers/tty/serial/serial_mctrl_gpio.h
> b/drivers/tty/serial/serial_mctrl_gpio.h
> index b134a0ffc894..fc76910fb105 100644
> --- a/drivers/tty/serial/serial_mctrl_gpio.h
> +++ b/drivers/tty/serial/serial_mctrl_gpio.h
> @@ -91,6 +91,16 @@ void mctrl_gpio_enable_ms(struct mctrl_gpios *gpios);
> */
> void mctrl_gpio_disable_ms(struct mctrl_gpios *gpios);
>
> +/*
> + * Enable gpio wakeup interrupts to enable wake up source.
> + */
> +void mctrl_gpio_enable_irq_wake(struct mctrl_gpios *gpios);
> +
> +/*
> + * Disable gpio wakeup interrupts to enable wake up source.
> + */
> +void mctrl_gpio_disable_irq_wake(struct mctrl_gpios *gpios);
> +
> #else /* GPIOLIB */
>
> static inline
> @@ -142,6 +152,14 @@ static inline void mctrl_gpio_disable_ms(struct
> mctrl_gpios *gpios)
> {
> }
>
> +static inline void mctrl_gpio_enable_irq_wake(struct mctrl_gpios
> *gpios)
> +{
> +}
> +
> +static inline void mctrl_gpio_disable_irq_wake(struct mctrl_gpios
> *gpios)
> +{
> +}
> +
> #endif /* GPIOLIB */
>
> #endif
> --
> 2.17.1
>
>
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Thanks for your review.
I fully agree with your comment, but I wrote this code like it is to
keep the same structure than all the other ops of serial_mcrtrl_gpio
driver. I preferred keeping an homogeneous code in the driver rather
than breaking the driver homogeneity with the addition of an optimized code.
Greg, can you please indicate which solution you recommend ?
Cheers, Erwan.
Powered by blists - more mailing lists