Re: [PATCH v7 5/9] iio: adc: qcom-spmi-rradc: introduce round robin adc

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 16 Feb 2022 13:49:16 +0000
Caleb Connolly <caleb.connolly@linaro.org> wrote:

> The Round Robin ADC is responsible for reading data about the rate of
> charge from the USB or DC input ports, it can also read the battery
> ID (resistence), skin temperature and the die temperature of the pmic.
> It is found on the PMI8998 and PM660 Qualcomm PMICs.
> 
> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>

Hi Caleb,

A few really trivial things noticed on a fresh read through. Given you
are spinning again might as well tidy them up.

Thanks,

Jonathan

...

> diff --git a/drivers/iio/adc/qcom-spmi-rradc.c b/drivers/iio/adc/qcom-spmi-rradc.c
> new file mode 100644
> index 000000000000..8d5675f625dc
> --- /dev/null
> +++ b/drivers/iio/adc/qcom-spmi-rradc.c
> @@ -0,0 +1,1016 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2022 Linaro Limited.
> + *  Author: Caleb Connolly <caleb.connolly@linaro.org>
> + *
> + * This driver is for the Round Robin ADC found in the pmi8998 and pm660 PMICs.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/kernel.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/spmi.h>
> +#include <linux/types.h>
> +#include <linux/units.h>
> +
> +#include <asm/unaligned.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/types.h>
> +
> +#include <soc/qcom/qcom-spmi-pmic.h>
> +
> +#define DRIVER_NAME "qcom-spmi-rradc"
> +

> +
> +#define RR_ADC_STS_CHANNEL_READING_MASK GENMASK(1, 0) // 0x03

Drop the // 0x03 as I'd hope that's obvious! :) 

...

