linux-kernel - Re: [PATCH 1/2] input: keyboard: snvs_pwrkey: Enable clock when accessing HP* registers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ZrpH8lXG4wkORlyA@google.com>
Date: Mon, 12 Aug 2024 10:35:46 -0700
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Frank Li <Frank.Li@....com>
Cc: Joy Zou <joy.zou@....com>, Dong Aisheng <aisheng.dong@....com>,
	Robin Gong <yibin.gong@....com>,
	"open list:INPUT (KEYBOARD, MOUSE, JOYSTICK, TOUCHSCREEN)..." <linux-input@...r.kernel.org>,
	open list <linux-kernel@...r.kernel.org>, imx@...ts.linux.dev
Subject: Re: [PATCH 1/2] input: keyboard: snvs_pwrkey: Enable clock when
 accessing HP* registers

Hi Frank,

On Tue, Jul 16, 2024 at 04:56:09PM -0400, Frank Li wrote:
> From: Robin Gong <yibin.gong@....com>
> 
> SNVS requires two clock sources:
> - LP (32k always on): All LP* registers need this clock. No management is
> needed as it is always on.
> - HP: All HP* registers require this clock to be enabled before accessing
> these registers. Some platforms (e.g., i.MX6SX/i.MX6UL) do not have a
> separate HP root clock and it is always on.
> 
> Add an optional "snvs-pwrkey" clock for the HP clock and enable it only
> when accessing HP* registers.
> 
> Signed-off-by: Robin Gong <yibin.gong@....com>
> Signed-off-by: Joy Zou <joy.zou@....com>
> Signed-off-by: Dong Aisheng <aisheng.dong@....com>
> Signed-off-by: Frank Li <Frank.Li@....com>
> ---
> - clock name snvs-pwrkey already documented at
> Documentation/devicetree/bindings/crypto/fsl,sec-v4.0-mon.yaml
> ---
>  drivers/input/keyboard/snvs_pwrkey.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
> index ad8660be0127c..b352148a0cfb2 100644
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c
> @@ -37,6 +37,7 @@ struct pwrkey_drv_data {
>  	int keycode;
>  	int keystate;  /* 1:pressed */
>  	int wakeup;
> +	struct clk *clk;
>  	struct timer_list check_timer;
>  	struct input_dev *input;
>  	u8 minor_rev;
> @@ -48,7 +49,10 @@ static void imx_imx_snvs_check_for_events(struct timer_list *t)
>  	struct input_dev *input = pdata->input;
>  	u32 state;
>  
> +	clk_prepare_enable(pdata->clk);

We are in timer context here, "prepare" is not allowed here. Can we
prepare the clock once and enable it as needed. Does it even need to be
disabled? Can it be also always on?

If you want enable/disable then error handling is needed.

>  	regmap_read(pdata->snvs, SNVS_HPSR_REG, &state);
> +	clk_disable_unprepare(pdata->clk);
> +
>  	state = state & SNVS_HPSR_BTN ? 1 : 0;
>  
>  	/* only report new event if status changed */
> @@ -169,7 +173,15 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
>  	if (pdata->irq < 0)
>  		return -EINVAL;
>  
> +	pdata->clk = devm_clk_get_optional(&pdev->dev, "snvs-pwrkey");
> +	if (IS_ERR(pdata->clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(pdata->clk),
> +				     "Could not get the snvs-pwrkey clock");
> +
> +	clk_prepare_enable(pdata->clk);
>  	regmap_read(pdata->snvs, SNVS_HPVIDR1_REG, &vid);
> +	clk_disable_unprepare(pdata->clk);
> +
>  	pdata->minor_rev = vid & 0xff;
>  
>  	regmap_update_bits(pdata->snvs, SNVS_LPCR_REG, SNVS_LPCR_DEP_EN, SNVS_LPCR_DEP_EN);
> -- 
> 2.34.1
> 

Thanks.

-- 
Dmitry