Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Mattijs Korpershoek <mkorpershoek@baylibre.com>
To:     Dmitry Torokhov <dmitry.torokhov@gmail.com>,
        linux-input@vger.kernel.org
Cc:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>,
        linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] Input: mtk-pmic-keys - use single update when
 configuring long press behavior
In-Reply-To: <YowH0Q5HAGU7Grx2@google.com>
References: <YowH0Q5HAGU7Grx2@google.com>
Date:   Tue, 24 May 2022 09:24:38 +0200
Message-ID: <87wnebe6kp.fsf@baylibre.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On lun., mai 23, 2022 at 15:16, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> Instead of doing 3 read-modify-write operations when configuring behavior
> of long-press, consolidate everything into one.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Hi Dmitry,

Thank you for improving the driver.

I've tested this with evtest on mt8183-pumpkin on input/next
(fabcf4d8696839a8da8a3f90d1fd30ae923ddde7).
Note that I needed to add [1] on top as well for the driver to be
loaded, since [1] is not part of input/next

[1] https://lore.kernel.org/r/20220121140323.4080640-5-mkorpershoek@baylibre.com/

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

Tested-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> # on mt8183-pumpkin
> ---
>  drivers/input/keyboard/mtk-pmic-keys.c | 71 ++++++++++----------------
>  1 file changed, 28 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/input/keyboard/mtk-pmic-keys.c b/drivers/input/keyboard/mtk-pmic-keys.c
> index c31ab4368388..d2f0db245ff6 100644
> --- a/drivers/input/keyboard/mtk-pmic-keys.c
> +++ b/drivers/input/keyboard/mtk-pmic-keys.c
> @@ -18,17 +18,11 @@
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
>  
> -#define MTK_PMIC_PWRKEY_RST_EN_MASK	0x1
> -#define MTK_PMIC_PWRKEY_RST_EN_SHIFT	6
> -#define MTK_PMIC_HOMEKEY_RST_EN_MASK	0x1
> -#define MTK_PMIC_HOMEKEY_RST_EN_SHIFT	5
> -#define MTK_PMIC_RST_DU_MASK		0x3
> -#define MTK_PMIC_RST_DU_SHIFT		8
> -
> -#define MTK_PMIC_PWRKEY_RST		\
> -	(MTK_PMIC_PWRKEY_RST_EN_MASK << MTK_PMIC_PWRKEY_RST_EN_SHIFT)
> -#define MTK_PMIC_HOMEKEY_RST		\
> -	(MTK_PMIC_HOMEKEY_RST_EN_MASK << MTK_PMIC_HOMEKEY_RST_EN_SHIFT)
> +#define MTK_PMIC_RST_DU_MASK	GENMASK(9, 8)
> +#define MTK_PMIC_RST_DU_SHIFT	8
> +#define MTK_PMIC_RST_KEY_MASK	GENMASK(6, 5)
> +#define MTK_PMIC_PWRKEY_RST	BIT(6)
> +#define MTK_PMIC_HOMEKEY_RST	BIT(5)
>  
>  #define MTK_PMIC_PWRKEY_INDEX	0
>  #define MTK_PMIC_HOMEKEY_INDEX	1
> @@ -108,53 +102,44 @@ enum mtk_pmic_keys_lp_mode {
>  };
>  
>  static void mtk_pmic_keys_lp_reset_setup(struct mtk_pmic_keys *keys,
> -		u32 pmic_rst_reg)
> +					 u32 pmic_rst_reg)
>  {
> -	int ret;
>  	u32 long_press_mode, long_press_debounce;
> +	u32 value, mask;
> +	int error;
>  
> -	ret = of_property_read_u32(keys->dev->of_node,
> -		"power-off-time-sec", &long_press_debounce);
> -	if (ret)
> +	error = of_property_read_u32(keys->dev->of_node, "power-off-time-sec",
> +				     &long_press_debounce);
> +	if (error)
>  		long_press_debounce = 0;
>  
> -	regmap_update_bits(keys->regmap, pmic_rst_reg,
> -			   MTK_PMIC_RST_DU_MASK << MTK_PMIC_RST_DU_SHIFT,
> -			   long_press_debounce << MTK_PMIC_RST_DU_SHIFT);
> +	mask = MTK_PMIC_RST_DU_MASK;
> +	value = long_press_debounce << MTK_PMIC_RST_DU_SHIFT;
>  
> -	ret = of_property_read_u32(keys->dev->of_node,
> -		"mediatek,long-press-mode", &long_press_mode);
> -	if (ret)
> +	error  = of_property_read_u32(keys->dev->of_node,
> +				      "mediatek,long-press-mode",
> +				      &long_press_mode);
> +	if (error)
>  		long_press_mode = LP_DISABLE;
>  
>  	switch (long_press_mode) {
> -	case LP_ONEKEY:
> -		regmap_update_bits(keys->regmap, pmic_rst_reg,
> -				   MTK_PMIC_PWRKEY_RST,
> -				   MTK_PMIC_PWRKEY_RST);
> -		regmap_update_bits(keys->regmap, pmic_rst_reg,
> -				   MTK_PMIC_HOMEKEY_RST,
> -				   0);
> -		break;
>  	case LP_TWOKEY:
> -		regmap_update_bits(keys->regmap, pmic_rst_reg,
> -				   MTK_PMIC_PWRKEY_RST,
> -				   MTK_PMIC_PWRKEY_RST);
> -		regmap_update_bits(keys->regmap, pmic_rst_reg,
> -				   MTK_PMIC_HOMEKEY_RST,
> -				   MTK_PMIC_HOMEKEY_RST);
> -		break;
> +		value |= MTK_PMIC_HOMEKEY_RST;
> +		fallthrough;
> +
> +	case LP_ONEKEY:
> +		value |= MTK_PMIC_PWRKEY_RST;
> +		fallthrough;
> +
>  	case LP_DISABLE:
> -		regmap_update_bits(keys->regmap, pmic_rst_reg,
> -				   MTK_PMIC_PWRKEY_RST,
> -				   0);
> -		regmap_update_bits(keys->regmap, pmic_rst_reg,
> -				   MTK_PMIC_HOMEKEY_RST,
> -				   0);
> +		mask |= MTK_PMIC_RST_KEY_MASK;
>  		break;
> +
>  	default:
>  		break;
>  	}
> +
> +	regmap_update_bits(keys->regmap, pmic_rst_reg, mask, value);
>  }
>  
>  static irqreturn_t mtk_pmic_keys_irq_handler_thread(int irq, void *data)
> -- 
> 2.36.1.124.g0e6072fb45-goog
>
>
> -- 
> Dmitry