> +
> +struct rradc_chip {
> +	struct device *dev;
> +	const struct qcom_spmi_pmic *pmic;
> +	/* Lock held while doing channel conversion
	/*
	 * Lock ...

> +	 * involving multiple register read/writes
> +	 */
> +	struct mutex conversion_lock;
> +	struct regmap *regmap;
> +	u32 base;
> +	int batt_id_delay;
> +	u16 batt_id_data;
> +};
> +
> +static const int batt_id_delays[] = { 0, 1, 4, 12, 20, 40, 60, 80 };
> +static const struct rradc_channel rradc_chans[RR_ADC_CHAN_MAX];
> +static const struct iio_chan_spec rradc_iio_chans[RR_ADC_CHAN_MAX];
> +
> +static int rradc_read(struct rradc_chip *chip, u16 addr, u8 *data, int len)
> +{
> +	int ret, retry_cnt = 0;
> +	u8 data_check[RR_ADC_CHAN_MAX_CONTINUOUS_BUFFER_LEN];
> +
> +	if (len > RR_ADC_CHAN_MAX_CONTINUOUS_BUFFER_LEN) {
> +		dev_err(chip->dev,
> +			"Can't read more than %d bytes, but asked to read %d bytes.\n",
> +			RR_ADC_CHAN_MAX_CONTINUOUS_BUFFER_LEN, len);
> +		return -EINVAL;
> +	}
> +
> +	while (retry_cnt < RR_ADC_COHERENT_CHECK_RETRY) {
> +		ret = regmap_bulk_read(chip->regmap, chip->base + addr, data,
> +				       len);
> +		if (ret < 0) {
> +			dev_err(chip->dev, "rr_adc reg 0x%x failed :%d\n", addr,
> +				ret);
> +			return ret;
> +		}
> +
> +		ret = regmap_bulk_read(chip->regmap, chip->base + addr,
> +				       data_check, len);
> +		if (ret < 0) {
> +			dev_err(chip->dev, "rr_adc reg 0x%x failed :%d\n", addr,
> +				ret);
> +			return ret;
> +		}
> +
> +		if (memcmp(data, data_check, len) != 0) {
> +			retry_cnt++;
> +			dev_dbg(chip->dev,
> +				"coherent read error, retry_cnt:%d\n",
> +				retry_cnt);
> +			continue;
> +		}
> +
> +		break;
> +	}
> +
> +	if (retry_cnt == RR_ADC_COHERENT_CHECK_RETRY)
> +		dev_err(chip->dev, "Retry exceeded for coherrency check\n");
> +
> +	return ret;
> +}
> +
> +static int rradc_get_fab_coeff(struct rradc_chip *chip, int64_t *offset,
> +			       int64_t *slope)
> +{
> +	if (chip->pmic->subtype == PM660_SUBTYPE) {
> +		switch (chip->pmic->fab_id) {
> +		case PM660_FAB_ID_GF:
> +			*offset = RR_ADC_CHG_TEMP_660_GF_OFFSET_UV;
> +			*slope = RR_ADC_CHG_TEMP_660_GF_SLOPE_UV_PER_C;
> +			break;
			return 0;

> +		case PM660_FAB_ID_TSMC:
> +			*offset = RR_ADC_CHG_TEMP_660_SMIC_OFFSET_UV;
> +			*slope = RR_ADC_CHG_TEMP_660_SMIC_SLOPE_UV_PER_C;
> +			break;
			return 0;
> +		default:
> +			*offset = RR_ADC_CHG_TEMP_660_MGNA_OFFSET_UV;
> +			*slope = RR_ADC_CHG_TEMP_660_MGNA_SLOPE_UV_PER_C;
			return 0;
> +		}
> +	} else if (chip->pmic->subtype == PMI8998_SUBTYPE) {
> +		switch (chip->pmic->fab_id) {
> +		case PMI8998_FAB_ID_GF:
> +			*offset = RR_ADC_CHG_TEMP_GF_OFFSET_UV;
> +			*slope = RR_ADC_CHG_TEMP_GF_SLOPE_UV_PER_C;
> +			break;
			return 0;
> +		case PMI8998_FAB_ID_SMIC:
> +			*offset = RR_ADC_CHG_TEMP_SMIC_OFFSET_UV;
> +			*slope = RR_ADC_CHG_TEMP_SMIC_SLOPE_UV_PER_C;
> +			break;
			return 0;
> +		default:
> +			return -EINVAL;
> +		}
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	return 0;
Can only get here via a few paths adn in all of those returning early
makes just as much sense.

> +}
> +

...

> +static int rradc_read_status_in_cont_mode(struct rradc_chip *chip,
> +					  enum rradc_channel_id chan_address)
> +{
> +	const struct rradc_channel *chan = &rradc_chans[chan_address];
> +	const struct iio_chan_spec *iio_chan = &rradc_iio_chans[chan_address];
> +	int ret, i;
> +
> +	if (chan->trigger_mask == 0) {
> +		dev_err(chip->dev, "Channel doesn't have a trigger mask\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = regmap_update_bits(chip->regmap, chip->base + chan->trigger_addr,
> +				 chan->trigger_mask, chan->trigger_mask);
> +	if (ret < 0) {
> +		dev_err(chip->dev,
> +			"Failed to apply trigger for channel '%s' ret=%d\n",
> +			iio_chan->extend_name, ret);
> +		return ret;
> +	}
> +
> +	ret = rradc_enable_continuous_mode(chip);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "Failed to switch to continuous mode\n");
> +		goto disable_trigger;
> +	}
> +
> +	/*
> +	 * The wait/sleep values were found through trial and error,
> +	 * this is mostly for the battery ID channel which takes some
> +	 * time to settle.
> +	 */
> +	for (i = 0; i < 5; i++) {
> +		if (rradc_is_ready(chip, chan_address))
> +			break;
> +		usleep_range(50000, 50000 + 500);
> +	}
> +
> +	if (i == 5) {
> +		dev_err(chip->dev, "Channel '%s' is not ready\n",
> +			iio_chan->extend_name);
> +		ret = -EINVAL;

Perhaps indicate a timeout rather than invalid?

> +	}
> +
> +	rradc_disable_continuous_mode(chip);
> +
> +disable_trigger:
> +	regmap_update_bits(chip->regmap, chip->base + chan->trigger_addr,
> +			   chan->trigger_mask, 0);
> +
> +	return ret;
> +}
> +
> +static int rradc_prepare_batt_id_conversion(struct rradc_chip *chip,
> +					    enum rradc_channel_id chan_address,
> +					    u16 *data)
> +{
> +	int ret;
> +
> +	ret = regmap_update_bits(chip->regmap, chip->base + RR_ADC_BATT_ID_CTRL,
> +				 RR_ADC_BATT_ID_CTRL_CHANNEL_CONV,
> +				 RR_ADC_BATT_ID_CTRL_CHANNEL_CONV);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "Enabling BATT ID channel failed:%d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_update_bits(chip->regmap,
> +				 chip->base + RR_ADC_BATT_ID_TRIGGER,
> +				 RR_ADC_TRIGGER_CTL, RR_ADC_TRIGGER_CTL);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "BATT_ID trigger set failed:%d\n", ret);
> +		goto out_disable_batt_id;
> +	}
> +
> +	ret = rradc_read_status_in_cont_mode(chip, chan_address);
> +
> +	/*
> +	 * Reset registers back to default values

Might as well do this as a single line comment.

> +	 */
> +	regmap_update_bits(chip->regmap, chip->base + RR_ADC_BATT_ID_TRIGGER,
> +			   RR_ADC_TRIGGER_CTL, 0);
> +
> +out_disable_batt_id:
> +	regmap_update_bits(chip->regmap, chip->base + RR_ADC_BATT_ID_CTRL,
> +			   RR_ADC_BATT_ID_CTRL_CHANNEL_CONV, 0);
> +
> +	return ret;
> +}
> +


...

