Message-ID: <6012ad64-3a10-4f05-9f37-f2d0c36df582@oss.qualcomm.com>
Date: Fri, 6 Feb 2026 18:45:02 +0530
From: Jishnu Prakash <jishnu.prakash@....qualcomm.com>
To: Jonathan Cameron <jic23@...nel.org>
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 V10 3/4] iio: adc: Add support for QCOM PMIC5 Gen3 ADC

Hi Jonathan,

On 1/31/2026 11:09 PM, Jonathan Cameron wrote:
> On Fri, 30 Jan 2026 17:24:20 +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
> 
> Whilst there are a couple of comments below, I think this is ready to go.
> Unfortunately this is just a few days too late to merge for this coming
> cycle as I need to send the IIO pull request today or tomorrow (due to going
> through char-misc) so this would get no soak time in next.
> 
> Also, I'm not sure how we actually want to merge this given close coupling with
> the thermal driver.  Perhaps best bet is I do an immutable branch of next rc1
> once available that we pull into both trees. That would have the first 3 patches
> on it. 
> 
> Jonathan
> 
>> 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..f8168a14b907
>> --- /dev/null
>> +++ b/drivers/iio/adc/qcom-spmi-adc5-gen3.c
>> @@ -0,0 +1,860 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
>> + */
>> +
>> +#include <linux/auxiliary_bus.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/bits.h>
>> +#include <linux/cleanup.h>
>> +#include <linux/completion.h>
>> +#include <linux/container_of.h>
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
> If you happen to be spinning again for some reason, I think you might be able to replace
> this device.h include with a forwards declaration of struct device;
> 
> If you can that would be good as we are trying to reduce includes of these
> mega headers.

I have a query about this. From what I understand, having a forwards
declaration of struct device may be valid if we only use the device struct
definition directly, but not if we try to dereference any of the struct's members.

I see that at some places in this file(qcom-spmi-adc5-gen3.c), we dereference
the device's parent:


One example:
void adc5_gen3_mutex_lock(struct device *dev)
{
    struct iio_dev *indio_dev = dev_get_drvdata(dev->parent);
    ....


Another example:
static int adc5_gen3_probe(struct platform_device *pdev)
{
    struct device *dev = &pdev->dev;
    regmap = dev_get_regmap(dev->parent, NULL);
    ....

We do not dereference the device's parent in the other new files
(qcom-adc5-gen3-common.h and qcom-spmi-adc-tm5-gen3.c), so
I can drop the device.h inclusion and use a forward declaration
of struct device in them.


In this file, what would you suggest I do, keep or drop the
device.h inclusion? 

I do see that device.h is included in auxiliary_bus.h,
iio.h and platform_device.h, so chances may be low that
it is somehow dropped later from all of those files and
will need to be explicitly included here.


Also, is it alright if I push the next series to address
your comments immediately? Would you be able to include patches 1-3
in the immutable branch you mentioned, once it's available?


> 
>> +#include <linux/device/devres.h>
>> +#include <linux/dev_printk.h>
>> +#include <linux/err.h>
>> +#include <linux/export.h>
>> +#include <linux/iio/adc/qcom-adc5-gen3-common.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/mutex.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/property.h>
>> +#include <linux/regmap.h>
>> +#include <linux/types.h>
>> +#include <linux/unaligned.h>
> 
> 
>> +static int adc5_gen3_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct iio_dev *indio_dev;
>> +	struct adc5_chip *adc;
>> +	struct regmap *regmap;
>> +	int ret, i;
>> +	u32 *reg;
>> +
>> +	regmap = dev_get_regmap(dev->parent, NULL);
>> +	if (!regmap)
>> +		return -ENODEV;
>> +
>> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*adc));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +
>> +	adc = iio_priv(indio_dev);
>> +	adc->dev_data.regmap = regmap;
>> +	adc->dev = dev;
>> +
>> +	ret = device_property_count_u32(dev, "reg");
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	adc->dev_data.num_sdams = ret;
>> +
>> +	reg = devm_kcalloc(dev, adc->dev_data.num_sdams, sizeof(u32),
>> +			   GFP_KERNEL);
>> +	if (!reg)
>> +		return -ENOMEM;
>> +
>> +	ret = device_property_read_u32_array(dev, "reg", reg,
>> +					     adc->dev_data.num_sdams);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret,
>> +				     "Failed to read reg property\n");
>> +
>> +	adc->dev_data.base = devm_kcalloc(dev, adc->dev_data.num_sdams,
>> +					  sizeof(*adc->dev_data.base),
>> +					  GFP_KERNEL);
>> +	if (!adc->dev_data.base)
>> +		return -ENOMEM;
>> +
>> +	platform_set_drvdata(pdev, indio_dev);
>> +	init_completion(&adc->complete);
>> +	ret = devm_mutex_init(dev, &adc->lock);
> 
> I'd move this stuff up to before you get reg so you can keep all the stuff
> related to num_sdams together.
> 

Will address this in the next patch series.

Thanks,
Jishnu

>> +	if (ret)
>> +		return ret;
>> +
>> +	for (i = 0; i < adc->dev_data.num_sdams; i++) {
>> +		adc->dev_data.base[i].base_addr = reg[i];
>>
> 

Powered by blists - more mailing lists