Message-ID: <31bd08ce-823a-4a71-baca-a9d1e02fcb6a@oss.qualcomm.com>
Date: Wed, 8 Oct 2025 19:50:30 +0530
From: Jishnu Prakash <jishnu.prakash@....qualcomm.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc: Jonathan Cameron <jic23@...nel.org>,
        Krzysztof Kozlowski
 <krzk@...nel.org>, robh@...nel.org,
        krzk+dt@...nel.org, conor+dt@...nel.org, agross@...nel.org,
        andersson@...nel.org, lumag@...nel.org,
        dmitry.baryshkov@....qualcomm.com, konradybcio@...nel.org,
        daniel.lezcano@...aro.org, sboyd@...nel.org, amitk@...nel.org,
        thara.gopinath@...il.com, lee@...nel.org, rafael@...nel.org,
        subbaraman.narayanamurthy@....qualcomm.com,
        david.collins@....qualcomm.com, anjelique.melendez@....qualcomm.com,
        kamal.wadhwa@....qualcomm.com, rui.zhang@...el.com,
        lukasz.luba@....com, devicetree@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
        cros-qcom-dts-watchers@...omium.org, quic_kotarake@...cinc.com,
        neil.armstrong@...aro.org, stephan.gerhold@...aro.org,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>
Subject: Re: [PATCH V7 3/5] dt-bindings: iio: adc: Add support for QCOM PMIC5
 Gen3 ADC

Hi Krzysztof,

On 10/4/2025 12:22 PM, Krzysztof Kozlowski wrote:
> On Sat, 4 Oct 2025 at 11:42, Jishnu Prakash
> <jishnu.prakash@....qualcomm.com> wrote:
>>
>> Hi Jonathan,
>>
>> On 9/27/2025 7:17 PM, Jonathan Cameron wrote:
>>> On Fri, 19 Sep 2025 20:17:43 +0530
>>> Jishnu Prakash <jishnu.prakash@....qualcomm.com> wrote:
>>>
>>>> Hi Krzysztof,
>>>>
>>>> On 9/18/2025 5:45 AM, Krzysztof Kozlowski wrote:
>>>>> On 18/09/2025 04:47, Jishnu Prakash wrote:
>>>>>> Hi Krzysztof,
>>>>>>
>>>>>> On 9/17/2025 5:59 AM, Krzysztof Kozlowski wrote:
>>>>>>> On 16/09/2025 16:28, Jishnu Prakash wrote:
>>>>>>>>> You cannot have empty spaces in ID constants. These are abstract
>>>>>>>>> numbers.
>>>>>>>>>
>>>>>>>>> Otherwise please point me to driver using this constant.
>>>>>>>>
>>>>>>>> These constants are for ADC channel numbers, which are fixed in HW.
>>>>>>>>
>>>>>>>> They are used in this driver: drivers/iio/adc/qcom-spmi-adc5-gen3.c,
>>>>>>>> which is added in patch 4 of this series.
>>>>>>>>
>>>>>>>> They can be found in the array named adc5_gen3_chans_pmic[].
>>>>>>>
>>>>>>> Really? So point me to the line there using ADC5_GEN3_VREF_BAT_THERM.
>>>>>>>
>>>>>>
>>>>>> We may not be using all of these channels right now - we can add them
>>>>>> later based on requirements coming up. For now, I'll remove the channels
>>>>>> not used in adc5_gen3_chans_pmic[].
>>>>>
>>>>> You are not implementing the feedback then. Please read it carefully.
>>>>>
>>>>
>>>> Sorry, I misunderstood - so you actually meant I should remove the
>>>> empty spaces in the definitions, like this?
>>>>
>>>> -#define ADC5_GEN3_VREF_BAT_THERM               0x15
>>>> +#define ADC5_GEN3_VREF_BAT_THERM 0x15
>>>>
>>>> I thought this at first, but I somehow doubted this later, as I saw some
>>>> other recently added files with empty spaces in #define lines, like:
>>>>
>>>> include/dt-bindings/iio/adc/mediatek,mt6373-auxadc.h
>>>> include/dt-bindings/regulator/st,stm32mp15-regulator.h
>>>>
>>>> I can make this change, if you prefer this. Please let me know
>>>> if I'm still missing something.
>>>>
>>>> Also please let me know if you want me to remove the unused
>>>> channels - I would prefer to keep them if there's no issue,
>>>> as we might need them later.
>>>>
>>> He is referring to 0x14 and below not being defined values.  So what
>>> do they mean if they turn up in the DT?
>>>
>>
>> Thanks for your clarification. To address your first point above, the macros
>> added here only represent the ADC channel numbers which are supported for
>> ADC5 Gen3 devices. If there are numbers missing in between (like 0x14),
>> that is because there exist no valid ADC channels in HW matching those
>> channel numbers.
>>
>> For your question above, if any of the undefined channels are used in the DT,
>> they should ideally be treated as invalid when parsed in the driver probe and
>> lead to an error. When I checked the code again, I saw we do not have such an
>> explicit check right now, so I will add that in the next patch series.
>>
>> And to be clear on which channel numbers are supported, I think it may be
>> best if, for now, we only add support for the channel numbers referenced in
>> the array adc5_gen3_chans_pmic[] in drivers/iio/adc/qcom-spmi-adc5-gen3.c.
>>
>> There are only 18 channel numbers used in this array and I would remove
>> all channels except for these from the binding files. During parsing, we
>> would use this array to confirm if an ADC channel added in DT is supported.
>>
>> In case we need to add support for any more channels later, we could add
>> their macros in the binding file and update the array correspondingly at
>> that time.
>>
>> Does all this sound fine? Please let me know if you have any more concerns
>> or queries.
> 
> No, it doesn't.  You keep ignoring my arguments and responding to
> something else. I prefer not to store hardware values as bindings,
> because these are not bindings (and you failed to prove which SW
> interface they bind) and it's really not necessary.

