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

Jishnu Prakash
2026-02-06T13:15:15Z

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@oss.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@oss.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];
>>
>