Re: [PATCH 1/7] dt-bindings: soc: qcom: Add qcom-pbs bindings

[Anjelique Melendez <quic_amelende@quicinc.com>]

On 7/10/2023 10:58 PM, Krzysztof Kozlowski wrote:
> On 11/07/2023 05:52, Anjelique Melendez wrote:
>>
>>
>> On 7/1/2023 4:03 AM, Krzysztof Kozlowski wrote:
>>> On 29/06/2023 03:19, Anjelique Melendez wrote:
>>>
>>>>>> +examples:
>>>>>> +  - |
>>>>>> +    pmic {
>>>>>> +      #address-cells = <1>;
>>>>>> +      #size-cells = <0>;
>>>>>> +
>>>>>> +      qcom,pbs@7400 {
>>>>>> +        compatible = "qcom,pbs";
>>>>>> +        reg = <0x7400>;
>>>>>> +      };
>>>>>
>>>>> Why do you need a child node for this? Is there more than 1 instance in 
>>>>> a PMIC? Every sub-function of a PMIC doesn't have to have a DT node.
>>>>>
>>>>
>>>> We currently have another downstream driver (which is planned to get upstreamed)
>>>> which also needs a handle to a pbs device in order to properly trigger events. 
>>>
>>> I don't see how does it answer Rob's concerns. Neither mine about
>>> incomplete binding. You don't need pbs node here for that.
>>>
>>> Anyway, whatever you have downstream also does not justify any changes.
>>> Either upstream these so we can see it or drop this binding.
>>>
>>> Best regards,
>>> Krzysztof
>>>
>>
>> On PMI632, peripherals are partitioned over 2 different SIDs
>> (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/pmi632.dtsi?h=v6.5-rc1#n42
>> and https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/pmi632.dtsi?h=v6.5-rc1#n149).
>> Unfortunately, the pbs peripheral and the lpg peripherals are on different
>> PMI632 devices and therefore have different regmaps.
>>  
>> If we get rid of the pbs node we need to get a handle to the proper regmap.
>> I see two possible options, we could either introduce a new client property
>> which points to a peripheral on the same device as pbs.
>>
>> i.e.
>> 	led-controller {
>> 		compatible = "qcom,pmi632-lpg";
>>       		#address-cells = <1>;
>>       		#size-cells = <0>;
>>       		#pwm-cells = <2>;
>>      		nvmem-names = "lpg_chan_sdam";
>>       		nvmem = <&pmi632_sdam7>;
>>       		qcom,pbs-phandle = <&pmi632_gpios>;
>>       		..... 
>> 	};
>> Then when client is probing could do something like the following to get the regmap
>>
>> 	dn = of_parse_phandle(node, "qcom,pbs-phandle", 0);
>> 	pdev = of_find_device_by_node(dn);
>> 	pbs_regmap = dev_get_regmap(&pdev->dev->parent, NULL);
>>
>>
>>
>> Or we could use the nvmem phandle and just have something like this in client's probe
>>
>> 	dn = of_parse_phandle(node, "nvmem", 0);
>> 	pdev = of_find_device_by_node(dn);
>> 	pbs_regmap = dev_get_regmap(&pdev->dev->parent, NULL);
>>
>>
>>
>> Let me know what your thoughts are on this.
> 
> Rob asked you - "Is there more than 1 instance in a PMIC?" - and you did
> not answer positively, just mentioned something about drivers in
> downstream, which do not matter. So is the answer for that question:
> yes, you have two instances of the same PMIC differing by presence of
> PBS and other features"?
> 
Sorry that was a misunderstanding on my part.
Yes, answer to Rob's question should have been "We have two instances of PMI632,
where one instance holds the pbs peripheral and the other holds the lpg
peripherals. The child node for pbs is needed so lpg client can access
the PMI632 regmap which contains the pbs peripheral."

Thanks,
Anjelique