Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230808072625.2109564-1-nick.hu@sifive.com> <12c1e981-b664-b80c-d0df-7e7a098e32b1@codethink.co.uk>
In-Reply-To: <12c1e981-b664-b80c-d0df-7e7a098e32b1@codethink.co.uk>
From:   Nick Hu <nick.hu@sifive.com>
Date:   Wed, 9 Aug 2023 10:10:19 +0800
Message-ID: <CAKddAkDBQSTf1Z9QDUghmV8GoyuuUYfz+T+rJPoMZ2qtd=TGfQ@mail.gmail.com>
Subject: Re: [PATCH 1/2] serial: sifive: Add suspend and resume operations
To:     Ben Dooks <ben.dooks@codethink.co.uk>
Cc:     zong.li@sifive.com, gregkh@linuxfoundation.org,
        jirislaby@kernel.org, palmer@dabbelt.com, paul.walmsley@sifive.com,
        linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-riscv@lists.infradead.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Ben


On Tue, Aug 8, 2023 at 4:47=E2=80=AFPM Ben Dooks <ben.dooks@codethink.co.uk=
> 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@sifive.com>
> > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>
> ^ 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 =3D 0;
> > +     struct sifive_serial_port *ssp =3D 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 !=3D PORT_UNKNOWN)
> > +             ret =3D uart_suspend_port(&sifive_serial_uart_driver, &ss=
p->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 =3D 0;
> > +     struct sifive_serial_port *ssp =3D dev_get_drvdata(dev);
> > +
> > +     if (ssp && ssp->port.type !=3D PORT_UNKNOWN)
> > +             ret =3D 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[] =3D {
> >       { .compatible =3D "sifive,fu540-c000-uart0" },
> >       { .compatible =3D "sifive,uart0" },
> > @@ -1034,6 +1059,7 @@ static struct platform_driver sifive_serial_platf=
orm_driver =3D {
> >       .remove         =3D sifive_serial_remove,
> >       .driver         =3D {
> >               .name   =3D SIFIVE_SERIAL_NAME,
> > +             .pm =3D pm_sleep_ptr(&sifive_uart_pm_ops),
> >               .of_match_table =3D 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
>