Hi Jonathan,
Sorry for sending the same email again. I miss to reply all in the last email..
Thanks for your valuable suggestions!
Jonathan Cameron Jonathan.Cameron@huawei.com 於 2022年5月31日 週二 下午9:21寫道：
On Tue, 31 May 2022 19:18:56 +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,
A few comments inline.
Thanks,
Jonathan
drivers/iio/adc/Kconfig | 9 ++ drivers/iio/adc/Makefile | 1 + drivers/iio/adc/mt6370-adc.c | 257 +++++++++++++++++++++++++++++++++++ 3 files changed, 267 insertions(+) create mode 100644 drivers/iio/adc/mt6370-adc.c
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index 71ab0a06aa82..d7932dd9b773 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_MT6370helpSay Y here to enable MT6370 ADC support.Integrated for System Monitoring includesThe wrapping of this text needs cleaning up.
is used in smartphones and tablets and supports a 9 channelgeneral 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..3320ebca17ad --- /dev/null +++ b/drivers/iio/adc/mt6370-adc.c @@ -0,0 +1,257 @@ +// 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/irq.h>
Not seeing any interrupt support in here.
Sorry for that I forgot remove this line, I'll refine it in the next patch.
+#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_ADC_CHAN_SHIFT 4
Prefer using a mask and then FIELD_PREP
OK, I got it!
+#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 lock;All locks need documentation. What is the scope of the lock? Looks like it protects device state when doing setup, wait for read, read cycles.
This mutex lock is for preventing the different adc channel from being read at the same time. So, if I just change its name to adc_chan_lock or adc_lock and add the comment for this mutex lock, does this change meet your requirement?
+};
+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->lock);reg_val = MT6370_ADC_START_MASK | (addr << MT6370_ADC_CHAN_SHIFT);FIELD_PREP for that shift?
I got it! I'll refine it in the next patch.
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->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 = 5000;This seems very large. Voltages are in millivolts as per Documentation/ABI/testing/sysfs-bus-iio and this means each step is 5 volts.
So value in mv is currently 5 * _raw
OK, I got it. Also, I will add the ABI file in the next version. Thanks!
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 &®_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 = (chan == MT6370_CHAN_TEMP_JC) ? -20 : 0;Offset default is 0, so for channels where it doesn't apply don't provide the offset attribute at all.
OK!
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] = {
Perhaps define an enum with which to index this and the chan spec and hence ensure they end up matching. [vbusdiv5] = "vbusdiv5", etc
Do you mean that I can refine this const char array to the following array??
static const char * const mt6370_channel_labels[MT6370_CHAN_MAX] = { [MT6370_CHAN_VBUSDIV5] = "vbusdiv5", [MT6370_CHAN_VBUSDIV2] = "vbusdiv2", ... ... [MT6370_CHAN_TEMP_JC] = "temp_jc", };
"vbusdiv5", "vbusdiv2", "vsys", "vbat","ts_bat", "ibus", "ibat", "chg_vddp","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) { \
.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) | \BIT(IIO_CHAN_INFO_OFFSET), \See above. Only temp_jc channel should hav an offset.
OK!
+}
+static const struct iio_chan_spec mt6370_adc_channels[] = {
MT6370_ADC_CHAN(VBUSDIV5, IIO_VOLTAGE, 1),MT6370_ADC_CHAN(VBUSDIV2, IIO_VOLTAGE, 2),MT6370_ADC_CHAN(VSYS, IIO_VOLTAGE, 3),MT6370_ADC_CHAN(VBAT, IIO_VOLTAGE, 4),MT6370_ADC_CHAN(TS_BAT, IIO_VOLTAGE, 6),MT6370_ADC_CHAN(IBUS, IIO_CURRENT, 8),MT6370_ADC_CHAN(IBAT, IIO_CURRENT, 9),MT6370_ADC_CHAN(CHG_VDDP, IIO_VOLTAGE, 11),MT6370_ADC_CHAN(TEMP_JC, IIO_TEMP, 12),+};
+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->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 = dev_name(&pdev->dev);What does this end up as? It's used for userspace ABI and should correspond to the part number, "mt6370-adc" probably appropriate in this case (I think it'll end up as simply "adc.x" currently?) Normally we just hard code this in the driver for whatever devices the driver supports.
I got it, I will change this name to "mt6370-adc" in the next version! Thanks!
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");
Best regards, ChiaEn Wu