Add support for the new "qcom,pmk8350-pon" comptaible string.
Signed-off-by: Anjelique Melendez <[email protected]>
---
drivers/power/reset/qcom-pon.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/power/reset/qcom-pon.c b/drivers/power/reset/qcom-pon.c
index 4a688741a88a..16bc01738be9 100644
--- a/drivers/power/reset/qcom-pon.c
+++ b/drivers/power/reset/qcom-pon.c
@@ -82,6 +82,7 @@ static const struct of_device_id pm8916_pon_id_table[] = {
{ .compatible = "qcom,pm8916-pon", .data = (void *)GEN1_REASON_SHIFT },
{ .compatible = "qcom,pms405-pon", .data = (void *)GEN1_REASON_SHIFT },
{ .compatible = "qcom,pm8998-pon", .data = (void *)GEN2_REASON_SHIFT },
+ { .compatible = "qcom,pmk8350-pon", .data = (void *)GEN2_REASON_SHIFT },
{ }
};
MODULE_DEVICE_TABLE(of, pm8916_pon_id_table);
--
2.35.1
On 25/07/2022 21:13, Anjelique Melendez wrote:
> Add support for the new "qcom,pmk8350-pon" comptaible string.
>
> Signed-off-by: Anjelique Melendez <[email protected]>
> ---
> drivers/power/reset/qcom-pon.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/power/reset/qcom-pon.c b/drivers/power/reset/qcom-pon.c
> index 4a688741a88a..16bc01738be9 100644
> --- a/drivers/power/reset/qcom-pon.c
> +++ b/drivers/power/reset/qcom-pon.c
> @@ -82,6 +82,7 @@ static const struct of_device_id pm8916_pon_id_table[] = {
> { .compatible = "qcom,pm8916-pon", .data = (void *)GEN1_REASON_SHIFT },
> { .compatible = "qcom,pms405-pon", .data = (void *)GEN1_REASON_SHIFT },
> { .compatible = "qcom,pm8998-pon", .data = (void *)GEN2_REASON_SHIFT },
> + { .compatible = "qcom,pmk8350-pon", .data = (void *)GEN2_REASON_SHIFT },
> { }
> };
> MODULE_DEVICE_TABLE(of, pm8916_pon_id_table);
This is now confusing. The new device has entirely different first and
second IO address spaces, but you do not code here any differences.
Best regards,
Krzysztof
On 7/26/2022 3:27 AM, Krzysztof Kozlowski wrote:
> On 25/07/2022 21:13, Anjelique Melendez wrote:
>> Add support for the new "qcom,pmk8350-pon" comptaible string.
>>
>> Signed-off-by: Anjelique Melendez <[email protected]>
>> ---
>> drivers/power/reset/qcom-pon.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/power/reset/qcom-pon.c b/drivers/power/reset/qcom-pon.c
>> index 4a688741a88a..16bc01738be9 100644
>> --- a/drivers/power/reset/qcom-pon.c
>> +++ b/drivers/power/reset/qcom-pon.c
>> @@ -82,6 +82,7 @@ static const struct of_device_id pm8916_pon_id_table[] = {
>> { .compatible = "qcom,pm8916-pon", .data = (void *)GEN1_REASON_SHIFT },
>> { .compatible = "qcom,pms405-pon", .data = (void *)GEN1_REASON_SHIFT },
>> { .compatible = "qcom,pm8998-pon", .data = (void *)GEN2_REASON_SHIFT },
>> + { .compatible = "qcom,pmk8350-pon", .data = (void *)GEN2_REASON_SHIFT },
>> { }
>> };
>> MODULE_DEVICE_TABLE(of, pm8916_pon_id_table);
>
> This is now confusing. The new device has entirely different first and
> second IO address spaces, but you do not code here any differences.
>
Based on previous responses to this patch series, it was decided that a new
"qcom,pmk8350-pon" compatible string is needed to differentiate between gen1/gen2
vs gen3 children pon devices. Currently the child handles obtaining the register
address(es) from its parent's regmap in drivers/input/misc/pm8941-pwrkey.c
(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/input/misc/pm8941-pwrkey.c?h=v6.0-rc1#n287).
The patch that handled that change can be found at
https://lore.kernel.org/linux-arm-msm/[email protected]/.
This driver, as the parent, does not actually need any changes besides adding the
new parent compatible string. Specifically this driver handles reboot mode support.
Everything needed to handle reboot mode is in the first register and reboot mode
is handled the same as a "qcom,pm8998-pon" parent. The split of the pon register
in pmk8350 does not affect reboot mode. This is why before we added a new compatible
string "qcom,pm8998-pon" still worked for gen3 children devices. Even though 2
registers could be defined in the device tree, as a 2nd register is optional for gen3
children devices,the fact that this driver uses of_property_read_u32() will ensure that
the base address used for reboot mode comes from the first register.
I do understand that this can be confusing but since "qcom,pmk8350-pon" still needs the
shift reason = 1 we could do
#define GEN1_REASON_SHIFT 2
#define GEN2_REASON_SHIFT 1
+ #define GEN3_REASON_SHIFT 1
{ .compatible = "qcom,pm8916-pon", .data = (void *)GEN1_REASON_SHIFT },
{ .compatible = "qcom,pms405-pon", .data = (void *)GEN1_REASON_SHIFT },
{ .compatible = "qcom,pm8998-pon", .data = (void *)GEN2_REASON_SHIFT },
+ { .compatible = "qcom,pmk8350-pon", .data = (void *)GEN3_REASON_SHIFT },
{ }
It would still have the exact same functionality but it may visually make more sense.
Thanks,
Anjelique
Hi,
Wanted to send a reminder for this patch conversation.
Thanks,
Anjelique
On 8/19/2022 1:26 PM, Anjelique Melendez wrote:
>
>
> On 7/26/2022 3:27 AM, Krzysztof Kozlowski wrote:
>> On 25/07/2022 21:13, Anjelique Melendez wrote:
>>> Add support for the new "qcom,pmk8350-pon" comptaible string.
>>>
>>> Signed-off-by: Anjelique Melendez <[email protected]>
>>> ---
>>> drivers/power/reset/qcom-pon.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/power/reset/qcom-pon.c b/drivers/power/reset/qcom-pon.c
>>> index 4a688741a88a..16bc01738be9 100644
>>> --- a/drivers/power/reset/qcom-pon.c
>>> +++ b/drivers/power/reset/qcom-pon.c
>>> @@ -82,6 +82,7 @@ static const struct of_device_id pm8916_pon_id_table[] = {
>>> { .compatible = "qcom,pm8916-pon", .data = (void *)GEN1_REASON_SHIFT },
>>> { .compatible = "qcom,pms405-pon", .data = (void *)GEN1_REASON_SHIFT },
>>> { .compatible = "qcom,pm8998-pon", .data = (void *)GEN2_REASON_SHIFT },
>>> + { .compatible = "qcom,pmk8350-pon", .data = (void *)GEN2_REASON_SHIFT },
>>> { }
>>> };
>>> MODULE_DEVICE_TABLE(of, pm8916_pon_id_table);
>>
>> This is now confusing. The new device has entirely different first and
>> second IO address spaces, but you do not code here any differences.
>>
>
> Based on previous responses to this patch series, it was decided that a new
> "qcom,pmk8350-pon" compatible string is needed to differentiate between gen1/gen2
> vs gen3 children pon devices. Currently the child handles obtaining the register
> address(es) from its parent's regmap in drivers/input/misc/pm8941-pwrkey.c
> (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/input/misc/pm8941-pwrkey.c?h=v6.0-rc1#n287).
> The patch that handled that change can be found at
> https://lore.kernel.org/linux-arm-msm/[email protected]/.
>
> This driver, as the parent, does not actually need any changes besides adding the
> new parent compatible string. Specifically this driver handles reboot mode support.
> Everything needed to handle reboot mode is in the first register and reboot mode
> is handled the same as a "qcom,pm8998-pon" parent. The split of the pon register
> in pmk8350 does not affect reboot mode. This is why before we added a new compatible
> string "qcom,pm8998-pon" still worked for gen3 children devices. Even though 2
> registers could be defined in the device tree, as a 2nd register is optional for gen3
> children devices,the fact that this driver uses of_property_read_u32() will ensure that
> the base address used for reboot mode comes from the first register.
>
> I do understand that this can be confusing but since "qcom,pmk8350-pon" still needs the
> shift reason = 1 we could do
>
> #define GEN1_REASON_SHIFT 2
> #define GEN2_REASON_SHIFT 1
> + #define GEN3_REASON_SHIFT 1
>
> { .compatible = "qcom,pm8916-pon", .data = (void *)GEN1_REASON_SHIFT },
> { .compatible = "qcom,pms405-pon", .data = (void *)GEN1_REASON_SHIFT },
> { .compatible = "qcom,pm8998-pon", .data = (void *)GEN2_REASON_SHIFT },
> + { .compatible = "qcom,pmk8350-pon", .data = (void *)GEN3_REASON_SHIFT },
> { }
>
>
> It would still have the exact same functionality but it may visually make more sense.
>
>
> Thanks,
> Anjelique
>
On 19/08/2022 22:26, Anjelique Melendez wrote:
>
>
> On 7/26/2022 3:27 AM, Krzysztof Kozlowski wrote:
>> On 25/07/2022 21:13, Anjelique Melendez wrote:
>>> Add support for the new "qcom,pmk8350-pon" comptaible string.
>>>
>>> Signed-off-by: Anjelique Melendez <[email protected]>
>>> ---
>>> drivers/power/reset/qcom-pon.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/power/reset/qcom-pon.c b/drivers/power/reset/qcom-pon.c
>>> index 4a688741a88a..16bc01738be9 100644
>>> --- a/drivers/power/reset/qcom-pon.c
>>> +++ b/drivers/power/reset/qcom-pon.c
>>> @@ -82,6 +82,7 @@ static const struct of_device_id pm8916_pon_id_table[] = {
>>> { .compatible = "qcom,pm8916-pon", .data = (void *)GEN1_REASON_SHIFT },
>>> { .compatible = "qcom,pms405-pon", .data = (void *)GEN1_REASON_SHIFT },
>>> { .compatible = "qcom,pm8998-pon", .data = (void *)GEN2_REASON_SHIFT },
>>> + { .compatible = "qcom,pmk8350-pon", .data = (void *)GEN2_REASON_SHIFT },
>>> { }
>>> };
>>> MODULE_DEVICE_TABLE(of, pm8916_pon_id_table);
>>
>> This is now confusing. The new device has entirely different first and
>> second IO address spaces, but you do not code here any differences.
>>
>
> Based on previous responses to this patch series, it was decided that a new
> "qcom,pmk8350-pon" compatible string is needed to differentiate between gen1/gen2
> vs gen3 children pon devices. Currently the child handles obtaining the register
> address(es) from its parent's regmap in drivers/input/misc/pm8941-pwrkey.c
> (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/input/misc/pm8941-pwrkey.c?h=v6.0-rc1#n287).
> The patch that handled that change can be found at
> https://lore.kernel.org/linux-arm-msm/[email protected]/.
>
> This driver, as the parent, does not actually need any changes besides adding the
> new parent compatible string. Specifically this driver handles reboot mode support.
> Everything needed to handle reboot mode is in the first register and reboot mode
> is handled the same as a "qcom,pm8998-pon" parent. The split of the pon register
> in pmk8350 does not affect reboot mode. This is why before we added a new compatible
> string "qcom,pm8998-pon" still worked for gen3 children devices. Even though 2
> registers could be defined in the device tree, as a 2nd register is optional for gen3
> children devices,the fact that this driver uses of_property_read_u32() will ensure that
> the base address used for reboot mode comes from the first register.
>
Ah, makes sense.
Best regards,
Krzysztof