> +static int rradc_read_scale(struct rradc_chip *chip, int chan_address, int *val,
> +			    int *val2)
> +{
> +	int64_t fab_offset, fab_slope;
> +	int ret;
> +
> +	ret = rradc_get_fab_coeff(chip, &fab_offset, &fab_slope);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "Unable to get fab id coefficients\n");
> +		return -EINVAL;
> +	}
> +
> +	switch (chan_address) {
> +	case RR_ADC_SKIN_TEMP:
> +		*val = MILLI;
> +		*val2 = RR_ADC_BATT_THERM_LSB_K;
> +		return IIO_VAL_FRACTIONAL;
> +	case RR_ADC_USBIN_I:
> +		*val = RR_ADC_CURR_USBIN_INPUT_FACTOR_MIL *
> +		       RR_ADC_FS_VOLTAGE_MV;
> +		*val2 = RR_ADC_CHAN_MSB;
> +		return IIO_VAL_FRACTIONAL;
> +	case RR_ADC_DCIN_I:
> +		*val = RR_ADC_CURR_INPUT_FACTOR * RR_ADC_FS_VOLTAGE_MV;
> +		*val2 = RR_ADC_CHAN_MSB;
> +		return IIO_VAL_FRACTIONAL;
> +	case RR_ADC_USBIN_V:
> +	case RR_ADC_DCIN_V:
> +		*val = RR_ADC_VOLT_INPUT_FACTOR * RR_ADC_FS_VOLTAGE_MV * MILLI;
> +		*val2 = RR_ADC_CHAN_MSB;
> +		return IIO_VAL_FRACTIONAL;
> +	case RR_ADC_GPIO:
> +		*val = RR_ADC_GPIO_FS_RANGE;
> +		*val2 = RR_ADC_CHAN_MSB;
> +		return IIO_VAL_FRACTIONAL;
> +	case RR_ADC_CHG_TEMP:
> +		/*
> +		 * We divide val2 by MILLI instead of multiplying val
> +		 * to avoid an integer overflow.
> +		 */
> +		*val = -RR_ADC_TEMP_FS_VOLTAGE_NUM;
> +		*val2 = div64_s64(RR_ADC_TEMP_FS_VOLTAGE_DEN * RR_ADC_CHAN_MSB *
> +					  fab_slope,
> +				  MILLI);
> +
> +		return IIO_VAL_FRACTIONAL;
> +	case RR_ADC_DIE_TEMP:
> +		*val = RR_ADC_TEMP_FS_VOLTAGE_NUM;
> +		*val2 = RR_ADC_TEMP_FS_VOLTAGE_DEN * RR_ADC_CHAN_MSB *
> +			RR_ADC_DIE_TEMP_SLOPE;
> +
> +		return IIO_VAL_FRACTIONAL;
> +	default:
> +		break;

		return -EINVAL; and drop the one below.

> +	}
> +
> +	return -EINVAL;
> +}
> +


...

> +static int rradc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct iio_dev *indio_dev;
> +	struct rradc_chip *chip;
> +	int ret, i, batt_id_delay;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*chip));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	chip = iio_priv(indio_dev);
> +	chip->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +	if (!chip->regmap) {
> +		dev_err(dev, "Couldn't get parent's regmap\n");
> +		return -EINVAL;
> +	}
> +
> +	chip->dev = dev;
> +	mutex_init(&chip->conversion_lock);
> +
> +	ret = device_property_read_u32(dev, "reg", &chip->base);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "Couldn't find reg address, ret = %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	batt_id_delay = -1;
> +	ret = device_property_read_u32(dev, "qcom,batt-id-delay-ms",
> +				       &batt_id_delay);
> +	if (!ret) {
> +		for (i = 0; i < RRADC_BATT_ID_DELAY_MAX; i++) {
> +			if (batt_id_delay == batt_id_delays[i])
> +				break;
> +		}
> +		if (i == RRADC_BATT_ID_DELAY_MAX)
> +			batt_id_delay = -1;
> +	}
> +
> +	if (batt_id_delay >= 0) {
> +		batt_id_delay = FIELD_PREP(BATT_ID_SETTLE_MASK, batt_id_delay);
> +		ret = regmap_update_bits(chip->regmap,
> +					 chip->base + RR_ADC_BATT_ID_CFG,
> +					 batt_id_delay, batt_id_delay);
> +		if (ret < 0) {
> +			dev_err(chip->dev,
> +				"BATT_ID settling time config failed:%d\n",
> +				ret);
> +		}
> +	}
> +
> +	/* Get the PMIC revision ID, we need to handle some varying coefficients */
> +	chip->pmic = qcom_pmic_get(chip->dev);
> +	if (IS_ERR_VALUE(chip->pmic)) {

Isn't IS_ERR() more appropriate?

> +		dev_err(chip->dev, "Unable to get reference to PMIC device\n");
> +		return PTR_ERR(chip->pmic);
> +	}