Re: [PATCH 06/11] iio: adc: Add QCOM PMIC5 Gen3 ADC bindings

From: Jishnu Prakash <quic_jprakash@quicinc.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
	<agross@kernel.org>, <devicetree@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <linus.walleij@linaro.org>,
	<Jonathan.Cameron@huawei.com>, <sboyd@kernel.org>,
	<dmitry.baryshkov@linaro.org>, <quic_subbaram@quicinc.com>,
	<quic_collinsd@quicinc.com>, <quic_kamalw@quicinc.com>,
	<marijn.suijten@somainline.org>,
	<andriy.shevchenko@linux.intel.com>,
	Jonathan Cameron <jic23@kernel.org>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Bjorn Andersson <andersson@kernel.org>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	Luca Weiss <luca@z3ntu.xyz>, <linux-iio@vger.kernel.org>,
	<linux-arm-msm@vger.kernel.org>
Cc: <linux-arm-msm-owner@vger.kernel.org>
Subject: Re: [PATCH 06/11] iio: adc: Add QCOM PMIC5 Gen3 ADC bindings
Date: Mon, 23 Oct 2023 11:44:29 +0530	[thread overview]
Message-ID: <99070bce-6188-82eb-c92c-cf7a323394e2@quicinc.com> (raw)
In-Reply-To: <bb225c12-f017-fac3-45f1-c828a10553e2@linaro.org>

Hi Krzysztof,

On 7/9/2023 10:53 PM, Krzysztof Kozlowski wrote:
> On 08/07/2023 09:28, Jishnu Prakash wrote:
>> For the PMIC5-Gen3 type PMICs, ADC peripheral is present in HW for the
>> following PMICs: PMK8550, PM8550, PM8550B and PM8550VX PMICs.
>> It is similar to PMIC5-Gen2, with SW communication to ADCs on all PMICs
>> going through PBS firmware through a single register interface. This
>> interface is implemented on an SDAM peripheral on the master PMIC PMK8550
>> rather than a dedicated ADC peripheral.
>>
>> Signed-off-by: Jishnu Prakash <quic_jprakash@quicinc.com>
>> ---
>>   properties:
>>     compatible:
>> @@ -27,10 +27,11 @@ properties:
>>             - qcom,spmi-adc5
>>             - qcom,spmi-adc-rev2
>>             - qcom,spmi-adc5-gen2
>> +          - qcom,spmi-adc5-gen3
>
> This could be ordered...

Yes, will do that in the next patchset.

>>   
>>     reg:
>>       description: VADC base address in the SPMI PMIC register map
>> -    maxItems: 1
>> +    minItems: 1
> Why? This does not make any sense. With previous patches it looks like
> random set of changes.

The idea here is to convey that reg can have multiple values for ADC5 
Gen3 as there can be more than one peripheral used for ADC, so there can 
be multiple base addresses. I'll try to make this more clear in the next 
patchset.

>
>
>>   
>>     '#address-cells':
>>       const: 1
>> @@ -38,6 +39,12 @@ properties:
>>     '#size-cells':
>>       const: 0
>>   
>>   
>> +      qcom,adc-tm-type:
>> +        description: |
>> +            Indicates if ADC_TM monitoring is done on this channel.
> Description does not match property name.

You mean it sounds more like an enum which can take several values 
rather than just a boolean? I can update it to "qcom,adc-tm" if that 
looks better.

>
>> +            Defined for compatible property "qcom,spmi-adc5-gen3".
> Then you need if:then:.

Yes, will update this explicitly for ADC5 Gen3 in the next patchset.

>
>> +        type: boolean
>> +
>>       required:
>>         - reg
>>   
>> @@ -213,7 +227,9 @@ allOf:
>>         properties:
>>           compatible:
>>             contains:
>> -            const: qcom,spmi-adc5-gen2
>> +            enum:
>> +              - qcom,spmi-adc5-gen2
>> +              - qcom,spmi-adc5-gen3
>>   
>>       then:
>>         patternProperties:
>> @@ -299,7 +315,7 @@ examples:
>>                   label = "xo_therm";
>>               };
>>   
>> -            channel@47 {
>> +            channel@147 {
> Why?

It would be needed if this channel number was supposed to be the virtual 
channel number made by combining PMIC SID and actual channel 
number....but I could drop it for now and do it in a separate fix as 
Jonathan suggested.

>
>>                   reg = <PM8350_ADC5_GEN2_AMUX_THM4_100K_PU(1)>;
>>                   qcom,ratiometric;
>>                   qcom,hw-settle-time = <200>;
>> @@ -307,3 +323,80 @@ examples:
>>               };
>>           };
>>       };
>> +
>> +  - |
>> +    #include <dt-bindings/iio/qcom,spmi-adc5-gen3-pmk8550.h>
>> +    #include <dt-bindings/iio/qcom,spmi-adc5-gen3-pm8550.h>
>> +    #include <dt-bindings/iio/qcom,spmi-adc5-gen3-pm8550b.h>
>> +    #include <dt-bindings/iio/qcom,spmi-adc5-gen3-pm8550vx.h>
>> +
>> +    pmic {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +
>> +      /* VADC node */
>> +      pmk8550_vadc: vadc@9000 {
>> +        compatible = "qcom,spmi-adc5-gen3";
> Don't add new examples which differ only in compatible.

This example does have differences unique to ADC5 Gen3 such as use of 
"#thermal-sensor-cells" and "qcom,adc-tm-type" properties....to make it 
clearer, I'll delete some of the excess nodes which don't highlight 
these differences.

>
>
>> diff --git a/include/dt-bindings/iio/qcom,spmi-adc5-gen3-pm8550.h b/include/dt-bindings/iio/qcom,spmi-adc5-gen3-pm8550.h
>> new file mode 100644
>> index 000000000000..74e6e2f6f9ed
>> --- /dev/null
>> +++ b/include/dt-bindings/iio/qcom,spmi-adc5-gen3-pm8550.h
>> @@ -0,0 +1,48 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
> Dual license.

I think we do have an internal rule by which we do have to add these two 
licenses....I'll check again and update them if required.

Thanks,

Jishnu

>
>
>
> Best regards,
> Krzysztof
>