[PATCH v2 0/2] iio: adc: Add Intel Dollar Cove TI PMIC ADC driver

[PATCH v2 0/2] iio: adc: Add Intel Dollar Cove TI PMIC ADC driver

[Hans de Goede]

Hi,

Here is v2 of my patch to add an IIO driver for the Intel Dollar Cove TI
PMIC ADC. Unlike what I mentioned in the review email thread of v1 I'm
sending this out as a separate series rather then bundling it together
with the power-supply series which adds a consumer of the ADC value:
https://lore.kernel.org/linux-pm/20250721122605.46724-1-hansg@kernel.org/
(in hindsight I think it is better to keep this a separate series).

Changes in v2:
- Add new "iio: Improve iio_read_channel_processed_scale() precision"
  patch to the series
- Add IIO_CHAN_INFO_SCALE and provide ADC scale info for Vbat
- Add IIO_CHAN_INFO_PROCESSED which applies calibration and
  scaling for the VBat channel
- Address some other small review remarks

Regards,

Hans


Hans de Goede (2):
  iio: Improve iio_read_channel_processed_scale() precision
  iio: adc: Add Intel Dollar Cove TI PMIC ADC driver

 drivers/iio/adc/Kconfig           |  11 +
 drivers/iio/adc/Makefile          |   1 +
 drivers/iio/adc/intel_dc_ti_adc.c | 348 ++++++++++++++++++++++++++++++
 drivers/iio/inkern.c              |  24 ++-
 4 files changed, 381 insertions(+), 3 deletions(-)
 create mode 100644 drivers/iio/adc/intel_dc_ti_adc.c

-- 
2.49.0

[PATCH v2 1/2] iio: Improve iio_read_channel_processed_scale() precision

[Hans de Goede]

Before this change iio_read_channel_processed_scale() always assumes that
channels which advertise IIO_CHAN_INFO_PROCESSED capability return
IIO_VAL_INT on success.

Ignoring any fractional values from drivers which return
IIO_VAL_INT_PLUS_MICRO / IIO_VAL_INT_PLUS_NANO. These fractional values
might become non fractional after scaling so these should be taken into
account.

