Re: [PATCH V7 3/5] dt-bindings: iio: adc: Add support for QCOM PMIC5 Gen3 ADC

[Jishnu Prakash <jishnu.prakash@oss.qualcomm.com>]

Hi Krzysztof,

On 10/27/2025 10:00 PM, Krzysztof Kozlowski wrote:
> On 22/10/2025 13:02, Konrad Dybcio wrote:
>>>>>>>>
>>>>>>>> On 10/4/2025 12:22 PM, Krzysztof Kozlowski wrote:
>>>>>>>>> On Sat, 4 Oct 2025 at 11:42, Jishnu Prakash
>>>>>>>>> <jishnu.prakash@oss.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@oss.qualcomm.com> wrote:
>>>>
>>>> [...]
>>>>
>>>>>> Can you please provide your suggestions on changes we can make
>>>>>> in the above points ?
>>>>>
>>>>> You just pasted DT. I asked about SW, software. Please read carefully
>>>>> previous comments.
>>>>
>>>> Is the problem that Jishnu included some indices in dt-bindings without
>>>> also adding them in the driver's adc5_gen3_chans_pmic[] array?
>>>>
>>>> As in, would the resolution to this thread be simply handling all of
>>>> them in the driver correctly?
>>>
>>> The solution is to remove them from the bindings, just like we do with
>>> many other hardware constants. Of course if these are not hardware
>>> constants, but part of ABI, then solution would be different but no one
>>> provided proof or argument that this is any binding. All proofs were
>>> "but I want to use it in my DTS", which proofs nothing. Not a binding.
>>>
>>> While this issue is not that important, we keep discussing it because
>>> author does not try to understand the problem or even keep up the
>>> discussion. Instead repeats the same without really reading my
>>> messages... and then disappears for month or more.
>>
>> In Bulgaria, people shake their heads left to right to say "yes", and
>> up&down to say "no" (or so I've heard).. I feel like we're having a
>> similar situation here..
>>
>> I'll try to make a case for keeping these defines in some form.
>> Here's hopefully all the related aspects, condensed down:
>>
>> 1. In a multi-PMIC setup, only the main PMIC's ADC is accessible by the OS.
>>    It then mediates accesses to secondary PMICs' ADCs through internal
>>    mechanisms, which requires the SID of the target to be retrieved and written
>>    to a register, along with the physical index of the desired channel to be
>>    measured (see patch 3/5 commit msg).
> 
> 
> SID is still a hardware value, right? Combination of two hardware values
> is still a hardware value, not a software ABI construct. Even if you
> claim only the driver can decode it. These are still the hardware values.
> 
> If you had two IIO cells - one for SID and one for ADC channel - would
> you claim they are both needed for software? Probably not.
> 
> io-channels = <&pm8550_adc SID_whatever CHANNEL_XYZ>
> 
> It's basically the same as some pin muxes, like NXP:
> git grep MX8MM_IOMUXC_GPIO1_IO04_GPIO1_IO4
> 
> Complex value, which driver parses. Is it SW construct? No. These are
> register values.
> 
> 
>>
>> 2. The PMIC SIDs are fixed per board and are the values of PMIC top-level
>>    nodes' reg property (since forever)
>>
>> 3. The channel indices are fixed in HW, but this patchset proposed to reuse
>>    them for logical mappings consumed through io-channels = <> as well (because
>>    of 1.), with the drivers taking the lower 8 bits that of reg/io-channels[1]
>>    value as the ADC channel id and the higher 8 bits as the SID (this is the
>>    define macros with an argument)
>>
>> 4. Fixing 3. in a "simply define all possible options and bind them to
>>    consecutive integers" fashion would require a huge table matching 0..n to
>>    [0-max_sid][0-max_chan] which is unreasonable
> 
> I do not insist on fixing anything or changing the interface. I only
> question their necessity to be a binding.
> 
>>
>> The alternative to the SID packing would be to reference the target PMIC
>> somehow, be it by referencing the PMIC itself:
>>
>> io-channels = <&pm8550_adc &pmr735a CHANNEL_XYZ>
>>
>> or by creating a faux node for the actual inaccessible ADC onboard each of
>> the PMICs:
>>
>> io-channels = <&pm8550_adc &pmr735a_adc CHANNEL_XYZ>
>>
>> and have the OS retrieve the SID from the DT node & encode that value instead
>> of hardcoding it in the DT, leaving just the actual channel IDs in dt-bindings.
>>
>>
>> The define macros without an argument do specify physical channel indices, but
>> we do need some sort of an identifier to put into io-channels (which is why this
>> lives in dt-bindings in the first place), and a 1:1 mapping to the physical id
>> sounds like a good option.
>>
>>
>> I don't think anyone objects to any of these resolutions, so long as they
>> are acceptable from your side
> 
> You can go these ways, but I never proposed to change the interface.
> Just don't store these as bindings, so they don't have to be treated as
> ABI, because they are not. They do not constitute the contract between
> software (driver) and DTS. Treat them the same as we treat all hardware
> constants - directly encoded or moved to DTS headers (which we did for
> many of such cases already - see commit
> 9d9292576810d0b36897718c24dfbc1a2835314b 3 years ago and other)
> 

Thanks for your explanation and the above suggestion and example,
it's clear now. I'll move the macros we need into a DTS header file.


Also to Lee/Rob/Jonathan, as you had given your Acked-by tags on
patch 1 - based on the discussion in this thread, I think it's
also best to update patch 1 similarly now, to move the ADC channel macros
from the include/dt-bindings/iio folder to the arch/arm(64)/boot/dts/qcom
folders. I'll make this update in the next version, please have a look
at it too.

Thanks,
Jishnu

> Best regards,
> Krzysztof