Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Message-ID: <4fe37ae7-1d72-5404-a9f3-ec4b1b51358a@quicinc.com>
Date:   Mon, 23 Oct 2023 11:43:15 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.1
Subject: Re: [PATCH 06/11] iio: adc: Add QCOM PMIC5 Gen3 ADC bindings
Content-Language: en-US
To:     Jonathan Cameron <jic23@kernel.org>
CC:     <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>,
        <krzysztof.kozlowski@linaro.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>,
        <linux-arm-msm-owner@vger.kernel.org>
References: <20230708072835.3035398-1-quic_jprakash@quicinc.com>
 <20230708072835.3035398-7-quic_jprakash@quicinc.com>
 <20230708161259.7af17fa9@jic23-huawei>
From:   Jishnu Prakash <quic_jprakash@quicinc.com>
In-Reply-To: <20230708161259.7af17fa9@jic23-huawei>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Jonathan,

On 7/8/2023 8:42 PM, Jonathan Cameron wrote:
> On Sat, 8 Jul 2023 12:58:30 +0530
> Jishnu Prakash <quic_jprakash@quicinc.com> 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
>>   
>>     reg:
>>       description: VADC base address in the SPMI PMIC register map
>> -    maxItems: 1
>> +    minItems: 1
> ?

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
>>   
>> +  "#thermal-sensor-cells":
>> +    const: 1
>> +    description:
>> +      Number of cells required to uniquely identify the thermal sensors. Since
>> +      we have multiple sensors this is set to 1.
>> +
> Defined only for the new gen3?  If so make make it false for the other devices.


Yes, will do that in the next patchset.

>
>>     '#io-channel-cells':
>>       const: 1
>>   
>> @@ -71,8 +78,8 @@ patternProperties:
>>           description: |
>>             ADC channel number.
>>   
>> +      qcom,adc-tm-type:
>> +        description: |
>> +            Indicates if ADC_TM monitoring is done on this channel.
>> +            Defined for compatible property "qcom,spmi-adc5-gen3".
>> +        type: boolean
> Enforce that in the binding, not via a comment. Once the binding
> performs that check (set it to false for non matching compatibles) then
> there is no need to also mention it in text.

Will do that in the next patchset.


>
>> +
>>       required:
>>         - reg
>>   
>> @@ -213,7 +227,9 @@ allOf:
>>         properties:
>>           compatible:
>>             contains:
>> -            const: qcom,spmi-adc5-gen2
>> +            enum:
>> +              - qcom,spmi-adc5-gen2
>> +              - qcom,spmi-adc5-gen3
> Side note - it's fine to have a single element enum, so you could
> use that option to reduce churn in this set...


I think we can remove this and instead specify properties explicitly for 
qcom,spmi-adc5-gen3 too separately in the next patchset.


>
>>   
>>       then:
>>         patternProperties:
>> @@ -299,7 +315,7 @@ examples:
>>                   label = "xo_therm";
>>               };
>>   
>> -            channel@47 {
>> +            channel@147 {
> ?  If that's a valid change, then it looks like a separate fix.


I think I can avoid this for now, although it would be needed if this 
channel number was the virtual channel number made by combining PMIC SID 
and actual channel number....maybe we can do it in a separate fix.


>
>>                   reg = <PM8350_ADC5_GEN2_AMUX_THM4_100K_PU(1)>;
>>                   qcom,ratiometric;
>>                   qcom,hw-settle-time = <200>;
>> @@ -307,3 +323,80 @@ examples:
>>               };
>>           };
>>       };
>> +
>> +#ifndef PM8550_SID
>> +#define PM8550_SID		1
>> +#endif
>> +
>> +/* ADC channels for PM8550_ADC for PMIC5 Gen3 */
>> +#define PM8550_ADC5_GEN3_OFFSET_REF			(PM8550_SID << 8 | 0x00)
> can we do the naming for the 0x00 as per Dmitry's set? That is get them from
> qcom,spmi-vadc.h
>
> https://patchwork.kernel.org/project/linux-iio/patch/20230707123027.1510723-2-dmitry.baryshkov@linaro.org/

Yes, will do that in the next patchset.

Thanks,

Jishnu