Re: [PATCH 1/3] input: misc: pm8941-pwrkey: add software key press debouncing support

[Stephen Boyd <swboyd@chromium.org>]

Quoting Anjelique Melendez (2022-01-20 12:41:33)
> From: David Collins <collinsd@codeaurora.org>
>
> On certain PMICs, an unexpected assertion of KPDPWR_DEB (the
> positive logic hardware debounced power key signal) may be seen
> during the falling edge of KPDPWR_N (i.e. a power key press) when
> it occurs close to the rising edge of SLEEP_CLK.  This then
> triggers a spurious KPDPWR interrupt.
>
> Handle this issue by adding software debouncing support to ignore
> key events that occur within the hardware debounce delay after the
> most recent key release event.
>
> Change-Id: Ifa3809935c01aab9078ba2302397bc9ebf390021

Please remove change-id when upstreaming.

> Signed-off-by: David Collins <collinsd@codeaurora.org>
> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
> ---
> diff --git a/drivers/input/misc/pm8941-pwrkey.c b/drivers/input/misc/pm8941-pwrkey.c
> index 33609603245d..b912ce00ce1c 100644
> --- a/drivers/input/misc/pm8941-pwrkey.c
> +++ b/drivers/input/misc/pm8941-pwrkey.c
> @@ -126,19 +144,65 @@ static irqreturn_t pm8941_pwrkey_irq(int irq, void *_data)
>         struct pm8941_pwrkey *pwrkey = _data;
>         unsigned int sts;
>         int error;
> +       u64 elapsed_us;
> +
> +       if (pwrkey->sw_debounce_time_us) {
> +               elapsed_us = ktime_us_delta(ktime_get(),
> +                                           pwrkey->last_release_time);
> +               if (elapsed_us < pwrkey->sw_debounce_time_us) {

Perhaps storing last_release_time + sw_debounce_time_us via
ktime_add_us() in the struct would be better. Then this line would be

	if (ktime_before(debounce_end, ktime_get()))

and we'd avoid a division when converting to microseconds to compare
time.

> +                       dev_dbg(pwrkey->dev, "ignoring key event received after %llu us, debounce time=%u us\n",
> +                               elapsed_us, pwrkey->sw_debounce_time_us);
> +                       return IRQ_HANDLED;
> +               }
> +       }
>
>         error = regmap_read(pwrkey->regmap,
>                             pwrkey->baseaddr + PON_RT_STS, &sts);

Nitpick: I'd prefer this be on one line. And 'ret' or 'err' be used as
it's shorter.

