Re: [PATCH V8 3/4] iio: adc: Add support for QCOM PMIC5 Gen3 ADC

[Jishnu Prakash]

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@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
> 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?

> Sorry I didn't notice this in earlier reviews!
> 
> 
>> diff --git a/drivers/iio/adc/qcom-spmi-adc5-gen3.c b/drivers/iio/adc/qcom-spmi-adc5-gen3.c
>> new file mode 100644
>> index 000000000000..effd4bd49989
>> --- /dev/null
>> +++ b/drivers/iio/adc/qcom-spmi-adc5-gen3.c
> 
>> +/**
>> + * struct adc5_chip - ADC private structure.
>> + * @dev: SPMI ADC5 Gen3 device.
>> + * @dev_data: Top-level ADC device data.
>> + * @nchannels: number of ADC channels.
>> + * @chan_props: array of ADC channel properties.
>> + * @iio_chans: array of IIO channels specification.
>> + * @complete: ADC result notification after interrupt is received.
>> + * @lock: ADC lock for access to the peripheral, to prevent concurrent
>> + * requests from multiple clients.
>> + * @data: software configuration data.
>> + * @n_tm_channels: number of ADC channels used for TM measurements.
>> + * @tm_aux: pointer to auxiliary TM device.
>> + */
>> +struct adc5_chip {
>> +	struct device *dev;
>> +	struct adc5_device_data dev_data;
>> +	unsigned int nchannels;
>> +	struct adc5_channel_prop *chan_props;
>> +	struct iio_chan_spec *iio_chans;
>> +	struct completion complete;
>> +	/*
>> +	 * lock for access to the peripheral, to prevent concurrent requests
>> +	 * from multiple clients.
>> +	 */
> 
> Whilst checkpatch is dumb on this and complains if you don't have a comment
> here feel free to drop it as the one in the kernel-doc is enough.
> 

I'll make this change in the next patch series.

Thanks,
Jishnu


>> +	struct mutex lock;
>> +	const struct adc5_data *data;
>> +	unsigned int n_tm_channels;
>> +	struct auxiliary_device *tm_aux;
>> +};
> 
> 
>>