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

  Add binding for the Qualcomm Programmable Boot Sequencer device.

Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
---
 .../bindings/soc/qcom/qcom-pbs.yaml           | 41 +++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml
  

On Wed, 21 Jun 2023 11:59:45 -0700, Anjelique Melendez wrote:
> Add binding for the Qualcomm Programmable Boot Sequencer device.
> 
> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
> ---
>  .../bindings/soc/qcom/qcom-pbs.yaml           | 41 +++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml:26:2: [warning] wrong indentation: expected 2 but found 1 (indentation)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230621185949.2068-2-quic_amelende@quicinc.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
  

On 21/06/2023 20:59, Anjelique Melendez wrote:
> Add binding for the Qualcomm Programmable Boot Sequencer device.
> 
> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
> ---
>  .../bindings/soc/qcom/qcom-pbs.yaml           | 41 +++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml

Except missing testing... few more comments:


> 
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml
> new file mode 100644
> index 000000000000..0a89c334f95c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml

Filename matching compatibles, so qcom,pbs

> @@ -0,0 +1,41 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/qcom/qcom-pbs.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Technologies, Inc. PBS

Qualcomm PBS foo bar a bit more than just one word.

E.g. expand PBS acronym

> +
> +maintainers:
> +  - Anjelique Melendez <quic_amelende@quicinc.com>
> +
> +description: |
> +  Qualcomm PBS (programmable boot sequencer) supports triggering sequences
> +  for clients upon request.

I don't understand what's this. What is "triggering sequences"? What
sequences?

> +
> +properties:
> +  compatible:
> +    const: qcom,pbs

Missing SoC specific comaptibles.


> +
> +  reg:
> +    description: |
> +      Base address of the PBS peripheral.

Drop description, it's obvious.

> +    maxItems: 1
> +

Binding looks very incomplete...

> +required:
> + - compatible
> + - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    pmic {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      qcom,pbs@7400 {

That's not a proper node name. Do you see anywhere such node? Please, do
not work on downstream code, but on mainline.

> +        compatible = "qcom,pbs";
> +        reg = <0x7400>;
> +      };
> +    };

Best regards,
Krzysztof
  

On Wed, Jun 21, 2023 at 11:59:45AM -0700, Anjelique Melendez wrote:
> Add binding for the Qualcomm Programmable Boot Sequencer device.
> 
> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
> ---
>  .../bindings/soc/qcom/qcom-pbs.yaml           | 41 +++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml
> 
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml
> new file mode 100644
> index 000000000000..0a89c334f95c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml
> @@ -0,0 +1,41 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/qcom/qcom-pbs.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Technologies, Inc. PBS
> +
> +maintainers:
> +  - Anjelique Melendez <quic_amelende@quicinc.com>
> +
> +description: |
> +  Qualcomm PBS (programmable boot sequencer) supports triggering sequences
> +  for clients upon request.
> +
> +properties:
> +  compatible:
> +    const: qcom,pbs
> +
> +  reg:
> +    description: |
> +      Base address of the PBS peripheral.
> +    maxItems: 1
> +
> +required:
> + - compatible
> + - reg
> +
> +additionalProperties: false
> +
> +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.

Rob
  

On 6/26/2023 6:58 AM, Rob Herring wrote:
> On Wed, Jun 21, 2023 at 11:59:45AM -0700, Anjelique Melendez wrote:
>> Add binding for the Qualcomm Programmable Boot Sequencer device.
>>
>> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
>> ---
>>  .../bindings/soc/qcom/qcom-pbs.yaml           | 41 +++++++++++++++++++
>>  1 file changed, 41 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml
>> new file mode 100644
>> index 000000000000..0a89c334f95c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml
>> @@ -0,0 +1,41 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/soc/qcom/qcom-pbs.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm Technologies, Inc. PBS
>> +
>> +maintainers:
>> +  - Anjelique Melendez <quic_amelende@quicinc.com>
>> +
>> +description: |
>> +  Qualcomm PBS (programmable boot sequencer) supports triggering sequences
>> +  for clients upon request.
>> +
>> +properties:
>> +  compatible:
>> +    const: qcom,pbs
>> +
>> +  reg:
>> +    description: |
>> +      Base address of the PBS peripheral.
>> +    maxItems: 1
>> +
>> +required:
>> + - compatible
>> + - reg
>> +
>> +additionalProperties: false
>> +
>> +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. 

> Rob
  