>         if (error)
>                 return IRQ_HANDLED;
>
> -       input_report_key(pwrkey->input, pwrkey->code,
> -                        sts & pwrkey->data->status_bit);
> +       sts &= pwrkey->data->status_bit;
> +
> +       if (pwrkey->sw_debounce_time_us && !sts)
> +               pwrkey->last_release_time = ktime_get();
> +
> +       input_report_key(pwrkey->input, pwrkey->code, sts);
>         input_sync(pwrkey->input);
>
>         return IRQ_HANDLED;
>  }
>
> +static int pm8941_pwrkey_sw_debounce_init(struct pm8941_pwrkey *pwrkey)
> +{
> +       unsigned int val, addr;
> +       int error;
> +
> +       if (pwrkey->data->has_pon_pbs && !pwrkey->pon_pbs_baseaddr) {
> +               dev_err(pwrkey->dev, "PON_PBS address missing, can't read HW debounce time\n");
> +               return 0;
> +       }
> +
> +       if (pwrkey->pon_pbs_baseaddr)
> +               addr = pwrkey->pon_pbs_baseaddr + PON_DBC_CTL;
> +       else
> +               addr = pwrkey->baseaddr + PON_DBC_CTL;
> +       error = regmap_read(pwrkey->regmap, addr, &val);
> +       if (error)
> +               return error;
> +
> +       if (pwrkey->subtype >= PON_SUBTYPE_GEN2_PRIMARY)
> +               pwrkey->sw_debounce_time_us = 2 * USEC_PER_SEC /
> +                                               (1 << (0xf - (val & 0xf)));
> +       else
> +               pwrkey->sw_debounce_time_us = 2 * USEC_PER_SEC /
> +                                               (1 << (0x7 - (val & 0x7)));

Can we have one more local variable like 'mask' or 'offset'. Then the
code would be easier to read

	if (pwrkey->subtype >= PON_SUBTYPE_GEN2_PRIMARY)
		mask = 0xf;
	else
		mask = 0x7
	
	pwrkey->sw_debounce_time_us = 2 * USEC_PER_SEC / (1 << mask - (val & 0x7));

> +
> +       dev_dbg(pwrkey->dev, "SW debounce time = %u us\n",
> +               pwrkey->sw_debounce_time_us);
> +
> +       return 0;
> +}
> +
>  static int __maybe_unused pm8941_pwrkey_suspend(struct device *dev)
>  {
>         struct pm8941_pwrkey *pwrkey = dev_get_drvdata(dev);
> @@ -167,6 +231,8 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
>         struct pm8941_pwrkey *pwrkey;
>         bool pull_up;
>         struct device *parent;
> +       struct device_node *regmap_node;
> +       const __be32 *addr;
>         u32 req_delay;
>         int error;
>
> @@ -188,8 +254,10 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
>         pwrkey->data = of_device_get_match_data(&pdev->dev);
>
>         parent = pdev->dev.parent;
> +       regmap_node = pdev->dev.of_node;
>         pwrkey->regmap = dev_get_regmap(parent, NULL);
>         if (!pwrkey->regmap) {
> +               regmap_node = parent->of_node;
>                 /*
>                  * We failed to get regmap for parent. Let's see if we are
>                  * a child of pon node and read regmap and reg from its
> @@ -200,15 +268,21 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
>                         dev_err(&pdev->dev, "failed to locate regmap\n");
>                         return -ENODEV;
>                 }
> +       }
>
> -               error = of_property_read_u32(parent->of_node,
> -                                            "reg", &pwrkey->baseaddr);
> -       } else {
> -               error = of_property_read_u32(pdev->dev.of_node, "reg",
> -                                            &pwrkey->baseaddr);
> +       addr = of_get_address(regmap_node, 0, NULL, NULL);
> +       if (!addr) {
> +               dev_err(&pdev->dev, "reg property missing\n");
> +               return -EINVAL;
> +       }
> +       pwrkey->baseaddr = be32_to_cpu(*addr);

Can this hunk be split off? A new API is used and it doesn't look
relevant to this patch.

> +
> +       if (pwrkey->data->has_pon_pbs) {
> +               /* PON_PBS base address is optional */
> +               addr = of_get_address(regmap_node, 1, NULL, NULL);
> +               if (addr)
> +                       pwrkey->pon_pbs_baseaddr = be32_to_cpu(*addr);
>         }
> -       if (error)
> -               return error;
>
>         pwrkey->irq = platform_get_irq(pdev, 0);
>         if (pwrkey->irq < 0)
> @@ -217,7 +291,14 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
>         error = regmap_read(pwrkey->regmap, pwrkey->baseaddr + PON_REV2,
>                             &pwrkey->revision);
>         if (error) {
> -               dev_err(&pdev->dev, "failed to set debounce: %d\n", error);
> +               dev_err(&pdev->dev, "failed to read revision: %d\n", error);

Nice error message fix!

> +               return error;
> +       }
> +
> +       error = regmap_read(pwrkey->regmap, pwrkey->baseaddr + PON_SUBTYPE,
> +                           &pwrkey->subtype);
> +       if (error) {
> +               dev_err(&pdev->dev, "failed to read subtype: %d\n", error);
>                 return error;
>         }
>
> @@ -255,6 +336,12 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
>                 }
>         }
>
> +       if (pwrkey->data->needs_sw_debounce) {
> +               error = pm8941_pwrkey_sw_debounce_init(pwrkey);
> +               if (error)
> +                       return error;
> +       }
> +
>         if (pwrkey->data->pull_up_bit) {
>                 error = regmap_update_bits(pwrkey->regmap,
>                                            pwrkey->baseaddr + PON_PULL_CTL,
> @@ -316,6 +403,8 @@ static const struct pm8941_data pwrkey_data = {
>         .phys = "pm8941_pwrkey/input0",
>         .supports_ps_hold_poff_config = true,
>         .supports_debounce_config = true,
> +       .needs_sw_debounce = true,

needs_sw_debounce is always true? Why is it even an option then?

> +       .has_pon_pbs = false,
>  };
>
>  static const struct pm8941_data resin_data = {