While at it also error out for IIO_VAL_* values which
iio_read_channel_processed_scale() does not know how to handle.

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
Changes in v2:
- New patch in v3 of this patch-series
---
 drivers/iio/inkern.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index c174ebb7d5e6..e9669f552eb3 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -714,18 +714,36 @@ int iio_read_channel_processed_scale(struct iio_channel *chan, int *val,
 				     unsigned int scale)
 {
 	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(chan->indio_dev);
-	int ret;
+	int ret, val2;
 
 	guard(mutex)(&iio_dev_opaque->info_exist_lock);
 	if (!chan->indio_dev->info)
 		return -ENODEV;
 
 	if (iio_channel_has_info(chan->channel, IIO_CHAN_INFO_PROCESSED)) {
-		ret = iio_channel_read(chan, val, NULL,
+		ret = iio_channel_read(chan, val, &val2,
 				       IIO_CHAN_INFO_PROCESSED);
 		if (ret < 0)
 			return ret;
-		*val *= scale;
+
+		switch (ret) {
+		case IIO_VAL_INT:
+			*val *= scale;
+			break;
+		case IIO_VAL_INT_PLUS_MICRO:
+			*val *= scale;
+			*val += div_u64((u64)val2 * scale, 1000000LLU);
+			break;
+		case IIO_VAL_INT_PLUS_NANO:
+			*val *= scale;
+			*val += div_u64((u64)val2 * scale, 1000000000LLU);
+			break;
+		default:
+			dev_err_once(&chan->indio_dev->dev,
+				     "unsupported processed IIO-val-type: %d\n",
+				     ret);
+			return -EINVAL;
+		}
 
 		return ret;
 	} else {
-- 
2.49.0

[PATCH v2 2/2] iio: adc: Add Intel Dollar Cove TI PMIC ADC driver

[Hans de Goede]

Intel has 2 completely different "Dollar Cove" PMICs for its Bay Trail /
Cherry Trail SoCs. One is made by X-Powers and is called the AXP288.
The AXP288's GPADC is already supported by the X-Powers AXP288 ADC driver.

The other "Dollar Cove" PMIC is made by TI and does not have any clear TI
denomination, its MFD driver calls it the "Intel Dollar Cove TI PMIC".

Add a driver for the Intel Dollar Cove TI PMIC's general purpose ADC,
binding to the "chtdc_ti_adc" MFD cell of the MFD driver.

The "cht" in the cell name comes from Cherry Trail, but it turns out that
just like the AXP288 the Intel Dollar Cove TI PMIC is also used with both
Intel Bay Trail and Intel Cherry Trail SoCs, so this new ADC driver does
not include the cht part in its name.

This is loosely based on kernel/drivers/iio/adc/iio_dc_ti_gpadc.c
from the Acer A1-840 Android kernel source-code archive named:
"App. Guide_Acer_20151221_A_A.zip"
which is distributed by Acer from the Acer A1-840 support page:
https://www.acer.com/us-en/support/product-support/A1-840/downloads

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
Changes in v2:
- Add IIO_CHAN_INFO_SCALE and provide ADC scale info for Vbat
- Add IIO_CHAN_INFO_PROCESSED which applies calibration and
  scaling for the VBat channel
- Address some other small review remarks
---
 drivers/iio/adc/Kconfig           |  11 +
 drivers/iio/adc/Makefile          |   1 +
 drivers/iio/adc/intel_dc_ti_adc.c | 348 ++++++++++++++++++++++++++++++
 3 files changed, 360 insertions(+)
 create mode 100644 drivers/iio/adc/intel_dc_ti_adc.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index ea3ba1397392..51a5fc6efbe1 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -723,6 +723,17 @@ config INGENIC_ADC
 	  This driver can also be built as a module. If so, the module will be
 	  called ingenic_adc.
 
+config INTEL_DC_TI_ADC
+	tristate "Intel Bay / Cherry Trail Dollar Cove TI ADC driver"
+	depends on INTEL_SOC_PMIC_CHTDC_TI
+	help
+	  Say yes here to have support for the Dollar Cove TI PMIC ADC device.
+	  Depending on platform configuration, this general purpose ADC can be
+	  used for sensors such as battery voltage and thermal resistors.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called intel_dc_ti_adc.
+
 config INTEL_MRFLD_ADC
 	tristate "Intel Merrifield Basin Cove ADC driver"
 	depends on INTEL_SOC_PMIC_MRFLD
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 09ae6edb2650..da99ba88b4e1 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_IMX8QXP_ADC) += imx8qxp-adc.o
 obj-$(CONFIG_IMX93_ADC) += imx93_adc.o
 obj-$(CONFIG_INA2XX_ADC) += ina2xx-adc.o
 obj-$(CONFIG_INGENIC_ADC) += ingenic-adc.o
+obj-$(CONFIG_INTEL_DC_TI_ADC) += intel_dc_ti_adc.o
 obj-$(CONFIG_INTEL_MRFLD_ADC) += intel_mrfld_adc.o
 obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
 obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
diff --git a/drivers/iio/adc/intel_dc_ti_adc.c b/drivers/iio/adc/intel_dc_ti_adc.c
new file mode 100644
index 000000000000..6f27c45679cc
--- /dev/null
+++ b/drivers/iio/adc/intel_dc_ti_adc.c
@@ -0,0 +1,348 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Intel Dollar Cove TI PMIC GPADC Driver
+ *
+ * Copyright (C) 2014 Intel Corporation (Ramakrishna Pallala <ramakrishna.pallala@intel.com>)
+ * Copyright (C) 2024 - 2025 Hans de Goede <hansg@kernel.org>
+ */
+
+#include <linux/bits.h>
+#include <linux/bitfield.h>
+#include <linux/cleanup.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/intel_soc_pmic.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/wait.h>
+
+#include <linux/iio/driver.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/machine.h>
+
+#define DC_TI_ADC_CNTL_REG			0x50
+#define DC_TI_ADC_START				BIT(0)
+#define DC_TI_ADC_CH_SEL			GENMASK(2, 1)
+#define DC_TI_ADC_EN				BIT(5)
+#define DC_TI_ADC_EN_EXT_BPTH_BIAS		BIT(6)
+
+#define DC_TI_VBAT_ZSE_GE_REG			0x53
+#define DC_TI_VBAT_GE				GENMASK(3, 0)
+#define DC_TI_VBAT_ZSE				GENMASK(7, 4)
+
+/* VBAT GE gain correction is in 0.0015 increments, ZSE is in 1.0 increments */
+#define DC_TI_VBAT_GE_STEP			15
+#define DC_TI_VBAT_GE_DIV			10000
+
+#define DC_TI_ADC_CH0_DATAH_REG			0x54
+#define DC_TI_ADC_CH0_DATAL_REG			0x55
+#define DC_TI_ADC_CH1_DATAH_REG			0x56
+#define DC_TI_ADC_CH1_DATAL_REG			0x57
+#define DC_TI_ADC_CH2_DATAH_REG			0x58
+#define DC_TI_ADC_CH2_DATAL_REG			0x59
+#define DC_TI_ADC_CH3_DATAH_REG			0x5A
+#define DC_TI_ADC_CH3_DATAL_REG			0x5B
+
+enum dc_ti_adc_id {
+	DC_TI_ADC_VBAT,
+	DC_TI_ADC_PMICTEMP,
+	DC_TI_ADC_BATTEMP,
+	DC_TI_ADC_SYSTEMP0,
+};
+
+struct dc_ti_adc_info {
+	struct mutex lock; /* Protects against concurrent accesses to the ADC */
+	wait_queue_head_t wait;
+	struct device *dev;
+	struct regmap *regmap;
+	int vbat_zse;
+	int vbat_ge;
+	bool conversion_done;
+};
+
+static const struct iio_chan_spec dc_ti_adc_channels[] = {
+	{
+		.indexed = 1,
+		.type = IIO_VOLTAGE,
+		.channel = DC_TI_ADC_VBAT,
+		.address = DC_TI_ADC_CH0_DATAH_REG,
+		.datasheet_name = "CH0",
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
+				      BIT(IIO_CHAN_INFO_PROCESSED),
+	}, {
+		.indexed = 1,
+		.type = IIO_TEMP,
+		.channel = DC_TI_ADC_PMICTEMP,
+		.address = DC_TI_ADC_CH1_DATAH_REG,
+		.datasheet_name = "CH1",
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	}, {
+		.indexed = 1,
+		.type = IIO_TEMP,
+		.channel = DC_TI_ADC_BATTEMP,
+		.address = DC_TI_ADC_CH2_DATAH_REG,
+		.datasheet_name = "CH2",
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	}, {
+		.indexed = 1,
+		.type = IIO_TEMP,
+		.channel = DC_TI_ADC_SYSTEMP0,
+		.address = DC_TI_ADC_CH3_DATAH_REG,
+		.datasheet_name = "CH3",
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	}
+};
+
+static struct iio_map dc_ti_adc_default_maps[] = {
+	IIO_MAP("CH0", "chtdc_ti_battery", "VBAT"),
+	IIO_MAP("CH1", "chtdc_ti_battery", "PMICTEMP"),
+	IIO_MAP("CH2", "chtdc_ti_battery", "BATTEMP"),
+	IIO_MAP("CH3", "chtdc_ti_battery", "SYSTEMP0"),
+	{ }
+};
+
+static irqreturn_t dc_ti_adc_isr(int irq, void *data)
+{
+	struct dc_ti_adc_info *info = data;
+
+	info->conversion_done = true;
+	wake_up(&info->wait);
+	return IRQ_HANDLED;
+}
+
+static int dc_ti_adc_scale(struct dc_ti_adc_info *info,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2)
+{
+	if (chan->channel != DC_TI_ADC_VBAT)
+		return -EINVAL;
+
+	/* Vbat ADC scale is 4.6875 mV / unit */
+	*val = 4;
+	*val2 = 687500;
+
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int dc_ti_adc_raw_to_processed(struct dc_ti_adc_info *info,
+				      struct iio_chan_spec const *chan,
+				      int raw, int *val, int *val2)
+{
+	if (chan->channel != DC_TI_ADC_VBAT)
+		return -EINVAL;
+
+	/* Apply calibration */
+	raw -= info->vbat_zse;
+	raw = raw * (DC_TI_VBAT_GE_DIV - info->vbat_ge * DC_TI_VBAT_GE_STEP) /
+	      DC_TI_VBAT_GE_DIV;
+	/* Vbat ADC scale is 4.6875 mV / unit */
+	raw *= 46875;
+
+	/* raw is now in 10000 units / mV, convert to milli + milli/1e6 */
+	*val = raw / 10000;
+	*val2 = (raw % 10000) * 100;
+
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int dc_ti_adc_sample(struct dc_ti_adc_info *info,
+			    struct iio_chan_spec const *chan, int *val)
+{
+	int ret, ch = chan->channel;
+	unsigned int lsb, msb;
+
+	info->conversion_done = false;
+
+	/*
+	 * As per TI (PMIC Vendor), the ADC enable and ADC start commands should
+	 * not be sent together. Hence send the commands separately
+	 */
+	ret = regmap_update_bits(info->regmap, DC_TI_ADC_CNTL_REG,
+				 DC_TI_ADC_EN, DC_TI_ADC_EN);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(info->regmap, DC_TI_ADC_CNTL_REG,
+				 DC_TI_ADC_CH_SEL, FIELD_PREP(DC_TI_ADC_CH_SEL, ch));
+	if (ret)
+		return ret;
+
+	/*
+	 * As per PMIC Vendor, a minimum of 50 micro seconds delay is required
+	 * between ADC Enable and ADC START commands. This is also recommended
+	 * by Intel Hardware team after the timing analysis of GPADC signals.
+	 * Since the I2C Write transaction to set the channel number also
+	 * imparts 25 micro seconds of delay, so we need to wait for another
+	 * 25 micro seconds before issuing ADC START command.
+	 */
+	fsleep(25);
+
+	ret = regmap_update_bits(info->regmap, DC_TI_ADC_CNTL_REG,
+				 DC_TI_ADC_START, DC_TI_ADC_START);
+	if (ret)
+		return ret;
+
+	/* TI (PMIC Vendor) recommends 5 sec timeout for conversion */
+	ret = wait_event_timeout(info->wait, info->conversion_done, 5 * HZ);
+	if (ret == 0) {
+		dev_err(info->dev, "Error sample timeout\n");
+		ret = -ETIMEDOUT;
+		goto disable_adc;
+	}
+
+	/* Reading multiple registers at once is not supported */
+	ret = regmap_read(info->regmap, chan->address, &msb);
+	if (ret)
+		goto disable_adc;
+
+	ret = regmap_read(info->regmap, chan->address + 1, &lsb);
+	if (ret)
+		goto disable_adc;
+
+	*val = ((msb << 8) | lsb) & 0x3ff;
+
+disable_adc:
+	regmap_update_bits(info->regmap, DC_TI_ADC_CNTL_REG,
+			   DC_TI_ADC_START | DC_TI_ADC_EN, 0);
+	return ret;
+}
+
+static int dc_ti_adc_read_raw(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      int *val, int *val2, long mask)
+{
+	struct dc_ti_adc_info *info = iio_priv(indio_dev);
+	int ret;
+
+	if (mask == IIO_CHAN_INFO_SCALE)
+		return dc_ti_adc_scale(info, chan, val, val2);
+
+	guard(mutex)(&info->lock);
+
+	/*
+	 * If channel BPTHERM has been selected, first enable the BPTHERM BIAS
+	 * which provides the VREFT Voltage reference to convert BPTHERM Input
+	 * voltage to temperature.
+	 * As per PMIC Vendor specifications, BPTHERM BIAS should be enabled
+	 * 35 ms before ADC_EN command.
+	 */
+	if (chan->channel == DC_TI_ADC_BATTEMP) {
+		ret = regmap_update_bits(info->regmap, DC_TI_ADC_CNTL_REG,
+					 DC_TI_ADC_EN_EXT_BPTH_BIAS,
+					 DC_TI_ADC_EN_EXT_BPTH_BIAS);
+		if (ret)
+			return ret;
+		msleep(35);
+	}
+
+	ret = dc_ti_adc_sample(info, chan, val);
+
+	if (chan->channel == DC_TI_ADC_BATTEMP)
+		regmap_update_bits(info->regmap, DC_TI_ADC_CNTL_REG,
+				   DC_TI_ADC_EN_EXT_BPTH_BIAS, 0);
+
+	if (ret)
+		return ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_PROCESSED:
+		return dc_ti_adc_raw_to_processed(info, chan, *val, val, val2);
+	}
+
+	return -EINVAL;
+}
+
+static const struct iio_info dc_ti_adc_iio_info = {
+	.read_raw = dc_ti_adc_read_raw,
+};
+
+static int dc_ti_adc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct intel_soc_pmic *pmic = dev_get_drvdata(dev->parent);
+	struct dc_ti_adc_info *info;
+	struct iio_dev *indio_dev;
+	unsigned int reg_val;
+	int irq, ret, val;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*info));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	info = iio_priv(indio_dev);
+
+	ret = devm_mutex_init(dev, &info->lock);
+	if (ret)
+		return ret;
+
+	init_waitqueue_head(&info->wait);
+
+	info->dev = dev;
+	info->regmap = pmic->regmap;
+
+	indio_dev->name = "dc_ti_adc";
+	indio_dev->channels = dc_ti_adc_channels;
+	indio_dev->num_channels = ARRAY_SIZE(dc_ti_adc_channels);
+	indio_dev->info = &dc_ti_adc_iio_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	ret = regmap_read(info->regmap, DC_TI_VBAT_ZSE_GE_REG, &reg_val);
+	if (ret)
+		return ret;
+
+	val = FIELD_GET(DC_TI_VBAT_ZSE, reg_val);
+	if (val >= 8)
+		info->vbat_zse = val - 16;
+	else
+		info->vbat_zse = val;
+
+	val = FIELD_GET(DC_TI_VBAT_GE, reg_val);
+	if (val >= 8)
+		info->vbat_ge = val - 16;
+	else
+		info->vbat_ge = val;
+
+	dev_dbg(dev, "vbat-zse %d vbat-ge %d\n", info->vbat_zse, info->vbat_ge);
+
+	ret = devm_iio_map_array_register(dev, indio_dev, dc_ti_adc_default_maps);
+	if (ret)
+		return ret;
+
+	ret = devm_request_threaded_irq(dev, irq, NULL, dc_ti_adc_isr,
+					IRQF_ONESHOT, indio_dev->name, info);
+	if (ret)
+		return ret;
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct platform_device_id dc_ti_adc_ids[] = {
+	{ .name = "chtdc_ti_adc", },
+	{ }
+};
+MODULE_DEVICE_TABLE(platform, dc_ti_adc_ids);
+
+static struct platform_driver dc_ti_adc_driver = {
+	.driver = {
+		.name	= "dc_ti_adc",
+	},
+	.probe		= dc_ti_adc_probe,
+	.id_table	= dc_ti_adc_ids,
+};
+module_platform_driver(dc_ti_adc_driver);
+
+MODULE_AUTHOR("Ramakrishna Pallala <ramakrishna.pallala@intel.com>");
+MODULE_AUTHOR("Hans de Goede <hansg@kernel.org>");
+MODULE_DESCRIPTION("Intel Dollar Cove (TI) GPADC Driver");
+MODULE_LICENSE("GPL");
-- 
2.49.0

Re: [PATCH v2 1/2] iio: Improve iio_read_channel_processed_scale() precision

[David Lechner]

On 7/21/25 10:06 AM, Hans de Goede wrote:
> Before this change iio_read_channel_processed_scale() always assumes that
> channels which advertise IIO_CHAN_INFO_PROCESSED capability return
> IIO_VAL_INT on success.
> 
> Ignoring any fractional values from drivers which return
> IIO_VAL_INT_PLUS_MICRO / IIO_VAL_INT_PLUS_NANO. These fractional values
> might become non fractional after scaling so these should be taken into
> account.
> 
> While at it also error out for IIO_VAL_* values which
> iio_read_channel_processed_scale() does not know how to handle.
> 
> Signed-off-by: Hans de Goede <hansg@kernel.org>
> ---
> Changes in v2:
> - New patch in v3 of this patch-series
> ---
>  drivers/iio/inkern.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index c174ebb7d5e6..e9669f552eb3 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -714,18 +714,36 @@ int iio_read_channel_processed_scale(struct iio_channel *chan, int *val,
>  				     unsigned int scale)
>  {
>  	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(chan->indio_dev);
> -	int ret;
> +	int ret, val2;
>  
>  	guard(mutex)(&iio_dev_opaque->info_exist_lock);
>  	if (!chan->indio_dev->info)
>  		return -ENODEV;
>  
>  	if (iio_channel_has_info(chan->channel, IIO_CHAN_INFO_PROCESSED)) {
> -		ret = iio_channel_read(chan, val, NULL,
> +		ret = iio_channel_read(chan, val, &val2,
>  				       IIO_CHAN_INFO_PROCESSED);
>  		if (ret < 0)
>  			return ret;
> -		*val *= scale;
> +
> +		switch (ret) {
> +		case IIO_VAL_INT:
> +			*val *= scale;
> +			break;

Just return 0 here.

> +		case IIO_VAL_INT_PLUS_MICRO:
> +			*val *= scale;
> +			*val += div_u64((u64)val2 * scale, 1000000LLU);

If the processed value is between 0 and -1, then val2 will be negative
(val is 0 in this case, which can't be negative so val2 contains the
sign), so casting without checking the sign first isn't safe.

Also, would just use MICRO from linux/units.h instead of 1000000LLU.

> +			break;
> +		case IIO_VAL_INT_PLUS_NANO:
> +			*val *= scale;
> +			*val += div_u64((u64)val2 * scale, 1000000000LLU);

Same applies here of course.

> +			break;
> +		default:
> +			dev_err_once(&chan->indio_dev->dev,
> +				     "unsupported processed IIO-val-type: %d\n",
> +				     ret);
> +			return -EINVAL;
> +		}
>  
>  		return ret;

And drop this return as all cases should return directly.

>  	} else {

Re: [PATCH v2 1/2] iio: Improve iio_read_channel_processed_scale() precision

[Hans de Goede]

Hi David,

Thank you from your review.

On 21-Jul-25 7:59 PM, David Lechner wrote:
> On 7/21/25 10:06 AM, Hans de Goede wrote:
>> Before this change iio_read_channel_processed_scale() always assumes that
>> channels which advertise IIO_CHAN_INFO_PROCESSED capability return
>> IIO_VAL_INT on success.
>>
>> Ignoring any fractional values from drivers which return
>> IIO_VAL_INT_PLUS_MICRO / IIO_VAL_INT_PLUS_NANO. These fractional values
>> might become non fractional after scaling so these should be taken into
>> account.
>>
>> While at it also error out for IIO_VAL_* values which
>> iio_read_channel_processed_scale() does not know how to handle.
>>
>> Signed-off-by: Hans de Goede <hansg@kernel.org>
>> ---
>> Changes in v2:
>> - New patch in v3 of this patch-series
>> ---
>>  drivers/iio/inkern.c | 24 +++++++++++++++++++++---
>>  1 file changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
>> index c174ebb7d5e6..e9669f552eb3 100644
>> --- a/drivers/iio/inkern.c
>> +++ b/drivers/iio/inkern.c
>> @@ -714,18 +714,36 @@ int iio_read_channel_processed_scale(struct iio_channel *chan, int *val,
>>  				     unsigned int scale)
>>  {
>>  	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(chan->indio_dev);
>> -	int ret;
>> +	int ret, val2;
>>  
>>  	guard(mutex)(&iio_dev_opaque->info_exist_lock);
>>  	if (!chan->indio_dev->info)
>>  		return -ENODEV;
>>  
>>  	if (iio_channel_has_info(chan->channel, IIO_CHAN_INFO_PROCESSED)) {
>> -		ret = iio_channel_read(chan, val, NULL,
>> +		ret = iio_channel_read(chan, val, &val2,
>>  				       IIO_CHAN_INFO_PROCESSED);
>>  		if (ret < 0)
>>  			return ret;
>> -		*val *= scale;
>> +
>> +		switch (ret) {
>> +		case IIO_VAL_INT:
>> +			*val *= scale;
>> +			break;
>
> Just return 0 here.

Hmm, before this commit this function used to return the iio_channel_read()
value as is, so e.g. IIO_VAL_INT. So this would be a behavior change.

I checked and at least drivers/iio/afe/iio-rescale.c relies on
the current behavior of returning IIO_VAL_INT, etc.

>> +		case IIO_VAL_INT_PLUS_MICRO:
>> +			*val *= scale;
>> +			*val += div_u64((u64)val2 * scale, 1000000LLU);
> 
> If the processed value is between 0 and -1, then val2 will be negative
> (val is 0 in this case, which can't be negative so val2 contains the
> sign), so casting without checking the sign first isn't safe.

Ah ok, so I'll change the 2 u64-s here to s64 then. I originally
had copied this from iio_convert_raw_to_processed_unlocked() with
s64 but then thought I might as well make it u64, now I've learned
I should not have changed this ...

> Also, would just use MICRO from linux/units.h instead of 1000000LLU.

iio_convert_raw_to_processed_unlocked() uses 1000000LL, so for
consistency I would prefer to change this to 1000000LL (dropping the 'U').

>> +			break;
>> +		case IIO_VAL_INT_PLUS_NANO:
>> +			*val *= scale;
>> +			*val += div_u64((u64)val2 * scale, 1000000000LLU);
> 
> Same applies here of course.

Ack.

>> +			break;
>> +		default:
>> +			dev_err_once(&chan->indio_dev->dev,
>> +				     "unsupported processed IIO-val-type: %d\n",
>> +				     ret);
>> +			return -EINVAL;
>> +		}
>>  
>>  		return ret;
> 
> And drop this return as all cases should return directly.

See above.

Also IIRC some older gcc versions will complain about a missing ret,
even if there is a switch case with a default: label and all cases
returning.

Regards,

Hans

Re: [PATCH v2 2/2] iio: adc: Add Intel Dollar Cove TI PMIC ADC driver

[David Lechner]

On 7/21/25 10:06 AM, Hans de Goede wrote:
> Intel has 2 completely different "Dollar Cove" PMICs for its Bay Trail /
> Cherry Trail SoCs. One is made by X-Powers and is called the AXP288.
> The AXP288's GPADC is already supported by the X-Powers AXP288 ADC driver.
> 
> The other "Dollar Cove" PMIC is made by TI and does not have any clear TI
> denomination, its MFD driver calls it the "Intel Dollar Cove TI PMIC".
> 
> Add a driver for the Intel Dollar Cove TI PMIC's general purpose ADC,
> binding to the "chtdc_ti_adc" MFD cell of the MFD driver.
> 
> The "cht" in the cell name comes from Cherry Trail, but it turns out that
> just like the AXP288 the Intel Dollar Cove TI PMIC is also used with both
> Intel Bay Trail and Intel Cherry Trail SoCs, so this new ADC driver does
> not include the cht part in its name.
> 
> This is loosely based on kernel/drivers/iio/adc/iio_dc_ti_gpadc.c
> from the Acer A1-840 Android kernel source-code archive named:
> "App. Guide_Acer_20151221_A_A.zip"
> which is distributed by Acer from the Acer A1-840 support page:
> https://www.acer.com/us-en/support/product-support/A1-840/downloads
> 
> Signed-off-by: Hans de Goede <hansg@kernel.org>
> ---
> Changes in v2:
> - Add IIO_CHAN_INFO_SCALE and provide ADC scale info for Vbat
> - Add IIO_CHAN_INFO_PROCESSED which applies calibration and
>   scaling for the VBat channel
> - Address some other small review remarks
> ---
>  drivers/iio/adc/Kconfig           |  11 +
>  drivers/iio/adc/Makefile          |   1 +
>  drivers/iio/adc/intel_dc_ti_adc.c | 348 ++++++++++++++++++++++++++++++
>  3 files changed, 360 insertions(+)
>  create mode 100644 drivers/iio/adc/intel_dc_ti_adc.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index ea3ba1397392..51a5fc6efbe1 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -723,6 +723,17 @@ config INGENIC_ADC
>  	  This driver can also be built as a module. If so, the module will be
>  	  called ingenic_adc.
>  
> +config INTEL_DC_TI_ADC
> +	tristate "Intel Bay / Cherry Trail Dollar Cove TI ADC driver"
> +	depends on INTEL_SOC_PMIC_CHTDC_TI
> +	help
> +	  Say yes here to have support for the Dollar Cove TI PMIC ADC device.
> +	  Depending on platform configuration, this general purpose ADC can be
> +	  used for sensors such as battery voltage and thermal resistors.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called intel_dc_ti_adc.
> +
>  config INTEL_MRFLD_ADC
>  	tristate "Intel Merrifield Basin Cove ADC driver"
>  	depends on INTEL_SOC_PMIC_MRFLD
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 09ae6edb2650..da99ba88b4e1 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -67,6 +67,7 @@ obj-$(CONFIG_IMX8QXP_ADC) += imx8qxp-adc.o
>  obj-$(CONFIG_IMX93_ADC) += imx93_adc.o
>  obj-$(CONFIG_INA2XX_ADC) += ina2xx-adc.o
>  obj-$(CONFIG_INGENIC_ADC) += ingenic-adc.o
> +obj-$(CONFIG_INTEL_DC_TI_ADC) += intel_dc_ti_adc.o
>  obj-$(CONFIG_INTEL_MRFLD_ADC) += intel_mrfld_adc.o
>  obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>  obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
> diff --git a/drivers/iio/adc/intel_dc_ti_adc.c b/drivers/iio/adc/intel_dc_ti_adc.c
> new file mode 100644
> index 000000000000..6f27c45679cc
> --- /dev/null
> +++ b/drivers/iio/adc/intel_dc_ti_adc.c
> @@ -0,0 +1,348 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Intel Dollar Cove TI PMIC GPADC Driver
> + *
> + * Copyright (C) 2014 Intel Corporation (Ramakrishna Pallala <ramakrishna.pallala@intel.com>)
> + * Copyright (C) 2024 - 2025 Hans de Goede <hansg@kernel.org>
> + */
> +
> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
> +#include <linux/cleanup.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/intel_soc_pmic.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/wait.h>
> +
> +#include <linux/iio/driver.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/machine.h>
> +
> +#define DC_TI_ADC_CNTL_REG			0x50
> +#define DC_TI_ADC_START				BIT(0)
> +#define DC_TI_ADC_CH_SEL			GENMASK(2, 1)
> +#define DC_TI_ADC_EN				BIT(5)
> +#define DC_TI_ADC_EN_EXT_BPTH_BIAS		BIT(6)
> +
> +#define DC_TI_VBAT_ZSE_GE_REG			0x53
> +#define DC_TI_VBAT_GE				GENMASK(3, 0)
> +#define DC_TI_VBAT_ZSE				GENMASK(7, 4)
> +
> +/* VBAT GE gain correction is in 0.0015 increments, ZSE is in 1.0 increments */
> +#define DC_TI_VBAT_GE_STEP			15
> +#define DC_TI_VBAT_GE_DIV			10000
> +
> +#define DC_TI_ADC_CH0_DATAH_REG			0x54
> +#define DC_TI_ADC_CH0_DATAL_REG			0x55
> +#define DC_TI_ADC_CH1_DATAH_REG			0x56
> +#define DC_TI_ADC_CH1_DATAL_REG			0x57
> +#define DC_TI_ADC_CH2_DATAH_REG			0x58
> +#define DC_TI_ADC_CH2_DATAL_REG			0x59
> +#define DC_TI_ADC_CH3_DATAH_REG			0x5A
> +#define DC_TI_ADC_CH3_DATAL_REG			0x5B

Could be less verbose with just:

#define DC_TI_ADC_CHX_DATAH_REG(x)			(0x54 + 2 * (x))

> +
> +enum dc_ti_adc_id {
> +	DC_TI_ADC_VBAT,
> +	DC_TI_ADC_PMICTEMP,
> +	DC_TI_ADC_BATTEMP,
> +	DC_TI_ADC_SYSTEMP0,
> +};
> +
> +struct dc_ti_adc_info {
> +	struct mutex lock; /* Protects against concurrent accesses to the ADC */
> +	wait_queue_head_t wait;
> +	struct device *dev;
> +	struct regmap *regmap;
> +	int vbat_zse;
> +	int vbat_ge;
> +	bool conversion_done;
> +};
> +
> +static const struct iio_chan_spec dc_ti_adc_channels[] = {
> +	{
> +		.indexed = 1,
> +		.type = IIO_VOLTAGE,
> +		.channel = DC_TI_ADC_VBAT,
> +		.address = DC_TI_ADC_CH0_DATAH_REG,
> +		.datasheet_name = "CH0",
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE) |
> +				      BIT(IIO_CHAN_INFO_PROCESSED),
> +	}, {
> +		.indexed = 1,
> +		.type = IIO_TEMP,
> +		.channel = DC_TI_ADC_PMICTEMP,
> +		.address = DC_TI_ADC_CH1_DATAH_REG,
> +		.datasheet_name = "CH1",
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),

Is the raw value already in millidegrees C or do we need
IIO_CHAN_INFO_SCALE here?

> +	}, {
> +		.indexed = 1,
> +		.type = IIO_TEMP,
> +		.channel = DC_TI_ADC_BATTEMP,
> +		.address = DC_TI_ADC_CH2_DATAH_REG,
> +		.datasheet_name = "CH2",
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +	}, {
> +		.indexed = 1,
> +		.type = IIO_TEMP,
> +		.channel = DC_TI_ADC_SYSTEMP0,
> +		.address = DC_TI_ADC_CH3_DATAH_REG,
> +		.datasheet_name = "CH3",
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +	}
> +};
> +
> +static struct iio_map dc_ti_adc_default_maps[] = {
> +	IIO_MAP("CH0", "chtdc_ti_battery", "VBAT"),
> +	IIO_MAP("CH1", "chtdc_ti_battery", "PMICTEMP"),
> +	IIO_MAP("CH2", "chtdc_ti_battery", "BATTEMP"),
> +	IIO_MAP("CH3", "chtdc_ti_battery", "SYSTEMP0"),
> +	{ }
> +};
> +
> +static irqreturn_t dc_ti_adc_isr(int irq, void *data)
> +{
> +	struct dc_ti_adc_info *info = data;
> +
> +	info->conversion_done = true;
> +	wake_up(&info->wait);
> +	return IRQ_HANDLED;
> +}
> +
> +static int dc_ti_adc_scale(struct dc_ti_adc_info *info,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2)
> +{
> +	if (chan->channel != DC_TI_ADC_VBAT)
> +		return -EINVAL;
> +
> +	/* Vbat ADC scale is 4.6875 mV / unit */
> +	*val = 4;
> +	*val2 = 687500;
> +
> +	return IIO_VAL_INT_PLUS_MICRO;
> +}
> +
> +static int dc_ti_adc_raw_to_processed(struct dc_ti_adc_info *info,
> +				      struct iio_chan_spec const *chan,
> +				      int raw, int *val, int *val2)
> +{
> +	if (chan->channel != DC_TI_ADC_VBAT)
> +		return -EINVAL;
> +
> +	/* Apply calibration */
> +	raw -= info->vbat_zse;
> +	raw = raw * (DC_TI_VBAT_GE_DIV - info->vbat_ge * DC_TI_VBAT_GE_STEP) /
> +	      DC_TI_VBAT_GE_DIV;
> +	/* Vbat ADC scale is 4.6875 mV / unit */
> +	raw *= 46875;
> +
> +	/* raw is now in 10000 units / mV, convert to milli + milli/1e6 */
> +	*val = raw / 10000;
> +	*val2 = (raw % 10000) * 100;
> +
> +	return IIO_VAL_INT_PLUS_MICRO;
> +}
> +
> +static int dc_ti_adc_sample(struct dc_ti_adc_info *info,
> +			    struct iio_chan_spec const *chan, int *val)
> +{
> +	int ret, ch = chan->channel;
> +	unsigned int lsb, msb;
> +
> +	info->conversion_done = false;
> +
> +	/*
> +	 * As per TI (PMIC Vendor), the ADC enable and ADC start commands should
> +	 * not be sent together. Hence send the commands separately
> +	 */
> +	ret = regmap_update_bits(info->regmap, DC_TI_ADC_CNTL_REG,
> +				 DC_TI_ADC_EN, DC_TI_ADC_EN);

Can just be simplified to regmap_set_bits(). (applies several other
places as well)

> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(info->regmap, DC_TI_ADC_CNTL_REG,
> +				 DC_TI_ADC_CH_SEL, FIELD_PREP(DC_TI_ADC_CH_SEL, ch));
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * As per PMIC Vendor, a minimum of 50 micro seconds delay is required
> +	 * between ADC Enable and ADC START commands. This is also recommended
> +	 * by Intel Hardware team after the timing analysis of GPADC signals.
> +	 * Since the I2C Write transaction to set the channel number also
> +	 * imparts 25 micro seconds of delay, so we need to wait for another
> +	 * 25 micro seconds before issuing ADC START command.
> +	 */
> +	fsleep(25);
> +
> +	ret = regmap_update_bits(info->regmap, DC_TI_ADC_CNTL_REG,
> +				 DC_TI_ADC_START, DC_TI_ADC_START);
> +	if (ret)
> +		return ret;
> +
> +	/* TI (PMIC Vendor) recommends 5 sec timeout for conversion */
> +	ret = wait_event_timeout(info->wait, info->conversion_done, 5 * HZ);

Does conversion_done need to be volatile since it is set in an interrupt
handler?

> +	if (ret == 0) {
> +		dev_err(info->dev, "Error sample timeout\n");

Error code is being passed to the caller, so don't need to spam
the kernel log with this.

> +		ret = -ETIMEDOUT;
> +		goto disable_adc;
> +	}
> +
> +	/* Reading multiple registers at once is not supported */
> +	ret = regmap_read(info->regmap, chan->address, &msb);
> +	if (ret)
> +		goto disable_adc;
> +
> +	ret = regmap_read(info->regmap, chan->address + 1, &lsb);
> +	if (ret)
> +		goto disable_adc;
> +
> +	*val = ((msb << 8) | lsb) & 0x3ff;
> +
> +disable_adc:
> +	regmap_update_bits(info->regmap, DC_TI_ADC_CNTL_REG,
> +			   DC_TI_ADC_START | DC_TI_ADC_EN, 0);

Can be simplified to regmap_clear_bits(). (same applies other
places too)

> +	return ret;
> +}
> +
> +static int dc_ti_adc_read_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      int *val, int *val2, long mask)
> +{
> +	struct dc_ti_adc_info *info = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (mask == IIO_CHAN_INFO_SCALE)
> +		return dc_ti_adc_scale(info, chan, val, val2);
> +
> +	guard(mutex)(&info->lock);
> +
> +	/*
> +	 * If channel BPTHERM has been selected, first enable the BPTHERM BIAS
> +	 * which provides the VREFT Voltage reference to convert BPTHERM Input
> +	 * voltage to temperature.
> +	 * As per PMIC Vendor specifications, BPTHERM BIAS should be enabled
> +	 * 35 ms before ADC_EN command.
> +	 */
> +	if (chan->channel == DC_TI_ADC_BATTEMP) {
> +		ret = regmap_update_bits(info->regmap, DC_TI_ADC_CNTL_REG,
> +					 DC_TI_ADC_EN_EXT_BPTH_BIAS,
> +					 DC_TI_ADC_EN_EXT_BPTH_BIAS);
> +		if (ret)
> +			return ret;
> +		msleep(35);
> +	}
> +
> +	ret = dc_ti_adc_sample(info, chan, val);
> +
> +	if (chan->channel == DC_TI_ADC_BATTEMP)
> +		regmap_update_bits(info->regmap, DC_TI_ADC_CNTL_REG,
> +				   DC_TI_ADC_EN_EXT_BPTH_BIAS, 0);
> +
> +	if (ret)
> +		return ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_PROCESSED:
> +		return dc_ti_adc_raw_to_processed(info, chan, *val, val, val2);
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static const struct iio_info dc_ti_adc_iio_info = {
> +	.read_raw = dc_ti_adc_read_raw,
> +};
> +
> +static int dc_ti_adc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct intel_soc_pmic *pmic = dev_get_drvdata(dev->parent);
> +	struct dc_ti_adc_info *info;
> +	struct iio_dev *indio_dev;
> +	unsigned int reg_val;
> +	int irq, ret, val;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*info));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	info = iio_priv(indio_dev);
> +
> +	ret = devm_mutex_init(dev, &info->lock);
> +	if (ret)
> +		return ret;
> +
> +	init_waitqueue_head(&info->wait);
> +
> +	info->dev = dev;
> +	info->regmap = pmic->regmap;
> +
> +	indio_dev->name = "dc_ti_adc";
> +	indio_dev->channels = dc_ti_adc_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(dc_ti_adc_channels);
> +	indio_dev->info = &dc_ti_adc_iio_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	ret = regmap_read(info->regmap, DC_TI_VBAT_ZSE_GE_REG, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	val = FIELD_GET(DC_TI_VBAT_ZSE, reg_val);
> +	if (val >= 8)
> +		info->vbat_zse = val - 16;
> +	else
> +		info->vbat_zse = val;

Is this just sign extending? If so, could do it in one line:

	val = sign_extend32(FIELD_GET(DC_TI_VBAT_ZSE, reg_val), 3);

> +
> +	val = FIELD_GET(DC_TI_VBAT_GE, reg_val);
> +	if (val >= 8)
> +		info->vbat_ge = val - 16;
> +	else
> +		info->vbat_ge = val;
> +
> +	dev_dbg(dev, "vbat-zse %d vbat-ge %d\n", info->vbat_zse, info->vbat_ge);
> +
> +	ret = devm_iio_map_array_register(dev, indio_dev, dc_ti_adc_default_maps);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_request_threaded_irq(dev, irq, NULL, dc_ti_adc_isr,
> +					IRQF_ONESHOT, indio_dev->name, info);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +static const struct platform_device_id dc_ti_adc_ids[] = {
> +	{ .name = "chtdc_ti_adc", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(platform, dc_ti_adc_ids);
> +
> +static struct platform_driver dc_ti_adc_driver = {
> +	.driver = {
> +		.name	= "dc_ti_adc",
> +	},
> +	.probe		= dc_ti_adc_probe,
> +	.id_table	= dc_ti_adc_ids,
> +};
> +module_platform_driver(dc_ti_adc_driver);
> +
> +MODULE_AUTHOR("Ramakrishna Pallala <ramakrishna.pallala@intel.com>");
> +MODULE_AUTHOR("Hans de Goede <hansg@kernel.org>");
> +MODULE_DESCRIPTION("Intel Dollar Cove (TI) GPADC Driver");
> +MODULE_LICENSE("GPL");

Re: [PATCH v2 1/2] iio: Improve iio_read_channel_processed_scale() precision

[David Lechner]

On 7/21/25 1:42 PM, Hans de Goede wrote:
> Hi David,
> 
> Thank you from your review.
> 
> On 21-Jul-25 7:59 PM, David Lechner wrote:
>> On 7/21/25 10:06 AM, Hans de Goede wrote:
>>> Before this change iio_read_channel_processed_scale() always assumes that
>>> channels which advertise IIO_CHAN_INFO_PROCESSED capability return
>>> IIO_VAL_INT on success.
>>>
>>> Ignoring any fractional values from drivers which return
>>> IIO_VAL_INT_PLUS_MICRO / IIO_VAL_INT_PLUS_NANO. These fractional values
>>> might become non fractional after scaling so these should be taken into
>>> account.
>>>
>>> While at it also error out for IIO_VAL_* values which
>>> iio_read_channel_processed_scale() does not know how to handle.
>>>
>>> Signed-off-by: Hans de Goede <hansg@kernel.org>
>>> ---
>>> Changes in v2:
>>> - New patch in v3 of this patch-series
>>> ---
>>>  drivers/iio/inkern.c | 24 +++++++++++++++++++++---
>>>  1 file changed, 21 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
>>> index c174ebb7d5e6..e9669f552eb3 100644
>>> --- a/drivers/iio/inkern.c
>>> +++ b/drivers/iio/inkern.c
>>> @@ -714,18 +714,36 @@ int iio_read_channel_processed_scale(struct iio_channel *chan, int *val,
>>>  				     unsigned int scale)
>>>  {
>>>  	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(chan->indio_dev);
>>> -	int ret;
>>> +	int ret, val2;
>>>  
>>>  	guard(mutex)(&iio_dev_opaque->info_exist_lock);
>>>  	if (!chan->indio_dev->info)
>>>  		return -ENODEV;
>>>  
>>>  	if (iio_channel_has_info(chan->channel, IIO_CHAN_INFO_PROCESSED)) {
>>> -		ret = iio_channel_read(chan, val, NULL,
>>> +		ret = iio_channel_read(chan, val, &val2,
>>>  				       IIO_CHAN_INFO_PROCESSED);
>>>  		if (ret < 0)
>>>  			return ret;
>>> -		*val *= scale;
>>> +
>>> +		switch (ret) {
>>> +		case IIO_VAL_INT:
>>> +			*val *= scale;
>>> +			break;
>>
>> Just return 0 here.
> 
> Hmm, before this commit this function used to return the iio_channel_read()
> value as is, so e.g. IIO_VAL_INT. So this would be a behavior change.
> 
> I checked and at least drivers/iio/afe/iio-rescale.c relies on
> the current behavior of returning IIO_VAL_INT, etc.

Ah right. So all successful cases should now return IIO_VAL_INT
even when the original processed was something else since we
are scaling an truncating it as an int.
> 
>>> +		case IIO_VAL_INT_PLUS_MICRO:
>>> +			*val *= scale;
>>> +			*val += div_u64((u64)val2 * scale, 1000000LLU);
>>
>> If the processed value is between 0 and -1, then val2 will be negative
>> (val is 0 in this case, which can't be negative so val2 contains the
>> sign), so casting without checking the sign first isn't safe.
> 
> Ah ok, so I'll change the 2 u64-s here to s64 then. I originally
> had copied this from iio_convert_raw_to_processed_unlocked() with
> s64 but then thought I might as well make it u64, now I've learned
> I should not have changed this ...
> 
>> Also, would just use MICRO from linux/units.h instead of 1000000LLU.
> 
> iio_convert_raw_to_processed_unlocked() uses 1000000LL, so for
> consistency I would prefer to change this to 1000000LL (dropping the 'U').
> 
>>> +			break;
>>> +		case IIO_VAL_INT_PLUS_NANO:
>>> +			*val *= scale;
>>> +			*val += div_u64((u64)val2 * scale, 1000000000LLU);
>>
>> Same applies here of course.
> 
> Ack.
> 
>>> +			break;
>>> +		default:
>>> +			dev_err_once(&chan->indio_dev->dev,
>>> +				     "unsupported processed IIO-val-type: %d\n",
>>> +				     ret);
>>> +			return -EINVAL;
>>> +		}
>>>  
>>>  		return ret;
>>
>> And drop this return as all cases should return directly.
> 
> See above.
> 
> Also IIRC some older gcc versions will complain about a missing ret,
> even if there is a switch case with a default: label and all cases
> returning.

We do early returns in switch statements like this in the IIO subsystem
and haven't had any issues with complaints from older compilers. So I
think it would still be preferred to do it that way to be consistent
with the rest of the subsystem.

> 
> Regards,
> 
> Hans
> 
> 

Re: [PATCH v2 1/2] iio: Improve iio_read_channel_processed_scale() precision

[Hans de Goede]

On 21-Jul-25 8:51 PM, David Lechner wrote:
> On 7/21/25 1:42 PM, Hans de Goede wrote:
>> Hi David,
>>
>> Thank you from your review.
>>
>> On 21-Jul-25 7:59 PM, David Lechner wrote:
>>> On 7/21/25 10:06 AM, Hans de Goede wrote:
>>>> Before this change iio_read_channel_processed_scale() always assumes that
>>>> channels which advertise IIO_CHAN_INFO_PROCESSED capability return
>>>> IIO_VAL_INT on success.
>>>>
>>>> Ignoring any fractional values from drivers which return
>>>> IIO_VAL_INT_PLUS_MICRO / IIO_VAL_INT_PLUS_NANO. These fractional values
>>>> might become non fractional after scaling so these should be taken into
>>>> account.
>>>>
>>>> While at it also error out for IIO_VAL_* values which
>>>> iio_read_channel_processed_scale() does not know how to handle.
>>>>
>>>> Signed-off-by: Hans de Goede <hansg@kernel.org>
>>>> ---
>>>> Changes in v2:
>>>> - New patch in v3 of this patch-series
>>>> ---
>>>>  drivers/iio/inkern.c | 24 +++++++++++++++++++++---
>>>>  1 file changed, 21 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
>>>> index c174ebb7d5e6..e9669f552eb3 100644
>>>> --- a/drivers/iio/inkern.c
>>>> +++ b/drivers/iio/inkern.c
>>>> @@ -714,18 +714,36 @@ int iio_read_channel_processed_scale(struct iio_channel *chan, int *val,
>>>>  				     unsigned int scale)
>>>>  {
>>>>  	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(chan->indio_dev);
>>>> -	int ret;
>>>> +	int ret, val2;
>>>>  
>>>>  	guard(mutex)(&iio_dev_opaque->info_exist_lock);
>>>>  	if (!chan->indio_dev->info)
>>>>  		return -ENODEV;
>>>>  
>>>>  	if (iio_channel_has_info(chan->channel, IIO_CHAN_INFO_PROCESSED)) {
>>>> -		ret = iio_channel_read(chan, val, NULL,
>>>> +		ret = iio_channel_read(chan, val, &val2,
>>>>  				       IIO_CHAN_INFO_PROCESSED);
>>>>  		if (ret < 0)
>>>>  			return ret;
>>>> -		*val *= scale;
>>>> +
>>>> +		switch (ret) {
>>>> +		case IIO_VAL_INT:
>>>> +			*val *= scale;
>>>> +			break;
>>>
>>> Just return 0 here.
>>
>> Hmm, before this commit this function used to return the iio_channel_read()
>> value as is, so e.g. IIO_VAL_INT. So this would be a behavior change.
>>
>> I checked and at least drivers/iio/afe/iio-rescale.c relies on
>> the current behavior of returning IIO_VAL_INT, etc.
> 
> Ah right. So all successful cases should now return IIO_VAL_INT
> even when the original processed was something else since we
> are scaling an truncating it as an int.
>>
>>>> +		case IIO_VAL_INT_PLUS_MICRO:
>>>> +			*val *= scale;
>>>> +			*val += div_u64((u64)val2 * scale, 1000000LLU);
>>>
>>> If the processed value is between 0 and -1, then val2 will be negative
>>> (val is 0 in this case, which can't be negative so val2 contains the
>>> sign), so casting without checking the sign first isn't safe.
>>
>> Ah ok, so I'll change the 2 u64-s here to s64 then. I originally
>> had copied this from iio_convert_raw_to_processed_unlocked() with
>> s64 but then thought I might as well make it u64, now I've learned
>> I should not have changed this ...
>>
>>> Also, would just use MICRO from linux/units.h instead of 1000000LLU.
>>
>> iio_convert_raw_to_processed_unlocked() uses 1000000LL, so for
>> consistency I would prefer to change this to 1000000LL (dropping the 'U').
>>
>>>> +			break;
>>>> +		case IIO_VAL_INT_PLUS_NANO:
>>>> +			*val *= scale;
>>>> +			*val += div_u64((u64)val2 * scale, 1000000000LLU);
>>>
>>> Same applies here of course.
>>
>> Ack.
>>
>>>> +			break;
>>>> +		default:
>>>> +			dev_err_once(&chan->indio_dev->dev,
>>>> +				     "unsupported processed IIO-val-type: %d\n",
>>>> +				     ret);
>>>> +			return -EINVAL;
>>>> +		}
>>>>  
>>>>  		return ret;
>>>
>>> And drop this return as all cases should return directly.
>>
>> See above.
>>
>> Also IIRC some older gcc versions will complain about a missing ret,
>> even if there is a switch case with a default: label and all cases
>> returning.
> 
> We do early returns in switch statements like this in the IIO subsystem
> and haven't had any issues with complaints from older compilers. So I
> think it would still be preferred to do it that way to be consistent
> with the rest of the subsystem.

Ok, I'll make all the cases early return IIO_VAL_INT and drop the
return ret at the end.

Regards,

Hans

Re: [PATCH v2 2/2] iio: adc: Add Intel Dollar Cove TI PMIC ADC driver

[Hans de Goede]

Hi David,

Thank you for the review.

On 21-Jul-25 8:43 PM, David Lechner wrote:
> On 7/21/25 10:06 AM, Hans de Goede wrote:
>> Intel has 2 completely different "Dollar Cove" PMICs for its Bay Trail /
>> Cherry Trail SoCs. One is made by X-Powers and is called the AXP288.
>> The AXP288's GPADC is already supported by the X-Powers AXP288 ADC driver.
>>
>> The other "Dollar Cove" PMIC is made by TI and does not have any clear TI
>> denomination, its MFD driver calls it the "Intel Dollar Cove TI PMIC".
>>
>> Add a driver for the Intel Dollar Cove TI PMIC's general purpose ADC,
>> binding to the "chtdc_ti_adc" MFD cell of the MFD driver.
>>
>> The "cht" in the cell name comes from Cherry Trail, but it turns out that
>> just like the AXP288 the Intel Dollar Cove TI PMIC is also used with both
>> Intel Bay Trail and Intel Cherry Trail SoCs, so this new ADC driver does
>> not include the cht part in its name.
>>
>> This is loosely based on kernel/drivers/iio/adc/iio_dc_ti_gpadc.c
>> from the Acer A1-840 Android kernel source-code archive named:
>> "App. Guide_Acer_20151221_A_A.zip"
>> which is distributed by Acer from the Acer A1-840 support page:
>> https://www.acer.com/us-en/support/product-support/A1-840/downloads
>>
>> Signed-off-by: Hans de Goede <hansg@kernel.org>
>> ---
>> Changes in v2:
>> - Add IIO_CHAN_INFO_SCALE and provide ADC scale info for Vbat
>> - Add IIO_CHAN_INFO_PROCESSED which applies calibration and
>>   scaling for the VBat channel
>> - Address some other small review remarks
>> ---
>>  drivers/iio/adc/Kconfig           |  11 +
>>  drivers/iio/adc/Makefile          |   1 +
>>  drivers/iio/adc/intel_dc_ti_adc.c | 348 ++++++++++++++++++++++++++++++
>>  3 files changed, 360 insertions(+)
>>  create mode 100644 drivers/iio/adc/intel_dc_ti_adc.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index ea3ba1397392..51a5fc6efbe1 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -723,6 +723,17 @@ config INGENIC_ADC
>>  	  This driver can also be built as a module. If so, the module will be
>>  	  called ingenic_adc.
>>  
>> +config INTEL_DC_TI_ADC
>> +	tristate "Intel Bay / Cherry Trail Dollar Cove TI ADC driver"
>> +	depends on INTEL_SOC_PMIC_CHTDC_TI
>> +	help
>> +	  Say yes here to have support for the Dollar Cove TI PMIC ADC device.
>> +	  Depending on platform configuration, this general purpose ADC can be
>> +	  used for sensors such as battery voltage and thermal resistors.
>> +
>> +	  To compile this driver as a module, choose M here: the module will be
>> +	  called intel_dc_ti_adc.
>> +
>>  config INTEL_MRFLD_ADC
>>  	tristate "Intel Merrifield Basin Cove ADC driver"
>>  	depends on INTEL_SOC_PMIC_MRFLD
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index 09ae6edb2650..da99ba88b4e1 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -67,6 +67,7 @@ obj-$(CONFIG_IMX8QXP_ADC) += imx8qxp-adc.o
>>  obj-$(CONFIG_IMX93_ADC) += imx93_adc.o
>>  obj-$(CONFIG_INA2XX_ADC) += ina2xx-adc.o
>>  obj-$(CONFIG_INGENIC_ADC) += ingenic-adc.o
>> +obj-$(CONFIG_INTEL_DC_TI_ADC) += intel_dc_ti_adc.o
>>  obj-$(CONFIG_INTEL_MRFLD_ADC) += intel_mrfld_adc.o
>>  obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>>  obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
>> diff --git a/drivers/iio/adc/intel_dc_ti_adc.c b/drivers/iio/adc/intel_dc_ti_adc.c
>> new file mode 100644
>> index 000000000000..6f27c45679cc
>> --- /dev/null
>> +++ b/drivers/iio/adc/intel_dc_ti_adc.c
>> @@ -0,0 +1,348 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Intel Dollar Cove TI PMIC GPADC Driver
>> + *
>> + * Copyright (C) 2014 Intel Corporation (Ramakrishna Pallala <ramakrishna.pallala@intel.com>)
>> + * Copyright (C) 2024 - 2025 Hans de Goede <hansg@kernel.org>
>> + */
>> +
>> +#include <linux/bits.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/cleanup.h>
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/mfd/intel_soc_pmic.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/wait.h>
>> +
>> +#include <linux/iio/driver.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/machine.h>
>> +
>> +#define DC_TI_ADC_CNTL_REG			0x50
>> +#define DC_TI_ADC_START				BIT(0)
>> +#define DC_TI_ADC_CH_SEL			GENMASK(2, 1)
>> +#define DC_TI_ADC_EN				BIT(5)
>> +#define DC_TI_ADC_EN_EXT_BPTH_BIAS		BIT(6)
>> +
>> +#define DC_TI_VBAT_ZSE_GE_REG			0x53
>> +#define DC_TI_VBAT_GE				GENMASK(3, 0)
>> +#define DC_TI_VBAT_ZSE				GENMASK(7, 4)
>> +
>> +/* VBAT GE gain correction is in 0.0015 increments, ZSE is in 1.0 increments */
>> +#define DC_TI_VBAT_GE_STEP			15
>> +#define DC_TI_VBAT_GE_DIV			10000
>> +
>> +#define DC_TI_ADC_CH0_DATAH_REG			0x54
>> +#define DC_TI_ADC_CH0_DATAL_REG			0x55
>> +#define DC_TI_ADC_CH1_DATAH_REG			0x56
>> +#define DC_TI_ADC_CH1_DATAL_REG			0x57
>> +#define DC_TI_ADC_CH2_DATAH_REG			0x58
>> +#define DC_TI_ADC_CH2_DATAL_REG			0x59
>> +#define DC_TI_ADC_CH3_DATAH_REG			0x5A
>> +#define DC_TI_ADC_CH3_DATAL_REG			0x5B
> 
> Could be less verbose with just:
> 
> #define DC_TI_ADC_CHX_DATAH_REG(x)			(0x54 + 2 * (x))

Ack, will fix.

>> +
>> +enum dc_ti_adc_id {
>> +	DC_TI_ADC_VBAT,
>> +	DC_TI_ADC_PMICTEMP,
>> +	DC_TI_ADC_BATTEMP,
>> +	DC_TI_ADC_SYSTEMP0,
>> +};
>> +
>> +struct dc_ti_adc_info {
>> +	struct mutex lock; /* Protects against concurrent accesses to the ADC */
>> +	wait_queue_head_t wait;
>> +	struct device *dev;
>> +	struct regmap *regmap;
>> +	int vbat_zse;
>> +	int vbat_ge;
>> +	bool conversion_done;
>> +};
>> +
>> +static const struct iio_chan_spec dc_ti_adc_channels[] = {
>> +	{
>> +		.indexed = 1,
>> +		.type = IIO_VOLTAGE,
>> +		.channel = DC_TI_ADC_VBAT,
>> +		.address = DC_TI_ADC_CH0_DATAH_REG,
>> +		.datasheet_name = "CH0",
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> +				      BIT(IIO_CHAN_INFO_SCALE) |
>> +				      BIT(IIO_CHAN_INFO_PROCESSED),
>> +	}, {
>> +		.indexed = 1,
>> +		.type = IIO_TEMP,
>> +		.channel = DC_TI_ADC_PMICTEMP,
>> +		.address = DC_TI_ADC_CH1_DATAH_REG,
>> +		.datasheet_name = "CH1",
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> 
> Is the raw value already in millidegrees C or do we need
> IIO_CHAN_INFO_SCALE here?

The scale is unknown (I guess this depends on the thermistor
connected), which is why there is no IIO_CHAN_INFO_SCALE
here.

Note I have no datasheet, this is all based on the Android
kernel sourcecode dump mentioned in the commit message.

>> +	}, {

...

>> +static int dc_ti_adc_sample(struct dc_ti_adc_info *info,
>> +			    struct iio_chan_spec const *chan, int *val)
>> +{
>> +	int ret, ch = chan->channel;
>> +	unsigned int lsb, msb;
>> +
>> +	info->conversion_done = false;
>> +
>> +	/*
>> +	 * As per TI (PMIC Vendor), the ADC enable and ADC start commands should
>> +	 * not be sent together. Hence send the commands separately
>> +	 */
>> +	ret = regmap_update_bits(info->regmap, DC_TI_ADC_CNTL_REG,
>> +				 DC_TI_ADC_EN, DC_TI_ADC_EN);
> 
> Can just be simplified to regmap_set_bits(). (applies several other
> places as well)

Ack, will fix for the next version.

>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_update_bits(info->regmap, DC_TI_ADC_CNTL_REG,
>> +				 DC_TI_ADC_CH_SEL, FIELD_PREP(DC_TI_ADC_CH_SEL, ch));
>> +	if (ret)
>> +		return ret;
>> +
>> +	/*
>> +	 * As per PMIC Vendor, a minimum of 50 micro seconds delay is required
>> +	 * between ADC Enable and ADC START commands. This is also recommended
>> +	 * by Intel Hardware team after the timing analysis of GPADC signals.
>> +	 * Since the I2C Write transaction to set the channel number also
>> +	 * imparts 25 micro seconds of delay, so we need to wait for another
>> +	 * 25 micro seconds before issuing ADC START command.
>> +	 */
>> +	fsleep(25);
>> +
>> +	ret = regmap_update_bits(info->regmap, DC_TI_ADC_CNTL_REG,
>> +				 DC_TI_ADC_START, DC_TI_ADC_START);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* TI (PMIC Vendor) recommends 5 sec timeout for conversion */
>> +	ret = wait_event_timeout(info->wait, info->conversion_done, 5 * HZ);
> 
> Does conversion_done need to be volatile since it is set in an interrupt
> handler?

Interesting question, I would expect the wait_event_timeout()
macro to deal with this, and it internally uses:

#define ___wait_cond_timeout(condition)                                         \
({                                                                              \
        bool __cond = (condition);                                              \
        if (__cond && !__ret)                                                   \
                __ret = 1;                                                      \
        __cond || !__ret;                                                       \
})

Where I guess the copy to the bool __conf causes a re-read every
time this is checked?  At least I'm reasonably sure that the kernel
is full of check for some flag wait conditions like this without all
of them being volatile.

>> +	if (ret == 0) {
>> +		dev_err(info->dev, "Error sample timeout\n");
> 
> Error code is being passed to the caller, so don't need to spam
> the kernel log with this.

Ok, I'll drop the error log.

>> +		ret = -ETIMEDOUT;
>> +		goto disable_adc;
>> +	}
>> +
>> +	/* Reading multiple registers at once is not supported */
>> +	ret = regmap_read(info->regmap, chan->address, &msb);
>> +	if (ret)
>> +		goto disable_adc;
>> +
>> +	ret = regmap_read(info->regmap, chan->address + 1, &lsb);
>> +	if (ret)
>> +		goto disable_adc;
>> +
>> +	*val = ((msb << 8) | lsb) & 0x3ff;
>> +
>> +disable_adc:
>> +	regmap_update_bits(info->regmap, DC_TI_ADC_CNTL_REG,
>> +			   DC_TI_ADC_START | DC_TI_ADC_EN, 0);
> 
> Can be simplified to regmap_clear_bits(). (same applies other
> places too)

Ack.

>> +	return ret;
>> +}

...

>> +static int dc_ti_adc_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct intel_soc_pmic *pmic = dev_get_drvdata(dev->parent);
>> +	struct dc_ti_adc_info *info;
>> +	struct iio_dev *indio_dev;
>> +	unsigned int reg_val;
>> +	int irq, ret, val;
>> +
>> +	irq = platform_get_irq(pdev, 0);
>> +	if (irq < 0)
>> +		return irq;
>> +
>> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*info));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +
>> +	info = iio_priv(indio_dev);
>> +
>> +	ret = devm_mutex_init(dev, &info->lock);
>> +	if (ret)
>> +		return ret;
>> +
>> +	init_waitqueue_head(&info->wait);
>> +
>> +	info->dev = dev;
>> +	info->regmap = pmic->regmap;
>> +
>> +	indio_dev->name = "dc_ti_adc";
>> +	indio_dev->channels = dc_ti_adc_channels;
>> +	indio_dev->num_channels = ARRAY_SIZE(dc_ti_adc_channels);
>> +	indio_dev->info = &dc_ti_adc_iio_info;
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +
>> +	ret = regmap_read(info->regmap, DC_TI_VBAT_ZSE_GE_REG, &reg_val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	val = FIELD_GET(DC_TI_VBAT_ZSE, reg_val);
>> +	if (val >= 8)
>> +		info->vbat_zse = val - 16;
>> +	else
>> +		info->vbat_zse = val;
> 
> Is this just sign extending?

I think so, let me double check before the next version.

> If so, could do it in one line:
> 
> 	val = sign_extend32(FIELD_GET(DC_TI_VBAT_ZSE, reg_val), 3);

Ack will do if this indeed is just sign extending.

>> +
>> +	val = FIELD_GET(DC_TI_VBAT_GE, reg_val);
>> +	if (val >= 8)
>> +		info->vbat_ge = val - 16;
>> +	else
>> +		info->vbat_ge = val;

and the same here then.


>> +
>> +	dev_dbg(dev, "vbat-zse %d vbat-ge %d\n", info->vbat_zse, info->vbat_ge);
>> +
>> +	ret = devm_iio_map_array_register(dev, indio_dev, dc_ti_adc_default_maps);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = devm_request_threaded_irq(dev, irq, NULL, dc_ti_adc_isr,
>> +					IRQF_ONESHOT, indio_dev->name, info);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return devm_iio_device_register(dev, indio_dev);
>> +}

...

Regards,

Hans

Re: [PATCH v2 2/2] iio: adc: Add Intel Dollar Cove TI PMIC ADC driver

[Jonathan Cameron]

On Mon, 21 Jul 2025 17:06:14 +0200
Hans de Goede <hansg@kernel.org> wrote:

> Intel has 2 completely different "Dollar Cove" PMICs for its Bay Trail /
> Cherry Trail SoCs. One is made by X-Powers and is called the AXP288.
> The AXP288's GPADC is already supported by the X-Powers AXP288 ADC driver.
> 
> The other "Dollar Cove" PMIC is made by TI and does not have any clear TI
> denomination, its MFD driver calls it the "Intel Dollar Cove TI PMIC".
> 
> Add a driver for the Intel Dollar Cove TI PMIC's general purpose ADC,
> binding to the "chtdc_ti_adc" MFD cell of the MFD driver.
> 
> The "cht" in the cell name comes from Cherry Trail, but it turns out that
> just like the AXP288 the Intel Dollar Cove TI PMIC is also used with both
> Intel Bay Trail and Intel Cherry Trail SoCs, so this new ADC driver does
> not include the cht part in its name.
> 
> This is loosely based on kernel/drivers/iio/adc/iio_dc_ti_gpadc.c
> from the Acer A1-840 Android kernel source-code archive named:
> "App. Guide_Acer_20151221_A_A.zip"
> which is distributed by Acer from the Acer A1-840 support page:
> https://www.acer.com/us-en/support/product-support/A1-840/downloads
> 
> Signed-off-by: Hans de Goede <hansg@kernel.org>
> ---
> Changes in v2:
> - Add IIO_CHAN_INFO_SCALE and provide ADC scale info for Vbat
> - Add IIO_CHAN_INFO_PROCESSED which applies calibration and
>   scaling for the VBat channel
> - Address some other small review remarks

A couple of really minor things from me.

Thanks,

Jonathan

> diff --git a/drivers/iio/adc/intel_dc_ti_adc.c b/drivers/iio/adc/intel_dc_ti_adc.c
> new file mode 100644
> index 000000000000..6f27c45679cc
> --- /dev/null
> +++ b/drivers/iio/adc/intel_dc_ti_adc.c

> +static int dc_ti_adc_sample(struct dc_ti_adc_info *info,
> +			    struct iio_chan_spec const *chan, int *val)
> +{
> +	int ret, ch = chan->channel;
> +	unsigned int lsb, msb;
> +
> +	info->conversion_done = false;
> +
> +	/*
> +	 * As per TI (PMIC Vendor), the ADC enable and ADC start commands should
> +	 * not be sent together. Hence send the commands separately
> +	 */
> +	ret = regmap_update_bits(info->regmap, DC_TI_ADC_CNTL_REG,
> +				 DC_TI_ADC_EN, DC_TI_ADC_EN);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(info->regmap, DC_TI_ADC_CNTL_REG,
> +				 DC_TI_ADC_CH_SEL, FIELD_PREP(DC_TI_ADC_CH_SEL, ch));

Bit of a long line. I'd still rather keep under 80 chars when there is no
strong readability argument for not doing so.

> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * As per PMIC Vendor, a minimum of 50 micro seconds delay is required
> +	 * between ADC Enable and ADC START commands. This is also recommended
> +	 * by Intel Hardware team after the timing analysis of GPADC signals.
> +	 * Since the I2C Write transaction to set the channel number also
> +	 * imparts 25 micro seconds of delay, so we need to wait for another
> +	 * 25 micro seconds before issuing ADC START command.
> +	 */
> +	fsleep(25);
> +
> +	ret = regmap_update_bits(info->regmap, DC_TI_ADC_CNTL_REG,
> +				 DC_TI_ADC_START, DC_TI_ADC_START);
> +	if (ret)
> +		return ret;
> +

When you are going to do unconditional disabling like here, might be better
to factor this bit out so we don't need the goto.  I don't care too much
though if you don't want to bother with that particularly as next change
suggested will reduce the number of times that goto happens.

> +	/* TI (PMIC Vendor) recommends 5 sec timeout for conversion */
> +	ret = wait_event_timeout(info->wait, info->conversion_done, 5 * HZ);
> +	if (ret == 0) {
> +		dev_err(info->dev, "Error sample timeout\n");
> +		ret = -ETIMEDOUT;
> +		goto disable_adc;
> +	}
> +
> +	/* Reading multiple registers at once is not supported */

Set the magic flag in regmap_config to allow build reads to be automatically
broken into parts.  I think this is what use_single_read controls.

Then you can read it into a u8 array and use an endian conversion.


> +	ret = regmap_read(info->regmap, chan->address, &msb);
> +	if (ret)
> +		goto disable_adc;
> +
> +	ret = regmap_read(info->regmap, chan->address + 1, &lsb);
> +	if (ret)
> +		goto disable_adc;
> +
> +	*val = ((msb << 8) | lsb) & 0x3ff;
> +
> +disable_adc:
> +	regmap_update_bits(info->regmap, DC_TI_ADC_CNTL_REG,
> +			   DC_TI_ADC_START | DC_TI_ADC_EN, 0);
> +	return ret;
> +}

Re: [PATCH v2 1/2] iio: Improve iio_read_channel_processed_scale() precision

[Andy Shevchenko]

On Mon, Jul 21, 2025 at 05:06:13PM +0200, Hans de Goede wrote:
> Before this change iio_read_channel_processed_scale() always assumes that
> channels which advertise IIO_CHAN_INFO_PROCESSED capability return
> IIO_VAL_INT on success.
> 
> Ignoring any fractional values from drivers which return
> IIO_VAL_INT_PLUS_MICRO / IIO_VAL_INT_PLUS_NANO. These fractional values
> might become non fractional after scaling so these should be taken into
> account.
> 
> While at it also error out for IIO_VAL_* values which
> iio_read_channel_processed_scale() does not know how to handle.

...

> +		case IIO_VAL_INT_PLUS_MICRO:
> +			*val *= scale;
> +			*val += div_u64((u64)val2 * scale, 1000000LLU);

MICRO from units.h

> +			break;
> +		case IIO_VAL_INT_PLUS_NANO:
> +			*val *= scale;
> +			*val += div_u64((u64)val2 * scale, 1000000000LLU);

Respectively NANO.

> +			break;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] iio: Improve iio_read_channel_processed_scale() precision

[Andy Shevchenko]

On Mon, Jul 21, 2025 at 08:42:36PM +0200, Hans de Goede wrote:
> On 21-Jul-25 7:59 PM, David Lechner wrote:
> > On 7/21/25 10:06 AM, Hans de Goede wrote:

...

> > Also, would just use MICRO from linux/units.h instead of 1000000LLU.
> 
> iio_convert_raw_to_processed_unlocked() uses 1000000LL, so for
> consistency I would prefer to change this to 1000000LL (dropping the 'U').

If you need a signed one, make it so `(s64)MICRO` (or whatever signed type you
want that holds it).


> >> +			break;
> >> +		case IIO_VAL_INT_PLUS_NANO:
> >> +			*val *= scale;
> >> +			*val += div_u64((u64)val2 * scale, 1000000000LLU);
> > 
> > Same applies here of course.
> 
> Ack.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] iio: adc: Add Intel Dollar Cove TI PMIC ADC driver

[Andy Shevchenko]

On Mon, Jul 21, 2025 at 09:02:10PM +0200, Hans de Goede wrote:
> On 21-Jul-25 8:43 PM, David Lechner wrote:
> > On 7/21/25 10:06 AM, Hans de Goede wrote:

...

> >> +#define DC_TI_ADC_CH3_DATAH_REG			0x5A
> >> +#define DC_TI_ADC_CH3_DATAL_REG			0x5B
> > 
> > Could be less verbose with just:
> > 
> > #define DC_TI_ADC_CHX_DATAH_REG(x)			(0x54 + 2 * (x))
> 
> Ack, will fix.

And drop H, it is not needed either if we properly use endianess wise types and
conversions.

...

> > Is the raw value already in millidegrees C or do we need
> > IIO_CHAN_INFO_SCALE here?
> 
> The scale is unknown (I guess this depends on the thermistor
> connected), which is why there is no IIO_CHAN_INFO_SCALE
> here.
> 
> Note I have no datasheet, this is all based on the Android
> kernel sourcecode dump mentioned in the commit message.

I think I have access to it. What should I look for specifically?

(list all your questions here, please, without spreading over the code and mails)

But note, I will be on vacation for 4 more weeks (and I think it's not an issue
as this is material for the next v6.18 cycle anyway).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] iio: Improve iio_read_channel_processed_scale() precision

[Hans de Goede]

Hi Andy,

On 23-Jul-25 5:15 PM, Andy Shevchenko wrote:
> On Mon, Jul 21, 2025 at 05:06:13PM +0200, Hans de Goede wrote:
>> Before this change iio_read_channel_processed_scale() always assumes that
>> channels which advertise IIO_CHAN_INFO_PROCESSED capability return
>> IIO_VAL_INT on success.
>>
>> Ignoring any fractional values from drivers which return
>> IIO_VAL_INT_PLUS_MICRO / IIO_VAL_INT_PLUS_NANO. These fractional values
>> might become non fractional after scaling so these should be taken into
>> account.
>>
>> While at it also error out for IIO_VAL_* values which
>> iio_read_channel_processed_scale() does not know how to handle.
> 
> ...
> 
>> +		case IIO_VAL_INT_PLUS_MICRO:
>> +			*val *= scale;
>> +			*val += div_u64((u64)val2 * scale, 1000000LLU);
> 
> MICRO from units.h
> 
>> +			break;
>> +		case IIO_VAL_INT_PLUS_NANO:
>> +			*val *= scale;
>> +			*val += div_u64((u64)val2 * scale, 1000000000LLU);
> 
> Respectively NANO.

As I already mentioned to the same remark from David:

iio_convert_raw_to_processed_unlocked() uses 1000000LL, so for
consistency I would prefer to change this to 1000000LL (dropping the 'U').

If we want to switch to using MICRO + NANO this should be doe
in a follow-up patch which makes this change consistently everywhere
in the IIO core code.

Regards,

Hans

Re: [PATCH v2 2/2] iio: adc: Add Intel Dollar Cove TI PMIC ADC driver

[Hans de Goede]

Hi Andy,

On 23-Jul-25 5:24 PM, Andy Shevchenko wrote:
> On Mon, Jul 21, 2025 at 09:02:10PM +0200, Hans de Goede wrote:
>> On 21-Jul-25 8:43 PM, David Lechner wrote:
>>> On 7/21/25 10:06 AM, Hans de Goede wrote:
> 
> ...
> 
>>>> +#define DC_TI_ADC_CH3_DATAH_REG			0x5A
>>>> +#define DC_TI_ADC_CH3_DATAL_REG			0x5B
>>>
>>> Could be less verbose with just:
>>>
>>> #define DC_TI_ADC_CHX_DATAH_REG(x)			(0x54 + 2 * (x))
>>
>> Ack, will fix.
> 
> And drop H, it is not needed either if we properly use endianess wise types and
> conversions.

Ack

...

>>> Is the raw value already in millidegrees C or do we need
>>> IIO_CHAN_INFO_SCALE here?
>>
>> The scale is unknown (I guess this depends on the thermistor
>> connected), which is why there is no IIO_CHAN_INFO_SCALE
>> here.
>>
>> Note I have no datasheet, this is all based on the Android
>> kernel sourcecode dump mentioned in the commit message.
> 
> I think I have access to it. What should I look for specifically?
> 
> (list all your questions here, please, without spreading over the code and mails)
> 
> But note, I will be on vacation for 4 more weeks (and I think it's not an issue
> as this is material for the next v6.18 cycle anyway).

Thank you for offering to look up the datasheet.

I don't really have any specific questions, I just mentioned this
because this is part of the reason why the scale for the temperature
channels is unknown.

But just the datasheet will not be enough since the scale also depends
on the thermistor / ntc / other-type-temp-sensor the channel is
connected to.

Regards,

Hans