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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251207165349.72f80659@jic23-huawei>
Date: Sun, 7 Dec 2025 16:53:49 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Jishnu Prakash <jishnu.prakash@....qualcomm.com>
Cc: robh@...nel.org, krzysztof.kozlowski@...aro.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 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). 

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.

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


>