Messages in this thread | | | Subject | Re: [PATCH 1/2] serial: mctrl_gpio: add a new API to enable / disable wake_irq | From | Erwan LE RAY <> | Date | Fri, 4 Feb 2022 16:41:58 +0100 |
| |
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.
| |