On Mon, 21 Feb 2022 22:07:39 +0000
Caleb Connolly <caleb.connolly@xxxxxxxxxx> 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@xxxxxxxxxx>
Hi Calib,
Unfortunately this fell for the normal rule that everytime someone
rereads some code they'll find something new :(
All minor stuff though so fingers crossed for v9.
The endian stuff isn't strictly necessary but it is always better to use explicit
endian types where possible as it hardens the code against forgetting to convert
them etc.
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..f69d95103c82
> --- /dev/null
> +++ b/drivers/iio/adc/qcom-spmi-rradc.c
> @@ -0,0 +1,1011 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2022 Linaro Limited.
> + * Author: Caleb Connolly <caleb.connolly@xxxxxxxxxx>
> + *
> + * This driver is for the Round Robin ADC found in the pmi8998 and pm660 PMICs.
> + */
...
> +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)
This function is only ever called in paths which then convert *data to le16.
As such, pass in __le16 *buf
regmap_bulk_read() takes a void * so that will work fine without casting.
The size should still be bytes to reflect that we are reading multiple 8 bit
registers.
> +{
> + int ret, retry_cnt = 0;
> + u8 data_check[RR_ADC_CHAN_MAX_CONTINUOUS_BUFFER_LEN];
elegance would make this __le16 as well but that probably doesn't matter.
Possibly you'll have to do something a bit clever at the memcmp to force
dropping of the endian markings - build with C=1, W=1 and see if it complains.
> +
> + 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_do_conversion(struct rradc_chip *chip,
> + enum rradc_channel_id chan_address, u16 *data)
> +{
> + const struct rradc_channel *chan = &rradc_chans[chan_address];
> + const struct iio_chan_spec *iio_chan = &rradc_iio_chans[chan_address];
> + int ret;
> + u8 buf[6];
I missed this until now, but buf is only ever used as __le16 buf[3]
so give it that type and you can use
le16_to_cpu() as it will be aligned.
> +
> + mutex_lock(&chip->conversion_lock);
> +
> + switch (chan_address) {
> + case RR_ADC_BATT_ID:
> + ret = rradc_prepare_batt_id_conversion(chip, chan_address, data);
> + if (ret < 0) {
> + dev_err(chip->dev, "Battery ID conversion failed:%d\n",
> + ret);
> + goto unlock_out;
> + }
> + break;
> +
> + case RR_ADC_USBIN_V:
> + case RR_ADC_DIE_TEMP:
> + ret = rradc_read_status_in_cont_mode(chip, chan_address);
> + if (ret < 0) {
> + dev_err(chip->dev,
> + "Error reading in continuous mode:%d\n", ret);
> + goto unlock_out;
> + }
> + break;
> + default:
> + if (!rradc_is_ready(chip, chan_address)) {
> + /*
> + * Usually this means the channel isn't attached, for example
> + * the in_voltage_usbin_v_input channel will not be ready if
> + * no USB cable is attached
> + */
> + dev_dbg(chip->dev, "channel '%s' is not ready\n",
> + iio_chan->extend_name);
> + ret = -ENODATA;
> + goto unlock_out;
> + }
> + break;
> + }
> +
> + ret = rradc_read(chip, chan->lsb, buf, chan->size);
> + if (ret) {
> + dev_err(chip->dev, "read data failed\n");
> + goto unlock_out;
> + }
> +
> + /*
> + * For the battery ID we read the register for every ID ADC and then
> + * see which one is actually connected.
> + */
> + if (chan_address == RR_ADC_BATT_ID) {
> + u16 batt_id_150 = get_unaligned_le16(buf + 4);
> + u16 batt_id_15 = get_unaligned_le16(buf + 2);
> + u16 batt_id_5 = get_unaligned_le16(buf);
> +
> + if (!batt_id_150 && !batt_id_15 && !batt_id_5) {
> + dev_err(chip->dev,
> + "Invalid batt_id values with all zeros\n");
> + ret = -EINVAL;
> + goto unlock_out;
> + }
> +
> + if (batt_id_150 <= RR_ADC_BATT_ID_RANGE) {
> + *data = batt_id_150;
> + chip->batt_id_data = 150;
> + } else if (batt_id_15 <= RR_ADC_BATT_ID_RANGE) {
> + *data = batt_id_15;
> + chip->batt_id_data = 15;
> + } else {
> + *data = batt_id_5;
> + chip->batt_id_data = 5;
> + }
> + } else {
> + /*
> + * All of the other channels are either 1 or 2 bytes.
> + * We can rely on the second byte being 0 for 1-byte channels.
> + */
> + *data = get_unaligned_le16(buf);
> + }
> +
> +unlock_out:
> + mutex_unlock(&chip->conversion_lock);
> +
> + return ret;
> +}
> +
...
> +
> +static int rradc_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan_spec, int *val,
> + int *val2, long mask)
> +{
> + struct rradc_chip *chip = iio_priv(indio_dev);
> + const struct rradc_channel *chan;
> + int ret;
> + u16 adc_code;
> +
> + if (chan_spec->address >= RR_ADC_CHAN_MAX) {
> + dev_err(chip->dev, "Invalid channel index:%lu\n",
> + chan_spec->address);
> + return -EINVAL;
> + }
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + return rradc_read_scale(chip, chan_spec->address, val, val2);
> + case IIO_CHAN_INFO_OFFSET:
> + return rradc_read_offset(chip, chan_spec->address, val);
> + case IIO_CHAN_INFO_RAW:
> + chan = &rradc_chans[chan_spec->address];
chan unused in this case statement.
> + ret = rradc_do_conversion(chip, chan_spec->address, &adc_code);
> + if (ret < 0)
> + return ret;
> +
> + *val = adc_code;
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_PROCESSED:
> + chan = &rradc_chans[chan_spec->address];
> + if (!chan->scale_fn)
> + return -EINVAL;
> + ret = rradc_do_conversion(chip, chan_spec->address, &adc_code);
> + if (ret < 0)
> + return ret;
> +
> + *val = chan->scale_fn(chip, adc_code, val);
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +}
> +
...
> +static const struct iio_chan_spec rradc_iio_chans[RR_ADC_CHAN_MAX] = {
> + {
> + .type = IIO_RESISTANCE,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + .address = RR_ADC_BATT_ID,
> + .channel = 0,
> + .indexed = 1,
> + }, {
> + .type = IIO_TEMP,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + .address = RR_ADC_BATT_THERM,
> + .channel = 0,
> + .indexed = 1,
> + }, {
> + .type = IIO_TEMP,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE) |
> + BIT(IIO_CHAN_INFO_OFFSET),
> + .address = RR_ADC_SKIN_TEMP,
> + .channel = 1,
> + .indexed = 1,
> + }, {
> + .type = IIO_CURRENT,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE),
> + .address = RR_ADC_USBIN_I,
> + .channel = 0,
> + .indexed = 1,
> + }, {
> + .type = IIO_VOLTAGE,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE),
Inconsistent indenting vs the other similar cases.
> + .address = RR_ADC_USBIN_V,
> + .channel = 0,
> + .indexed = 1,
> + }, {
> + .type = IIO_CURRENT,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE),
> + .address = RR_ADC_DCIN_I,
> + .channel = 1,
> + .indexed = 1,
> + }, {
> + .type = IIO_VOLTAGE,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE),
> + .address = RR_ADC_DCIN_V,
> + .channel = 1,
> + .indexed = 1,
> + }, {
> + .type = IIO_TEMP,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE) |
> + BIT(IIO_CHAN_INFO_OFFSET),
> + .address = RR_ADC_DIE_TEMP,
> + .channel = 2,
> + .indexed = 1,
> + }, {
> + .type = IIO_TEMP,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_OFFSET) |
> + BIT(IIO_CHAN_INFO_SCALE),
> + .address = RR_ADC_CHG_TEMP,
> + .channel = 3,
> + .indexed = 1,
> + }, {
> + .type = IIO_VOLTAGE,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE),
> + .address = RR_ADC_GPIO,
> + .channel = 2,
> + .indexed = 1,
> + },
> +};
> +
> +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(chip->pmic)) {
> + dev_err(chip->dev, "Unable to get reference to PMIC device\n");
> + return PTR_ERR(chip->pmic);
> + }
> +
> + indio_dev->name = DRIVER_NAME;
I missed this in earlier versions, but this should be the specific
part number if possible, so probably
pm660-rradc / pmi8998-rradc as appropriate.
You can set it based on chip->pmic->sub_type I think
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->info = &rradc_info;
> + indio_dev->channels = rradc_iio_chans;
> + indio_dev->num_channels = ARRAY_SIZE(rradc_iio_chans);
> +
> + return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +static const struct of_device_id rradc_match_table[] = {
> + { .compatible = "qcom,pm660-rradc" },
> + { .compatible = "qcom,pmi8998-rradc" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, rradc_match_table);
> +
> +static struct platform_driver rradc_driver = {
> + .driver = {
> + .name = DRIVER_NAME,
> + .of_match_table = rradc_match_table,
> + },
> + .probe = rradc_probe,
> +};
> +module_platform_driver(rradc_driver);
> +
> +MODULE_DESCRIPTION("QCOM SPMI PMIC RR ADC driver");
> +MODULE_AUTHOR("Caleb Connolly <caleb.connolly@xxxxxxxxxx>");
> +MODULE_LICENSE("GPL v2");
