Re: [PATCH 1/2] serial: sifive: Add suspend and resume operations

[Nick Hu]

Hi Ben



On Tue, Aug 8, 2023 at 4:47 PM Ben Dooks <ben.dooks@xxxxxxxxxxxxxxx> wrote:
>
> On 08/08/2023 08:26, Nick Hu wrote:
> > If the Sifive Uart is not used as the wake up source, suspend the uart
> > before the system enter the suspend state to prevent it woken up by
> > unexpected uart interrupt. Resume the uart once the system woken up.
> >
> > Signed-off-by: Nick Hu <nick.hu@xxxxxxxxxx>
> > Signed-off-by: Ben Dooks <ben.dooks@xxxxxxxxxxxxxxx>
>
> ^ This should be Reviewed-by, as I did review on this earlier.
>
I'll correct it in V2
> > ---
> >   drivers/tty/serial/sifive.c | 26 ++++++++++++++++++++++++++
> >   1 file changed, 26 insertions(+)
> >
> > diff --git a/drivers/tty/serial/sifive.c b/drivers/tty/serial/sifive.c
> > index a19db49327e2..87994cb69007 100644
> > --- a/drivers/tty/serial/sifive.c
> > +++ b/drivers/tty/serial/sifive.c
> > @@ -1022,6 +1022,31 @@ static int sifive_serial_remove(struct platform_device *dev)
> >       return 0;
> >   }
> >
> > +static int sifive_serial_suspend(struct device *dev)
> > +{
> > +     int ret = 0;
> > +     struct sifive_serial_port *ssp = dev_get_drvdata(dev);
>
> Minor annyonance is ordering of ssp and ret, I think the showrter one
> last is the nicest looking.
>
> > +     if (ssp && ssp->port.type != PORT_UNKNOWN)
> > +             ret = uart_suspend_port(&sifive_serial_uart_driver, &ssp->port);
>
> Do we really need a test for ssp being valid if the device is bound.
> Not sure if the port.type is also useful?
>
You are right. This might be an unnecessary check.
The ssp is bound in the probe function and the PORT_UNKNOWN only be
set when we do the sifive_serial_remove().
And for the ordering of ret, We don't need the ret if we move the check.
We can just return uart_suspend_port(&sifive_serial_uart_driver, &ssp->port);

Will correct it in V2.
> > +     return ret;
> > +}
> > +
> > +static int sifive_serial_resume(struct device *dev)
> > +{
> > +     int ret = 0;
> > +     struct sifive_serial_port *ssp = dev_get_drvdata(dev);
> > +
> > +     if (ssp && ssp->port.type != PORT_UNKNOWN)
> > +             ret = uart_resume_port(&sifive_serial_uart_driver, &ssp->port);
> > +
> > +     return ret;
> > +}
> > +
> > +DEFINE_SIMPLE_DEV_PM_OPS(sifive_uart_pm_ops, sifive_serial_suspend,
> > +                      sifive_serial_resume);
> > +
> >   static const struct of_device_id sifive_serial_of_match[] = {
> >       { .compatible = "sifive,fu540-c000-uart0" },
> >       { .compatible = "sifive,uart0" },
> > @@ -1034,6 +1059,7 @@ static struct platform_driver sifive_serial_platform_driver = {
> >       .remove         = sifive_serial_remove,
> >       .driver         = {
> >               .name   = SIFIVE_SERIAL_NAME,
> > +             .pm = pm_sleep_ptr(&sifive_uart_pm_ops),
> >               .of_match_table = of_match_ptr(sifive_serial_of_match),
> >       },
> >   };
>
> --
> Ben Dooks                               http://www.codethink.co.uk/
> Senior Engineer                         Codethink - Providing Genius
>
> https://www.codethink.co.uk/privacy.html
>