Re: [PATCH] Input: snvs_pwrkey - Add clk handling - Uwe Kleine-König

From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: "André Draszik" <git@andred.net>,
	"Shawn Guo" <shawnguo@kernel.org>,
	"NXP Linux Team" <linux-imx@nxp.com>,
	kernel@pengutronix.de, linux-input@vger.kernel.org,
	"Rob Herring" <robh@kernel.org>
Subject: Re: [PATCH] Input: snvs_pwrkey - Add clk handling
Date: Tue, 12 Oct 2021 09:39:59 +0200	[thread overview]
Message-ID: <20211012073959.4wnkarxwtrmuxtuz@pengutronix.de> (raw)
In-Reply-To: <YWTpg35wyYS1uoFZ@google.com>

[-- Attachment #1: Type: text/plain, Size: 3721 bytes --]

Hello Dmitry,

On Mon, Oct 11, 2021 at 06:48:51PM -0700, Dmitry Torokhov wrote:
> On Tue, Oct 05, 2021 at 10:00:05PM +0200, Uwe Kleine-König wrote:
> > On Wed, Sep 22, 2021 at 11:43:00AM +0200, Uwe Kleine-König wrote:
> > > On i.MX7S and i.MX8M* (but not i.MX6*) the pwrkey device has an
> > > associated clock. Accessing the registers requires that this clock is
> > > enabled. Binding the driver on at least i.MX7S and i.MX8MP while not
> > > having the clock enabled results in a complete hang of the machine.
> > > (This usually only happens if snvs_pwrkey is built as a module and the
> > > rtc-snvs driver isn't already bound because at bootup the required clk
> > > is on and only gets disabled when the clk framework disables unused clks
> > > late during boot.)
> > > 
> > > This completes the fix in commit 135be16d3505 ("ARM: dts: imx7s: add
> > > snvs clock to pwrkey").
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > 
> > This patch fixes a hard machine hang that occurs on an i.MX8MP based
> > machine in ~10% of the boot ups. In my eyes it's suitable to be applied
> > before v5.14 even.
> > 
> > Any feedback on it?
> 
> Sorry for the delay. As you may know I strongly dislike dev_err_probe()
> as it conflates the 2 issue - error printing and noting the deferral
> event that should be implemented by the resource providers (and I
> believe Rob had WIP patches to push this reporting down too providers).

I didn't know your dislike (and I probably will forget it again soon,
given that there seems to be disagreement among maintainers :-), and
from your words I don't understand it. The improved idea is that
devm_clk_get_optional() already registers the deferral event for the
clk? My first intuition is that this won't work, so I'd like to see the
WIP series. (Added Rob to Cc.) Someone has a link?

Also I don't share that sentiment, given that today
devm_clk_get_optional() and all the other resource providers don't do
the necessary stuff for deferral handling, I strongly prefer to use the
mechanism that is available today (even if it might be possible to
improve it) instead of open coding it. And if it's only because once the
improved variant is available it's easier to identify the code locations
that need adaption if they all use a common function instead of
identifying something like

	clk = devm_clk_get_optional(&pdev->dev, NULL);
	if (IS_ERR(clk)) {
		error = PTR_ERR(clk);
		if (error != -EPROBE_DEFER)
			dev_err(pdev->dev, "Failed to get clk: %pe\n", clk)
		else
			device_set_deferred_probe_reason(dev, oh_I_need_a_struct_va_format_how_do_I_get_this?);
		return error;
	}

instead of

	clk = devm_clk_get_optional(&pdev->dev, NULL);
	if (IS_ERR(clk))
		return dev_err_probe(&pdev->dev, PTR_ERR(clk), "Failed to get clock\n");

Even if the driver does not call device_set_deferred_probe_reason(), the
additional check for error != -EPROBE_DEFER is ugly, isn't it?

> Could you p lease resubmit with "normal" dev_err()/dev_warn()/etc and I
> will be happy to apply.

Is the above the variant you prefer? Maybe without the call to
device_set_deferred_probe_reason()? Or maybe even without the check for
-EPROBE_DEFER (which however might result in wrong error messages which
is IMHO worse than the ugliness of the additional check)?

Please advice. Given that adding clk handling prevents a machine hang,
I'm willing to add it in the way you prefer, even if I don't agree to
your reasoning.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]