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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Frank,

On Tue, Jul 16, 2024 at 04:56:09PM -0400, Frank Li wrote:
> From: Robin Gong <yibin.gong@nxp.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@nxp.com>
> Signed-off-by: Joy Zou <joy.zou@nxp.com>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> Signed-off-by: Frank Li <Frank.Li@nxp.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