On 29/06/2023 04:19, Anjelique Melendez wrote:
> 
> 
> On 6/26/2023 6:58 AM, Rob Herring wrote:
>> On Wed, Jun 21, 2023 at 11:59:45AM -0700, Anjelique Melendez wrote:
>>> Add binding for the Qualcomm Programmable Boot Sequencer device.
>>>
>>> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
>>> ---
>>>   .../bindings/soc/qcom/qcom-pbs.yaml           | 41 +++++++++++++++++++
>>>   1 file changed, 41 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml
>>> new file mode 100644
>>> index 000000000000..0a89c334f95c
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml
>>> @@ -0,0 +1,41 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/soc/qcom/qcom-pbs.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Qualcomm Technologies, Inc. PBS
>>> +
>>> +maintainers:
>>> +  - Anjelique Melendez <quic_amelende@quicinc.com>
>>> +
>>> +description: |
>>> +  Qualcomm PBS (programmable boot sequencer) supports triggering sequences
>>> +  for clients upon request.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: qcom,pbs
>>> +
>>> +  reg:
>>> +    description: |
>>> +      Base address of the PBS peripheral.
>>> +    maxItems: 1
>>> +
>>> +required:
>>> + - compatible
>>> + - reg
>>> +
>>> +additionalProperties: false
>>> +
>>> +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.

Does it have to be a separate driver? Or is it a part of the LPG driver, 
just being artificially split away?

> 
>> Rob
> 
> 
>
  

On 6/29/2023 1:45 AM, Dmitry Baryshkov wrote:
> On 29/06/2023 04:19, Anjelique Melendez wrote:
>>
>>
>> On 6/26/2023 6:58 AM, Rob Herring wrote:
>>> On Wed, Jun 21, 2023 at 11:59:45AM -0700, Anjelique Melendez wrote:
>>>> Add binding for the Qualcomm Programmable Boot Sequencer device.
>>>>
>>>> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
>>>> ---
>>>>   .../bindings/soc/qcom/qcom-pbs.yaml           | 41 +++++++++++++++++++
>>>>   1 file changed, 41 insertions(+)
>>>>   create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml
>>>> new file mode 100644
>>>> index 000000000000..0a89c334f95c
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml
>>>> @@ -0,0 +1,41 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/soc/qcom/qcom-pbs.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Qualcomm Technologies, Inc. PBS
>>>> +
>>>> +maintainers:
>>>> +  - Anjelique Melendez <quic_amelende@quicinc.com>
>>>> +
>>>> +description: |
>>>> +  Qualcomm PBS (programmable boot sequencer) supports triggering sequences
>>>> +  for clients upon request.
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: qcom,pbs
>>>> +
>>>> +  reg:
>>>> +    description: |
>>>> +      Base address of the PBS peripheral.
>>>> +    maxItems: 1
>>>> +
>>>> +required:
>>>> + - compatible
>>>> + - reg
>>>> +
>>>> +additionalProperties: false
>>>> +
>>>> +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.
> 
> Does it have to be a separate driver? Or is it a part of the LPG driver, just being artificially split away?

Sure, I just discussed with team and we are ok with removing this as a separate driver. Will have that 
for next version. 
> 
>>
>>> Rob
>>
>>
>>
>
  

On 30/06/2023 00:53, Anjelique Melendez wrote:
> 
> 
> On 6/29/2023 1:45 AM, Dmitry Baryshkov wrote:
>> On 29/06/2023 04:19, Anjelique Melendez wrote:
>>>
>>>
>>> On 6/26/2023 6:58 AM, Rob Herring wrote:
>>>> On Wed, Jun 21, 2023 at 11:59:45AM -0700, Anjelique Melendez wrote:
>>>>> Add binding for the Qualcomm Programmable Boot Sequencer device.
>>>>>
>>>>> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
>>>>> ---
>>>>>    .../bindings/soc/qcom/qcom-pbs.yaml           | 41 +++++++++++++++++++
>>>>>    1 file changed, 41 insertions(+)
>>>>>    create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..0a89c334f95c
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml
>>>>> @@ -0,0 +1,41 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/soc/qcom/qcom-pbs.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: Qualcomm Technologies, Inc. PBS
>>>>> +
>>>>> +maintainers:
>>>>> +  - Anjelique Melendez <quic_amelende@quicinc.com>
>>>>> +
>>>>> +description: |
>>>>> +  Qualcomm PBS (programmable boot sequencer) supports triggering sequences
>>>>> +  for clients upon request.
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: qcom,pbs
>>>>> +
>>>>> +  reg:
>>>>> +    description: |
>>>>> +      Base address of the PBS peripheral.
>>>>> +    maxItems: 1
>>>>> +
>>>>> +required:
>>>>> + - compatible
>>>>> + - reg
>>>>> +
>>>>> +additionalProperties: false
>>>>> +
>>>>> +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.
>>
>> Does it have to be a separate driver? Or is it a part of the LPG driver, just being artificially split away?
> 
> Sure, I just discussed with team and we are ok with removing this as a separate driver. Will have that
> for next version.

