MIME-Version: 1.0
Message-ID: <trinity-18dddf5d-bc73-453d-a9eb-954de1ab7cc7-1639672581601@3c-app-gmx-bap16>
From:   Frank Wunderlich <frank-w@public-files.de>
To:     Robin Murphy <robin.murphy@arm.com>,
        Peter Geis <pgwipeout@gmail.com>
Cc:     Frank Wunderlich <linux@fw-web.de>,
        Lee Jones <lee.jones@linaro.org>, linux-kernel@vger.kernel.org,
        linux-rockchip@lists.infradead.org
Subject: Aw: Re: [PATCH 1/2] mfd: rk808: add reboot support to rk808 pmic
Content-Type: text/plain; charset=UTF-8
Date:   Thu, 16 Dec 2021 17:36:21 +0100
Importance: normal
Sensitivity: Normal
In-Reply-To: <fe895d71-5f1f-daf0-bde8-c8ab5f4c0128@arm.com>
References: <20211215213300.4778-1-linux@fw-web.de>
 <20211215213300.4778-2-linux@fw-web.de>
 <fe895d71-5f1f-daf0-bde8-c8ab5f4c0128@arm.com>
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Robin

thanks for your review

> 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

> > @@ -552,6 +554,7 @@ static void rk808_pm_power_off(void)
> >   		bit =3D 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 in=
to this. So i would leave this message.

registering handler depending on supported devices (for poweroff/restart f=
unction) imho makes code less readable.

> > @@ -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 =3D i2c_get_clientdata(rk808_i2c_client);
> > +
> > +	switch (rk808->variant) {
> > +	case RK805_ID:
> > +		reg =3D RK805_DEV_CTRL_REG;
> > +		bit =3D DEV_OFF_RST;
> > +		break;
> > +	case RK808_ID:
> > +		reg =3D RK808_DEVCTRL_REG,
> > +		bit =3D 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

> > @@ -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 =3D client;
> >   		pm_power_off =3D rk808_pm_power_off;
> > +		rk808->nb =3D &rk808_restart_handler;
>
> The handler just relies on the global rk808_i2c_client anyway, so does
> this serve any purpose?

i guess

ret =3D register_restart_handler(&rk808_restart_handler);

is better here too.

i cannot figure out why storing the function-pointer in rk808->nb too as t=
his seems not to be used anywhere else

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

ack

> Thanks,
> Robin.

@Peter, what do you think?

regards Frank