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

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? 


> >> -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.

Thanks,

Jonathan