I saw that the PBS can also be used with the haptics device. Will it 
talk to the LPG driver?

>>
>>>
>>>> Rob
>>>
>>>
>>>
>>
  

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

Thanks,
Anjelique
  

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

Best regards,
Krzysztof
  

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
  

On 11/07/2023 22:12, Anjelique Melendez wrote:

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

I guess I miss here something. What is "LPG client"? I don't understand
why this LPG client needs existence of PBS node, to be able to get the
regmap.

PBS is a child of PMIC, so it can get regmap from the parent. What's
more, which DT property passes the regmap from PMIC to LPG client?

Best regards,
Krzysztof
  

On Wed, 12 Jul 2023 at 17:22, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 11/07/2023 22:12, Anjelique Melendez wrote:
>
> >>>
> >>> 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."
>
> I guess I miss here something. What is "LPG client"? I don't understand
> why this LPG client needs existence of PBS node, to be able to get the
> regmap.
>
> PBS is a child of PMIC, so it can get regmap from the parent. What's
> more, which DT property passes the regmap from PMIC to LPG client?

There are some PMICs which claim two SPMI SIDs. For such PMICs, each
SID is a separate device, so it is not directly possible to get the
regmap of the other SID.
  

On 12/07/2023 16:35, Dmitry Baryshkov wrote:
>>>> 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."
>>
>> I guess I miss here something. What is "LPG client"? I don't understand
>> why this LPG client needs existence of PBS node, to be able to get the
>> regmap.
>>
>> PBS is a child of PMIC, so it can get regmap from the parent. What's
>> more, which DT property passes the regmap from PMIC to LPG client?
> 
> There are some PMICs which claim two SPMI SIDs. For such PMICs, each
> SID is a separate device, so it is not directly possible to get the
> regmap of the other SID.

OK, maybe after implementing all the review changes - including dropping
that singleton pattern - this will be clearer. Please send new version
and we will discuss it from there.

Thank you.
Best regards,
Krzysztof
  

On 7/12/2023 1:11 PM, Krzysztof Kozlowski wrote:
> On 12/07/2023 16:35, Dmitry Baryshkov wrote:
>>>>> 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."
>>>
>>> I guess I miss here something. What is "LPG client"? I don't understand
>>> why this LPG client needs existence of PBS node, to be able to get the
>>> regmap.
>>>
>>> PBS is a child of PMIC, so it can get regmap from the parent. What's
>>> more, which DT property passes the regmap from PMIC to LPG client?
>>
>> There are some PMICs which claim two SPMI SIDs. For such PMICs, each
>> SID is a separate device, so it is not directly possible to get the
>> regmap of the other SID.
> 
> OK, maybe after implementing all the review changes - including dropping
> that singleton pattern - this will be clearer. Please send new version
> and we will discuss it from there.
> 
Sure, will work on getting that new version sent but I did just have clarifying question.
When you say "dropping that singleton pattern" are you referring to dropping the 
PBS node?
Want to make sure we are all on the same page with what the next version will include :)

Thanks,
Anjelique
  

On 14/07/2023 22:32, Anjelique Melendez wrote:
> 
> 
> On 7/12/2023 1:11 PM, Krzysztof Kozlowski wrote:
>> On 12/07/2023 16:35, Dmitry Baryshkov wrote:
>>>>>> 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."
>>>>
>>>> I guess I miss here something. What is "LPG client"? I don't understand
>>>> why this LPG client needs existence of PBS node, to be able to get the
>>>> regmap.
>>>>
>>>> PBS is a child of PMIC, so it can get regmap from the parent. What's
>>>> more, which DT property passes the regmap from PMIC to LPG client?
>>>
>>> There are some PMICs which claim two SPMI SIDs. For such PMICs, each
>>> SID is a separate device, so it is not directly possible to get the
>>> regmap of the other SID.
>>
>> OK, maybe after implementing all the review changes - including dropping
>> that singleton pattern - this will be clearer. Please send new version
>> and we will discuss it from there.
>>
> Sure, will work on getting that new version sent but I did just have clarifying question.
> When you say "dropping that singleton pattern" are you referring to dropping the 
> PBS node?
> Want to make sure we are all on the same page with what the next version will include :)

I was referring to my comments on driver, that you should not have
file-scope variable and get_pbs_client_device() iterating over global
list. It isn't singleton, actually, but the pattern in coding is very
similar to singleton approach.

Best regards,
Krzysztof