| Message ID | 20220613111146.25221-12-peterwu.pub@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add Mediatek MT6370 PMIC support | expand |
Hi, On 6/13/22 04:11, ChiaEn Wu wrote: > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index 71ab0a06aa82..09576fb478ad 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -737,6 +737,15 @@ config MEDIATEK_MT6360_ADC > is used in smartphones and tablets and supports a 11 channel > general purpose ADC. > > +config MEDIATEK_MT6370_ADC > + tristate "Mediatek MT6370 ADC driver" > + depends on MFD_MT6370 > + help > + Say Y here to enable MT6370 ADC support. > + > + Integrated for System Monitoring includes is used in smartphones Please try again on the help text. I can't decode that. > + and tablets and supports a 9 channel general purpose ADC.
Hi Randy, Thanks for your comment! I apologize for any inconvenience caused when you decoded this help text. I will refine this in the next patch, thanks! Randy Dunlap <rdunlap@infradead.org> 於 2022年6月14日 週二 凌晨4:17寫道： > > Hi, > > On 6/13/22 04:11, ChiaEn Wu wrote: > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > > index 71ab0a06aa82..09576fb478ad 100644 > > --- a/drivers/iio/adc/Kconfig > > +++ b/drivers/iio/adc/Kconfig > > @@ -737,6 +737,15 @@ config MEDIATEK_MT6360_ADC > > is used in smartphones and tablets and supports a 11 channel > > general purpose ADC. > > > > +config MEDIATEK_MT6370_ADC > > + tristate "Mediatek MT6370 ADC driver" > > + depends on MFD_MT6370 > > + help > > + Say Y here to enable MT6370 ADC support. > > + > > + Integrated for System Monitoring includes is used in smartphones > > Please try again on the help text. I can't decode that. > > > + and tablets and supports a 9 channel general purpose ADC. > > -- > ~Randy Best Regards, ChiaEn Wu
On Mon, 13 Jun 2022 19:11:42 +0800 ChiaEn Wu <peterwu.pub@gmail.com> wrote: > From: ChiaEn Wu <chiaen_wu@richtek.com> > > Add Mediatek MT6370 ADC support. > > Signed-off-by: ChiaEn Wu <chiaen_wu@richtek.com> Hi ChiaEn Wu, A few comments inline, but mostly looks good to me with the exception of the scales which look far too large. Thanks, Jonathan > --- > drivers/iio/adc/Kconfig | 9 ++ > drivers/iio/adc/Makefile | 1 + > drivers/iio/adc/mt6370-adc.c | 262 +++++++++++++++++++++++++++++++++++ > 3 files changed, 272 insertions(+) > create mode 100644 drivers/iio/adc/mt6370-adc.c > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index 71ab0a06aa82..09576fb478ad 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -737,6 +737,15 @@ config MEDIATEK_MT6360_ADC > is used in smartphones and tablets and supports a 11 channel > general purpose ADC. > > +config MEDIATEK_MT6370_ADC > + tristate "Mediatek MT6370 ADC driver" > + depends on MFD_MT6370 > + help > + Say Y here to enable MT6370 ADC support. > + > + Integrated for System Monitoring includes is used in smartphones > + and tablets and supports a 9 channel general purpose ADC. > + > config MEDIATEK_MT6577_AUXADC > tristate "MediaTek AUXADC driver" > depends on ARCH_MEDIATEK || COMPILE_TEST > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile > index 39d806f6d457..0ce285c7e2d0 100644 > --- a/drivers/iio/adc/Makefile > +++ b/drivers/iio/adc/Makefile > @@ -68,6 +68,7 @@ obj-$(CONFIG_MCP320X) += mcp320x.o > obj-$(CONFIG_MCP3422) += mcp3422.o > obj-$(CONFIG_MCP3911) += mcp3911.o > obj-$(CONFIG_MEDIATEK_MT6360_ADC) += mt6360-adc.o > +obj-$(CONFIG_MEDIATEK_MT6370_ADC) += mt6370-adc.o > obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o > obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o > obj-$(CONFIG_MESON_SARADC) += meson_saradc.o > diff --git a/drivers/iio/adc/mt6370-adc.c b/drivers/iio/adc/mt6370-adc.c > new file mode 100644 > index 000000000000..c30e1290973a > --- /dev/null > +++ b/drivers/iio/adc/mt6370-adc.c > @@ -0,0 +1,262 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +#include <dt-bindings/iio/adc/mediatek,mt6370_adc.h> > +#include <linux/bits.h> > +#include <linux/bitfield.h> > +#include <linux/iio/iio.h> > +#include <linux/kernel.h> > +#include <linux/module.h> #include <linux/mod_devicetable.h> rather than relying on indirect include for struct of_device_id We've just removed such an include path from IIO and had to fix up a lot of drivers that falsely assumed that would available. > +#include <linux/mutex.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > + > +#define MT6370_REG_CHG_CTRL3 0x113 /* AICR */ > +#define MT6370_REG_CHG_CTRL7 0x117 /* ICHG */ > +#define MT6370_REG_CHG_ADC 0x121 > +#define MT6370_REG_ADC_DATA_H 0x14C > + > +#define MT6370_ADC_START_MASK BIT(0) > +#define MT6370_ADC_IN_SEL_MASK GENMASK(7, 4) > +#define MT6370_AICR_ICHG_MASK GENMASK(7, 2) > + > +#define MT6370_AICR_400MA 0x6 > +#define MT6370_ICHG_500MA 0x4 > +#define MT6370_ICHG_900MA 0x8 > + > +#define ADC_CONV_TIME_US 35000 > +#define ADC_CONV_POLLING_TIME 1000 > + > +struct mt6370_adc_data { > + struct device *dev; > + struct regmap *regmap; > + struct mutex adc_lock; Please document scope of the lock. I think it's to synchronize access to the device state concerned with channel reads, but there should be a comment here to say something about that. > +}; > + > +static int mt6370_adc_read_channel(struct mt6370_adc_data *priv, int chan, > + unsigned long addr, int *val) > +{ > + __be16 be_val; > + unsigned int reg_val; > + int ret; > + > + mutex_lock(&priv->adc_lock); > + > + reg_val = MT6370_ADC_START_MASK | > + FIELD_PREP(MT6370_ADC_IN_SEL_MASK, addr); > + ret = regmap_write(priv->regmap, MT6370_REG_CHG_ADC, reg_val); > + if (ret) > + goto adc_unlock; > + > + msleep(ADC_CONV_TIME_US / 1000); > + > + ret = regmap_read_poll_timeout(priv->regmap, > + MT6370_REG_CHG_ADC, reg_val, > + !(reg_val & MT6370_ADC_START_MASK), > + ADC_CONV_POLLING_TIME, > + ADC_CONV_TIME_US * 3); > + if (ret) { > + if (ret == -ETIMEDOUT) > + dev_err(priv->dev, "Failed to wait adc conversion\n"); Why are any other error here not worth reporting? I'd print a message for all return values. > + goto adc_unlock; > + } > + > + ret = regmap_raw_read(priv->regmap, MT6370_REG_ADC_DATA_H, > + &be_val, sizeof(be_val)); > + if (ret) > + goto adc_unlock; > + > + *val = be16_to_cpu(be_val); > + ret = IIO_VAL_INT; > + > +adc_unlock: > + mutex_unlock(&priv->adc_lock); > + > + return ret; > +} > + > +static int mt6370_adc_read_scale(struct mt6370_adc_data *priv, > + int chan, int *val1, int *val2) > +{ > + unsigned int reg_val; > + int ret; > + > + switch (chan) { > + case MT6370_CHAN_VBAT: > + case MT6370_CHAN_VSYS: > + case MT6370_CHAN_CHG_VDDP: > + *val1 = 5; > + return IIO_VAL_INT; > + case MT6370_CHAN_IBUS: > + ret = regmap_read(priv->regmap, MT6370_REG_CHG_CTRL3, ®_val); > + if (ret) > + return ret; > + > + reg_val = FIELD_GET(MT6370_AICR_ICHG_MASK, reg_val); > + if (reg_val < MT6370_AICR_400MA) > + *val1 = 33500; As (scale * raw) must give a value in milliamps, this seems very large as each ADC reading currently represents 33Amps. That would make an impressive PMIC!) Same for the various entries below. Note that scale is often not an integer value (or even as large as 1) Hence the many different precisions of data type that IIO provides and the useful types like IIO_VAL_FRACTIONAL; > + else > + *val1 = 50000; > + > + return IIO_VAL_INT; > + case MT6370_CHAN_IBAT: > + ret = regmap_read(priv->regmap, MT6370_REG_CHG_CTRL7, ®_val); > + if (ret) > + return ret; > + > + reg_val = FIELD_GET(MT6370_AICR_ICHG_MASK, reg_val); > + if (reg_val < MT6370_ICHG_500MA) > + *val1 = 23750; > + else if (reg_val >= MT6370_ICHG_500MA && > + reg_val < MT6370_ICHG_900MA) > + *val1 = 26800; > + else > + *val1 = 50000; > + > + return IIO_VAL_INT; > + case MT6370_CHAN_VBUSDIV5: > + *val1 = 25000; > + return IIO_VAL_INT; > + case MT6370_CHAN_VBUSDIV2: > + *val1 = 50000; > + return IIO_VAL_INT; > + case MT6370_CHAN_TS_BAT: > + *val1 = 25; > + *val2 = 10000; > + return IIO_VAL_FRACTIONAL; > + case MT6370_CHAN_TEMP_JC: > + *val1 = 2; > + return IIO_VAL_INT; > + } > + > + return -EINVAL; As below, I'd prefer this as a default: in the switch statement. > +} > + ... > + > +static int mt6370_adc_read_raw(struct iio_dev *iio_dev, > + const struct iio_chan_spec *chan, > + int *val, int *val2, long mask) > +{ > + struct mt6370_adc_data *priv = iio_priv(iio_dev); > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + return mt6370_adc_read_channel(priv, chan->channel, > + chan->address, val); > + case IIO_CHAN_INFO_SCALE: > + return mt6370_adc_read_scale(priv, chan->channel, val, val2); > + case IIO_CHAN_INFO_OFFSET: > + return mt6370_adc_read_offset(priv, chan->channel, val); > + } > + > + return -EINVAL; Add a default to the switch statement and return -EINVAL in there. That makes it explicit that you are handling all the cases you care about. Sure, right now it's obvious that is the case, but it might not be so obvious if more code happens to get added here in future. > +} > + >
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index 71ab0a06aa82..09576fb478ad 100644 --- a/drivers/iio/adc/Kconfig +++ b/drivers/iio/adc/Kconfig @@ -737,6 +737,15 @@ config MEDIATEK_MT6360_ADC is used in smartphones and tablets and supports a 11 channel general purpose ADC. +config MEDIATEK_MT6370_ADC + tristate "Mediatek MT6370 ADC driver" + depends on MFD_MT6370 + help + Say Y here to enable MT6370 ADC support. + + Integrated for System Monitoring includes is used in smartphones + and tablets and supports a 9 channel general purpose ADC. + config MEDIATEK_MT6577_AUXADC tristate "MediaTek AUXADC driver" depends on ARCH_MEDIATEK || COMPILE_TEST diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile index 39d806f6d457..0ce285c7e2d0 100644 --- a/drivers/iio/adc/Makefile +++ b/drivers/iio/adc/Makefile @@ -68,6 +68,7 @@ obj-$(CONFIG_MCP320X) += mcp320x.o obj-$(CONFIG_MCP3422) += mcp3422.o obj-$(CONFIG_MCP3911) += mcp3911.o obj-$(CONFIG_MEDIATEK_MT6360_ADC) += mt6360-adc.o +obj-$(CONFIG_MEDIATEK_MT6370_ADC) += mt6370-adc.o obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o obj-$(CONFIG_MESON_SARADC) += meson_saradc.o diff --git a/drivers/iio/adc/mt6370-adc.c b/drivers/iio/adc/mt6370-adc.c new file mode 100644 index 000000000000..c30e1290973a --- /dev/null +++ b/drivers/iio/adc/mt6370-adc.c @@ -0,0 +1,262 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <dt-bindings/iio/adc/mediatek,mt6370_adc.h> +#include <linux/bits.h> +#include <linux/bitfield.h> +#include <linux/iio/iio.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> + +#define MT6370_REG_CHG_CTRL3 0x113 /* AICR */ +#define MT6370_REG_CHG_CTRL7 0x117 /* ICHG */ +#define MT6370_REG_CHG_ADC 0x121 +#define MT6370_REG_ADC_DATA_H 0x14C + +#define MT6370_ADC_START_MASK BIT(0) +#define MT6370_ADC_IN_SEL_MASK GENMASK(7, 4) +#define MT6370_AICR_ICHG_MASK GENMASK(7, 2) + +#define MT6370_AICR_400MA 0x6 +#define MT6370_ICHG_500MA 0x4 +#define MT6370_ICHG_900MA 0x8 + +#define ADC_CONV_TIME_US 35000 +#define ADC_CONV_POLLING_TIME 1000 + +struct mt6370_adc_data { + struct device *dev; + struct regmap *regmap; + struct mutex adc_lock; +}; + +static int mt6370_adc_read_channel(struct mt6370_adc_data *priv, int chan, + unsigned long addr, int *val) +{ + __be16 be_val; + unsigned int reg_val; + int ret; + + mutex_lock(&priv->adc_lock); + + reg_val = MT6370_ADC_START_MASK | + FIELD_PREP(MT6370_ADC_IN_SEL_MASK, addr); + ret = regmap_write(priv->regmap, MT6370_REG_CHG_ADC, reg_val); + if (ret) + goto adc_unlock; + + msleep(ADC_CONV_TIME_US / 1000); + + ret = regmap_read_poll_timeout(priv->regmap, + MT6370_REG_CHG_ADC, reg_val, + !(reg_val & MT6370_ADC_START_MASK), + ADC_CONV_POLLING_TIME, + ADC_CONV_TIME_US * 3); + if (ret) { + if (ret == -ETIMEDOUT) + dev_err(priv->dev, "Failed to wait adc conversion\n"); + goto adc_unlock; + } + + ret = regmap_raw_read(priv->regmap, MT6370_REG_ADC_DATA_H, + &be_val, sizeof(be_val)); + if (ret) + goto adc_unlock; + + *val = be16_to_cpu(be_val); + ret = IIO_VAL_INT; + +adc_unlock: + mutex_unlock(&priv->adc_lock); + + return ret; +} + +static int mt6370_adc_read_scale(struct mt6370_adc_data *priv, + int chan, int *val1, int *val2) +{ + unsigned int reg_val; + int ret; + + switch (chan) { + case MT6370_CHAN_VBAT: + case MT6370_CHAN_VSYS: + case MT6370_CHAN_CHG_VDDP: + *val1 = 5; + return IIO_VAL_INT; + case MT6370_CHAN_IBUS: + ret = regmap_read(priv->regmap, MT6370_REG_CHG_CTRL3, ®_val); + if (ret) + return ret; + + reg_val = FIELD_GET(MT6370_AICR_ICHG_MASK, reg_val); + if (reg_val < MT6370_AICR_400MA) + *val1 = 33500; + else + *val1 = 50000; + + return IIO_VAL_INT; + case MT6370_CHAN_IBAT: + ret = regmap_read(priv->regmap, MT6370_REG_CHG_CTRL7, ®_val); + if (ret) + return ret; + + reg_val = FIELD_GET(MT6370_AICR_ICHG_MASK, reg_val); + if (reg_val < MT6370_ICHG_500MA) + *val1 = 23750; + else if (reg_val >= MT6370_ICHG_500MA && + reg_val < MT6370_ICHG_900MA) + *val1 = 26800; + else + *val1 = 50000; + + return IIO_VAL_INT; + case MT6370_CHAN_VBUSDIV5: + *val1 = 25000; + return IIO_VAL_INT; + case MT6370_CHAN_VBUSDIV2: + *val1 = 50000; + return IIO_VAL_INT; + case MT6370_CHAN_TS_BAT: + *val1 = 25; + *val2 = 10000; + return IIO_VAL_FRACTIONAL; + case MT6370_CHAN_TEMP_JC: + *val1 = 2; + return IIO_VAL_INT; + } + + return -EINVAL; +} + +static int mt6370_adc_read_offset(struct mt6370_adc_data *priv, + int chan, int *val) +{ + *val = -20; + + return IIO_VAL_INT; +} + +static int mt6370_adc_read_raw(struct iio_dev *iio_dev, + const struct iio_chan_spec *chan, + int *val, int *val2, long mask) +{ + struct mt6370_adc_data *priv = iio_priv(iio_dev); + + switch (mask) { + case IIO_CHAN_INFO_RAW: + return mt6370_adc_read_channel(priv, chan->channel, + chan->address, val); + case IIO_CHAN_INFO_SCALE: + return mt6370_adc_read_scale(priv, chan->channel, val, val2); + case IIO_CHAN_INFO_OFFSET: + return mt6370_adc_read_offset(priv, chan->channel, val); + } + + return -EINVAL; +} + +static const char * const mt6370_channel_labels[MT6370_CHAN_MAX] = { + [MT6370_CHAN_VBUSDIV5] = "vbusdiv5", + [MT6370_CHAN_VBUSDIV2] = "vbusdiv2", + [MT6370_CHAN_VSYS] = "vsys", + [MT6370_CHAN_VBAT] = "vbat", + [MT6370_CHAN_TS_BAT] = "ts_bat", + [MT6370_CHAN_IBUS] = "ibus", + [MT6370_CHAN_IBAT] = "ibat", + [MT6370_CHAN_CHG_VDDP] = "chg_vddp", + [MT6370_CHAN_TEMP_JC] = "temp_jc", +}; + +static int mt6370_adc_read_label(struct iio_dev *iio_dev, + struct iio_chan_spec const *chan, char *label) +{ + return snprintf(label, PAGE_SIZE, "%s\n", + mt6370_channel_labels[chan->channel]); +} + +static const struct iio_info mt6370_adc_iio_info = { + .read_raw = mt6370_adc_read_raw, + .read_label = mt6370_adc_read_label, +}; + +#define MT6370_ADC_CHAN(_idx, _type, _addr, _extra_info) { \ + .type = _type, \ + .channel = MT6370_CHAN_##_idx, \ + .address = _addr, \ + .scan_index = MT6370_CHAN_##_idx, \ + .indexed = 1, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ + BIT(IIO_CHAN_INFO_SCALE) | \ + _extra_info \ +} + +static const struct iio_chan_spec mt6370_adc_channels[] = { + MT6370_ADC_CHAN(VBUSDIV5, IIO_VOLTAGE, 1, 0), + MT6370_ADC_CHAN(VBUSDIV2, IIO_VOLTAGE, 2, 0), + MT6370_ADC_CHAN(VSYS, IIO_VOLTAGE, 3, 0), + MT6370_ADC_CHAN(VBAT, IIO_VOLTAGE, 4, 0), + MT6370_ADC_CHAN(TS_BAT, IIO_VOLTAGE, 6, 0), + MT6370_ADC_CHAN(IBUS, IIO_CURRENT, 8, 0), + MT6370_ADC_CHAN(IBAT, IIO_CURRENT, 9, 0), + MT6370_ADC_CHAN(CHG_VDDP, IIO_VOLTAGE, 11, 0), + MT6370_ADC_CHAN(TEMP_JC, IIO_TEMP, 12, BIT(IIO_CHAN_INFO_OFFSET)), +}; + +static int mt6370_adc_probe(struct platform_device *pdev) +{ + int ret; + struct mt6370_adc_data *priv; + struct regmap *regmap; + struct iio_dev *indio_dev; + + regmap = dev_get_regmap(pdev->dev.parent, NULL); + if (!regmap) { + dev_err(&pdev->dev, "Failed to get regmap\n"); + return -ENODEV; + } + + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*priv)); + if (!indio_dev) + return -ENOMEM; + + priv = iio_priv(indio_dev); + priv->dev = &pdev->dev; + priv->regmap = regmap; + mutex_init(&priv->adc_lock); + + ret = regmap_write(priv->regmap, MT6370_REG_CHG_ADC, 0); + if (ret) { + dev_err(&pdev->dev, "Failed to reset adc\n"); + return ret; + } + + indio_dev->name = "mt6370-adc"; + indio_dev->info = &mt6370_adc_iio_info; + indio_dev->modes = INDIO_DIRECT_MODE; + indio_dev->channels = mt6370_adc_channels; + indio_dev->num_channels = ARRAY_SIZE(mt6370_adc_channels); + + return devm_iio_device_register(&pdev->dev, indio_dev); +} + +static const struct of_device_id mt6370_adc_of_id[] = { + { .compatible = "mediatek,mt6370-adc", }, + {} +}; +MODULE_DEVICE_TABLE(of, mt6370_adc_of_id); + +static struct platform_driver mt6370_adc_driver = { + .driver = { + .name = "mt6370-adc", + .of_match_table = mt6370_adc_of_id, + }, + .probe = mt6370_adc_probe, +}; +module_platform_driver(mt6370_adc_driver); + +MODULE_AUTHOR("ChiaEn Wu <chiaen_wu@richtek.com>"); +MODULE_DESCRIPTION("MT6370 ADC Drvier"); +MODULE_LICENSE("GPL v2");