| Message ID | 20220203171644.12231-2-erwan.leray@foss.st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | STM32 enable gpio irqs as wakeup irqs for uart port | expand |
Hi Andy, On 2/4/22 10:07 AM, Andy Shevchenko wrote: > > > On Thursday, February 3, 2022, Erwan Le Ray <erwan.leray@foss.st.com > <mailto:erwan.leray@foss.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@foss.st.com > <mailto:erwan.leray@foss.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.
On Fri, Feb 04, 2022 at 04:41:58PM +0100, Erwan LE RAY wrote: > Hi Andy, > > On 2/4/22 10:07 AM, Andy Shevchenko wrote: > > > > > > On Thursday, February 3, 2022, Erwan Le Ray <erwan.leray@foss.st.com > > <mailto:erwan.leray@foss.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@foss.st.com > > <mailto:erwan.leray@foss.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 ? Sadly, this is the format in this file, so I'll take this as-is. thanks, greg k-h
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; + + 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
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@foss.st.com>