Message-ID: <20250301032519.16e77288@jic23-huawei>
Date: Sat, 1 Mar 2025 03:25:19 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Jishnu Prakash <jishnu.prakash@....qualcomm.com>
Cc: robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 agross@...nel.org, andersson@...nel.org, dmitry.baryshkov@...aro.org,
 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,
 quic_kamalw@...cinc.com, rui.zhang@...el.com, lukasz.luba@....com,
 lars@...afoo.de, 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_skakitap@...cinc.com, neil.armstrong@...aro.org
Subject: Re: [PATCH V5 4/5] iio: adc: Add support for QCOM PMIC5 Gen3 ADC

On Wed, 26 Feb 2025 14:22:05 +0530
Jishnu Prakash <jishnu.prakash@....qualcomm.com> wrote:

> Hi Jonathan,
> 
> On 2/1/2025 5:41 PM, Jonathan Cameron wrote:
> > On Sat,  1 Feb 2025 00:02:41 +0530
> > Jishnu Prakash <jishnu.prakash@....qualcomm.com> wrote:
> >   
> >> The ADC architecture on PMIC5 Gen3 is similar to that on PMIC5 Gen2,
> >> with all SW communication to ADC going through PMK8550 which
> >> communicates with other PMICs through PBS.
> >>
> >> One major difference is that the register interface used here is that
> >> of an SDAM (Shared Direct Access Memory) peripheral present on PMK8550.
> >> There may be more than one SDAM used for ADC5 Gen3 and each has eight
> >> channels, which may be used for either immediate reads (same functionality
> >> as previous PMIC5 and PMIC5 Gen2 ADC peripherals) or recurring measurements
> >> (same as ADC_TM functionality).
> >>
> >> By convention, we reserve the first channel of the first SDAM for all
> >> immediate reads and use the remaining channels across all SDAMs for
> >> ADC_TM monitoring functionality.
> >>
> >> Add support for PMIC5 Gen3 ADC driver for immediate read functionality.
> >> ADC_TM is implemented as an auxiliary thermal driver under this ADC
> >> driver.
> >>
> >> Signed-off-by: Jishnu Prakash <jishnu.prakash@....qualcomm.com>  
> > Hi,
> > 
> > A few minor things inline.  One general one is keep to under 80 chars
> > for line wrap unless going over that makes a significant improvement
> > to readability.
> > 
> > Jonathan
> >   
> >> ---
> >> Changes since v4:
> >> - Moved out common funtions from newly added .h file into a separate .c
> >>   file to avoid duplicating them. Updated interrupt name as suggested
> >>   by reviewer. Updated namespace export symbol statement to have a string
> >>   as second argument to follow framework change.
> >>  
> 
> ...
> 
> >> +
> >> +			if (!conv_req)
> >> +				return 0;
> >> +		}
> >> +
> >> +		usleep_range(ADC5_GEN3_HS_DELAY_MIN_US, ADC5_GEN3_HS_DELAY_MAX_US);  
> > fsleep() perhaps as I doubt the extra tolerance that will give will matter
> > much.  
> >> +	}
> >> +
> >> +	pr_err("Setting HS ready bit timed out, sdam_index:%d, status:%#x\n", sdam_index, status);
> >> +	return -ETIMEDOUT;
> >> +}
> >> +EXPORT_SYMBOL(adc5_gen3_poll_wait_hs);  
> > 
> > At some point may be worth namespacing all these exports.
> > Probably not in this series though!  
> 
> In the main driver file (qcom-spmi-adc5-gen3.c), I have already exported some functions to a namespace ("QCOM_SPMI_ADC5_GEN3"),
> which is imported in the auxiliary driver file (qcom-spmi-adc-tm5-gen3.c).
> 
> Do you think I should export these functions to the same or a different namespace? Or should we check this later?
Later is fine.

> >> +void adc5_take_mutex_lock(struct device *dev, bool lock)
> >> +{
> >> +	struct iio_dev *indio_dev = dev_get_drvdata(dev->parent);
> >> +	struct adc5_chip *adc = iio_priv(indio_dev);
> >> +
> >> +	if (lock)
> >> +		mutex_lock(&adc->lock);
> >> +	else
> >> +		mutex_unlock(&adc->lock);
> >> +}
> >> +EXPORT_SYMBOL_NS_GPL(adc5_take_mutex_lock, "QCOM_SPMI_ADC5_GEN3");  
> > 
> > This is potentially going to make a mess for sparse.  Might be better to split
> > it in two so you can had __acquires and __releases markings.
> > 
> > If you don't get any warnings with sparse then I guess we are fine.
> >   
> 
> I had tried building with sparse in my local workspace and I did not get any errors in this file. Do you think I can keep this unchanged?
> Also, would any kernel bots run sparse later on this patch, if it's not already done?

Problems around this tend to turn up a bit late in build tests as requires
particular combinations of features.  Here you may not see problems because
sparse can't see far enough to understand the locking.

I would still split this into lock / unlock as that matches better
with common syntax for locks.  We can then add markings
as necessary later.

> >> +/*  
> > 
> > Looks like valid kernel doc, so /** and check it builds fine
> > with the kernel-doc script.
> >   
> >> + * struct adc5_channel_prop - ADC channel property.
> >> + * @channel: channel number, refer to the channel list.
> >> + * @cal_method: calibration method.
> >> + * @decimation: sampling rate supported for the channel.
> >> + * @sid: slave id of PMIC owning the channel.  
> > 
> > In common with most of the kernel, if there is another name that
> > can be used, I'd prefer avoiding that term.
> > ID probably fine for example or leave it ambiguous as SID
> >   
> 
> Just to be sure, does this look fine?
> 
> @sid: ID of PMIC owning the channel.
Ok.
> 
> I'll address all your other comments in the next patch series.
> 
When replying with feedback on some items crop out the rest
of the email just to maintain relevant context.
Saves time and makes less likely important parts of your reply
might be missed.

thanks,

Jonathan

> Thanks,
> Jishnu

Powered by blists - more mailing lists