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.
