MIME-Version: 1.0
References: <20211215213300.4778-1-linux@fw-web.de> <20211215213300.4778-2-linux@fw-web.de>
 <fe895d71-5f1f-daf0-bde8-c8ab5f4c0128@arm.com> <trinity-18dddf5d-bc73-453d-a9eb-954de1ab7cc7-1639672581601@3c-app-gmx-bap16>
In-Reply-To: <trinity-18dddf5d-bc73-453d-a9eb-954de1ab7cc7-1639672581601@3c-app-gmx-bap16>
From:   Peter Geis <pgwipeout@gmail.com>
Date:   Thu, 16 Dec 2021 21:21:20 -0500
Message-ID: <CAMdYzYpqtJ9E+NJ7Ra9rdMPj5idjrRvQ3=+N+ye7MTLuQtzJSw@mail.gmail.com>
Subject: Re: Re: [PATCH 1/2] mfd: rk808: add reboot support to rk808 pmic
To:     Frank Wunderlich <frank-w@public-files.de>
Cc:     Robin Murphy <robin.murphy@arm.com>,
        Frank Wunderlich <linux@fw-web.de>,
        Lee Jones <lee.jones@linaro.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "open list:ARM/Rockchip SoC..." <linux-rockchip@lists.infradead.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Dec 16, 2021 at 11:36 AM Frank Wunderlich
<frank-w@public-files.de> wrote:
>
> Hi Robin
>
> thanks for your review

Good Evening,

>
> > Gesendet: Donnerstag, 16. Dezember 2021 um 14:17 Uhr
> > Von: "Robin Murphy" <robin.murphy@arm.com>
>
> > > +   dev_err(&rk808_i2c_client->dev, "poweroff device!\n");
> >
> > This is not an error.
>
> ack, we can change to dev_dbg/dev_info or drop completely

Yes, this was set to isolate down when individuals were having issues
on a specific distro that unfortunately defaults to a silent boot.
It can be dropped at this point.

>
> > > @@ -552,6 +554,7 @@ static void rk808_pm_power_off(void)
> > >             bit = DEV_OFF;
> > >             break;
> > >     default:
> > > +           dev_err(&rk808_i2c_client->dev, "poweroff device not supported!\n");
> >
> > If we really don't support power off for the present PMIC then we should
> > avoid registering the power off handler in the first place. These
> > default cases should mostly just serve to keep static checkers happy.
>
> currently we had added all device-ids supported by probe. Before my Patch (Part2) rk809 was missing.
> It is not registered for (currently) unsupported IDs as probe exits with -EINVAL for unsupported IDs before.
>
> If anyone adds init-code in future but not poweroff/restart part we ran into this. So i would leave this message.
>
> registering handler depending on supported devices (for poweroff/restart function) imho makes code less readable.

I added that just in case someone put in support for a new device
without checking everything.
I don't have any particular attachment to it however.

>
> > > @@ -559,6 +562,44 @@ static void rk808_pm_power_off(void)
> > >             dev_err(&rk808_i2c_client->dev, "Failed to shutdown device!\n");
> > >   }
> > >
> > > +static int rk808_restart_notify(struct notifier_block *this, unsigned long mode, void *cmd)
> > > +{
> > > +   int ret;
> > > +   unsigned int reg, bit;
> > > +   struct rk808 *rk808 = i2c_get_clientdata(rk808_i2c_client);
> > > +
> > > +   switch (rk808->variant) {
> > > +   case RK805_ID:
> > > +           reg = RK805_DEV_CTRL_REG;
> > > +           bit = DEV_OFF_RST;
> > > +           break;
> > > +   case RK808_ID:
> > > +           reg = RK808_DEVCTRL_REG,
> > > +           bit = DEV_OFF;
> >
> > Is that right? The RK808 datasheet does say that this bit resets itself
> > once the OFF state is reached, but there doesn't seem to be any obvious
> > guarantee of a power-on condition being present to automatically
> > transition back to ACTIVE.
>
> i think you're right...imho it should be DEV_OFF_RST in restart_notify and DEV_OFF in poweroff

I've tested this on the rockpro64, DEV_OFF_RST and DEV_OFF both power
down the rk808.
The difference is the DEV_OFF_RST "activate reset of the digital core".
This is similar to the description of pressing the RESET key, with the
exception of not powering up the PMIC.
It seems the rk808 doesn't support software resetting at all per the trm.
We should drop reset support for the rk808.
The only way I can see triggering a reset in the rk808 being possible
is by setting the RTC alarm to wake the device shortly after powering
down.

>
> > > @@ -727,6 +768,13 @@ static int rk808_probe(struct i2c_client *client,
> > >     if (of_property_read_bool(np, "rockchip,system-power-controller")) {
> > >             rk808_i2c_client = client;
> > >             pm_power_off = rk808_pm_power_off;
> > > +           rk808->nb = &rk808_restart_handler;
> >
> > The handler just relies on the global rk808_i2c_client anyway, so does
> > this serve any purpose?
>
> i guess
>
> ret = register_restart_handler(&rk808_restart_handler);
>
> is better here too.
>
> i cannot figure out why storing the function-pointer in rk808->nb too as this seems not to be used anywhere else

I can't remember why I set this up like this, probably "borrowed" from
another driver somewhere.
Go ahead and change it.

>
> > > +
> > > +           dev_warn(&client->dev, "register restart handler\n");
> >
> > Don't scream a warning about normal and expected operation.
>
> ack

This was to check that it was in fact registering, drop it.

>
> > Thanks,
> > Robin.
>
> @Peter, what do you think?
>
> regards Frank

Thank you for the review Robin and the submission Frank.

Always,
Peter