Re: [PATCH 2/7] iio: adc: qcom-spmi-adc5-gen3: Add support for QCOM PMIC5 Gen4 ADC

Hi Jonathan,

On 8/7/2026 4:48 AM, Jonathan Cameron wrote:
> On Thu, 6 Aug 2026 16:22:16 +0530
> Jishnu Prakash <[email protected]> wrote:
> 
>> Hi Jonathan,
>>
>> On 8/3/2026 5:44 AM, Jonathan Cameron wrote:
>>> On Fri, 31 Jul 2026 23:36:15 +0530
>>> Jishnu Prakash <[email protected]> wrote:
>>>   
>>>> PMIC5 Gen4 ADC is similar to PMIC5 Gen3 ADC, with several changes made
>>>> for improved performance, mostly at the hardware level.
>>>>
>>>> One significant software change is that ratiometric conversion resolution
>>>> has been increased from 14 bits to 16 bits, so the maximum value of
>>>> these measurements needs to be updated for Gen4. Add a new scaling
>>>> function for thermistor channels which use this type of conversion.
>>>>
>>>> In the latest PMIC arbiter version (v8), there can be up to 4 buses
>>>> under the PMIC arbiter and 32 PMICs under each bus. In order to
>>>> support communication between ADC on the master PMIC and ADCs on any
>>>> of the other PMICs, a field of width 2 bits is added for bus index
>>>> and the bits for SID are extended from 4 to 5 bits, in the SID
>>>> register. Add support for this.
>>>>
>>>> In addition, it is possible that the master PMIC has ADC of one generation
>>>> and it needs to communicate with another PMIC with ADC of a different
>>>> generation.  
>>>
>>> "Possible" sounds a bit hypothetical.  Can we state this actually happens
>>> on some devices?  
>>
>> Considering existing upstream platforms, this is applicable for SM8750. I'll
>> mention this in the next version of the series.
> 
> What happens on that platform today?  Wrong values?  Should this be treated
> as a fix rather than a feature? 

There is no issue on that platform right now as ADC channels needing this
support are not added yet. SM8750 board files only include pmk8550.dtsi, which
has the top-level Gen3 ADC peripheral and some ADC channels on the same PMIC.
Gen4 support would be needed only when PMIH0108 ADC channels are added.

> 
> 
>>>> -static int adc5_gen3_read_voltage_data(struct adc5_chip *adc, u16 *data)
>>>> +static int adc5_gen3_read_voltage_data(struct adc5_chip *adc,
>>>> +				       struct adc5_channel_common_prop *prop,
>>>> +				       u16 *data)
>>>>  {
>>>>  	u8 rslt[2];
>>>>  	int ret;
>>>> @@ -99,8 +101,10 @@ static int adc5_gen3_read_voltage_data(struct adc5_chip *adc, u16 *data)
>>>>  	*data = get_unaligned_le16(rslt);
>>>>  
>>>>  	if (*data == ADC5_USR_DATA_CHECK) {  
>>> This feels backwards.   We first check for the top bit being
>>> set then check whether we care if it is set or not.  I think
>>> checking if we should look at it first makes more sense.  Also
>>> shorter max line length:
>>>
>>> 	if (!(prop->generation == ADC5_GEN4 && prop->cal_method == ADC5_RATIOMETRIC_CAL)) {
>>> 		if (*data == ADC5_USR_DATA_CHECK) {
>>> 	...	  
>>
>> I thought it was more efficient to have the check like this because
>> there may be several channels satisfying the longer if() check for Gen4
>> ratiometric channels, but the chance of *data being exactly equal to
>> the error value ADC5_USR_DATA_CHECK is lower generally. So if I reverse
>> the checks as you suggested, the outer if() check would pass for every
>> read done on a channel which is not both Gen4 and ratiometric, which
>> seems inefficient for an error check.
>>
>> What do you think, should I make any changes here?
> 
> Efficiency here is going to be hard to judge without guessing how good
> the prefetchers are and to me this doesn't smell like a performance
> critical path.   I'd go for clarity of code and check only
> for the invalid value after you've established it might actually be
> invalid. So change it.

Sure, I'll make this change.

Thanks,
Jishnu

> 
> Thanks,
> 
> Jonathan