In my previous replies in this thread, I missed mentioning that the macros
defined in include/dt-bindings/iio/adc/qcom,spmi-vadc.h are also used in
other places than the driver file - they are also used in the PMIC-specific
binding files added in this patch, for channel definitions. Considering
one channel for example:
 
We have this in include/dt-bindings/iio/adc/qcom,spmi-vadc.h:
+#define ADC5_GEN3_DIE_TEMP			0x03
 
The above is used in include/dt-bindings/iio/adc/qcom,pm8550vx-adc5-gen3.h:
+#define PM8550VS_ADC5_GEN3_DIE_TEMP(sid)			((sid) << 8 | ADC5_GEN3_DIE_TEMP)
 
And the above definition may be used in device tree, like in the example added
in Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5-gen3.yaml:
 
+        channel@203 {
+          reg = <PM8550VS_ADC5_GEN3_DIE_TEMP(2)>;
+          label = "pm8550vs_c_die_temp";
+          qcom,pre-scaling = <1 1>;
+        };

Referencing the same macros in driver and device tree should also help with
readability and lower the chances of accidental wrong configurations.
Based on this, can we consider ADC5_GEN3_DIE_TEMP is a valid binding and keep
it in place?
 
If not, and if you want the ADC5_GEN3_DIE_TEMP definition removed from bindings,
I can see two ways to do this:
 
1. Keep the PMIC-specific binding definitions, making updates to them like this:
 
-#define PM8550VS_ADC5_GEN3_DIE_TEMP(sid)			((sid) << 8 | ADC5_GEN3_DIE_TEMP)
+#define PM8550VS_ADC5_GEN3_DIE_TEMP(sid)			((sid) << 8 | 0x03)
 
and use the same macros in device tree, like above.
 
2. Drop the PMIC-specific binding definitions completely and update reg property like this:

-          reg = <PM8550VS_ADC5_GEN3_DIE_TEMP(2)>;
+          reg = <0x203>;
 
Which way would you prefer here?

Thanks,
Jishnu

Powered by blists - more mailing lists