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

[Anjelique Melendez]

On 1/24/2022 11:33 AM, Stephen Boyd wrote:
> Quoting Anjelique Melendez (2022-01-21 16:04:13)
>>
>> On 1/20/2022 8:08 PM, Stephen Boyd wrote:
>>> Quoting Anjelique Melendez (2022-01-20 12:41:33)
>>>> @@ -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.
> Got it, but do we ned to change to of_get_address() in this patch? I was
> suggesting that the change to the new API be done first so that it's
> clearer what's going on with the change in address location.

Ok, makes sense. Will separate this into it's own patch for v2.

>>>> +
>>>> +       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!
> This can be split off to a different patch as well.

Will do.

>>>> +               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.
> Hmm ok. Why can't future chips be supported in this series? What happens
> if nobody ever adds support for the new chips? We're left with this
> condition that looks like dead code.

Sure, makes sense. Will remove "needs_sw_debounce" property for V2.