On Tue, 26 Aug 2025 14:06:56 +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,
A few additional comments from a fresh read through.
Thanks,
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..c84e75859958
> --- /dev/null
> +++ b/drivers/iio/adc/qcom-adc5-gen3-common.c
> @@ -0,0 +1,106 @@
> +// 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>
Follow (approximate) include what you use principles for includes.
So here I'd expect to at least see a regmap include.
> +
> +int adc5_gen3_read(struct adc5_device_data *adc, unsigned int sdam_index,
> + u16 offset, u8 *data, int len)
> +{
> + return regmap_bulk_read(adc->regmap,
> + adc->base[sdam_index].base_addr + offset,
> + data, len);
> +}
> +EXPORT_SYMBOL_NS_GPL(adc5_gen3_read, "QCOM_SPMI_ADC5_GEN3");
> +
> +int adc5_gen3_write(struct adc5_device_data *adc, unsigned int sdam_index,
> + u16 offset, u8 *data, int len)
> +{
> + return regmap_bulk_write(adc->regmap,
> + adc->base[sdam_index].base_addr + offset,
> + data, len);
> +}
> +EXPORT_SYMBOL_NS_GPL(adc5_gen3_write, "QCOM_SPMI_ADC5_GEN3");
> 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..f01a56363389
> --- /dev/null
> +++ b/drivers/iio/adc/qcom-spmi-adc5-gen3.c
> +
> +#define ADC5_GEN3_READ_CONFIG_REGS 7
> +
> +static int adc5_gen3_configure(struct adc5_chip *adc,
> + struct adc5_channel_common_prop *prop)
> +{
> + u8 buf[ADC5_GEN3_READ_CONFIG_REGS];
> + u8 conv_req = 0;
> + int ret;
> +
> + ret = adc5_gen3_read(&adc->dev_data, ADC5_GEN3_VADC_SDAM, ADC5_GEN3_SID,
> + buf, sizeof(buf));
> + if (ret)
> + return ret;
> +
> + /* Write SID */
> + buf[0] = FIELD_PREP(ADC5_GEN3_SID_MASK, prop->sid);
> +
> + /*
> + * Use channel 0 by default for immediate conversion and
> + * to indicate there is an actual conversion request
Wrap to 80 chars, not 68ish.
> + */
> +static irqreturn_t adc5_gen3_isr(int irq, void *dev_id)
> +{
> + u8 status, tm_status[2], eoc_status, val;
> + struct adc_tm5_auxiliary_drv *adrv_tm;
> + struct adc5_chip *adc = dev_id;
> + struct device *dev = adc->dev;
> + struct auxiliary_device *adev;
> + int ret;
> +
> + ret = adc5_gen3_read(&adc->dev_data, ADC5_GEN3_VADC_SDAM,
> + ADC5_GEN3_STATUS1, &status, sizeof(status));
> + if (ret) {
> + dev_err(dev, "adc read status1 failed with %d\n", ret);
> + return IRQ_HANDLED;
> + }
> +
> + ret = adc5_gen3_read(&adc->dev_data, ADC5_GEN3_VADC_SDAM,
> + ADC5_GEN3_EOC_STS, &eoc_status, sizeof(eoc_status));
> + if (ret) {
> + dev_err(dev, "adc read eoc status failed with %d\n", ret);
> + return IRQ_HANDLED;
> + }
> +
> + if (status & ADC5_GEN3_STATUS1_CONV_FAULT) {
> + dev_err_ratelimited(dev,
> + "Unexpected conversion fault, status:%#x, eoc_status:%#x\n",
> + status, eoc_status);
> + val = ADC5_GEN3_CONV_ERR_CLR_REQ;
> + adc5_gen3_status_clear(&adc->dev_data, ADC5_GEN3_VADC_SDAM,
> + ADC5_GEN3_CONV_ERR_CLR, &val, 1);
> + return IRQ_HANDLED;
> + }
> +
> + /* CHAN0 is the preconfigured channel for immediate conversion */
> + if (eoc_status & ADC5_GEN3_EOC_CHAN_0)
> + complete(&adc->complete);
> +
> + ret = adc5_gen3_read(&adc->dev_data, ADC5_GEN3_VADC_SDAM,
> + ADC5_GEN3_TM_HIGH_STS, tm_status, sizeof(tm_status));
> + if (ret) {
> + dev_err(dev, "adc read TM status failed with %d\n", ret);
> + return IRQ_HANDLED;
> + }
> +
> + dev_dbg(dev, "Interrupt status:%#x, EOC status:%#x, high:%#x, low:%#x\n",
> + status, eoc_status, tm_status[0], tm_status[1]);
> +
> + if (tm_status[0] || tm_status[1]) {
> + adev = adc->tm_aux;
> + if (!adev || !adev->dev.driver) {
> + dev_err(dev, "adc_tm auxiliary device not initialized\n");
> + return IRQ_HANDLED;
> + }
> +
> + adrv_tm = container_of(adev->dev.driver,
> + struct adc_tm5_auxiliary_drv,
> + adrv.driver);
> +
> + if (adrv_tm && adrv_tm->tm_event_notify)
Container_of is never going to return NULL unless the offset is 0 and the thing
passed in is null (already checked above).
Also flip this to keep the error as the only out of line bit.
if (!adrv_tm->tm_event_notify) {
dev_err(dev, "adc_tm auxiliary driver not initialized\n");
return IRQ_HANDLED;
}
adrv_tm->tm_event_notify(adev);
}
> + adrv_tm->tm_event_notify(adev);
> + else
> + dev_err(dev, "adc_tm auxiliary driver not initialized\n");
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int adc5_gen3_get_fw_channel_data(struct adc5_chip *adc,
> + struct adc5_channel_prop *prop,
> + struct fwnode_handle *fwnode)
> +{
> + const char *name = fwnode_get_name(fwnode);
> + const struct adc5_data *data = adc->data;
> + u32 chan, value, varr[2], sid = 0;
Why initialize sid?
> + struct device *dev = adc->dev;
> + const char *channel_name;
> + int ret;
> +
> + ret = fwnode_property_read_u32(fwnode, "reg", &chan);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "invalid channel number %s\n",
> + name);
> +
> + /*
> + * Value read from "reg" is virtual channel number
> + * virtual channel number = sid << 8 | channel number
> + */
> + sid = FIELD_GET(ADC5_GEN3_VIRTUAL_SID_MASK, chan);
> + chan = FIELD_GET(ADC5_GEN3_CHANNEL_MASK, chan);
> + return 0;
> +}
