Message-ID: <20251221190010.2d111e0e@jic23-huawei>
Date: Sun, 21 Dec 2025 19:00:10 +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, 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
Subject: Re: [PATCH V8 3/4] iio: adc: Add support for QCOM PMIC5 Gen3 ADC

On Fri, 19 Dec 2025 18:45:32 +0530
Jishnu Prakash <jishnu.prakash@....qualcomm.com> wrote:

> Hi Jonathan,
> 
> On 12/7/2025 10:23 PM, Jonathan Cameron wrote:
> > On Thu, 27 Nov 2025 19:10:35 +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 Jishnu
> > 
> > Biggest thing I noticed on a fresh review is that you include
> > very few headers.  This only compiles (I think) because of lots
> > of deeply nested includes.  General principle in kernel code is
> > to follow IWYU approach with a few exceptions.  That makes code
> > much less prone to changes deep in the header hierarchy.
> > 
> > You can even use the tooling that exists for clang to give you suggestions
> > though search around for config files (I posted one a long time back)
> > that reduce the noise somewhat.
> > 
> > Jonathan
> > 
> >   
> >> diff --git a/drivers/iio/adc/qcom-adc5-gen3-common.c b/drivers/iio/adc/qcom-adc5-gen3-common.c
> >> new file mode 100644
> >> index 000000000000..46bb09424f22
> >> --- /dev/null
> >> +++ b/drivers/iio/adc/qcom-adc5-gen3-common.c
> >> @@ -0,0 +1,107 @@
> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +/*
> >> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> >> + *
> >> + * Code shared between the main and auxiliary Qualcomm PMIC voltage ADCs
> >> + * of type ADC5 Gen3.
> >> + */
> >> +
> >> +#include <linux/bitfield.h>
> >> +#include <linux/delay.h>
> >> +#include <linux/iio/adc/qcom-adc5-gen3-common.h>
> >> +#include <linux/regmap.h>  
> > This seems like very light set of includes.
> > If nothing else should be seeing linux/types.h I think
> > 
> > In general try to follow include what you use principles (loosely as some
> > conventions exit for not including particular headers). 
> >   
> 
> I have a question about this - I'm including some header files in my
> newly added common header file too (include/linux/iio/adc/qcom-adc5-gen3-common.h).
> Do I need to repeat those in the driver files where this header is already
> included?

Yes. If things defined in those headers are used directly in this
c code. 

Just because they are in the header today, doesn't mean they will be 
after some future change and we shouldn't make that sort of future
change harder by requiring people look at all the files that include your
header with those includes.

It's also fairly common for stuff to be needed in the header that isn't needed
directly in the c file (maybe cause it's only needed for a macro). In those
cases no need to include in the c file.

Jonathan

Powered by blists - more mailing lists