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@xxxxxxxxxxx > > <mailto:erwan.leray@xxxxxxxxxxx>> 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@xxxxxxxxxxx > > <mailto:erwan.leray@xxxxxxxxxxx>> > > > > 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
