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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 1/20/2022 8:08 PM, Stephen Boyd wrote:
> Quoting Anjelique Melendez (2022-01-20 12:41:33)
>> From: David Collins <collinsd@xxxxxxxxxxxxxx>
>>
>> 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.

Will remove change-id from other 2 patches as well.

>
>> Signed-off-by: David Collins <collinsd@xxxxxxxxxxxxxx>
>> Signed-off-by: Anjelique Melendez <quic_amelende@xxxxxxxxxxx>
>> ---
>> 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.

Sure this can be done!

>> +                       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.

ACK

>
>>         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));

Sure not a problem!

>> +
>> +       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.

In PMK8350 and following chips the reg property will have the pon hlos address first,
followed by a second pon pbs address. This change is needed so that both the older chipsets
and the newer can be used regardless of how many reg addresses are being used.

>
>> +
>> +       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?

As of right now the "needs_sw_debounce" property is being used for a sw work around for a hw
problem. We anticipate that chips in the future will fix this hw problem and we would then have
devices where needs_sw_debounce would be set to false.

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




[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux