Message ID | 20220323162820.110806-1-caleb@connolly.tech |
---|---|
Headers | show |
Series | iio: adc: introduce Qualcomm SPMI Round Robin ADC | expand |
On Wed, 23 Mar 2022 16:28:16 +0000 Caleb Connolly <caleb.connolly@linaro.org> wrote: > From: Caleb Connolly <caleb.connolly@linaro.org> > > 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> Whilst I said this patch was fine a few versions ago I didn't add a tag on basis I would probably end up picking it up. To make things more flexible, I'll add one and either Lee or I can do the immutable branch once Lee has had a chance to take a look and is happy with the mfd parts. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/iio/adc/Kconfig | 12 + > drivers/iio/adc/Makefile | 1 + > drivers/iio/adc/qcom-spmi-rradc.c | 1021 +++++++++++++++++++++++++++++ > 3 files changed, 1034 insertions(+) > create mode 100644 drivers/iio/adc/qcom-spmi-rradc.c > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index 4fdc8bfbb407..66557b434fa8 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -812,6 +812,18 @@ config QCOM_PM8XXX_XOADC > To compile this driver as a module, choose M here: the module > will be called qcom-pm8xxx-xoadc. > > +config QCOM_SPMI_RRADC > + tristate "Qualcomm SPMI RRADC" > + depends on MFD_SPMI_PMIC > + help > + This is for the PMIC Round Robin ADC driver. > + > + This driver exposes the battery ID resistor, battery thermal, PMIC die > + temperature, charger USB in and DC in voltage and current. > + > + To compile this driver as a module, choose M here: the module will > + be called qcom-qpmi-rradc. > + > config QCOM_SPMI_IADC > tristate "Qualcomm SPMI PMIC current ADC" > depends on SPMI > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile > index 4a8f1833993b..b0dd7f142abd 100644 > --- a/drivers/iio/adc/Makefile > +++ b/drivers/iio/adc/Makefile > @@ -77,6 +77,7 @@ obj-$(CONFIG_NPCM_ADC) += npcm_adc.o > obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o > obj-$(CONFIG_QCOM_SPMI_ADC5) += qcom-spmi-adc5.o > obj-$(CONFIG_QCOM_SPMI_IADC) += qcom-spmi-iadc.o > +obj-$(CONFIG_QCOM_SPMI_RRADC) += qcom-spmi-rradc.o > obj-$(CONFIG_QCOM_VADC_COMMON) += qcom-vadc-common.o > obj-$(CONFIG_QCOM_SPMI_VADC) += qcom-spmi-vadc.o > obj-$(CONFIG_QCOM_PM8XXX_XOADC) += qcom-pm8xxx-xoadc.o > diff --git a/drivers/iio/adc/qcom-spmi-rradc.c b/drivers/iio/adc/qcom-spmi-rradc.c > new file mode 100644 > index 000000000000..230477fc45d2 > --- /dev/null > +++ b/drivers/iio/adc/qcom-spmi-rradc.c > @@ -0,0 +1,1021 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2016-2017, 2019, The Linux Foundation. All rights reserved. > + * 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_EN_CTL 0x46 > +#define RR_ADC_SKIN_TEMP_LSB 0x50 > +#define RR_ADC_SKIN_TEMP_MSB 0x51 > +#define RR_ADC_CTL 0x52 > +#define RR_ADC_CTL_CONTINUOUS_SEL BIT(3) > +#define RR_ADC_LOG 0x53 > +#define RR_ADC_LOG_CLR_CTRL BIT(0) > + > +#define RR_ADC_FAKE_BATT_LOW_LSB 0x58 > +#define RR_ADC_FAKE_BATT_LOW_MSB 0x59 > +#define RR_ADC_FAKE_BATT_HIGH_LSB 0x5A > +#define RR_ADC_FAKE_BATT_HIGH_MSB 0x5B > + > +#define RR_ADC_BATT_ID_CTRL 0x60 > +#define RR_ADC_BATT_ID_CTRL_CHANNEL_CONV BIT(0) > +#define RR_ADC_BATT_ID_TRIGGER 0x61 > +#define RR_ADC_BATT_ID_STS 0x62 > +#define RR_ADC_BATT_ID_CFG 0x63 > +#define BATT_ID_SETTLE_MASK GENMASK(7, 5) > +#define RR_ADC_BATT_ID_5_LSB 0x66 > +#define RR_ADC_BATT_ID_5_MSB 0x67 > +#define RR_ADC_BATT_ID_15_LSB 0x68 > +#define RR_ADC_BATT_ID_15_MSB 0x69 > +#define RR_ADC_BATT_ID_150_LSB 0x6A > +#define RR_ADC_BATT_ID_150_MSB 0x6B > + > +#define RR_ADC_BATT_THERM_CTRL 0x70 > +#define RR_ADC_BATT_THERM_TRIGGER 0x71 > +#define RR_ADC_BATT_THERM_STS 0x72 > +#define RR_ADC_BATT_THERM_CFG 0x73 > +#define RR_ADC_BATT_THERM_LSB 0x74 > +#define RR_ADC_BATT_THERM_MSB 0x75 > +#define RR_ADC_BATT_THERM_FREQ 0x76 > + > +#define RR_ADC_AUX_THERM_CTRL 0x80 > +#define RR_ADC_AUX_THERM_TRIGGER 0x81 > +#define RR_ADC_AUX_THERM_STS 0x82 > +#define RR_ADC_AUX_THERM_CFG 0x83 > +#define RR_ADC_AUX_THERM_LSB 0x84 > +#define RR_ADC_AUX_THERM_MSB 0x85 > + > +#define RR_ADC_SKIN_HOT 0x86 > +#define RR_ADC_SKIN_TOO_HOT 0x87 > + > +#define RR_ADC_AUX_THERM_C1 0x88 > +#define RR_ADC_AUX_THERM_C2 0x89 > +#define RR_ADC_AUX_THERM_C3 0x8A > +#define RR_ADC_AUX_THERM_HALF_RANGE 0x8B > + > +#define RR_ADC_USB_IN_V_CTRL 0x90 > +#define RR_ADC_USB_IN_V_TRIGGER 0x91 > +#define RR_ADC_USB_IN_V_STS 0x92 > +#define RR_ADC_USB_IN_V_LSB 0x94 > +#define RR_ADC_USB_IN_V_MSB 0x95 > +#define RR_ADC_USB_IN_I_CTRL 0x98 > +#define RR_ADC_USB_IN_I_TRIGGER 0x99 > +#define RR_ADC_USB_IN_I_STS 0x9A > +#define RR_ADC_USB_IN_I_LSB 0x9C > +#define RR_ADC_USB_IN_I_MSB 0x9D > + > +#define RR_ADC_DC_IN_V_CTRL 0xA0 > +#define RR_ADC_DC_IN_V_TRIGGER 0xA1 > +#define RR_ADC_DC_IN_V_STS 0xA2 > +#define RR_ADC_DC_IN_V_LSB 0xA4 > +#define RR_ADC_DC_IN_V_MSB 0xA5 > +#define RR_ADC_DC_IN_I_CTRL 0xA8 > +#define RR_ADC_DC_IN_I_TRIGGER 0xA9 > +#define RR_ADC_DC_IN_I_STS 0xAA > +#define RR_ADC_DC_IN_I_LSB 0xAC > +#define RR_ADC_DC_IN_I_MSB 0xAD > + > +#define RR_ADC_PMI_DIE_TEMP_CTRL 0xB0 > +#define RR_ADC_PMI_DIE_TEMP_TRIGGER 0xB1 > +#define RR_ADC_PMI_DIE_TEMP_STS 0xB2 > +#define RR_ADC_PMI_DIE_TEMP_CFG 0xB3 > +#define RR_ADC_PMI_DIE_TEMP_LSB 0xB4 > +#define RR_ADC_PMI_DIE_TEMP_MSB 0xB5 > + > +#define RR_ADC_CHARGER_TEMP_CTRL 0xB8 > +#define RR_ADC_CHARGER_TEMP_TRIGGER 0xB9 > +#define RR_ADC_CHARGER_TEMP_STS 0xBA > +#define RR_ADC_CHARGER_TEMP_CFG 0xBB > +#define RR_ADC_CHARGER_TEMP_LSB 0xBC > +#define RR_ADC_CHARGER_TEMP_MSB 0xBD > +#define RR_ADC_CHARGER_HOT 0xBE > +#define RR_ADC_CHARGER_TOO_HOT 0xBF > + > +#define RR_ADC_GPIO_CTRL 0xC0 > +#define RR_ADC_GPIO_TRIGGER 0xC1 > +#define RR_ADC_GPIO_STS 0xC2 > +#define RR_ADC_GPIO_LSB 0xC4 > +#define RR_ADC_GPIO_MSB 0xC5 > + > +#define RR_ADC_ATEST_CTRL 0xC8 > +#define RR_ADC_ATEST_TRIGGER 0xC9 > +#define RR_ADC_ATEST_STS 0xCA > +#define RR_ADC_ATEST_LSB 0xCC > +#define RR_ADC_ATEST_MSB 0xCD > +#define RR_ADC_SEC_ACCESS 0xD0 > + > +#define RR_ADC_PERPH_RESET_CTL2 0xD9 > +#define RR_ADC_PERPH_RESET_CTL3 0xDA > +#define RR_ADC_PERPH_RESET_CTL4 0xDB > +#define RR_ADC_INT_TEST1 0xE0 > +#define RR_ADC_INT_TEST_VAL 0xE1 > + > +#define RR_ADC_TM_TRIGGER_CTRLS 0xE2 > +#define RR_ADC_TM_ADC_CTRLS 0xE3 > +#define RR_ADC_TM_CNL_CTRL 0xE4 > +#define RR_ADC_TM_BATT_ID_CTRL 0xE5 > +#define RR_ADC_TM_THERM_CTRL 0xE6 > +#define RR_ADC_TM_CONV_STS 0xE7 > +#define RR_ADC_TM_ADC_READ_LSB 0xE8 > +#define RR_ADC_TM_ADC_READ_MSB 0xE9 > +#define RR_ADC_TM_ATEST_MUX_1 0xEA > +#define RR_ADC_TM_ATEST_MUX_2 0xEB > +#define RR_ADC_TM_REFERENCES 0xED > +#define RR_ADC_TM_MISC_CTL 0xEE > +#define RR_ADC_TM_RR_CTRL 0xEF > + > +#define RR_ADC_TRIGGER_EVERY_CYCLE BIT(7) > +#define RR_ADC_TRIGGER_CTL BIT(0) > + > +#define RR_ADC_BATT_ID_RANGE 820 > + > +#define RR_ADC_BITS 10 > +#define RR_ADC_CHAN_MSB (1 << RR_ADC_BITS) > +#define RR_ADC_FS_VOLTAGE_MV 2500 > + > +/* BATT_THERM 0.25K/LSB */ > +#define RR_ADC_BATT_THERM_LSB_K 4 > + > +#define RR_ADC_TEMP_FS_VOLTAGE_NUM 5000000 > +#define RR_ADC_TEMP_FS_VOLTAGE_DEN 3 > +#define RR_ADC_DIE_TEMP_OFFSET 601400 > +#define RR_ADC_DIE_TEMP_SLOPE 2 > +#define RR_ADC_DIE_TEMP_OFFSET_MILLI_DEGC 25000 > + > +#define RR_ADC_CHG_TEMP_GF_OFFSET_UV 1303168 > +#define RR_ADC_CHG_TEMP_GF_SLOPE_UV_PER_C 3784 > +#define RR_ADC_CHG_TEMP_SMIC_OFFSET_UV 1338433 > +#define RR_ADC_CHG_TEMP_SMIC_SLOPE_UV_PER_C 3655 > +#define RR_ADC_CHG_TEMP_660_GF_OFFSET_UV 1309001 > +#define RR_ADC_CHG_TEMP_660_GF_SLOPE_UV_PER_C 3403 > +#define RR_ADC_CHG_TEMP_660_SMIC_OFFSET_UV 1295898 > +#define RR_ADC_CHG_TEMP_660_SMIC_SLOPE_UV_PER_C 3596 > +#define RR_ADC_CHG_TEMP_660_MGNA_OFFSET_UV 1314779 > +#define RR_ADC_CHG_TEMP_660_MGNA_SLOPE_UV_PER_C 3496 > +#define RR_ADC_CHG_TEMP_OFFSET_MILLI_DEGC 25000 > +#define RR_ADC_CHG_THRESHOLD_SCALE 4 > + > +#define RR_ADC_VOLT_INPUT_FACTOR 8 > +#define RR_ADC_CURR_INPUT_FACTOR 2000 > +#define RR_ADC_CURR_USBIN_INPUT_FACTOR_MIL 1886 > +#define RR_ADC_CURR_USBIN_660_FACTOR_MIL 9 > +#define RR_ADC_CURR_USBIN_660_UV_VAL 579500 > + > +#define RR_ADC_GPIO_FS_RANGE 5000 > +#define RR_ADC_COHERENT_CHECK_RETRY 5 > +#define RR_ADC_CHAN_MAX_CONTINUOUS_BUFFER_LEN 16 > + > +#define RR_ADC_STS_CHANNEL_READING_MASK GENMASK(1, 0) > +#define RR_ADC_STS_CHANNEL_STS BIT(1) > + > +#define RR_ADC_TP_REV_VERSION1 21 > +#define RR_ADC_TP_REV_VERSION2 29 > +#define RR_ADC_TP_REV_VERSION3 32 > + > +#define RRADC_BATT_ID_DELAY_MAX 8 > + > +enum rradc_channel_id { > + RR_ADC_BATT_ID = 0, > + RR_ADC_BATT_THERM, > + RR_ADC_SKIN_TEMP, > + RR_ADC_USBIN_I, > + RR_ADC_USBIN_V, > + RR_ADC_DCIN_I, > + RR_ADC_DCIN_V, > + RR_ADC_DIE_TEMP, > + RR_ADC_CHG_TEMP, > + RR_ADC_GPIO, > + RR_ADC_CHAN_MAX > +}; > + > +struct rradc_chip; > + > +/** > + * struct rradc_channel - rradc channel data > + * @label: channel label > + * @lsb: Channel least significant byte > + * @status: Channel status address > + * @size: number of bytes to read > + * @trigger_addr: Trigger address, trigger is only used on some channels > + * @trigger_mask: Trigger mask > + * @scale_fn: Post process callback for channels which can't be exposed > + * as offset + scale. > + */ > +struct rradc_channel { > + const char *label; > + u8 lsb; > + u8 status; > + int size; > + int trigger_addr; > + int trigger_mask; > + int (*scale_fn)(struct rradc_chip *chip, u16 adc_code, int *result); > +}; > + > +struct rradc_chip { > + struct device *dev; > + const struct qcom_spmi_pmic *pmic; > + /* > + * Lock held while doing channel conversion > + * 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, __le16 *buf, int len) > +{ > + int ret, retry_cnt = 0; > + __le16 data_check[RR_ADC_CHAN_MAX_CONTINUOUS_BUFFER_LEN / 2]; > + > + 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, buf, > + 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(buf, 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; > + 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; > + return 0; > + default: > + *offset = RR_ADC_CHG_TEMP_660_MGNA_OFFSET_UV; > + *slope = RR_ADC_CHG_TEMP_660_MGNA_SLOPE_UV_PER_C; > + } > + } 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; > + 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; > + return 0; > + default: > + return -EINVAL; > + } > + } > + > + return -EINVAL; > +} > + > +/* > + * These functions explicitly cast int64_t to int. > + * They will never overflow, as the values are small enough. > + */ > +static int rradc_post_process_batt_id(struct rradc_chip *chip, u16 adc_code, > + int *result_ohms) > +{ > + uint32_t current_value; > + int64_t r_id; > + > + current_value = chip->batt_id_data; > + r_id = ((int64_t)adc_code * RR_ADC_FS_VOLTAGE_MV); > + r_id = div64_s64(r_id, (RR_ADC_CHAN_MSB * current_value)); > + *result_ohms = (int)(r_id * MILLI); > + > + return 0; > +} > + > +static int rradc_enable_continuous_mode(struct rradc_chip *chip) > +{ > + int ret; > + > + /* Clear channel log */ > + ret = regmap_update_bits(chip->regmap, chip->base + RR_ADC_LOG, > + RR_ADC_LOG_CLR_CTRL, RR_ADC_LOG_CLR_CTRL); > + if (ret < 0) { > + dev_err(chip->dev, "log ctrl update to clear failed:%d\n", ret); > + return ret; > + } > + > + ret = regmap_update_bits(chip->regmap, chip->base + RR_ADC_LOG, > + RR_ADC_LOG_CLR_CTRL, 0); > + if (ret < 0) { > + dev_err(chip->dev, "log ctrl update to not clear failed:%d\n", > + ret); > + return ret; > + } > + > + /* Switch to continuous mode */ > + ret = regmap_update_bits(chip->regmap, chip->base + RR_ADC_CTL, > + RR_ADC_CTL_CONTINUOUS_SEL, > + RR_ADC_CTL_CONTINUOUS_SEL); > + if (ret < 0) > + dev_err(chip->dev, "Update to continuous mode failed:%d\n", > + ret); > + > + return ret; > +} > + > +static int rradc_disable_continuous_mode(struct rradc_chip *chip) > +{ > + int ret; > + > + /* Switch to non continuous mode */ > + ret = regmap_update_bits(chip->regmap, chip->base + RR_ADC_CTL, > + RR_ADC_CTL_CONTINUOUS_SEL, 0); > + if (ret < 0) > + dev_err(chip->dev, "Update to non-continuous mode failed:%d\n", > + ret); > + > + return ret; > +} > + > +static bool rradc_is_ready(struct rradc_chip *chip, > + enum rradc_channel_id chan_address) > +{ > + const struct rradc_channel *chan = &rradc_chans[chan_address]; > + int ret; > + unsigned int status, mask; > + > + /* BATT_ID STS bit does not get set initially */ > + switch (chan_address) { > + case RR_ADC_BATT_ID: > + mask = RR_ADC_STS_CHANNEL_STS; > + break; > + default: > + mask = RR_ADC_STS_CHANNEL_READING_MASK; > + break; > + } > + > + ret = regmap_read(chip->regmap, chip->base + chan->status, &status); > + if (ret < 0 || !(status & mask)) > + return false; > + > + return true; > +} > + > +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 = -ETIMEDOUT; > + } > + > + 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 */ > + 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_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; > + __le16 buf[3]; > + > + 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 = le16_to_cpu(buf[2]); > + u16 batt_id_15 = le16_to_cpu(buf[1]); > + u16 batt_id_5 = le16_to_cpu(buf[0]); > + > + 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 = le16_to_cpu(buf[0]); > + } > + > +unlock_out: > + mutex_unlock(&chip->conversion_lock); > + > + 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: > + return -EINVAL; > + } > +} > + > +static int rradc_read_offset(struct rradc_chip *chip, int chan_address, int *val) > +{ > + int64_t fab_offset, fab_slope; > + int64_t offset1, offset2; > + int ret; > + > + switch (chan_address) { > + case RR_ADC_SKIN_TEMP: > + /* > + * Offset from kelvin to degC, divided by the > + * scale factor (250). We lose some precision here. > + * 273150 / 250 = 1092.6 > + */ > + *val = div64_s64(ABSOLUTE_ZERO_MILLICELSIUS, > + (MILLI / RR_ADC_BATT_THERM_LSB_K)); > + return IIO_VAL_INT; > + case RR_ADC_CHG_TEMP: > + 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; > + } > + offset1 = -(fab_offset * RR_ADC_TEMP_FS_VOLTAGE_DEN * > + RR_ADC_CHAN_MSB); > + offset1 += (int64_t)RR_ADC_TEMP_FS_VOLTAGE_NUM / 2ULL; > + offset1 = div64_s64(offset1, > + (int64_t)(RR_ADC_TEMP_FS_VOLTAGE_NUM)); > + > + offset2 = (int64_t)RR_ADC_CHG_TEMP_OFFSET_MILLI_DEGC * > + RR_ADC_TEMP_FS_VOLTAGE_DEN * RR_ADC_CHAN_MSB * > + (int64_t)fab_slope; > + offset2 += ((int64_t)MILLI * RR_ADC_TEMP_FS_VOLTAGE_NUM) / 2; > + offset2 = div64_s64( > + offset2, ((int64_t)MILLI * RR_ADC_TEMP_FS_VOLTAGE_NUM)); > + > + /* > + * The -1 is to compensate for lost precision. > + * It should actually be -0.7906976744186046. > + * This works out to every value being off > + * by about +0.091 degrees C after applying offset and scale. > + */ > + *val = (int)(offset1 - offset2 - 1); > + return IIO_VAL_INT; > + case RR_ADC_DIE_TEMP: > + offset1 = -RR_ADC_DIE_TEMP_OFFSET * > + (int64_t)RR_ADC_TEMP_FS_VOLTAGE_DEN * > + (int64_t)RR_ADC_CHAN_MSB; > + offset1 = div64_s64(offset1, RR_ADC_TEMP_FS_VOLTAGE_NUM); > + > + offset2 = -(int64_t)RR_ADC_CHG_TEMP_OFFSET_MILLI_DEGC * > + RR_ADC_TEMP_FS_VOLTAGE_DEN * RR_ADC_CHAN_MSB * > + RR_ADC_DIE_TEMP_SLOPE; > + offset2 = div64_s64(offset2, > + ((int64_t)RR_ADC_TEMP_FS_VOLTAGE_NUM)); > + > + /* > + * The result is -339, it should be -338.69789, this results > + * in the calculated die temp being off by > + * -0.004 - -0.0175 degrees C > + */ > + *val = (int)(offset1 - offset2); > + return IIO_VAL_INT; > + default: > + break; > + } > + return -EINVAL; > +} > + > +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: > + 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 int rradc_read_label(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, char *label) > +{ > + return snprintf(label, PAGE_SIZE, "%s\n", > + rradc_chans[chan->address].label); > +} > + > +static const struct iio_info rradc_info = { > + .read_raw = rradc_read_raw, > + .read_label = rradc_read_label, > +}; > + > +static const struct rradc_channel rradc_chans[RR_ADC_CHAN_MAX] = { > + { > + .label = "batt_id", > + .scale_fn = rradc_post_process_batt_id, > + .lsb = RR_ADC_BATT_ID_5_LSB, > + .status = RR_ADC_BATT_ID_STS, > + .size = 6, > + .trigger_addr = RR_ADC_BATT_ID_TRIGGER, > + .trigger_mask = BIT(0), > + }, { > + .label = "batt", > + .lsb = RR_ADC_BATT_THERM_LSB, > + .status = RR_ADC_BATT_THERM_STS, > + .size = 2, > + .trigger_addr = RR_ADC_BATT_THERM_TRIGGER, > + }, { > + .label = "pmi8998_skin", > + .lsb = RR_ADC_SKIN_TEMP_LSB, > + .status = RR_ADC_AUX_THERM_STS, > + .size = 2, > + .trigger_addr = RR_ADC_AUX_THERM_TRIGGER, > + }, { > + .label = "usbin_i", > + .lsb = RR_ADC_USB_IN_I_LSB, > + .status = RR_ADC_USB_IN_I_STS, > + .size = 2, > + .trigger_addr = RR_ADC_USB_IN_I_TRIGGER, > + }, { > + .label = "usbin_v", > + .lsb = RR_ADC_USB_IN_V_LSB, > + .status = RR_ADC_USB_IN_V_STS, > + .size = 2, > + .trigger_addr = RR_ADC_USB_IN_V_TRIGGER, > + .trigger_mask = BIT(7), > + }, { > + .label = "dcin_i", > + .lsb = RR_ADC_DC_IN_I_LSB, > + .status = RR_ADC_DC_IN_I_STS, > + .size = 2, > + .trigger_addr = RR_ADC_DC_IN_I_TRIGGER, > + }, { > + .label = "dcin_v", > + .lsb = RR_ADC_DC_IN_V_LSB, > + .status = RR_ADC_DC_IN_V_STS, > + .size = 2, > + .trigger_addr = RR_ADC_DC_IN_V_TRIGGER, > + }, { > + .label = "pmi8998_die", > + .lsb = RR_ADC_PMI_DIE_TEMP_LSB, > + .status = RR_ADC_PMI_DIE_TEMP_STS, > + .size = 2, > + .trigger_addr = RR_ADC_PMI_DIE_TEMP_TRIGGER, > + .trigger_mask = RR_ADC_TRIGGER_EVERY_CYCLE, > + }, { > + .label = "chg", > + .lsb = RR_ADC_CHARGER_TEMP_LSB, > + .status = RR_ADC_CHARGER_TEMP_STS, > + .size = 2, > + .trigger_addr = RR_ADC_CHARGER_TEMP_TRIGGER, > + }, { > + .label = "gpio", > + .lsb = RR_ADC_GPIO_LSB, > + .status = RR_ADC_GPIO_STS, > + .size = 2, > + .trigger_addr = RR_ADC_GPIO_TRIGGER, > + }, > +}; > + > +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), > + .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, we need it 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); > + } > + > + switch (chip->pmic->subtype) { > + case PMI8998_SUBTYPE: > + indio_dev->name = "pmi8998-rradc"; > + break; > + case PM660_SUBTYPE: > + indio_dev->name = "pm660-rradc"; > + break; > + default: > + indio_dev->name = DRIVER_NAME; > + break; > + } > + 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@linaro.org>"); > +MODULE_LICENSE("GPL v2");
On Sun 27 Mar 08:03 PDT 2022, Jonathan Cameron wrote: > On Wed, 23 Mar 2022 16:28:16 +0000 > Caleb Connolly <caleb.connolly@linaro.org> wrote: > > > From: Caleb Connolly <caleb.connolly@linaro.org> > > > > 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> > Whilst I said this patch was fine a few versions ago I didn't add a tag > on basis I would probably end up picking it up. > > To make things more flexible, I'll add one and either Lee or I can > do the immutable branch once Lee has had a chance to take a look and > is happy with the mfd parts. > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Are either of you planning to pick up the driver changes? The dts changes can be merged independently, so I would prefer to pick those through my tree. But I would like to know that the binding and implementation is agreed upon before doing so. Thanks, Bjorn
On Tue, 12 Apr 2022 20:08:03 -0700 Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > On Sun 27 Mar 08:03 PDT 2022, Jonathan Cameron wrote: > > > On Wed, 23 Mar 2022 16:28:16 +0000 > > Caleb Connolly <caleb.connolly@linaro.org> wrote: > > > > > From: Caleb Connolly <caleb.connolly@linaro.org> > > > > > > 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> > > Whilst I said this patch was fine a few versions ago I didn't add a tag > > on basis I would probably end up picking it up. > > > > To make things more flexible, I'll add one and either Lee or I can > > do the immutable branch once Lee has had a chance to take a look and > > is happy with the mfd parts. > > > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > Are either of you planning to pick up the driver changes? > > The dts changes can be merged independently, so I would prefer to pick > those through my tree. But I would like to know that the binding and > implementation is agreed upon before doing so. I need an Ack from Lee for the mfd changes and ideally Stephen for the SPMI patch. Lee, guessing you are busy, but if you have a chance to check this set over that would be great. Jonathan > > Thanks, > Bjorn
On Wed, 23 Mar 2022, Caleb Connolly wrote: > From: Caleb Connolly <caleb.connolly@linaro.org> > > Some PMIC functions such as the RRADC need to be aware of the PMIC > chip revision information to implement errata or otherwise adjust > behaviour, export the PMIC information to enable this. > > This is specifically required to enable the RRADC to adjust > coefficients based on which chip fab the PMIC was produced in, > this can vary per unique device and therefore has to be read at > runtime. > > Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Tested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/mfd/qcom-spmi-pmic.c | 261 +++++++++++++++++++----------- > include/soc/qcom/qcom-spmi-pmic.h | 60 +++++++ > 2 files changed, 231 insertions(+), 90 deletions(-) > create mode 100644 include/soc/qcom/qcom-spmi-pmic.h Apologies for the delay. I've been snowed under. > diff --git a/drivers/mfd/qcom-spmi-pmic.c b/drivers/mfd/qcom-spmi-pmic.c > index 1cacc00aa6c9..d8e54d9d3448 100644 > --- a/drivers/mfd/qcom-spmi-pmic.c > +++ b/drivers/mfd/qcom-spmi-pmic.c > @@ -3,11 +3,16 @@ > * Copyright (c) 2014, The Linux Foundation. All rights reserved. > */ > > +#include <linux/device.h> > +#include <linux/errno.h> > +#include <linux/gfp.h> > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/spmi.h> > +#include <linux/types.h> > #include <linux/regmap.h> > #include <linux/of_platform.h> > +#include <soc/qcom/qcom-spmi-pmic.h> > > #define PMIC_REV2 0x101 > #define PMIC_REV3 0x102 > @@ -17,106 +22,151 @@ > > #define PMIC_TYPE_VALUE 0x51 > > -#define COMMON_SUBTYPE 0x00 > -#define PM8941_SUBTYPE 0x01 > -#define PM8841_SUBTYPE 0x02 > -#define PM8019_SUBTYPE 0x03 > -#define PM8226_SUBTYPE 0x04 > -#define PM8110_SUBTYPE 0x05 > -#define PMA8084_SUBTYPE 0x06 > -#define PMI8962_SUBTYPE 0x07 > -#define PMD9635_SUBTYPE 0x08 > -#define PM8994_SUBTYPE 0x09 > -#define PMI8994_SUBTYPE 0x0a > -#define PM8916_SUBTYPE 0x0b > -#define PM8004_SUBTYPE 0x0c > -#define PM8909_SUBTYPE 0x0d > -#define PM8028_SUBTYPE 0x0e > -#define PM8901_SUBTYPE 0x0f > -#define PM8950_SUBTYPE 0x10 > -#define PMI8950_SUBTYPE 0x11 > -#define PM8998_SUBTYPE 0x14 > -#define PMI8998_SUBTYPE 0x15 > -#define PM8005_SUBTYPE 0x18 > -#define PM660L_SUBTYPE 0x1A > -#define PM660_SUBTYPE 0x1B > -#define PM8150_SUBTYPE 0x1E > -#define PM8150L_SUBTYPE 0x1f > -#define PM8150B_SUBTYPE 0x20 > -#define PMK8002_SUBTYPE 0x21 > -#define PM8009_SUBTYPE 0x24 > -#define PM8150C_SUBTYPE 0x26 > -#define SMB2351_SUBTYPE 0x29 > +struct qcom_spmi_dev { > + int num_usids; > + struct qcom_spmi_pmic pmic; > +}; > + > +#define N_USIDS(n) ((void *)n) > > static const struct of_device_id pmic_spmi_id_table[] = { > - { .compatible = "qcom,pm660", .data = (void *)PM660_SUBTYPE }, > - { .compatible = "qcom,pm660l", .data = (void *)PM660L_SUBTYPE }, > - { .compatible = "qcom,pm8004", .data = (void *)PM8004_SUBTYPE }, > - { .compatible = "qcom,pm8005", .data = (void *)PM8005_SUBTYPE }, > - { .compatible = "qcom,pm8019", .data = (void *)PM8019_SUBTYPE }, > - { .compatible = "qcom,pm8028", .data = (void *)PM8028_SUBTYPE }, > - { .compatible = "qcom,pm8110", .data = (void *)PM8110_SUBTYPE }, > - { .compatible = "qcom,pm8150", .data = (void *)PM8150_SUBTYPE }, > - { .compatible = "qcom,pm8150b", .data = (void *)PM8150B_SUBTYPE }, > - { .compatible = "qcom,pm8150c", .data = (void *)PM8150C_SUBTYPE }, > - { .compatible = "qcom,pm8150l", .data = (void *)PM8150L_SUBTYPE }, > - { .compatible = "qcom,pm8226", .data = (void *)PM8226_SUBTYPE }, > - { .compatible = "qcom,pm8841", .data = (void *)PM8841_SUBTYPE }, > - { .compatible = "qcom,pm8901", .data = (void *)PM8901_SUBTYPE }, > - { .compatible = "qcom,pm8909", .data = (void *)PM8909_SUBTYPE }, > - { .compatible = "qcom,pm8916", .data = (void *)PM8916_SUBTYPE }, > - { .compatible = "qcom,pm8941", .data = (void *)PM8941_SUBTYPE }, > - { .compatible = "qcom,pm8950", .data = (void *)PM8950_SUBTYPE }, > - { .compatible = "qcom,pm8994", .data = (void *)PM8994_SUBTYPE }, > - { .compatible = "qcom,pm8998", .data = (void *)PM8998_SUBTYPE }, > - { .compatible = "qcom,pma8084", .data = (void *)PMA8084_SUBTYPE }, > - { .compatible = "qcom,pmd9635", .data = (void *)PMD9635_SUBTYPE }, > - { .compatible = "qcom,pmi8950", .data = (void *)PMI8950_SUBTYPE }, > - { .compatible = "qcom,pmi8962", .data = (void *)PMI8962_SUBTYPE }, > - { .compatible = "qcom,pmi8994", .data = (void *)PMI8994_SUBTYPE }, > - { .compatible = "qcom,pmi8998", .data = (void *)PMI8998_SUBTYPE }, > - { .compatible = "qcom,pmk8002", .data = (void *)PMK8002_SUBTYPE }, > - { .compatible = "qcom,smb2351", .data = (void *)SMB2351_SUBTYPE }, > - { .compatible = "qcom,spmi-pmic", .data = (void *)COMMON_SUBTYPE }, > + { .compatible = "qcom,pm660", .data = N_USIDS(2) }, > + { .compatible = "qcom,pm660l", .data = N_USIDS(2) }, > + { .compatible = "qcom,pm8004", .data = N_USIDS(2) }, > + { .compatible = "qcom,pm8005", .data = N_USIDS(2) }, > + { .compatible = "qcom,pm8019", .data = N_USIDS(2) }, > + { .compatible = "qcom,pm8028", .data = N_USIDS(2) }, > + { .compatible = "qcom,pm8110", .data = N_USIDS(2) }, > + { .compatible = "qcom,pm8150", .data = N_USIDS(2) }, > + { .compatible = "qcom,pm8150b", .data = N_USIDS(2) }, > + { .compatible = "qcom,pm8150c", .data = N_USIDS(2) }, > + { .compatible = "qcom,pm8150l", .data = N_USIDS(2) }, > + { .compatible = "qcom,pm8226", .data = N_USIDS(2) }, > + { .compatible = "qcom,pm8841", .data = N_USIDS(2) }, > + { .compatible = "qcom,pm8901", .data = N_USIDS(2) }, > + { .compatible = "qcom,pm8909", .data = N_USIDS(2) }, > + { .compatible = "qcom,pm8916", .data = N_USIDS(2) }, > + { .compatible = "qcom,pm8941", .data = N_USIDS(2) }, > + { .compatible = "qcom,pm8950", .data = N_USIDS(2) }, > + { .compatible = "qcom,pm8994", .data = N_USIDS(2) }, > + { .compatible = "qcom,pm8998", .data = N_USIDS(2) }, > + { .compatible = "qcom,pma8084", .data = N_USIDS(2) }, > + { .compatible = "qcom,pmd9635", .data = N_USIDS(2) }, > + { .compatible = "qcom,pmi8950", .data = N_USIDS(2) }, > + { .compatible = "qcom,pmi8962", .data = N_USIDS(2) }, > + { .compatible = "qcom,pmi8994", .data = N_USIDS(2) }, > + { .compatible = "qcom,pmi8998", .data = N_USIDS(2) }, > + { .compatible = "qcom,pmk8002", .data = N_USIDS(2) }, > + { .compatible = "qcom,smb2351", .data = N_USIDS(2) }, > + { .compatible = "qcom,spmi-pmic", .data = N_USIDS(1) }, > { } > }; > > -static void pmic_spmi_show_revid(struct regmap *map, struct device *dev) > +#undef N_USIDS Why is this here? Can't we use a unique macro instead? > +/* > + * A PMIC can be represented by multiple SPMI devices, but > + * only the base PMIC device will contain a reference to > + * the revision information. > + * > + * This function takes a pointer to a function device and > + * returns a pointer to the base PMIC device. > + * > + * This only supports PMICs with 1 or 2 USIDs. > + */ > +static struct spmi_device *qcom_pmic_get_base_usid(struct device *dev) > { > - unsigned int rev2, minor, major, type, subtype; > - const char *name = "unknown"; > - int ret, i; > + struct spmi_device *sdev; > + struct qcom_spmi_dev *ctx; > + struct device_node *spmi_bus; > + struct device_node *other_usid = NULL; > + int function_parent_usid, ret; > + u32 pmic_addr; > > - ret = regmap_read(map, PMIC_TYPE, &type); > - if (ret < 0) > - return; > + if (!of_match_device(pmic_spmi_id_table, dev)) > + return ERR_PTR(-EINVAL); Can this happen? How else would the device have been enumerated? > + sdev = to_spmi_device(dev); > + ctx = spmi_device_get_drvdata(sdev); This function looks like abstraction for the sake of abstraction. Why not just use dev_get_drvdata()? > + /* > + * Quick return if the function device is already in the base > + * USID. This will always be hit for PMICs with only 1 USID. > + */ > + if (sdev->usid % ctx->num_usids == 0) > + return sdev; > + > + function_parent_usid = sdev->usid; > + > + /* > + * Walk through the list of PMICs until we find the sibling USID. > + * The goal is to find the first USID which is less than the > + * number of USIDs in the PMIC away, e.g. for a PMIC with 2 USIDs "array" perhaps? > + * where the function device is under USID 3, we want to find the > + * device for USID 2. > + */ > + spmi_bus = of_get_parent(sdev->dev.of_node); > + do { > + other_usid = of_get_next_child(spmi_bus, other_usid); '\n' > + ret = of_property_read_u32_index(other_usid, "reg", 0, &pmic_addr); > + if (ret) > + return ERR_PTR(ret); '\n' > + sdev = spmi_device_from_of(other_usid); > + if (sdev == NULL) { if (!sdev) > + /* > + * If the base USID for this PMIC hasn't probed yet > + * but the secondary USID has, then we need to defer > + * the function driver so that it will attempt to > + * probe again when the base USID is ready. > + */ > + if (pmic_addr == function_parent_usid - (ctx->num_usids - 1)) Double " ". Over-bracketing of statements with matching operands. > + return ERR_PTR(-EPROBE_DEFER); > + > + continue; > + } > + > + if (pmic_addr == function_parent_usid - (ctx->num_usids - 1)) > + return sdev; Wouldn't it be better written like this: > + if (pmic_addr == function_parent_usid - (ctx->num_usids - 1)) { > + sdev = spmi_device_from_of(other_usid); > + if (!sdev) > + /* > + * If the base USID for this PMIC hasn't probed yet > + * but the secondary USID has, then we need to defer > + * the function driver so that it will attempt to > + * probe again when the base USID is ready. > + */ > + return ERR_PTR(-EPROBE_DEFER); > + return sdev; > + } [...] > + } while (other_usid->sibling); > + > + return ERR_PTR(-ENODATA); > +} > + > +static inline void pmic_print_info(struct device *dev, struct qcom_spmi_pmic *pmic) > +{ > + dev_dbg(dev, "%x: %s v%d.%d\n", > + pmic->subtype, pmic->name, pmic->major, pmic->minor); > +} More abstraction for no apparent reason. > - if (type != PMIC_TYPE_VALUE) > - return; > +static int pmic_spmi_load_revid(struct regmap *map, struct device *dev, > + struct qcom_spmi_pmic *pmic) > +{ > + int ret; > > - ret = regmap_read(map, PMIC_SUBTYPE, &subtype); > + ret = regmap_read(map, PMIC_TYPE, &pmic->type); > if (ret < 0) > - return; > + return ret; > > - for (i = 0; i < ARRAY_SIZE(pmic_spmi_id_table); i++) { > - if (subtype == (unsigned long)pmic_spmi_id_table[i].data) > - break; > - } > + if (pmic->type != PMIC_TYPE_VALUE) > + return ret; > > - if (i != ARRAY_SIZE(pmic_spmi_id_table)) > - name = pmic_spmi_id_table[i].compatible; > + ret = regmap_read(map, PMIC_SUBTYPE, &pmic->subtype); > + if (ret < 0) > + return ret; > + > + pmic->name = of_match_device(pmic_spmi_id_table, dev)->compatible; > > - ret = regmap_read(map, PMIC_REV2, &rev2); > + ret = regmap_read(map, PMIC_REV2, &pmic->rev2); > if (ret < 0) > - return; > + return ret; > > - ret = regmap_read(map, PMIC_REV3, &minor); > + ret = regmap_read(map, PMIC_REV3, &pmic->minor); > if (ret < 0) > - return; > + return ret; > > - ret = regmap_read(map, PMIC_REV4, &major); > + ret = regmap_read(map, PMIC_REV4, &pmic->major); > if (ret < 0) > - return; > + return ret; > > /* > * In early versions of PM8941 and PM8226, the major revision number > @@ -124,15 +174,34 @@ static void pmic_spmi_show_revid(struct regmap *map, struct device *dev) > * Increment the major revision number here if the chip is an early > * version of PM8941 or PM8226. > */ > - if ((subtype == PM8941_SUBTYPE || subtype == PM8226_SUBTYPE) && > - major < 0x02) > - major++; > + if ((pmic->subtype == PM8941_SUBTYPE || pmic->subtype == PM8226_SUBTYPE) && > + pmic->major < 0x02) Please define this magic number while you're at it. > + pmic->major++; > + > + if (pmic->subtype == PM8110_SUBTYPE) > + pmic->minor = pmic->rev2; > + > + pmic_print_info(dev, pmic); > > - if (subtype == PM8110_SUBTYPE) > - minor = rev2; > + return 0; > +} > + > +/** > + * qcom_pmic_get() - Get a pointer to the base PMIC device > + * > + * @dev: the pmic function device > + * @return: the struct qcom_spmi_pmic* pointer associated with the function device > + */ > +inline const struct qcom_spmi_pmic *qcom_pmic_get(struct device *dev) > +{ > + struct spmi_device *sdev = qcom_pmic_get_base_usid(dev->parent); > > - dev_dbg(dev, "%x: %s v%d.%d\n", subtype, name, major, minor); > + if (IS_ERR(sdev)) > + return ERR_CAST(sdev); > + > + return &((struct qcom_spmi_dev *)spmi_device_get_drvdata(sdev))->pmic; This is horrible. Please expand it out. > } > +EXPORT_SYMBOL(qcom_pmic_get); > > static const struct regmap_config spmi_regmap_config = { > .reg_bits = 16, > @@ -144,14 +213,26 @@ static const struct regmap_config spmi_regmap_config = { > static int pmic_spmi_probe(struct spmi_device *sdev) > { > struct regmap *regmap; > + struct qcom_spmi_dev *ctx; > + int ret; > > regmap = devm_regmap_init_spmi_ext(sdev, &spmi_regmap_config); > if (IS_ERR(regmap)) > return PTR_ERR(regmap); > > + ctx = devm_kzalloc(&sdev->dev, sizeof(*ctx), GFP_KERNEL); > + if (!ctx) > + return -ENOMEM; > + > + ctx->num_usids = (long)of_device_get_match_data(&sdev->dev); Why does this need to be long? In fact, it's not is it? It's an int: +struct qcom_spmi_dev { + int num_usids; + struct qcom_spmi_pmic pmic; +}; > /* Only the first slave id for a PMIC contains this information */ > - if (sdev->usid % 2 == 0) > - pmic_spmi_show_revid(regmap, &sdev->dev); > + if (sdev->usid % ctx->num_usids == 0) { > + ret = pmic_spmi_load_revid(regmap, &sdev->dev, &ctx->pmic); > + if (ret < 0) > + return ret; > + } > + spmi_device_set_drvdata(sdev, ctx); > > return devm_of_platform_populate(&sdev->dev); > } > diff --git a/include/soc/qcom/qcom-spmi-pmic.h b/include/soc/qcom/qcom-spmi-pmic.h > new file mode 100644 > index 000000000000..5400e6509fe8 > --- /dev/null > +++ b/include/soc/qcom/qcom-spmi-pmic.h > @@ -0,0 +1,60 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* Copyright (c) 2021 Linaro. All rights reserved. > + * Copyright (c) 2021 Caleb Connolly <caleb.connolly@linaro.org> Your very own Copyright? Why? Any not Author: instead? > + */ > + > +#ifndef __QCOM_SPMI_PMIC_H__ > +#define __QCOM_SPMI_PMIC_H__ > + > +#define COMMON_SUBTYPE 0x00 > +#define PM8941_SUBTYPE 0x01 > +#define PM8841_SUBTYPE 0x02 > +#define PM8019_SUBTYPE 0x03 > +#define PM8226_SUBTYPE 0x04 > +#define PM8110_SUBTYPE 0x05 > +#define PMA8084_SUBTYPE 0x06 > +#define PMI8962_SUBTYPE 0x07 > +#define PMD9635_SUBTYPE 0x08 > +#define PM8994_SUBTYPE 0x09 > +#define PMI8994_SUBTYPE 0x0a > +#define PM8916_SUBTYPE 0x0b > +#define PM8004_SUBTYPE 0x0c > +#define PM8909_SUBTYPE 0x0d > +#define PM8028_SUBTYPE 0x0e > +#define PM8901_SUBTYPE 0x0f > +#define PM8950_SUBTYPE 0x10 > +#define PMI8950_SUBTYPE 0x11 > +#define PM8998_SUBTYPE 0x14 > +#define PMI8998_SUBTYPE 0x15 > +#define PM8005_SUBTYPE 0x18 > +#define PM660L_SUBTYPE 0x1A > +#define PM660_SUBTYPE 0x1B > +#define PM8150_SUBTYPE 0x1E > +#define PM8150L_SUBTYPE 0x1f > +#define PM8150B_SUBTYPE 0x20 > +#define PMK8002_SUBTYPE 0x21 > +#define PM8009_SUBTYPE 0x24 > +#define PM8150C_SUBTYPE 0x26 > +#define SMB2351_SUBTYPE 0x29 > + > +#define PMI8998_FAB_ID_SMIC 0x11 > +#define PMI8998_FAB_ID_GF 0x30 > + > +#define PM660_FAB_ID_GF 0x0 > +#define PM660_FAB_ID_TSMC 0x2 > +#define PM660_FAB_ID_MX 0x3 > + > +struct qcom_spmi_pmic { > + unsigned int type; > + unsigned int subtype; > + unsigned int major; > + unsigned int minor; > + unsigned int rev2; > + const char *name; > +}; > + > +struct device; Can't you just include the correct header? > +inline const struct qcom_spmi_pmic *qcom_pmic_get(struct device *dev); > + > +#endif /* __QCOM_SPMI_PMIC_H__ */
On Wed, 23 Mar 2022, Caleb Connolly wrote: > From: Caleb Connolly <caleb.connolly@linaro.org> > > The PMI8998 and PM660 expose the fab_id, this is needed by drivers like > the RRADC to calibrate ADC values. > > Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Tested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/mfd/qcom-spmi-pmic.c | 7 +++++++ > include/soc/qcom/qcom-spmi-pmic.h | 1 + > 2 files changed, 8 insertions(+) Please change the Subject line to match the style of the sub-system? Once changed: Acked-by: Lee Jones <lee.jones@linaro.org>
On 25/04/2022 16:05, Lee Jones wrote: > On Wed, 23 Mar 2022, Caleb Connolly wrote: > >> From: Caleb Connolly <caleb.connolly@linaro.org> >> >> The PMI8998 and PM660 expose the fab_id, this is needed by drivers like >> the RRADC to calibrate ADC values. >> >> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> >> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> Tested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> --- >> drivers/mfd/qcom-spmi-pmic.c | 7 +++++++ >> include/soc/qcom/qcom-spmi-pmic.h | 1 + >> 2 files changed, 8 insertions(+) > > Please change the Subject line to match the style of the sub-system? Hi, sorry if this is a silly question, I don't quite understand what you want me to change here, the subject line is in the same "mfd: driver:" format as other patches in the subsystem? > > Once changed: > > Acked-by: Lee Jones <lee.jones@linaro.org> >
On Tue, 26 Apr 2022, Caleb Connolly wrote: > On 25/04/2022 16:05, Lee Jones wrote: > > On Wed, 23 Mar 2022, Caleb Connolly wrote: > > > > > From: Caleb Connolly <caleb.connolly@linaro.org> > > > > > > The PMI8998 and PM660 expose the fab_id, this is needed by drivers like > > > the RRADC to calibrate ADC values. > > > > > > Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> > > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > Tested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > --- > > > drivers/mfd/qcom-spmi-pmic.c | 7 +++++++ > > > include/soc/qcom/qcom-spmi-pmic.h | 1 + > > > 2 files changed, 8 insertions(+) > > > > Please change the Subject line to match the style of the sub-system? > Hi, sorry if this is a silly question, I don't quite understand what you > want me to change here, the subject line is in the same "mfd: driver:" > format as other patches in the subsystem? mfd: qcom-spmi-pmic: Read fab ID on supported PMICs What's 'fab' should that be capitalised too? > > Once changed: > > > > Acked-by: Lee Jones <lee.jones@linaro.org> > > >
On 26/04/2022 19:24, Lee Jones wrote: > On Tue, 26 Apr 2022, Caleb Connolly wrote: >> On 25/04/2022 16:05, Lee Jones wrote: >>> On Wed, 23 Mar 2022, Caleb Connolly wrote: >>> >>>> From: Caleb Connolly <caleb.connolly@linaro.org> >>>> >>>> The PMI8998 and PM660 expose the fab_id, this is needed by drivers like >>>> the RRADC to calibrate ADC values. >>>> >>>> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> >>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>> Tested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>> --- >>>> drivers/mfd/qcom-spmi-pmic.c | 7 +++++++ >>>> include/soc/qcom/qcom-spmi-pmic.h | 1 + >>>> 2 files changed, 8 insertions(+) >>> >>> Please change the Subject line to match the style of the sub-system? >> Hi, sorry if this is a silly question, I don't quite understand what you >> want me to change here, the subject line is in the same "mfd: driver:" >> format as other patches in the subsystem? > > mfd: qcom-spmi-pmic: Read fab ID on supported PMICs > > What's 'fab' should that be capitalised too? "fab" is short for fabrication I think, the register value can be used to determine which factory the chip was manufactured in. I can make it clearer and go for mfd: qcom-spmi-pmic: Read fabrication ID on supported PMICs > >>> Once changed: >>> >>> Acked-by: Lee Jones <lee.jones@linaro.org> >>> >> >
On 25/04/2022 16:03, Lee Jones wrote: > > On Wed, 23 Mar 2022, Caleb Connolly wrote: >> From: Caleb Connolly <caleb.connolly@linaro.org> >> >> Some PMIC functions such as the RRADC need to be aware of the PMIC >> chip revision information to implement errata or otherwise adjust >> behaviour, export the PMIC information to enable this. >> >> This is specifically required to enable the RRADC to adjust >> coefficients based on which chip fab the PMIC was produced in, >> this can vary per unique device and therefore has to be read at >> runtime. >> >> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> >> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> Tested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> --- >> drivers/mfd/qcom-spmi-pmic.c | 261 +++++++++++++++++++----------- >> include/soc/qcom/qcom-spmi-pmic.h | 60 +++++++ >> 2 files changed, 231 insertions(+), 90 deletions(-) >> create mode 100644 include/soc/qcom/qcom-spmi-pmic.h > > Apologies for the delay. I've been snowed under. Hi Lee, Thanks a lot for the review, I had a few questions/clarifications > >> diff --git a/drivers/mfd/qcom-spmi-pmic.c b/drivers/mfd/qcom-spmi-pmic.c >> index 1cacc00aa6c9..d8e54d9d3448 100644 >> --- a/drivers/mfd/qcom-spmi-pmic.c >> +++ b/drivers/mfd/qcom-spmi-pmic.c >> @@ -3,11 +3,16 @@ >> * Copyright (c) 2014, The Linux Foundation. All rights reserved. >> */ >> >> +#include <linux/device.h> >> +#include <linux/errno.h> >> +#include <linux/gfp.h> >> #include <linux/kernel.h> >> #include <linux/module.h> >> #include <linux/spmi.h> >> +#include <linux/types.h> >> #include <linux/regmap.h> >> #include <linux/of_platform.h> >> +#include <soc/qcom/qcom-spmi-pmic.h> >> >> #define PMIC_REV2 0x101 >> #define PMIC_REV3 0x102 >> @@ -17,106 +22,151 @@ >> >> #define PMIC_TYPE_VALUE 0x51 >> >> -#define COMMON_SUBTYPE 0x00 >> -#define PM8941_SUBTYPE 0x01 >> -#define PM8841_SUBTYPE 0x02 >> -#define PM8019_SUBTYPE 0x03 >> -#define PM8226_SUBTYPE 0x04 >> -#define PM8110_SUBTYPE 0x05 >> -#define PMA8084_SUBTYPE 0x06 >> -#define PMI8962_SUBTYPE 0x07 >> -#define PMD9635_SUBTYPE 0x08 >> -#define PM8994_SUBTYPE 0x09 >> -#define PMI8994_SUBTYPE 0x0a >> -#define PM8916_SUBTYPE 0x0b >> -#define PM8004_SUBTYPE 0x0c >> -#define PM8909_SUBTYPE 0x0d >> -#define PM8028_SUBTYPE 0x0e >> -#define PM8901_SUBTYPE 0x0f >> -#define PM8950_SUBTYPE 0x10 >> -#define PMI8950_SUBTYPE 0x11 >> -#define PM8998_SUBTYPE 0x14 >> -#define PMI8998_SUBTYPE 0x15 >> -#define PM8005_SUBTYPE 0x18 >> -#define PM660L_SUBTYPE 0x1A >> -#define PM660_SUBTYPE 0x1B >> -#define PM8150_SUBTYPE 0x1E >> -#define PM8150L_SUBTYPE 0x1f >> -#define PM8150B_SUBTYPE 0x20 >> -#define PMK8002_SUBTYPE 0x21 >> -#define PM8009_SUBTYPE 0x24 >> -#define PM8150C_SUBTYPE 0x26 >> -#define SMB2351_SUBTYPE 0x29 >> +struct qcom_spmi_dev { >> + int num_usids; >> + struct qcom_spmi_pmic pmic; >> +}; >> + >> +#define N_USIDS(n) ((void *)n) >> >> static const struct of_device_id pmic_spmi_id_table[] = { >> - { .compatible = "qcom,pm660", .data = (void *)PM660_SUBTYPE }, >> - { .compatible = "qcom,pm660l", .data = (void *)PM660L_SUBTYPE }, >> - { .compatible = "qcom,pm8004", .data = (void *)PM8004_SUBTYPE }, >> - { .compatible = "qcom,pm8005", .data = (void *)PM8005_SUBTYPE }, >> - { .compatible = "qcom,pm8019", .data = (void *)PM8019_SUBTYPE }, >> - { .compatible = "qcom,pm8028", .data = (void *)PM8028_SUBTYPE }, >> - { .compatible = "qcom,pm8110", .data = (void *)PM8110_SUBTYPE }, >> - { .compatible = "qcom,pm8150", .data = (void *)PM8150_SUBTYPE }, >> - { .compatible = "qcom,pm8150b", .data = (void *)PM8150B_SUBTYPE }, >> - { .compatible = "qcom,pm8150c", .data = (void *)PM8150C_SUBTYPE }, >> - { .compatible = "qcom,pm8150l", .data = (void *)PM8150L_SUBTYPE }, >> - { .compatible = "qcom,pm8226", .data = (void *)PM8226_SUBTYPE }, >> - { .compatible = "qcom,pm8841", .data = (void *)PM8841_SUBTYPE }, >> - { .compatible = "qcom,pm8901", .data = (void *)PM8901_SUBTYPE }, >> - { .compatible = "qcom,pm8909", .data = (void *)PM8909_SUBTYPE }, >> - { .compatible = "qcom,pm8916", .data = (void *)PM8916_SUBTYPE }, >> - { .compatible = "qcom,pm8941", .data = (void *)PM8941_SUBTYPE }, >> - { .compatible = "qcom,pm8950", .data = (void *)PM8950_SUBTYPE }, >> - { .compatible = "qcom,pm8994", .data = (void *)PM8994_SUBTYPE }, >> - { .compatible = "qcom,pm8998", .data = (void *)PM8998_SUBTYPE }, >> - { .compatible = "qcom,pma8084", .data = (void *)PMA8084_SUBTYPE }, >> - { .compatible = "qcom,pmd9635", .data = (void *)PMD9635_SUBTYPE }, >> - { .compatible = "qcom,pmi8950", .data = (void *)PMI8950_SUBTYPE }, >> - { .compatible = "qcom,pmi8962", .data = (void *)PMI8962_SUBTYPE }, >> - { .compatible = "qcom,pmi8994", .data = (void *)PMI8994_SUBTYPE }, >> - { .compatible = "qcom,pmi8998", .data = (void *)PMI8998_SUBTYPE }, >> - { .compatible = "qcom,pmk8002", .data = (void *)PMK8002_SUBTYPE }, >> - { .compatible = "qcom,smb2351", .data = (void *)SMB2351_SUBTYPE }, >> - { .compatible = "qcom,spmi-pmic", .data = (void *)COMMON_SUBTYPE }, >> + { .compatible = "qcom,pm660", .data = N_USIDS(2) }, >> + { .compatible = "qcom,pm660l", .data = N_USIDS(2) }, >> + { .compatible = "qcom,pm8004", .data = N_USIDS(2) }, >> + { .compatible = "qcom,pm8005", .data = N_USIDS(2) }, >> + { .compatible = "qcom,pm8019", .data = N_USIDS(2) }, >> + { .compatible = "qcom,pm8028", .data = N_USIDS(2) }, >> + { .compatible = "qcom,pm8110", .data = N_USIDS(2) }, >> + { .compatible = "qcom,pm8150", .data = N_USIDS(2) }, >> + { .compatible = "qcom,pm8150b", .data = N_USIDS(2) }, >> + { .compatible = "qcom,pm8150c", .data = N_USIDS(2) }, >> + { .compatible = "qcom,pm8150l", .data = N_USIDS(2) }, >> + { .compatible = "qcom,pm8226", .data = N_USIDS(2) }, >> + { .compatible = "qcom,pm8841", .data = N_USIDS(2) }, >> + { .compatible = "qcom,pm8901", .data = N_USIDS(2) }, >> + { .compatible = "qcom,pm8909", .data = N_USIDS(2) }, >> + { .compatible = "qcom,pm8916", .data = N_USIDS(2) }, >> + { .compatible = "qcom,pm8941", .data = N_USIDS(2) }, >> + { .compatible = "qcom,pm8950", .data = N_USIDS(2) }, >> + { .compatible = "qcom,pm8994", .data = N_USIDS(2) }, >> + { .compatible = "qcom,pm8998", .data = N_USIDS(2) }, >> + { .compatible = "qcom,pma8084", .data = N_USIDS(2) }, >> + { .compatible = "qcom,pmd9635", .data = N_USIDS(2) }, >> + { .compatible = "qcom,pmi8950", .data = N_USIDS(2) }, >> + { .compatible = "qcom,pmi8962", .data = N_USIDS(2) }, >> + { .compatible = "qcom,pmi8994", .data = N_USIDS(2) }, >> + { .compatible = "qcom,pmi8998", .data = N_USIDS(2) }, >> + { .compatible = "qcom,pmk8002", .data = N_USIDS(2) }, >> + { .compatible = "qcom,smb2351", .data = N_USIDS(2) }, >> + { .compatible = "qcom,spmi-pmic", .data = N_USIDS(1) }, >> { } >> }; >> >> -static void pmic_spmi_show_revid(struct regmap *map, struct device *dev) >> +#undef N_USIDS > > Why is this here? Can't we use a unique macro instead? This felt neat to do but yeah there's no reason to undefine the macro, I'll drop this. > >> +/* >> + * A PMIC can be represented by multiple SPMI devices, but >> + * only the base PMIC device will contain a reference to >> + * the revision information. >> + * >> + * This function takes a pointer to a function device and >> + * returns a pointer to the base PMIC device. >> + * >> + * This only supports PMICs with 1 or 2 USIDs. >> + */ >> +static struct spmi_device *qcom_pmic_get_base_usid(struct device *dev) >> { >> - unsigned int rev2, minor, major, type, subtype; >> - const char *name = "unknown"; >> - int ret, i; >> + struct spmi_device *sdev; >> + struct qcom_spmi_dev *ctx; >> + struct device_node *spmi_bus; >> + struct device_node *other_usid = NULL; >> + int function_parent_usid, ret; >> + u32 pmic_addr; >> >> - ret = regmap_read(map, PMIC_TYPE, &type); >> - if (ret < 0) >> - return; >> + if (!of_match_device(pmic_spmi_id_table, dev)) >> + return ERR_PTR(-EINVAL); > > Can this happen? How else would the device have been enumerated? This function gets called from "qcom_pmic_get()" which can be called by any driver, the check is to make sure it's only called from drivers which are children of a qcom PMIC. > >> + sdev = to_spmi_device(dev); >> + ctx = spmi_device_get_drvdata(sdev); > > This function looks like abstraction for the sake of abstraction. > > Why not just use dev_get_drvdata()? That seems more sensible. > >> + /* >> + * Quick return if the function device is already in the base >> + * USID. This will always be hit for PMICs with only 1 USID. >> + */ >> + if (sdev->usid % ctx->num_usids == 0) >> + return sdev; >> + >> + function_parent_usid = sdev->usid; >> + >> + /* >> + * Walk through the list of PMICs until we find the sibling USID. >> + * The goal is to find the first USID which is less than the >> + * number of USIDs in the PMIC away, e.g. for a PMIC with 2 USIDs > > "array" perhaps? > >> + * where the function device is under USID 3, we want to find the >> + * device for USID 2. >> + */ >> + spmi_bus = of_get_parent(sdev->dev.of_node); >> + do { >> + other_usid = of_get_next_child(spmi_bus, other_usid); > > '\n' > >> + ret = of_property_read_u32_index(other_usid, "reg", 0, &pmic_addr); >> + if (ret) >> + return ERR_PTR(ret); > > '\n' > >> + sdev = spmi_device_from_of(other_usid); >> + if (sdev == NULL) { > > if (!sdev) > >> + /* >> + * If the base USID for this PMIC hasn't probed yet >> + * but the secondary USID has, then we need to defer >> + * the function driver so that it will attempt to >> + * probe again when the base USID is ready. >> + */ >> + if (pmic_addr == function_parent_usid - (ctx->num_usids - 1)) > > Double " ". Ack > > Over-bracketing of statements with matching operands. I don't think x - (y - 1) is equal to x - y - 1? Or am I misunderstanding you here? > >> + return ERR_PTR(-EPROBE_DEFER); >> + >> + continue; >> + } >> + >> + if (pmic_addr == function_parent_usid - (ctx->num_usids - 1)) >> + return sdev; > > Wouldn't it be better written like this: Yeah > >> + if (pmic_addr == function_parent_usid - (ctx->num_usids - 1)) { >> + sdev = spmi_device_from_of(other_usid); >> + if (!sdev) >> + /* >> + * If the base USID for this PMIC hasn't probed yet >> + * but the secondary USID has, then we need to defer >> + * the function driver so that it will attempt to >> + * probe again when the base USID is ready. >> + */ >> + return ERR_PTR(-EPROBE_DEFER); >> + return sdev; >> + } > > [...] > >> + } while (other_usid->sibling); >> + >> + return ERR_PTR(-ENODATA); >> +} >> + >> +static inline void pmic_print_info(struct device *dev, struct qcom_spmi_pmic *pmic) >> +{ >> + dev_dbg(dev, "%x: %s v%d.%d\n", >> + pmic->subtype, pmic->name, pmic->major, pmic->minor); >> +} > > More abstraction for no apparent reason. Oops, this got a bit messy over several reworks, I'll stick the dev_dbg back in pmic_spmi_load_revid() and drop this function. > >> - if (type != PMIC_TYPE_VALUE) >> - return; >> +static int pmic_spmi_load_revid(struct regmap *map, struct device *dev, >> + struct qcom_spmi_pmic *pmic) >> +{ >> + int ret; >> >> - ret = regmap_read(map, PMIC_SUBTYPE, &subtype); >> + ret = regmap_read(map, PMIC_TYPE, &pmic->type); >> if (ret < 0) >> - return; >> + return ret; >> >> - for (i = 0; i < ARRAY_SIZE(pmic_spmi_id_table); i++) { >> - if (subtype == (unsigned long)pmic_spmi_id_table[i].data) >> - break; >> - } >> + if (pmic->type != PMIC_TYPE_VALUE) >> + return ret; >> >> - if (i != ARRAY_SIZE(pmic_spmi_id_table)) >> - name = pmic_spmi_id_table[i].compatible; >> + ret = regmap_read(map, PMIC_SUBTYPE, &pmic->subtype); >> + if (ret < 0) >> + return ret; >> + >> + pmic->name = of_match_device(pmic_spmi_id_table, dev)->compatible; >> >> - ret = regmap_read(map, PMIC_REV2, &rev2); >> + ret = regmap_read(map, PMIC_REV2, &pmic->rev2); >> if (ret < 0) >> - return; >> + return ret; >> >> - ret = regmap_read(map, PMIC_REV3, &minor); >> + ret = regmap_read(map, PMIC_REV3, &pmic->minor); >> if (ret < 0) >> - return; >> + return ret; >> >> - ret = regmap_read(map, PMIC_REV4, &major); >> + ret = regmap_read(map, PMIC_REV4, &pmic->major); >> if (ret < 0) >> - return; >> + return ret; >> >> /* >> * In early versions of PM8941 and PM8226, the major revision number >> @@ -124,15 +174,34 @@ static void pmic_spmi_show_revid(struct regmap *map, struct device *dev) >> * Increment the major revision number here if the chip is an early >> * version of PM8941 or PM8226. >> */ >> - if ((subtype == PM8941_SUBTYPE || subtype == PM8226_SUBTYPE) && >> - major < 0x02) >> - major++; >> + if ((pmic->subtype == PM8941_SUBTYPE || pmic->subtype == PM8226_SUBTYPE) && >> + pmic->major < 0x02) > > Please define this magic number while you're at it. > >> + pmic->major++; >> + >> + if (pmic->subtype == PM8110_SUBTYPE) >> + pmic->minor = pmic->rev2; >> + >> + pmic_print_info(dev, pmic); >> >> - if (subtype == PM8110_SUBTYPE) >> - minor = rev2; >> + return 0; >> +} >> + >> +/** >> + * qcom_pmic_get() - Get a pointer to the base PMIC device >> + * >> + * @dev: the pmic function device >> + * @return: the struct qcom_spmi_pmic* pointer associated with the function device >> + */ >> +inline const struct qcom_spmi_pmic *qcom_pmic_get(struct device *dev) >> +{ >> + struct spmi_device *sdev = qcom_pmic_get_base_usid(dev->parent); >> >> - dev_dbg(dev, "%x: %s v%d.%d\n", subtype, name, major, minor); >> + if (IS_ERR(sdev)) >> + return ERR_CAST(sdev); >> + >> + return &((struct qcom_spmi_dev *)spmi_device_get_drvdata(sdev))->pmic; > > This is horrible. Please expand it out. > >> } >> +EXPORT_SYMBOL(qcom_pmic_get); >> >> static const struct regmap_config spmi_regmap_config = { >> .reg_bits = 16, >> @@ -144,14 +213,26 @@ static const struct regmap_config spmi_regmap_config = { >> static int pmic_spmi_probe(struct spmi_device *sdev) >> { >> struct regmap *regmap; >> + struct qcom_spmi_dev *ctx; >> + int ret; >> >> regmap = devm_regmap_init_spmi_ext(sdev, &spmi_regmap_config); >> if (IS_ERR(regmap)) >> return PTR_ERR(regmap); >> >> + ctx = devm_kzalloc(&sdev->dev, sizeof(*ctx), GFP_KERNEL); >> + if (!ctx) >> + return -ENOMEM; >> + >> + ctx->num_usids = (long)of_device_get_match_data(&sdev->dev); > > Why does this need to be long? The compiler complains with ../drivers/mfd/qcom-spmi-pmic.c: In function 'pmic_spmi_probe': ../drivers/mfd/qcom-spmi-pmic.c:236:26: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] 236 | ctx->num_usids = (int)of_device_get_match_data(&sdev->dev); | Due to match data being void*. I wasn't able to find a better solution than casting this way, do you have any suggestions? I just found uintptr_t, maybe that might be more explicit? > > In fact, it's not is it? It's an int: > > +struct qcom_spmi_dev { > + int num_usids; > + struct qcom_spmi_pmic pmic; > +}; > >> /* Only the first slave id for a PMIC contains this information */ >> - if (sdev->usid % 2 == 0) >> - pmic_spmi_show_revid(regmap, &sdev->dev); >> + if (sdev->usid % ctx->num_usids == 0) { >> + ret = pmic_spmi_load_revid(regmap, &sdev->dev, &ctx->pmic); >> + if (ret < 0) >> + return ret; >> + } >> + spmi_device_set_drvdata(sdev, ctx); >> >> return devm_of_platform_populate(&sdev->dev); >> } >> diff --git a/include/soc/qcom/qcom-spmi-pmic.h b/include/soc/qcom/qcom-spmi-pmic.h >> new file mode 100644 >> index 000000000000..5400e6509fe8 >> --- /dev/null >> +++ b/include/soc/qcom/qcom-spmi-pmic.h >> @@ -0,0 +1,60 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> +/* Copyright (c) 2021 Linaro. All rights reserved. >> + * Copyright (c) 2021 Caleb Connolly <caleb.connolly@linaro.org> > > Your very own Copyright? Why? I did some research originally and couldn't find any docs on Linaro precedent here, so went with this originally... I switched over the other files when I realised that "Author:" was the preferred approach but I guess this one slipped through the crack. > > Any not Author: instead? > >> + */ >> + >> +#ifndef __QCOM_SPMI_PMIC_H__ >> +#define __QCOM_SPMI_PMIC_H__ >> + >> +#define COMMON_SUBTYPE 0x00 >> +#define PM8941_SUBTYPE 0x01 >> +#define PM8841_SUBTYPE 0x02 >> +#define PM8019_SUBTYPE 0x03 >> +#define PM8226_SUBTYPE 0x04 >> +#define PM8110_SUBTYPE 0x05 >> +#define PMA8084_SUBTYPE 0x06 >> +#define PMI8962_SUBTYPE 0x07 >> +#define PMD9635_SUBTYPE 0x08 >> +#define PM8994_SUBTYPE 0x09 >> +#define PMI8994_SUBTYPE 0x0a >> +#define PM8916_SUBTYPE 0x0b >> +#define PM8004_SUBTYPE 0x0c >> +#define PM8909_SUBTYPE 0x0d >> +#define PM8028_SUBTYPE 0x0e >> +#define PM8901_SUBTYPE 0x0f >> +#define PM8950_SUBTYPE 0x10 >> +#define PMI8950_SUBTYPE 0x11 >> +#define PM8998_SUBTYPE 0x14 >> +#define PMI8998_SUBTYPE 0x15 >> +#define PM8005_SUBTYPE 0x18 >> +#define PM660L_SUBTYPE 0x1A >> +#define PM660_SUBTYPE 0x1B >> +#define PM8150_SUBTYPE 0x1E >> +#define PM8150L_SUBTYPE 0x1f >> +#define PM8150B_SUBTYPE 0x20 >> +#define PMK8002_SUBTYPE 0x21 >> +#define PM8009_SUBTYPE 0x24 >> +#define PM8150C_SUBTYPE 0x26 >> +#define SMB2351_SUBTYPE 0x29 >> + >> +#define PMI8998_FAB_ID_SMIC 0x11 >> +#define PMI8998_FAB_ID_GF 0x30 >> + >> +#define PM660_FAB_ID_GF 0x0 >> +#define PM660_FAB_ID_TSMC 0x2 >> +#define PM660_FAB_ID_MX 0x3 >> + >> +struct qcom_spmi_pmic { >> + unsigned int type; >> + unsigned int subtype; >> + unsigned int major; >> + unsigned int minor; >> + unsigned int rev2; >> + const char *name; >> +}; >> + >> +struct device; > > Can't you just include the correct header?Sure > >> +inline const struct qcom_spmi_pmic *qcom_pmic_get(struct device *dev); >> + >> +#endif /* __QCOM_SPMI_PMIC_H__ */ >
On Wed, 27 Apr 2022, Caleb Connolly wrote: > On 25/04/2022 16:03, Lee Jones wrote: > > > > On Wed, 23 Mar 2022, Caleb Connolly wrote: > > > From: Caleb Connolly <caleb.connolly@linaro.org> > > > > > > Some PMIC functions such as the RRADC need to be aware of the PMIC > > > chip revision information to implement errata or otherwise adjust > > > behaviour, export the PMIC information to enable this. > > > > > > This is specifically required to enable the RRADC to adjust > > > coefficients based on which chip fab the PMIC was produced in, > > > this can vary per unique device and therefore has to be read at > > > runtime. > > > > > > Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> > > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > Tested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > --- > > > drivers/mfd/qcom-spmi-pmic.c | 261 +++++++++++++++++++----------- > > > include/soc/qcom/qcom-spmi-pmic.h | 60 +++++++ > > > 2 files changed, 231 insertions(+), 90 deletions(-) > > > create mode 100644 include/soc/qcom/qcom-spmi-pmic.h [...] > > > + /* > > > + * If the base USID for this PMIC hasn't probed yet > > > + * but the secondary USID has, then we need to defer > > > + * the function driver so that it will attempt to > > > + * probe again when the base USID is ready. > > > + */ > > > + if (pmic_addr == function_parent_usid - (ctx->num_usids - 1)) > > > > Double " ". > Ack > > > > Over-bracketing of statements with matching operands. > I don't think x - (y - 1) is equal to x - y - 1? Or am I misunderstanding you here? Can you give me an example when this would be the case?
On Wed, 27 Apr 2022, Caleb Connolly wrote: > > > On 26/04/2022 19:24, Lee Jones wrote: > > On Tue, 26 Apr 2022, Caleb Connolly wrote: > > > On 25/04/2022 16:05, Lee Jones wrote: > > > > On Wed, 23 Mar 2022, Caleb Connolly wrote: > > > > > > > > > From: Caleb Connolly <caleb.connolly@linaro.org> > > > > > > > > > > The PMI8998 and PM660 expose the fab_id, this is needed by drivers like > > > > > the RRADC to calibrate ADC values. > > > > > > > > > > Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> > > > > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > > Tested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > > --- > > > > > drivers/mfd/qcom-spmi-pmic.c | 7 +++++++ > > > > > include/soc/qcom/qcom-spmi-pmic.h | 1 + > > > > > 2 files changed, 8 insertions(+) > > > > > > > > Please change the Subject line to match the style of the sub-system? > > > Hi, sorry if this is a silly question, I don't quite understand what you > > > want me to change here, the subject line is in the same "mfd: driver:" > > > format as other patches in the subsystem? > > > > mfd: qcom-spmi-pmic: Read fab ID on supported PMICs > > > > What's 'fab' should that be capitalised too? > "fab" is short for fabrication I think, the register value can be used to > determine which factory the chip was manufactured in. > > I can make it clearer and go for > > mfd: qcom-spmi-pmic: Read fabrication ID on supported PMICs Clearer, thanks.
On 28/04/2022 17:14, Lee Jones wrote: > On Wed, 27 Apr 2022, Caleb Connolly wrote: >> On 25/04/2022 16:03, Lee Jones wrote: >>> >>> On Wed, 23 Mar 2022, Caleb Connolly wrote: >>>> From: Caleb Connolly <caleb.connolly@linaro.org> >>>> >>>> Some PMIC functions such as the RRADC need to be aware of the PMIC >>>> chip revision information to implement errata or otherwise adjust >>>> behaviour, export the PMIC information to enable this. >>>> >>>> This is specifically required to enable the RRADC to adjust >>>> coefficients based on which chip fab the PMIC was produced in, >>>> this can vary per unique device and therefore has to be read at >>>> runtime. >>>> >>>> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> >>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>> Tested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>> --- >>>> drivers/mfd/qcom-spmi-pmic.c | 261 +++++++++++++++++++----------- >>>> include/soc/qcom/qcom-spmi-pmic.h | 60 +++++++ >>>> 2 files changed, 231 insertions(+), 90 deletions(-) >>>> create mode 100644 include/soc/qcom/qcom-spmi-pmic.h > > [...] > >>>> + /* >>>> + * If the base USID for this PMIC hasn't probed yet >>>> + * but the secondary USID has, then we need to defer >>>> + * the function driver so that it will attempt to >>>> + * probe again when the base USID is ready. >>>> + */ >>>> + if (pmic_addr == function_parent_usid - (ctx->num_usids - 1)) >>> >>> Double " ". >> Ack >>> >>> Over-bracketing of statements with matching operands. >> I don't think x - (y - 1) is equal to x - y - 1? Or am I misunderstanding you here? > > Can you give me an example when this would be the case? According to the Python interpreter: >>> x=7 >>> y=4 >>> x - y - 1 2 >>> x - (y - 1) 4 C does also respect the first rule of BODMAS - parenthesis are always evaluated first. >
On Thu, 28 Apr 2022, Caleb Connolly wrote: > On 28/04/2022 17:14, Lee Jones wrote: > > On Wed, 27 Apr 2022, Caleb Connolly wrote: > > > On 25/04/2022 16:03, Lee Jones wrote: > > > > > > > > On Wed, 23 Mar 2022, Caleb Connolly wrote: > > > > > From: Caleb Connolly <caleb.connolly@linaro.org> > > > > > > > > > > Some PMIC functions such as the RRADC need to be aware of the PMIC > > > > > chip revision information to implement errata or otherwise adjust > > > > > behaviour, export the PMIC information to enable this. > > > > > > > > > > This is specifically required to enable the RRADC to adjust > > > > > coefficients based on which chip fab the PMIC was produced in, > > > > > this can vary per unique device and therefore has to be read at > > > > > runtime. > > > > > > > > > > Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> > > > > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > > Tested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > > --- > > > > > drivers/mfd/qcom-spmi-pmic.c | 261 +++++++++++++++++++----------- > > > > > include/soc/qcom/qcom-spmi-pmic.h | 60 +++++++ > > > > > 2 files changed, 231 insertions(+), 90 deletions(-) > > > > > create mode 100644 include/soc/qcom/qcom-spmi-pmic.h > > > > [...] > > > > > > > + /* > > > > > + * If the base USID for this PMIC hasn't probed yet > > > > > + * but the secondary USID has, then we need to defer > > > > > + * the function driver so that it will attempt to > > > > > + * probe again when the base USID is ready. > > > > > + */ > > > > > + if (pmic_addr == function_parent_usid - (ctx->num_usids - 1)) > > > > > > > > Double " ". > > > Ack > > > > > > > > Over-bracketing of statements with matching operands. > > > I don't think x - (y - 1) is equal to x - y - 1? Or am I misunderstanding you here? > > > > Can you give me an example when this would be the case? > According to the Python interpreter: > >>> x=7 > >>> y=4 > >>> x - y - 1 > 2 > >>> x - (y - 1) > 4 > > C does also respect the first rule of BODMAS - parenthesis are always > evaluated first. Fair point, well presented. Thanks for this - I understand the problem now.
On 29/04/2022 11:06, Lee Jones wrote: > On Thu, 28 Apr 2022, Caleb Connolly wrote: >> On 28/04/2022 17:14, Lee Jones wrote: >>> On Wed, 27 Apr 2022, Caleb Connolly wrote: >>>> On 25/04/2022 16:03, Lee Jones wrote: >>>>> >>>>> On Wed, 23 Mar 2022, Caleb Connolly wrote: >>>>>> From: Caleb Connolly <caleb.connolly@linaro.org> >>>>>> >>>>>> Some PMIC functions such as the RRADC need to be aware of the PMIC >>>>>> chip revision information to implement errata or otherwise adjust >>>>>> behaviour, export the PMIC information to enable this. >>>>>> >>>>>> This is specifically required to enable the RRADC to adjust >>>>>> coefficients based on which chip fab the PMIC was produced in, >>>>>> this can vary per unique device and therefore has to be read at >>>>>> runtime. >>>>>> >>>>>> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> >>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>>>> Tested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>>>> --- >>>>>> drivers/mfd/qcom-spmi-pmic.c | 261 +++++++++++++++++++----------- >>>>>> include/soc/qcom/qcom-spmi-pmic.h | 60 +++++++ >>>>>> 2 files changed, 231 insertions(+), 90 deletions(-) >>>>>> create mode 100644 include/soc/qcom/qcom-spmi-pmic.h >>> >>> [...] >>> >>>>>> + /* >>>>>> + * If the base USID for this PMIC hasn't probed yet >>>>>> + * but the secondary USID has, then we need to defer >>>>>> + * the function driver so that it will attempt to >>>>>> + * probe again when the base USID is ready. >>>>>> + */ >>>>>> + if (pmic_addr == function_parent_usid - (ctx->num_usids - 1)) >>>>> >>>>> Double " ". >>>> Ack >>>>> >>>>> Over-bracketing of statements with matching operands. >>>> I don't think x - (y - 1) is equal to x - y - 1? Or am I misunderstanding you here? >>> >>> Can you give me an example when this would be the case? >> According to the Python interpreter: >>>>> x=7 >>>>> y=4 >>>>> x - y - 1 >> 2 >>>>> x - (y - 1) >> 4 >> >> C does also respect the first rule of BODMAS - parenthesis are always >> evaluated first. > > Fair point, well presented. > > Thanks for this - I understand the problem now. Thanks, Could you respond to my other two points in https://lore.kernel.org/linux-arm-msm/2763f103-6947-e431-cef5-e202c324d678@linaro.org/ Regarding if (!of_match_device(pmic_spmi_id_table, dev)) and ctx->num_usids = (long)of_device_get_match_data(&sdev->dev); >