| Message ID | 20170102163723.7939-4-quentin.schulz@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 3, 2017 at 12:37 AM, Quentin Schulz <quentin.schulz@free-electrons.com> wrote: > The X-Powers AXP20X and AXP22X PMICs have multiple ADCs. They expose the > battery voltage, battery charge and discharge currents, AC-in and VBUS > voltages and currents, 2 GPIOs muxable in ADC mode and PMIC temperature. > > This adds support for most of AXP20X and AXP22X ADCs. > > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> > --- > drivers/iio/adc/Kconfig | 10 + > drivers/iio/adc/Makefile | 1 + > drivers/iio/adc/axp20x_adc.c | 490 +++++++++++++++++++++++++++++++++++++++++++ > include/linux/mfd/axp20x.h | 4 + > 4 files changed, 505 insertions(+) > create mode 100644 drivers/iio/adc/axp20x_adc.c > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index 38bc319..5c5b51f 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -154,6 +154,16 @@ config AT91_SAMA5D2_ADC > To compile this driver as a module, choose M here: the module will be > called at91-sama5d2_adc. > > +config AXP20X_ADC > + tristate "X-Powers AXP20X and AXP22X ADC driver" > + depends on MFD_AXP20X > + help > + Say yes here to have support for X-Powers power management IC (PMIC) > + AXP20X and AXP22X ADC devices. > + > + To compile this driver as a module, choose M here: the module will be > + called axp20x_adc. > + > config AXP288_ADC > tristate "X-Powers AXP288 ADC driver" > depends on MFD_AXP20X > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile > index d36c4be..f5c28a5 100644 > --- a/drivers/iio/adc/Makefile > +++ b/drivers/iio/adc/Makefile > @@ -16,6 +16,7 @@ obj-$(CONFIG_AD7887) += ad7887.o > obj-$(CONFIG_AD799X) += ad799x.o > obj-$(CONFIG_AT91_ADC) += at91_adc.o > obj-$(CONFIG_AT91_SAMA5D2_ADC) += at91-sama5d2_adc.o > +obj-$(CONFIG_AXP20X_ADC) += axp20x_adc.o > obj-$(CONFIG_AXP288_ADC) += axp288_adc.o > obj-$(CONFIG_BCM_IPROC_ADC) += bcm_iproc_adc.o > obj-$(CONFIG_BERLIN2_ADC) += berlin2-adc.o > diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c > new file mode 100644 > index 0000000..8df972a > --- /dev/null > +++ b/drivers/iio/adc/axp20x_adc.c > @@ -0,0 +1,490 @@ > +/* ADC driver for AXP20X and AXP22X PMICs > + * > + * Copyright (c) 2016 Free Electrons NextThing Co. > + * Quentin Schulz <quentin.schulz@free-electrons.com> > + * > + * This program is free software; you can redistribute it and/or modify it under > + * the terms of the GNU General Public License version 2 as published by the > + * Free Software Foundation. > + * > + */ > + > +#include <linux/completion.h> > +#include <linux/interrupt.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > +#include <linux/regmap.h> > +#include <linux/thermal.h> > + > +#include <linux/iio/iio.h> > +#include <linux/mfd/axp20x.h> > + > +#define AXP20X_ADC_EN1_MASK GENMASK(7, 0) > + > +#define AXP20X_ADC_EN2_MASK (GENMASK(3, 2) | BIT(7)) > +#define AXP22X_ADC_EN1_MASK (GENMASK(7, 5) | BIT(0)) > +#define AXP20X_ADC_EN2_TEMP_ADC BIT(7) > +#define AXP20X_ADC_EN2_GPIO0_ADC BIT(3) > +#define AXP20X_ADC_EN2_GPIO1_ADC BIT(2) The latter 3 individual bits aren't used anywhere. Please remove them for now. > + > +#define AXP20X_GPIO10_IN_RANGE_GPIO0 BIT(0) > +#define AXP20X_GPIO10_IN_RANGE_GPIO1 BIT(1) > +#define AXP20X_GPIO10_IN_RANGE_GPIO0_VAL(x) ((x) & BIT(0)) > +#define AXP20X_GPIO10_IN_RANGE_GPIO1_VAL(x) (((x) & BIT(0)) << 1) > + > +#define AXP20X_ADC_RATE_MASK (3 << 6) > +#define AXP20X_ADC_RATE_25HZ (0 << 6) > +#define AXP20X_ADC_RATE_50HZ BIT(6) Please be consistent with the format. > +#define AXP20X_ADC_RATE_100HZ (2 << 6) > +#define AXP20X_ADC_RATE_200HZ (3 << 6) > + > +#define AXP22X_ADC_RATE_100HZ (0 << 6) > +#define AXP22X_ADC_RATE_200HZ BIT(6) > +#define AXP22X_ADC_RATE_400HZ (2 << 6) > +#define AXP22X_ADC_RATE_800HZ (3 << 6) These are power-of-2 multiples of some base rate. May I suggest a formula macro instead. Either way, you seem to be using only one value. Will this be made configurable in the future? > + > +#define AXP20X_ADC_CHANNEL(_channel, _name, _type, _reg) \ > + { \ > + .type = _type, \ > + .indexed = 1, \ > + .channel = _channel, \ > + .address = _reg, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ > + BIT(IIO_CHAN_INFO_SCALE), \ > + .datasheet_name = _name, \ > + } > + > +#define AXP20X_ADC_CHANNEL_OFFSET(_channel, _name, _type, _reg) \ > + { \ > + .type = _type, \ > + .indexed = 1, \ > + .channel = _channel, \ > + .address = _reg, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ > + BIT(IIO_CHAN_INFO_SCALE) |\ > + BIT(IIO_CHAN_INFO_OFFSET),\ > + .datasheet_name = _name, \ > + } > + > +struct axp20x_adc_iio { > + struct iio_dev *indio_dev; > + struct regmap *regmap; > +}; > + > +enum axp20x_adc_channel { > + AXP20X_ACIN_V = 0, > + AXP20X_ACIN_I, > + AXP20X_VBUS_V, > + AXP20X_VBUS_I, > + AXP20X_TEMP_ADC, PMIC_TEMP would be better. And please save a slot for TS input. > + AXP20X_GPIO0_V, > + AXP20X_GPIO1_V, Please skip a slot for "battery instantaneous power". > + AXP20X_BATT_V, > + AXP20X_BATT_CHRG_I, > + AXP20X_BATT_DISCHRG_I, > + AXP20X_IPSOUT_V, > +}; > + > +enum axp22x_adc_channel { > + AXP22X_TEMP_ADC = 0, Same comments as AXP20X_TEMP_ADC. > + AXP22X_BATT_V, > + AXP22X_BATT_CHRG_I, > + AXP22X_BATT_DISCHRG_I, > +}; Shouldn't these channel numbers be exported as part of the device tree bindings? At the very least, they shouldn't be changed. Also please add a comment saying that the channels are numbered in the order of their respective registers, and not the table describing the ADCs in the datasheet (9.7 Signal Capture for AXP209 and 9.5 E-Gauge for AXP221). > + > +static const struct iio_chan_spec axp20x_adc_channels[] = { > + AXP20X_ADC_CHANNEL(AXP20X_ACIN_V, "acin_v", IIO_VOLTAGE, > + AXP20X_ACIN_V_ADC_H), > + AXP20X_ADC_CHANNEL(AXP20X_ACIN_I, "acin_i", IIO_CURRENT, > + AXP20X_ACIN_I_ADC_H), > + AXP20X_ADC_CHANNEL(AXP20X_VBUS_V, "vbus_v", IIO_VOLTAGE, > + AXP20X_VBUS_V_ADC_H), > + AXP20X_ADC_CHANNEL(AXP20X_VBUS_I, "vbus_i", IIO_CURRENT, > + AXP20X_VBUS_I_ADC_H), > + AXP20X_ADC_CHANNEL_OFFSET(AXP20X_TEMP_ADC, "temp_adc", IIO_TEMP, > + AXP20X_TEMP_ADC_H), > + AXP20X_ADC_CHANNEL_OFFSET(AXP20X_GPIO0_V, "gpio0_v", IIO_VOLTAGE, > + AXP20X_GPIO0_V_ADC_H), > + AXP20X_ADC_CHANNEL_OFFSET(AXP20X_GPIO1_V, "gpio1_v", IIO_VOLTAGE, > + AXP20X_GPIO1_V_ADC_H), > + AXP20X_ADC_CHANNEL(AXP20X_BATT_V, "batt_v", IIO_VOLTAGE, > + AXP20X_BATT_V_H), > + AXP20X_ADC_CHANNEL(AXP20X_BATT_CHRG_I, "batt_chrg_i", IIO_CURRENT, > + AXP20X_BATT_CHRG_I_H), > + AXP20X_ADC_CHANNEL(AXP20X_BATT_DISCHRG_I, "batt_dischrg_i", IIO_CURRENT, > + AXP20X_BATT_DISCHRG_I_H), > + AXP20X_ADC_CHANNEL(AXP20X_IPSOUT_V, "ipsout_v", IIO_VOLTAGE, > + AXP20X_IPSOUT_V_HIGH_H), > +}; > + > +static const struct iio_chan_spec axp22x_adc_channels[] = { > + AXP20X_ADC_CHANNEL_OFFSET(AXP22X_TEMP_ADC, "temp_adc", IIO_TEMP, > + AXP22X_TEMP_ADC_H), > + AXP20X_ADC_CHANNEL(AXP22X_BATT_V, "batt_v", IIO_VOLTAGE, > + AXP20X_BATT_V_H), > + AXP20X_ADC_CHANNEL(AXP22X_BATT_CHRG_I, "batt_chrg_i", IIO_CURRENT, > + AXP20X_BATT_CHRG_I_H), > + AXP20X_ADC_CHANNEL(AXP22X_BATT_DISCHRG_I, "batt_dischrg_i", IIO_CURRENT, > + AXP20X_BATT_DISCHRG_I_H), > +}; > + > +static int axp20x_adc_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *channel, int *val, > + int *val2) > +{ > + struct axp20x_adc_iio *info = iio_priv(indio_dev); > + int size = 12, ret; > + > + switch (channel->channel) { > + case AXP20X_BATT_DISCHRG_I: > + size = 13; > + case AXP20X_ACIN_V: > + case AXP20X_ACIN_I: > + case AXP20X_VBUS_V: > + case AXP20X_VBUS_I: > + case AXP20X_TEMP_ADC: > + case AXP20X_BATT_V: > + case AXP20X_BATT_CHRG_I: > + case AXP20X_IPSOUT_V: > + case AXP20X_GPIO0_V: > + case AXP20X_GPIO1_V: > + ret = axp20x_read_variable_width(info->regmap, channel->address, > + size); > + if (ret < 0) > + return ret; > + *val = ret; > + return IIO_VAL_INT; > + > + default: > + return -EINVAL; > + } > + > + return -EINVAL; > +} > + > +static int axp22x_adc_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *channel, int *val, > + int *val2) > +{ > + struct axp20x_adc_iio *info = iio_priv(indio_dev); > + int size = 12, ret; > + > + switch (channel->channel) { > + case AXP22X_BATT_DISCHRG_I: > + size = 13; > + case AXP22X_TEMP_ADC: > + case AXP22X_BATT_V: > + case AXP22X_BATT_CHRG_I: According to the datasheet, AXP22X_BATT_CHRG_I is also 13 bits wide. > + ret = axp20x_read_variable_width(info->regmap, channel->address, > + size); > + if (ret < 0) > + return ret; > + *val = ret; > + return IIO_VAL_INT; > + > + default: > + return -EINVAL; > + } > + > + return -EINVAL; > +} > + > +static int axp20x_adc_scale(int channel, int *val, int *val2) > +{ > + switch (channel) { > + case AXP20X_ACIN_V: > + case AXP20X_VBUS_V: > + *val = 1; > + *val2 = 700000; > + return IIO_VAL_INT_PLUS_MICRO; > + > + case AXP20X_ACIN_I: > + *val = 0; > + *val2 = 625000; > + return IIO_VAL_INT_PLUS_MICRO; > + > + case AXP20X_VBUS_I: > + *val = 0; > + *val2 = 375000; > + return IIO_VAL_INT_PLUS_MICRO; > + > + case AXP20X_TEMP_ADC: > + *val = 100; > + return IIO_VAL_INT; > + > + case AXP20X_GPIO0_V: > + case AXP20X_GPIO1_V: > + *val = 0; > + *val2 = 500000; > + return IIO_VAL_INT_PLUS_MICRO; > + > + case AXP20X_BATT_V: > + *val = 1; > + *val2 = 100000; > + return IIO_VAL_INT_PLUS_MICRO; > + > + case AXP20X_BATT_DISCHRG_I: > + case AXP20X_BATT_CHRG_I: > + *val = 0; > + *val2 = 500000; > + return IIO_VAL_INT_PLUS_MICRO; > + > + case AXP20X_IPSOUT_V: > + *val = 1; > + *val2 = 400000; > + return IIO_VAL_INT_PLUS_MICRO; > + > + default: > + return -EINVAL; > + } > + > + return -EINVAL; > +} > + > +static int axp22x_adc_scale(int channel, int *val, int *val2) > +{ > + switch (channel) { > + case AXP22X_TEMP_ADC: > + *val = 100; > + return IIO_VAL_INT; > + > + case AXP22X_BATT_V: > + *val = 1; > + *val2 = 100000; > + return IIO_VAL_INT_PLUS_MICRO; > + > + case AXP22X_BATT_DISCHRG_I: > + case AXP22X_BATT_CHRG_I: > + *val = 0; > + *val2 = 500000; > + return IIO_VAL_INT_PLUS_MICRO; > + > + default: > + return -EINVAL; > + } > + > + return -EINVAL; > +} > + > +static int axp20x_adc_offset(struct iio_dev *indio_dev, int channel, int *val) > +{ > + struct axp20x_adc_iio *info = iio_priv(indio_dev); > + int ret, reg; > + > + switch (channel) { > + case AXP20X_TEMP_ADC: > + *val = -1447; > + return IIO_VAL_INT; > + > + case AXP20X_GPIO0_V: > + case AXP20X_GPIO1_V: > + ret = regmap_read(info->regmap, AXP20X_GPIO10_IN_RANGE, ®); > + if (ret < 0) > + return ret; > + > + if (channel == AXP20X_GPIO0_V) > + *val = reg & AXP20X_GPIO10_IN_RANGE_GPIO0; > + else > + *val = reg & AXP20X_GPIO10_IN_RANGE_GPIO1; > + > + *val = !!(*val) * 700000; > + > + return IIO_VAL_INT; > + > + default: > + return -EINVAL; > + } > + > + return -EINVAL; > +} > + > +static int axp20x_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, int *val, > + int *val2, long mask) > +{ > + switch (mask) { > + case IIO_CHAN_INFO_OFFSET: > + return axp20x_adc_offset(indio_dev, chan->channel, val); > + > + case IIO_CHAN_INFO_SCALE: > + return axp20x_adc_scale(chan->channel, val, val2); > + > + case IIO_CHAN_INFO_RAW: > + return axp20x_adc_read_raw(indio_dev, chan, val, val2); > + > + default: > + return -EINVAL; > + } > + > + return -EINVAL; > +} > + > +static int axp22x_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, int *val, > + int *val2, long mask) > +{ > + switch (mask) { > + case IIO_CHAN_INFO_OFFSET: > + *val = -2667; Datasheet says -267.7 C, or -2677 here. > + return IIO_VAL_INT; > + > + case IIO_CHAN_INFO_SCALE: > + return axp22x_adc_scale(chan->channel, val, val2); > + > + case IIO_CHAN_INFO_RAW: > + return axp22x_adc_read_raw(indio_dev, chan, val, val2); > + > + default: > + return -EINVAL; > + } > + > + return -EINVAL; > +} > + > +static int axp20x_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, int val, int val2, > + long mask) > +{ > + struct axp20x_adc_iio *info = iio_priv(indio_dev); > + > + /* > + * The AXP20X PMIC allows the user to choose between 0V and 0.7V offsets > + * for (independently) GPIO0 and GPIO1 when in ADC mode. > + */ > + if (mask != IIO_CHAN_INFO_OFFSET) > + return -EINVAL; > + > + if (chan->channel != AXP20X_GPIO0_V && chan->channel != AXP20X_GPIO1_V) > + return -EINVAL; > + > + if (val != 0 && val != 700000) > + return -EINVAL; > + > + if (chan->channel == AXP20X_GPIO0_V) > + return regmap_update_bits(info->regmap, AXP20X_GPIO10_IN_RANGE, > + AXP20X_GPIO10_IN_RANGE_GPIO0, > + AXP20X_GPIO10_IN_RANGE_GPIO0_VAL(!!val)); > + > + return regmap_update_bits(info->regmap, AXP20X_GPIO10_IN_RANGE, > + AXP20X_GPIO10_IN_RANGE_GPIO1, > + AXP20X_GPIO10_IN_RANGE_GPIO1_VAL(!!val)); > +} > + > +static const struct iio_info axp20x_adc_iio_info = { > + .read_raw = axp20x_read_raw, > + .write_raw = axp20x_write_raw, > + .driver_module = THIS_MODULE, > +}; > + > +static const struct iio_info axp22x_adc_iio_info = { > + .read_raw = axp22x_read_raw, > + .driver_module = THIS_MODULE, > +}; > + > +static const struct of_device_id axp20x_adc_of_match[] = { > + { .compatible = "x-powers,axp209-adc", .data = (void *)AXP209_ID, }, > + { .compatible = "x-powers,axp221-adc", .data = (void *)AXP221_ID, }, > + { /* sentinel */ }, > +}; > + > +static int axp20x_probe(struct platform_device *pdev) > +{ > + struct axp20x_adc_iio *info; > + struct iio_dev *indio_dev; > + struct axp20x_dev *axp20x_dev; > + int ret, axp20x_id; > + > + axp20x_dev = dev_get_drvdata(pdev->dev.parent); > + > + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info)); > + if (!indio_dev) > + return -ENOMEM; > + > + info = iio_priv(indio_dev); > + platform_set_drvdata(pdev, indio_dev); > + > + info->regmap = axp20x_dev->regmap; > + info->indio_dev = indio_dev; > + indio_dev->name = dev_name(&pdev->dev); > + indio_dev->dev.parent = &pdev->dev; > + indio_dev->dev.of_node = pdev->dev.of_node; > + indio_dev->modes = INDIO_DIRECT_MODE; > + > + axp20x_id = (int)of_device_get_match_data(&pdev->dev); > + > + switch (axp20x_id) { > + case AXP209_ID: > + indio_dev->info = &axp20x_adc_iio_info; > + indio_dev->num_channels = ARRAY_SIZE(axp20x_adc_channels); > + indio_dev->channels = axp20x_adc_channels; > + > + /* Enable the ADCs on IP */ > + regmap_write(info->regmap, AXP20X_ADC_EN1, AXP20X_ADC_EN1_MASK); > + > + /* Enable GPIO0/1 and internal temperature ADCs */ > + regmap_update_bits(info->regmap, AXP20X_ADC_EN2, > + AXP20X_ADC_EN2_MASK, AXP20X_ADC_EN2_MASK); > + > + /* Configure ADCs rate */ > + regmap_update_bits(info->regmap, AXP20X_ADC_RATE, > + AXP20X_ADC_RATE_MASK, AXP20X_ADC_RATE_50HZ); > + break; > + > + case AXP221_ID: > + indio_dev->info = &axp22x_adc_iio_info; > + indio_dev->num_channels = ARRAY_SIZE(axp22x_adc_channels); > + indio_dev->channels = axp22x_adc_channels; > + > + /* Enable the ADCs on IP */ > + regmap_write(info->regmap, AXP20X_ADC_EN1, AXP22X_ADC_EN1_MASK); > + > + /* Configure ADCs rate */ > + regmap_update_bits(info->regmap, AXP20X_ADC_RATE, > + AXP20X_ADC_RATE_MASK, AXP22X_ADC_RATE_200HZ); > + break; > + > + default: > + return -EINVAL; > + } > + > + ret = devm_iio_device_register(&pdev->dev, indio_dev); > + if (ret < 0) { > + dev_err(&pdev->dev, "could not register the device\n"); > + regmap_write(info->regmap, AXP20X_ADC_EN1, 0); > + regmap_write(info->regmap, AXP20X_ADC_EN2, 0); > + return ret; > + } > + > + return 0; > +} > + > +static int axp20x_remove(struct platform_device *pdev) > +{ > + struct axp20x_adc_iio *info; > + struct iio_dev *indio_dev = platform_get_drvdata(pdev); > + > + info = iio_priv(indio_dev); Nit: you could just reverse the 2 declarations above and join this line after struct axp20x_adc_iio *info; > + regmap_write(info->regmap, AXP20X_ADC_EN1, 0); > + regmap_write(info->regmap, AXP20X_ADC_EN2, 0); The existing VBUS power supply driver enables the VBUS ADC bits itself, and does not check them later on. This means if one were to remove this axp20x-adc module, the voltage/current readings in the VBUS power supply would be invalid. Some sort of workaround would be needed here in this driver of the VBUS driver. > + > + return 0; > +} > + > +static struct platform_driver axp20x_adc_driver = { > + .driver = { > + .name = "axp20x-adc", > + .of_match_table = axp20x_adc_of_match, > + }, > + .probe = axp20x_probe, > + .remove = axp20x_remove, > +}; > + > +module_platform_driver(axp20x_adc_driver); > + > +MODULE_DESCRIPTION("ADC driver for AXP20X and AXP22X PMICs"); > +MODULE_AUTHOR("Quentin Schulz <quentin.schulz@free-electrons.com>"); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h > index a4860bc..650c6f6 100644 > --- a/include/linux/mfd/axp20x.h > +++ b/include/linux/mfd/axp20x.h > @@ -150,6 +150,10 @@ enum { > #define AXP20X_VBUS_I_ADC_L 0x5d > #define AXP20X_TEMP_ADC_H 0x5e > #define AXP20X_TEMP_ADC_L 0x5f > + > +#define AXP22X_TEMP_ADC_H 0x56 > +#define AXP22X_TEMP_ADC_L 0x57 > + This is in the wrong patch. Also we already have /* AXP22X specific registers */ #define AXP22X_PMIC_ADC_H 0x56 #define AXP22X_PMIC_ADC_L 0x57 #define AXP22X_TS_ADC_H 0x58 #define AXP22X_TS_ADC_L 0x59 If you want, you could just rename them to be consistent. Regards ChenYu > #define AXP20X_TS_IN_H 0x62 > #define AXP20X_TS_IN_L 0x63 > #define AXP20X_GPIO0_V_ADC_H 0x64 > -- > 2.9.3 >
Hi Chen-Yu, On 05/01/2017 06:42, Chen-Yu Tsai wrote: > On Tue, Jan 3, 2017 at 12:37 AM, Quentin Schulz > <quentin.schulz@free-electrons.com> wrote: [...] >> + >> +#define AXP20X_ADC_RATE_MASK (3 << 6) >> +#define AXP20X_ADC_RATE_25HZ (0 << 6) >> +#define AXP20X_ADC_RATE_50HZ BIT(6) > > Please be consistent with the format. > >> +#define AXP20X_ADC_RATE_100HZ (2 << 6) >> +#define AXP20X_ADC_RATE_200HZ (3 << 6) >> + >> +#define AXP22X_ADC_RATE_100HZ (0 << 6) >> +#define AXP22X_ADC_RATE_200HZ BIT(6) >> +#define AXP22X_ADC_RATE_400HZ (2 << 6) >> +#define AXP22X_ADC_RATE_800HZ (3 << 6) > > These are power-of-2 multiples of some base rate. May I suggest > a formula macro instead. Either way, you seem to be using only > one value. Will this be made configurable in the future? > Yes, I could use a formula macro instead. No plan to make it configurable, should I make it configurable? >> + >> +#define AXP20X_ADC_CHANNEL(_channel, _name, _type, _reg) \ >> + { \ >> + .type = _type, \ >> + .indexed = 1, \ >> + .channel = _channel, \ >> + .address = _reg, \ >> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ >> + BIT(IIO_CHAN_INFO_SCALE), \ >> + .datasheet_name = _name, \ >> + } >> + >> +#define AXP20X_ADC_CHANNEL_OFFSET(_channel, _name, _type, _reg) \ >> + { \ >> + .type = _type, \ >> + .indexed = 1, \ >> + .channel = _channel, \ >> + .address = _reg, \ >> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ >> + BIT(IIO_CHAN_INFO_SCALE) |\ >> + BIT(IIO_CHAN_INFO_OFFSET),\ >> + .datasheet_name = _name, \ >> + } >> + >> +struct axp20x_adc_iio { >> + struct iio_dev *indio_dev; >> + struct regmap *regmap; >> +}; >> + >> +enum axp20x_adc_channel { >> + AXP20X_ACIN_V = 0, >> + AXP20X_ACIN_I, >> + AXP20X_VBUS_V, >> + AXP20X_VBUS_I, >> + AXP20X_TEMP_ADC, > > PMIC_TEMP would be better. And please save a slot for TS input. > ACK. Hum.. I'm wondering what should be the IIO type of the TS input channel then? The TS Pin can be used in two modes: either to monitor the temperature of the battery or as an external ADC, at least that's what I understand from the datasheet. >> + AXP20X_GPIO0_V, >> + AXP20X_GPIO1_V, > > Please skip a slot for "battery instantaneous power". > >> + AXP20X_BATT_V, >> + AXP20X_BATT_CHRG_I, >> + AXP20X_BATT_DISCHRG_I, >> + AXP20X_IPSOUT_V, >> +}; >> + >> +enum axp22x_adc_channel { >> + AXP22X_TEMP_ADC = 0, > > Same comments as AXP20X_TEMP_ADC. > >> + AXP22X_BATT_V, >> + AXP22X_BATT_CHRG_I, >> + AXP22X_BATT_DISCHRG_I, >> +}; > > Shouldn't these channel numbers be exported as part of the device tree > bindings? At the very least, they shouldn't be changed. > I don't understand what you mean by that. Do you mean you want a consistent numbering between the AXP20X and the AXP22X, so that AXP22X_BATT_V would have the same channel number than AXP20X_BATT_V? Could you explain a bit more your thoughts on the channel numbers being exported as part of the device tree bindings? > Also please add a comment saying that the channels are numbered > in the order of their respective registers, and not the table > describing the ADCs in the datasheet (9.7 Signal Capture for AXP209 > and 9.5 E-Gauge for AXP221). > Yes I can. What about Rob wanting channel numbers to start at zero for each different IIO type (i.e., today we have AXP22X_BATT_CHRG_I being exported as in_current1_raw whereas he wants in_current0_raw). [...] >> +static int axp22x_adc_read_raw(struct iio_dev *indio_dev, >> + struct iio_chan_spec const *channel, int *val, >> + int *val2) >> +{ >> + struct axp20x_adc_iio *info = iio_priv(indio_dev); >> + int size = 12, ret; >> + >> + switch (channel->channel) { >> + case AXP22X_BATT_DISCHRG_I: >> + size = 13; >> + case AXP22X_TEMP_ADC: >> + case AXP22X_BATT_V: >> + case AXP22X_BATT_CHRG_I: > > According to the datasheet, AXP22X_BATT_CHRG_I is also 13 bits wide. > Where did you get that? Also, the datasheet is inconsistent: - 9.5 E-Gauge Fuel Gauge system => the min value is at 0x0 and the max value at 0xfff for all channels, that's 12 bits. - 10.1.4 ADC Data => all channels except battery discharge current are on 12 bits (8 high, 4 low). [...] >> +static int axp22x_read_raw(struct iio_dev *indio_dev, >> + struct iio_chan_spec const *chan, int *val, >> + int *val2, long mask) >> +{ >> + switch (mask) { >> + case IIO_CHAN_INFO_OFFSET: >> + *val = -2667; > > Datasheet says -267.7 C, or -2677 here. > The formula in the datasheet is (in milli Celsius): processed = raw * 100 - 266700; while the IIO framework asks for a scale and an offset which are then applied as: processed = (raw + offset) * scale; Thus by factorizing, we get: processed = (raw - 2667) * 100; [...] >> +static int axp20x_remove(struct platform_device *pdev) >> +{ >> + struct axp20x_adc_iio *info; >> + struct iio_dev *indio_dev = platform_get_drvdata(pdev); >> + >> + info = iio_priv(indio_dev); > > Nit: you could just reverse the 2 declarations above and join this > line after struct axp20x_adc_iio *info; > >> + regmap_write(info->regmap, AXP20X_ADC_EN1, 0); >> + regmap_write(info->regmap, AXP20X_ADC_EN2, 0); > > The existing VBUS power supply driver enables the VBUS ADC bits itself, > and does not check them later on. This means if one were to remove this > axp20x-adc module, the voltage/current readings in the VBUS power supply > would be invalid. Some sort of workaround would be needed here in this > driver of the VBUS driver. > That would be one reason to migrate the VBUS driver to use the IIO channels, wouldn't it? But ACK, I'll think about something to work around this issue. >> + >> + return 0; >> +} >> + >> +static struct platform_driver axp20x_adc_driver = { >> + .driver = { >> + .name = "axp20x-adc", >> + .of_match_table = axp20x_adc_of_match, >> + }, >> + .probe = axp20x_probe, >> + .remove = axp20x_remove, >> +}; >> + >> +module_platform_driver(axp20x_adc_driver); >> + >> +MODULE_DESCRIPTION("ADC driver for AXP20X and AXP22X PMICs"); >> +MODULE_AUTHOR("Quentin Schulz <quentin.schulz@free-electrons.com>"); >> +MODULE_LICENSE("GPL"); >> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h >> index a4860bc..650c6f6 100644 >> --- a/include/linux/mfd/axp20x.h >> +++ b/include/linux/mfd/axp20x.h >> @@ -150,6 +150,10 @@ enum { >> #define AXP20X_VBUS_I_ADC_L 0x5d >> #define AXP20X_TEMP_ADC_H 0x5e >> #define AXP20X_TEMP_ADC_L 0x5f >> + >> +#define AXP22X_TEMP_ADC_H 0x56 >> +#define AXP22X_TEMP_ADC_L 0x57 >> + > > This is in the wrong patch. Also we already have > > /* AXP22X specific registers */ > #define AXP22X_PMIC_ADC_H 0x56 > #define AXP22X_PMIC_ADC_L 0x57 > #define AXP22X_TS_ADC_H 0x58 > #define AXP22X_TS_ADC_L 0x59 > > If you want, you could just rename them to be consistent. > ACK. Thanks, Quentin
On Thu, Jan 5, 2017 at 4:06 PM, Quentin Schulz <quentin.schulz@free-electrons.com> wrote: > Hi Chen-Yu, > > On 05/01/2017 06:42, Chen-Yu Tsai wrote: >> On Tue, Jan 3, 2017 at 12:37 AM, Quentin Schulz >> <quentin.schulz@free-electrons.com> wrote: > [...] >>> + >>> +#define AXP20X_ADC_RATE_MASK (3 << 6) >>> +#define AXP20X_ADC_RATE_25HZ (0 << 6) >>> +#define AXP20X_ADC_RATE_50HZ BIT(6) >> >> Please be consistent with the format. >> >>> +#define AXP20X_ADC_RATE_100HZ (2 << 6) >>> +#define AXP20X_ADC_RATE_200HZ (3 << 6) >>> + >>> +#define AXP22X_ADC_RATE_100HZ (0 << 6) >>> +#define AXP22X_ADC_RATE_200HZ BIT(6) >>> +#define AXP22X_ADC_RATE_400HZ (2 << 6) >>> +#define AXP22X_ADC_RATE_800HZ (3 << 6) >> >> These are power-of-2 multiples of some base rate. May I suggest >> a formula macro instead. Either way, you seem to be using only >> one value. Will this be made configurable in the future? >> > > Yes, I could use a formula macro instead. No plan to make it > configurable, should I make it configurable? I don't see a use case for that atm. >>> + >>> +#define AXP20X_ADC_CHANNEL(_channel, _name, _type, _reg) \ >>> + { \ >>> + .type = _type, \ >>> + .indexed = 1, \ >>> + .channel = _channel, \ >>> + .address = _reg, \ >>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ >>> + BIT(IIO_CHAN_INFO_SCALE), \ >>> + .datasheet_name = _name, \ >>> + } >>> + >>> +#define AXP20X_ADC_CHANNEL_OFFSET(_channel, _name, _type, _reg) \ >>> + { \ >>> + .type = _type, \ >>> + .indexed = 1, \ >>> + .channel = _channel, \ >>> + .address = _reg, \ >>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ >>> + BIT(IIO_CHAN_INFO_SCALE) |\ >>> + BIT(IIO_CHAN_INFO_OFFSET),\ >>> + .datasheet_name = _name, \ >>> + } >>> + >>> +struct axp20x_adc_iio { >>> + struct iio_dev *indio_dev; >>> + struct regmap *regmap; >>> +}; >>> + >>> +enum axp20x_adc_channel { >>> + AXP20X_ACIN_V = 0, >>> + AXP20X_ACIN_I, >>> + AXP20X_VBUS_V, >>> + AXP20X_VBUS_I, >>> + AXP20X_TEMP_ADC, >> >> PMIC_TEMP would be better. And please save a slot for TS input. >> > > ACK. > > Hum.. I'm wondering what should be the IIO type of the TS input channel > then? The TS Pin can be used in two modes: either to monitor the > temperature of the battery or as an external ADC, at least that's what I > understand from the datasheet. AFAIK the battery charge/discharge high/low temperature threshold registers take values in terms of voltage, not actual temperature. And the temperature readout kind of depends on the thermoresistor one is using. So I think "voltage" would be the proper type. > >>> + AXP20X_GPIO0_V, >>> + AXP20X_GPIO1_V, >> >> Please skip a slot for "battery instantaneous power". >> >>> + AXP20X_BATT_V, >>> + AXP20X_BATT_CHRG_I, >>> + AXP20X_BATT_DISCHRG_I, >>> + AXP20X_IPSOUT_V, >>> +}; >>> + >>> +enum axp22x_adc_channel { >>> + AXP22X_TEMP_ADC = 0, >> >> Same comments as AXP20X_TEMP_ADC. >> >>> + AXP22X_BATT_V, >>> + AXP22X_BATT_CHRG_I, >>> + AXP22X_BATT_DISCHRG_I, >>> +}; >> >> Shouldn't these channel numbers be exported as part of the device tree >> bindings? At the very least, they shouldn't be changed. >> > > I don't understand what you mean by that. Do you mean you want a > consistent numbering between the AXP20X and the AXP22X, so that > AXP22X_BATT_V would have the same channel number than AXP20X_BATT_V? > > Could you explain a bit more your thoughts on the channel numbers being > exported as part of the device tree bindings? What I meant was that, since you are referencing the channels in the device tree, the numbering scheme would be part of the device tree binding, and should never be changed. So either these would be macros in include/dt-bindings/, or a big warning should be put before it. But see my reply on patch 7, about do we actually need to expose this in the device tree. >> Also please add a comment saying that the channels are numbered >> in the order of their respective registers, and not the table >> describing the ADCs in the datasheet (9.7 Signal Capture for AXP209 >> and 9.5 E-Gauge for AXP221). >> > > Yes I can. > > What about Rob wanting channel numbers to start at zero for each > different IIO type (i.e., today we have AXP22X_BATT_CHRG_I being > exported as in_current1_raw whereas he wants in_current0_raw). Hmm... I missed this. Are you talking about IIO or hwmon? IIRC hwmon numbers things starting at 1. > [...] >>> +static int axp22x_adc_read_raw(struct iio_dev *indio_dev, >>> + struct iio_chan_spec const *channel, int *val, >>> + int *val2) >>> +{ >>> + struct axp20x_adc_iio *info = iio_priv(indio_dev); >>> + int size = 12, ret; >>> + >>> + switch (channel->channel) { >>> + case AXP22X_BATT_DISCHRG_I: >>> + size = 13; >>> + case AXP22X_TEMP_ADC: >>> + case AXP22X_BATT_V: >>> + case AXP22X_BATT_CHRG_I: >> >> According to the datasheet, AXP22X_BATT_CHRG_I is also 13 bits wide. >> > > Where did you get that? > > Also, the datasheet is inconsistent: > - 9.5 E-Gauge Fuel Gauge system => the min value is at 0x0 and the max > value at 0xfff for all channels, that's 12 bits. > - 10.1.4 ADC Data => all channels except battery discharge current are > on 12 bits (8 high, 4 low). My datasheets (AXP221 v1.6, AXP221s v1.2, AXP223 v1.1, all Chinese) say in 10.1.4: - 7A: battery charge current high 5 bits - 7B: battery charge current low 8 bits - 7C: battery discharge current high 5 bits - 7D: battery discharge current low 8 bits > > [...] >>> +static int axp22x_read_raw(struct iio_dev *indio_dev, >>> + struct iio_chan_spec const *chan, int *val, >>> + int *val2, long mask) >>> +{ >>> + switch (mask) { >>> + case IIO_CHAN_INFO_OFFSET: >>> + *val = -2667; >> >> Datasheet says -267.7 C, or -2677 here. >> > > The formula in the datasheet is (in milli Celsius): > processed = raw * 100 - 266700; > > while the IIO framework asks for a scale and an offset which are then > applied as: > processed = (raw + offset) * scale; > > Thus by factorizing, we get: > processed = (raw - 2667) * 100; What I meant was that your lower end value is off by one degree, -266.7 in your code vs -267.7 in the datasheet. > > [...] >>> +static int axp20x_remove(struct platform_device *pdev) >>> +{ >>> + struct axp20x_adc_iio *info; >>> + struct iio_dev *indio_dev = platform_get_drvdata(pdev); >>> + >>> + info = iio_priv(indio_dev); >> >> Nit: you could just reverse the 2 declarations above and join this >> line after struct axp20x_adc_iio *info; >> >>> + regmap_write(info->regmap, AXP20X_ADC_EN1, 0); >>> + regmap_write(info->regmap, AXP20X_ADC_EN2, 0); >> >> The existing VBUS power supply driver enables the VBUS ADC bits itself, >> and does not check them later on. This means if one were to remove this >> axp20x-adc module, the voltage/current readings in the VBUS power supply >> would be invalid. Some sort of workaround would be needed here in this >> driver of the VBUS driver. >> > > That would be one reason to migrate the VBUS driver to use the IIO > channels, wouldn't it? It is, preferably without changing the device tree. Regards ChenYu > > But ACK, I'll think about something to work around this issue. > >>> + >>> + return 0; >>> +} >>> + >>> +static struct platform_driver axp20x_adc_driver = { >>> + .driver = { >>> + .name = "axp20x-adc", >>> + .of_match_table = axp20x_adc_of_match, >>> + }, >>> + .probe = axp20x_probe, >>> + .remove = axp20x_remove, >>> +}; >>> + >>> +module_platform_driver(axp20x_adc_driver); >>> + >>> +MODULE_DESCRIPTION("ADC driver for AXP20X and AXP22X PMICs"); >>> +MODULE_AUTHOR("Quentin Schulz <quentin.schulz@free-electrons.com>"); >>> +MODULE_LICENSE("GPL"); >>> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h >>> index a4860bc..650c6f6 100644 >>> --- a/include/linux/mfd/axp20x.h >>> +++ b/include/linux/mfd/axp20x.h >>> @@ -150,6 +150,10 @@ enum { >>> #define AXP20X_VBUS_I_ADC_L 0x5d >>> #define AXP20X_TEMP_ADC_H 0x5e >>> #define AXP20X_TEMP_ADC_L 0x5f >>> + >>> +#define AXP22X_TEMP_ADC_H 0x56 >>> +#define AXP22X_TEMP_ADC_L 0x57 >>> + >> >> This is in the wrong patch. Also we already have >> >> /* AXP22X specific registers */ >> #define AXP22X_PMIC_ADC_H 0x56 >> #define AXP22X_PMIC_ADC_L 0x57 >> #define AXP22X_TS_ADC_H 0x58 >> #define AXP22X_TS_ADC_L 0x59 >> >> If you want, you could just rename them to be consistent. >> > > ACK. > > Thanks, > Quentin > > -- > Quentin Schulz, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com
On 05/01/2017 09:27, Chen-Yu Tsai wrote: > On Thu, Jan 5, 2017 at 4:06 PM, Quentin Schulz > <quentin.schulz@free-electrons.com> wrote: >> Hi Chen-Yu, >> >> On 05/01/2017 06:42, Chen-Yu Tsai wrote: >>> On Tue, Jan 3, 2017 at 12:37 AM, Quentin Schulz >>> <quentin.schulz@free-electrons.com> wrote: >> [...] >>>> + >>>> +#define AXP20X_ADC_RATE_MASK (3 << 6) >>>> +#define AXP20X_ADC_RATE_25HZ (0 << 6) >>>> +#define AXP20X_ADC_RATE_50HZ BIT(6) >>> >>> Please be consistent with the format. >>> >>>> +#define AXP20X_ADC_RATE_100HZ (2 << 6) >>>> +#define AXP20X_ADC_RATE_200HZ (3 << 6) >>>> + >>>> +#define AXP22X_ADC_RATE_100HZ (0 << 6) >>>> +#define AXP22X_ADC_RATE_200HZ BIT(6) >>>> +#define AXP22X_ADC_RATE_400HZ (2 << 6) >>>> +#define AXP22X_ADC_RATE_800HZ (3 << 6) >>> >>> These are power-of-2 multiples of some base rate. May I suggest >>> a formula macro instead. Either way, you seem to be using only >>> one value. Will this be made configurable in the future? >>> >> >> Yes, I could use a formula macro instead. No plan to make it >> configurable, should I make it configurable? > > I don't see a use case for that atm. > >>>> + >>>> +#define AXP20X_ADC_CHANNEL(_channel, _name, _type, _reg) \ >>>> + { \ >>>> + .type = _type, \ >>>> + .indexed = 1, \ >>>> + .channel = _channel, \ >>>> + .address = _reg, \ >>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ >>>> + BIT(IIO_CHAN_INFO_SCALE), \ >>>> + .datasheet_name = _name, \ >>>> + } >>>> + >>>> +#define AXP20X_ADC_CHANNEL_OFFSET(_channel, _name, _type, _reg) \ >>>> + { \ >>>> + .type = _type, \ >>>> + .indexed = 1, \ >>>> + .channel = _channel, \ >>>> + .address = _reg, \ >>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ >>>> + BIT(IIO_CHAN_INFO_SCALE) |\ >>>> + BIT(IIO_CHAN_INFO_OFFSET),\ >>>> + .datasheet_name = _name, \ >>>> + } >>>> + >>>> +struct axp20x_adc_iio { >>>> + struct iio_dev *indio_dev; >>>> + struct regmap *regmap; >>>> +}; >>>> + >>>> +enum axp20x_adc_channel { >>>> + AXP20X_ACIN_V = 0, >>>> + AXP20X_ACIN_I, >>>> + AXP20X_VBUS_V, >>>> + AXP20X_VBUS_I, >>>> + AXP20X_TEMP_ADC, >>> >>> PMIC_TEMP would be better. And please save a slot for TS input. >>> >> >> ACK. >> >> Hum.. I'm wondering what should be the IIO type of the TS input channel >> then? The TS Pin can be used in two modes: either to monitor the >> temperature of the battery or as an external ADC, at least that's what I >> understand from the datasheet. > > AFAIK the battery charge/discharge high/low temperature threshold > registers take values in terms of voltage, not actual temperature. > And the temperature readout kind of depends on the thermoresistor > one is using. So I think "voltage" would be the proper type. > ACK. Should I just add TS_IN in axp20x_adc_channel enum but not add it in the exposed IIO channels ("saving" the slot but not using it)? >> >>>> + AXP20X_GPIO0_V, >>>> + AXP20X_GPIO1_V, >>> >>> Please skip a slot for "battery instantaneous power". >>> >>>> + AXP20X_BATT_V, >>>> + AXP20X_BATT_CHRG_I, >>>> + AXP20X_BATT_DISCHRG_I, >>>> + AXP20X_IPSOUT_V, >>>> +}; >>>> + >>>> +enum axp22x_adc_channel { >>>> + AXP22X_TEMP_ADC = 0, >>> >>> Same comments as AXP20X_TEMP_ADC. >>> >>>> + AXP22X_BATT_V, >>>> + AXP22X_BATT_CHRG_I, >>>> + AXP22X_BATT_DISCHRG_I, >>>> +}; >>> >>> Shouldn't these channel numbers be exported as part of the device tree >>> bindings? At the very least, they shouldn't be changed. >>> >> >> I don't understand what you mean by that. Do you mean you want a >> consistent numbering between the AXP20X and the AXP22X, so that >> AXP22X_BATT_V would have the same channel number than AXP20X_BATT_V? >> >> Could you explain a bit more your thoughts on the channel numbers being >> exported as part of the device tree bindings? > > What I meant was that, since you are referencing the channels in the > device tree, the numbering scheme would be part of the device tree > binding, and should never be changed. So either these would be macros > in include/dt-bindings/, or a big warning should be put before it. > ACK. > But see my reply on patch 7, about do we actually need to expose this > in the device tree. > I don't know what's the best. >>> Also please add a comment saying that the channels are numbered >>> in the order of their respective registers, and not the table >>> describing the ADCs in the datasheet (9.7 Signal Capture for AXP209 >>> and 9.5 E-Gauge for AXP221). >>> >> >> Yes I can. >> >> What about Rob wanting channel numbers to start at zero for each >> different IIO type (i.e., today we have AXP22X_BATT_CHRG_I being >> exported as in_current1_raw whereas he wants in_current0_raw). > > Hmm... I missed this. Are you talking about IIO or hwmon? IIRC > hwmon numbers things starting at 1. > About IIO. Today, we have exposed: in_voltage0_raw for acin_v in_current1_raw for acin_i in_voltage2_raw for vbus_v in_current3_raw for vbus_i in_temp4_raw for adc_temp in_voltage5_raw for gpio0_v in_voltage6_raw for gpio1_v in_voltage7_raw for batt_v in_current8_raw for batt_chrg_i in_current9_raw for batt_dischrg_i in_voltage10_raw for ipsout_v but I think what Rob wants is: in_voltage0_raw acin_v in_current0_raw for acin_i in_voltage1_raw for vbus_v in_current1_raw for vbus_i in_temp_raw for adc_temp in_voltage2_raw for gpio0_v in_voltage3_raw for gpio1_v in_voltage4_raw for batt_v in_current2_raw for batt_chrg_i in_current3_raw for batt_dischrg_i in_voltage5_raw for ipsout_v >> [...] >>>> +static int axp22x_adc_read_raw(struct iio_dev *indio_dev, >>>> + struct iio_chan_spec const *channel, int *val, >>>> + int *val2) >>>> +{ >>>> + struct axp20x_adc_iio *info = iio_priv(indio_dev); >>>> + int size = 12, ret; >>>> + >>>> + switch (channel->channel) { >>>> + case AXP22X_BATT_DISCHRG_I: >>>> + size = 13; >>>> + case AXP22X_TEMP_ADC: >>>> + case AXP22X_BATT_V: >>>> + case AXP22X_BATT_CHRG_I: >>> >>> According to the datasheet, AXP22X_BATT_CHRG_I is also 13 bits wide. >>> >> >> Where did you get that? >> >> Also, the datasheet is inconsistent: >> - 9.5 E-Gauge Fuel Gauge system => the min value is at 0x0 and the max >> value at 0xfff for all channels, that's 12 bits. >> - 10.1.4 ADC Data => all channels except battery discharge current are >> on 12 bits (8 high, 4 low). > > My datasheets (AXP221 v1.6, AXP221s v1.2, AXP223 v1.1, all Chinese) say > in 10.1.4: > > - 7A: battery charge current high 5 bits > - 7B: battery charge current low 8 bits > - 7C: battery discharge current high 5 bits > - 7D: battery discharge current low 8 bits > AXP223 v1.1 in English in 10.1.4[1]: - 7A: battery charge current high 8 bits - 7B: battery charge current low 4 bits - 7C: battery discharge current high 8 bits - 7D: battery discharge current low 5 bits Note that it's 8 bits for high and 4/5 bits for low while you wrote 4/5 bits high and low 8 bits (typos or actually what's written in the datasheet?). Hum.. from the reg reading function[2] I would say that the correct formula is high on 8 bits and low on 4/5 bits. So hum.. what do we do? [1] http://dl.linux-sunxi.org/AXP/AXP223-en.pdf [2] http://lxr.free-electrons.com/source/include/linux/mfd/axp20x.h#L564 >> >> [...] >>>> +static int axp22x_read_raw(struct iio_dev *indio_dev, >>>> + struct iio_chan_spec const *chan, int *val, >>>> + int *val2, long mask) >>>> +{ >>>> + switch (mask) { >>>> + case IIO_CHAN_INFO_OFFSET: >>>> + *val = -2667; >>> >>> Datasheet says -267.7 C, or -2677 here. >>> >> >> The formula in the datasheet is (in milli Celsius): >> processed = raw * 100 - 266700; >> >> while the IIO framework asks for a scale and an offset which are then >> applied as: >> processed = (raw + offset) * scale; >> >> Thus by factorizing, we get: >> processed = (raw - 2667) * 100; > > What I meant was that your lower end value is off by one degree, > -266.7 in your code vs -267.7 in the datasheet. > Indeed. Thanks. >> >> [...] >>>> +static int axp20x_remove(struct platform_device *pdev) >>>> +{ >>>> + struct axp20x_adc_iio *info; >>>> + struct iio_dev *indio_dev = platform_get_drvdata(pdev); >>>> + >>>> + info = iio_priv(indio_dev); >>> >>> Nit: you could just reverse the 2 declarations above and join this >>> line after struct axp20x_adc_iio *info; >>> >>>> + regmap_write(info->regmap, AXP20X_ADC_EN1, 0); >>>> + regmap_write(info->regmap, AXP20X_ADC_EN2, 0); >>> >>> The existing VBUS power supply driver enables the VBUS ADC bits itself, >>> and does not check them later on. This means if one were to remove this >>> axp20x-adc module, the voltage/current readings in the VBUS power supply >>> would be invalid. Some sort of workaround would be needed here in this >>> driver of the VBUS driver. >>> >> >> That would be one reason to migrate the VBUS driver to use the IIO >> channels, wouldn't it? > > It is, preferably without changing the device tree. > Yes, of course. Thanks, Quentin
On Thu, Jan 5, 2017 at 5:50 PM, Quentin Schulz <quentin.schulz@free-electrons.com> wrote: > On 05/01/2017 09:27, Chen-Yu Tsai wrote: >> On Thu, Jan 5, 2017 at 4:06 PM, Quentin Schulz >> <quentin.schulz@free-electrons.com> wrote: >>> Hi Chen-Yu, >>> >>> On 05/01/2017 06:42, Chen-Yu Tsai wrote: >>>> On Tue, Jan 3, 2017 at 12:37 AM, Quentin Schulz >>>> <quentin.schulz@free-electrons.com> wrote: >>> [...] >>>>> + >>>>> +#define AXP20X_ADC_RATE_MASK (3 << 6) >>>>> +#define AXP20X_ADC_RATE_25HZ (0 << 6) >>>>> +#define AXP20X_ADC_RATE_50HZ BIT(6) >>>> >>>> Please be consistent with the format. >>>> >>>>> +#define AXP20X_ADC_RATE_100HZ (2 << 6) >>>>> +#define AXP20X_ADC_RATE_200HZ (3 << 6) >>>>> + >>>>> +#define AXP22X_ADC_RATE_100HZ (0 << 6) >>>>> +#define AXP22X_ADC_RATE_200HZ BIT(6) >>>>> +#define AXP22X_ADC_RATE_400HZ (2 << 6) >>>>> +#define AXP22X_ADC_RATE_800HZ (3 << 6) >>>> >>>> These are power-of-2 multiples of some base rate. May I suggest >>>> a formula macro instead. Either way, you seem to be using only >>>> one value. Will this be made configurable in the future? >>>> >>> >>> Yes, I could use a formula macro instead. No plan to make it >>> configurable, should I make it configurable? >> >> I don't see a use case for that atm. >> >>>>> + >>>>> +#define AXP20X_ADC_CHANNEL(_channel, _name, _type, _reg) \ >>>>> + { \ >>>>> + .type = _type, \ >>>>> + .indexed = 1, \ >>>>> + .channel = _channel, \ >>>>> + .address = _reg, \ >>>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ >>>>> + BIT(IIO_CHAN_INFO_SCALE), \ >>>>> + .datasheet_name = _name, \ >>>>> + } >>>>> + >>>>> +#define AXP20X_ADC_CHANNEL_OFFSET(_channel, _name, _type, _reg) \ >>>>> + { \ >>>>> + .type = _type, \ >>>>> + .indexed = 1, \ >>>>> + .channel = _channel, \ >>>>> + .address = _reg, \ >>>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ >>>>> + BIT(IIO_CHAN_INFO_SCALE) |\ >>>>> + BIT(IIO_CHAN_INFO_OFFSET),\ >>>>> + .datasheet_name = _name, \ >>>>> + } >>>>> + >>>>> +struct axp20x_adc_iio { >>>>> + struct iio_dev *indio_dev; >>>>> + struct regmap *regmap; >>>>> +}; >>>>> + >>>>> +enum axp20x_adc_channel { >>>>> + AXP20X_ACIN_V = 0, >>>>> + AXP20X_ACIN_I, >>>>> + AXP20X_VBUS_V, >>>>> + AXP20X_VBUS_I, >>>>> + AXP20X_TEMP_ADC, >>>> >>>> PMIC_TEMP would be better. And please save a slot for TS input. >>>> >>> >>> ACK. >>> >>> Hum.. I'm wondering what should be the IIO type of the TS input channel >>> then? The TS Pin can be used in two modes: either to monitor the >>> temperature of the battery or as an external ADC, at least that's what I >>> understand from the datasheet. >> >> AFAIK the battery charge/discharge high/low temperature threshold >> registers take values in terms of voltage, not actual temperature. >> And the temperature readout kind of depends on the thermoresistor >> one is using. So I think "voltage" would be the proper type. >> > > ACK. Should I just add TS_IN in axp20x_adc_channel enum but not add it > in the exposed IIO channels ("saving" the slot but not using it)? Sure. Or you could skip the number with AXP20X_GPIO0_V = 6, >>> >>>>> + AXP20X_GPIO0_V, >>>>> + AXP20X_GPIO1_V, >>>> >>>> Please skip a slot for "battery instantaneous power". >>>> >>>>> + AXP20X_BATT_V, >>>>> + AXP20X_BATT_CHRG_I, >>>>> + AXP20X_BATT_DISCHRG_I, >>>>> + AXP20X_IPSOUT_V, >>>>> +}; >>>>> + >>>>> +enum axp22x_adc_channel { >>>>> + AXP22X_TEMP_ADC = 0, >>>> >>>> Same comments as AXP20X_TEMP_ADC. >>>> >>>>> + AXP22X_BATT_V, >>>>> + AXP22X_BATT_CHRG_I, >>>>> + AXP22X_BATT_DISCHRG_I, >>>>> +}; >>>> >>>> Shouldn't these channel numbers be exported as part of the device tree >>>> bindings? At the very least, they shouldn't be changed. >>>> >>> >>> I don't understand what you mean by that. Do you mean you want a >>> consistent numbering between the AXP20X and the AXP22X, so that >>> AXP22X_BATT_V would have the same channel number than AXP20X_BATT_V? >>> >>> Could you explain a bit more your thoughts on the channel numbers being >>> exported as part of the device tree bindings? >> >> What I meant was that, since you are referencing the channels in the >> device tree, the numbering scheme would be part of the device tree >> binding, and should never be changed. So either these would be macros >> in include/dt-bindings/, or a big warning should be put before it. >> > > ACK. > >> But see my reply on patch 7, about do we actually need to expose this >> in the device tree. >> > > I don't know what's the best. Then let's not expose it in the device tree for now. It's easier to add it later than to remove it. > >>>> Also please add a comment saying that the channels are numbered >>>> in the order of their respective registers, and not the table >>>> describing the ADCs in the datasheet (9.7 Signal Capture for AXP209 >>>> and 9.5 E-Gauge for AXP221). >>>> >>> >>> Yes I can. >>> >>> What about Rob wanting channel numbers to start at zero for each >>> different IIO type (i.e., today we have AXP22X_BATT_CHRG_I being >>> exported as in_current1_raw whereas he wants in_current0_raw). >> >> Hmm... I missed this. Are you talking about IIO or hwmon? IIRC >> hwmon numbers things starting at 1. >> > > About IIO. > > Today, we have exposed: > in_voltage0_raw for acin_v > in_current1_raw for acin_i > in_voltage2_raw for vbus_v > in_current3_raw for vbus_i > in_temp4_raw for adc_temp > in_voltage5_raw for gpio0_v > in_voltage6_raw for gpio1_v > in_voltage7_raw for batt_v > in_current8_raw for batt_chrg_i > in_current9_raw for batt_dischrg_i > in_voltage10_raw for ipsout_v > > but I think what Rob wants is: > > in_voltage0_raw acin_v > in_current0_raw for acin_i > in_voltage1_raw for vbus_v > in_current1_raw for vbus_i > in_temp_raw for adc_temp > in_voltage2_raw for gpio0_v > in_voltage3_raw for gpio1_v > in_voltage4_raw for batt_v > in_current2_raw for batt_chrg_i > in_current3_raw for batt_dischrg_i > in_voltage5_raw for ipsout_v I think that's doable. If I understand IIO code correctly, the channel number is not used anywhere in the core code for dereferencing. It's used for sysfs entries (when .indexed is set) and events. However the twl4030 and twl6030 drivers use our current exposed scheme. I suppose its best to get some input from the IIO maintainer. So if we want, we could have the following channel mapping: 0 -> acin 1 -> vbus 2 -> pmic 3 -> gpio0 4 -> gpio1 5 -> ipsout 6 -> battery Each channel could have mulitple entries in axp20x_adc_channels[], one for each type of reading. You might need multiple channels for the battery to cover charge and discharge currents. Your callback functions would get a bit messier though. > >>> [...] >>>>> +static int axp22x_adc_read_raw(struct iio_dev *indio_dev, >>>>> + struct iio_chan_spec const *channel, int *val, >>>>> + int *val2) >>>>> +{ >>>>> + struct axp20x_adc_iio *info = iio_priv(indio_dev); >>>>> + int size = 12, ret; >>>>> + >>>>> + switch (channel->channel) { >>>>> + case AXP22X_BATT_DISCHRG_I: >>>>> + size = 13; >>>>> + case AXP22X_TEMP_ADC: >>>>> + case AXP22X_BATT_V: >>>>> + case AXP22X_BATT_CHRG_I: >>>> >>>> According to the datasheet, AXP22X_BATT_CHRG_I is also 13 bits wide. >>>> >>> >>> Where did you get that? >>> >>> Also, the datasheet is inconsistent: >>> - 9.5 E-Gauge Fuel Gauge system => the min value is at 0x0 and the max >>> value at 0xfff for all channels, that's 12 bits. >>> - 10.1.4 ADC Data => all channels except battery discharge current are >>> on 12 bits (8 high, 4 low). >> >> My datasheets (AXP221 v1.6, AXP221s v1.2, AXP223 v1.1, all Chinese) say >> in 10.1.4: >> >> - 7A: battery charge current high 5 bits >> - 7B: battery charge current low 8 bits >> - 7C: battery discharge current high 5 bits >> - 7D: battery discharge current low 8 bits >> > > AXP223 v1.1 in English in 10.1.4[1]: > - 7A: battery charge current high 8 bits > - 7B: battery charge current low 4 bits > - 7C: battery discharge current high 8 bits > - 7D: battery discharge current low 5 bits > > Note that it's 8 bits for high and 4/5 bits for low while you wrote 4/5 > bits high and low 8 bits (typos or actually what's written in the > datasheet?). Typo on my end, sorry. It's high 8bits and low 5/4 bits. Apart from that, the Chinese and English versions don't match for the battery charge current. :( Would it be possible for you to test this? As in, have the module running, and charging a battery, and have a multimeter in series with the battery to verify the readings. Regards ChenYu > > Hum.. from the reg reading function[2] I would say that the correct > formula is high on 8 bits and low on 4/5 bits. > > So hum.. what do we do? > > [1] http://dl.linux-sunxi.org/AXP/AXP223-en.pdf > [2] http://lxr.free-electrons.com/source/include/linux/mfd/axp20x.h#L564 > >>> >>> [...] >>>>> +static int axp22x_read_raw(struct iio_dev *indio_dev, >>>>> + struct iio_chan_spec const *chan, int *val, >>>>> + int *val2, long mask) >>>>> +{ >>>>> + switch (mask) { >>>>> + case IIO_CHAN_INFO_OFFSET: >>>>> + *val = -2667; >>>> >>>> Datasheet says -267.7 C, or -2677 here. >>>> >>> >>> The formula in the datasheet is (in milli Celsius): >>> processed = raw * 100 - 266700; >>> >>> while the IIO framework asks for a scale and an offset which are then >>> applied as: >>> processed = (raw + offset) * scale; >>> >>> Thus by factorizing, we get: >>> processed = (raw - 2667) * 100; >> >> What I meant was that your lower end value is off by one degree, >> -266.7 in your code vs -267.7 in the datasheet. >> > > Indeed. Thanks. > >>> >>> [...] >>>>> +static int axp20x_remove(struct platform_device *pdev) >>>>> +{ >>>>> + struct axp20x_adc_iio *info; >>>>> + struct iio_dev *indio_dev = platform_get_drvdata(pdev); >>>>> + >>>>> + info = iio_priv(indio_dev); >>>> >>>> Nit: you could just reverse the 2 declarations above and join this >>>> line after struct axp20x_adc_iio *info; >>>> >>>>> + regmap_write(info->regmap, AXP20X_ADC_EN1, 0); >>>>> + regmap_write(info->regmap, AXP20X_ADC_EN2, 0); >>>> >>>> The existing VBUS power supply driver enables the VBUS ADC bits itself, >>>> and does not check them later on. This means if one were to remove this >>>> axp20x-adc module, the voltage/current readings in the VBUS power supply >>>> would be invalid. Some sort of workaround would be needed here in this >>>> driver of the VBUS driver. >>>> >>> >>> That would be one reason to migrate the VBUS driver to use the IIO >>> channels, wouldn't it? >> >> It is, preferably without changing the device tree. >> > > Yes, of course. > > Thanks, > Quentin > > -- > Quentin Schulz, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com
On Thu, Jan 05, 2017 at 10:50:33AM +0100, Quentin Schulz wrote: > >>>> +static int axp20x_remove(struct platform_device *pdev) > >>>> +{ > >>>> + struct axp20x_adc_iio *info; > >>>> + struct iio_dev *indio_dev = platform_get_drvdata(pdev); > >>>> + > >>>> + info = iio_priv(indio_dev); > >>> > >>> Nit: you could just reverse the 2 declarations above and join this > >>> line after struct axp20x_adc_iio *info; > >>> > >>>> + regmap_write(info->regmap, AXP20X_ADC_EN1, 0); > >>>> + regmap_write(info->regmap, AXP20X_ADC_EN2, 0); > >>> > >>> The existing VBUS power supply driver enables the VBUS ADC bits itself, > >>> and does not check them later on. This means if one were to remove this > >>> axp20x-adc module, the voltage/current readings in the VBUS power supply > >>> would be invalid. Some sort of workaround would be needed here in this > >>> driver of the VBUS driver. > >>> > >> > >> That would be one reason to migrate the VBUS driver to use the IIO > >> channels, wouldn't it? > > > > It is, preferably without changing the device tree. > > > > Yes, of course. Note that you can have something like a test for the iio channel property in the VBUS driver, and keep the old behaviour in the case where we wouldn't have it. Maxime
On Mon, Jan 02, 2017 at 05:37:03PM +0100, Quentin Schulz wrote: > + switch (axp20x_id) { > + case AXP209_ID: > + indio_dev->info = &axp20x_adc_iio_info; > + indio_dev->num_channels = ARRAY_SIZE(axp20x_adc_channels); > + indio_dev->channels = axp20x_adc_channels; > + > + /* Enable the ADCs on IP */ > + regmap_write(info->regmap, AXP20X_ADC_EN1, AXP20X_ADC_EN1_MASK); > + > + /* Enable GPIO0/1 and internal temperature ADCs */ > + regmap_update_bits(info->regmap, AXP20X_ADC_EN2, > + AXP20X_ADC_EN2_MASK, AXP20X_ADC_EN2_MASK); > + > + /* Configure ADCs rate */ > + regmap_update_bits(info->regmap, AXP20X_ADC_RATE, > + AXP20X_ADC_RATE_MASK, AXP20X_ADC_RATE_50HZ); > + break; > + > + case AXP221_ID: > + indio_dev->info = &axp22x_adc_iio_info; > + indio_dev->num_channels = ARRAY_SIZE(axp22x_adc_channels); > + indio_dev->channels = axp22x_adc_channels; > + > + /* Enable the ADCs on IP */ > + regmap_write(info->regmap, AXP20X_ADC_EN1, AXP22X_ADC_EN1_MASK); > + > + /* Configure ADCs rate */ > + regmap_update_bits(info->regmap, AXP20X_ADC_RATE, > + AXP20X_ADC_RATE_MASK, AXP22X_ADC_RATE_200HZ); > + break; > + > + default: > + return -EINVAL; > + } I'm not a big fan of those IDs. It always ends up with a ever increasing list of cases without any consolidation. In this case, the only thing you need is a list of pointers and values that can definitely be stored in a structure. Maxime
On 02/01/17 11:37, Quentin Schulz wrote: > The X-Powers AXP20X and AXP22X PMICs have multiple ADCs. They expose the > battery voltage, battery charge and discharge currents, AC-in and VBUS > voltages and currents, 2 GPIOs muxable in ADC mode and PMIC temperature. > > This adds support for most of AXP20X and AXP22X ADCs. > > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> Couple of little bits from me. Peter got the bigger stuff. Jonathan > --- > drivers/iio/adc/Kconfig | 10 + > drivers/iio/adc/Makefile | 1 + > drivers/iio/adc/axp20x_adc.c | 490 +++++++++++++++++++++++++++++++++++++++++++ > include/linux/mfd/axp20x.h | 4 + > 4 files changed, 505 insertions(+) > create mode 100644 drivers/iio/adc/axp20x_adc.c > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index 38bc319..5c5b51f 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -154,6 +154,16 @@ config AT91_SAMA5D2_ADC > To compile this driver as a module, choose M here: the module will be > called at91-sama5d2_adc. > > +config AXP20X_ADC > + tristate "X-Powers AXP20X and AXP22X ADC driver" > + depends on MFD_AXP20X > + help > + Say yes here to have support for X-Powers power management IC (PMIC) > + AXP20X and AXP22X ADC devices. > + > + To compile this driver as a module, choose M here: the module will be > + called axp20x_adc. > + > config AXP288_ADC > tristate "X-Powers AXP288 ADC driver" > depends on MFD_AXP20X > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile > index d36c4be..f5c28a5 100644 > --- a/drivers/iio/adc/Makefile > +++ b/drivers/iio/adc/Makefile > @@ -16,6 +16,7 @@ obj-$(CONFIG_AD7887) += ad7887.o > obj-$(CONFIG_AD799X) += ad799x.o > obj-$(CONFIG_AT91_ADC) += at91_adc.o > obj-$(CONFIG_AT91_SAMA5D2_ADC) += at91-sama5d2_adc.o > +obj-$(CONFIG_AXP20X_ADC) += axp20x_adc.o > obj-$(CONFIG_AXP288_ADC) += axp288_adc.o > obj-$(CONFIG_BCM_IPROC_ADC) += bcm_iproc_adc.o > obj-$(CONFIG_BERLIN2_ADC) += berlin2-adc.o > diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c > new file mode 100644 > index 0000000..8df972a > --- /dev/null > +++ b/drivers/iio/adc/axp20x_adc.c > @@ -0,0 +1,490 @@ > +/* ADC driver for AXP20X and AXP22X PMICs > + * > + * Copyright (c) 2016 Free Electrons NextThing Co. > + * Quentin Schulz <quentin.schulz@free-electrons.com> > + * > + * This program is free software; you can redistribute it and/or modify it under > + * the terms of the GNU General Public License version 2 as published by the > + * Free Software Foundation. > + * Pet hate : why a blank line here? ;) I'm grumpy - my flight home is delayed. > + */ > + > +#include <linux/completion.h> > +#include <linux/interrupt.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > +#include <linux/regmap.h> > +#include <linux/thermal.h> > + > +#include <linux/iio/iio.h> > +#include <linux/mfd/axp20x.h> > + > +#define AXP20X_ADC_EN1_MASK GENMASK(7, 0) > + > +#define AXP20X_ADC_EN2_MASK (GENMASK(3, 2) | BIT(7)) > +#define AXP22X_ADC_EN1_MASK (GENMASK(7, 5) | BIT(0)) > +#define AXP20X_ADC_EN2_TEMP_ADC BIT(7) > +#define AXP20X_ADC_EN2_GPIO0_ADC BIT(3) > +#define AXP20X_ADC_EN2_GPIO1_ADC BIT(2) > + > +#define AXP20X_GPIO10_IN_RANGE_GPIO0 BIT(0) > +#define AXP20X_GPIO10_IN_RANGE_GPIO1 BIT(1) > +#define AXP20X_GPIO10_IN_RANGE_GPIO0_VAL(x) ((x) & BIT(0)) > +#define AXP20X_GPIO10_IN_RANGE_GPIO1_VAL(x) (((x) & BIT(0)) << 1) > + > +#define AXP20X_ADC_RATE_MASK (3 << 6) > +#define AXP20X_ADC_RATE_25HZ (0 << 6) > +#define AXP20X_ADC_RATE_50HZ BIT(6) > +#define AXP20X_ADC_RATE_100HZ (2 << 6) > +#define AXP20X_ADC_RATE_200HZ (3 << 6) > + > +#define AXP22X_ADC_RATE_100HZ (0 << 6) > +#define AXP22X_ADC_RATE_200HZ BIT(6) > +#define AXP22X_ADC_RATE_400HZ (2 << 6) > +#define AXP22X_ADC_RATE_800HZ (3 << 6) > + > +#define AXP20X_ADC_CHANNEL(_channel, _name, _type, _reg) \ > + { \ > + .type = _type, \ > + .indexed = 1, \ > + .channel = _channel, \ > + .address = _reg, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ > + BIT(IIO_CHAN_INFO_SCALE), \ > + .datasheet_name = _name, \ > + } > + > +#define AXP20X_ADC_CHANNEL_OFFSET(_channel, _name, _type, _reg) \ > + { \ > + .type = _type, \ > + .indexed = 1, \ > + .channel = _channel, \ > + .address = _reg, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ > + BIT(IIO_CHAN_INFO_SCALE) |\ > + BIT(IIO_CHAN_INFO_OFFSET),\ > + .datasheet_name = _name, \ > + } > + > +struct axp20x_adc_iio { Why? I'm not seeing this used anywhere. > + struct iio_dev *indio_dev; > + struct regmap *regmap; > +}; > + > +enum axp20x_adc_channel { > + AXP20X_ACIN_V = 0, > + AXP20X_ACIN_I, > + AXP20X_VBUS_V, > + AXP20X_VBUS_I, > + AXP20X_TEMP_ADC, > + AXP20X_GPIO0_V, > + AXP20X_GPIO1_V, > + AXP20X_BATT_V, > + AXP20X_BATT_CHRG_I, > + AXP20X_BATT_DISCHRG_I, > + AXP20X_IPSOUT_V, > +}; > + > +enum axp22x_adc_channel { > + AXP22X_TEMP_ADC = 0, > + AXP22X_BATT_V, > + AXP22X_BATT_CHRG_I, > + AXP22X_BATT_DISCHRG_I, > +}; > + > +static const struct iio_chan_spec axp20x_adc_channels[] = { > + AXP20X_ADC_CHANNEL(AXP20X_ACIN_V, "acin_v", IIO_VOLTAGE, > + AXP20X_ACIN_V_ADC_H), > + AXP20X_ADC_CHANNEL(AXP20X_ACIN_I, "acin_i", IIO_CURRENT, > + AXP20X_ACIN_I_ADC_H), > + AXP20X_ADC_CHANNEL(AXP20X_VBUS_V, "vbus_v", IIO_VOLTAGE, > + AXP20X_VBUS_V_ADC_H), > + AXP20X_ADC_CHANNEL(AXP20X_VBUS_I, "vbus_i", IIO_CURRENT, > + AXP20X_VBUS_I_ADC_H), > + AXP20X_ADC_CHANNEL_OFFSET(AXP20X_TEMP_ADC, "temp_adc", IIO_TEMP, > + AXP20X_TEMP_ADC_H), > + AXP20X_ADC_CHANNEL_OFFSET(AXP20X_GPIO0_V, "gpio0_v", IIO_VOLTAGE, > + AXP20X_GPIO0_V_ADC_H), > + AXP20X_ADC_CHANNEL_OFFSET(AXP20X_GPIO1_V, "gpio1_v", IIO_VOLTAGE, > + AXP20X_GPIO1_V_ADC_H), > + AXP20X_ADC_CHANNEL(AXP20X_BATT_V, "batt_v", IIO_VOLTAGE, > + AXP20X_BATT_V_H), > + AXP20X_ADC_CHANNEL(AXP20X_BATT_CHRG_I, "batt_chrg_i", IIO_CURRENT, > + AXP20X_BATT_CHRG_I_H), > + AXP20X_ADC_CHANNEL(AXP20X_BATT_DISCHRG_I, "batt_dischrg_i", IIO_CURRENT, > + AXP20X_BATT_DISCHRG_I_H), > + AXP20X_ADC_CHANNEL(AXP20X_IPSOUT_V, "ipsout_v", IIO_VOLTAGE, > + AXP20X_IPSOUT_V_HIGH_H), > +}; > + > +static const struct iio_chan_spec axp22x_adc_channels[] = { > + AXP20X_ADC_CHANNEL_OFFSET(AXP22X_TEMP_ADC, "temp_adc", IIO_TEMP, > + AXP22X_TEMP_ADC_H), > + AXP20X_ADC_CHANNEL(AXP22X_BATT_V, "batt_v", IIO_VOLTAGE, > + AXP20X_BATT_V_H), > + AXP20X_ADC_CHANNEL(AXP22X_BATT_CHRG_I, "batt_chrg_i", IIO_CURRENT, > + AXP20X_BATT_CHRG_I_H), > + AXP20X_ADC_CHANNEL(AXP22X_BATT_DISCHRG_I, "batt_dischrg_i", IIO_CURRENT, > + AXP20X_BATT_DISCHRG_I_H), > +}; > + > +static int axp20x_adc_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *channel, int *val, > + int *val2) > +{ > + struct axp20x_adc_iio *info = iio_priv(indio_dev); > + int size = 12, ret; > + > + switch (channel->channel) { > + case AXP20X_BATT_DISCHRG_I: > + size = 13; > + case AXP20X_ACIN_V: > + case AXP20X_ACIN_I: > + case AXP20X_VBUS_V: > + case AXP20X_VBUS_I: > + case AXP20X_TEMP_ADC: > + case AXP20X_BATT_V: > + case AXP20X_BATT_CHRG_I: > + case AXP20X_IPSOUT_V: > + case AXP20X_GPIO0_V: > + case AXP20X_GPIO1_V: > + ret = axp20x_read_variable_width(info->regmap, channel->address, > + size); > + if (ret < 0) > + return ret; > + *val = ret; > + return IIO_VAL_INT; > + > + default: > + return -EINVAL; > + } > + > + return -EINVAL; > +} > + > +static int axp22x_adc_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *channel, int *val, > + int *val2) > +{ > + struct axp20x_adc_iio *info = iio_priv(indio_dev); > + int size = 12, ret; > + > + switch (channel->channel) { > + case AXP22X_BATT_DISCHRG_I: > + size = 13; > + case AXP22X_TEMP_ADC: > + case AXP22X_BATT_V: > + case AXP22X_BATT_CHRG_I: > + ret = axp20x_read_variable_width(info->regmap, channel->address, > + size); > + if (ret < 0) > + return ret; > + *val = ret; > + return IIO_VAL_INT; > + > + default: > + return -EINVAL; > + } > + > + return -EINVAL; > +} > + > +static int axp20x_adc_scale(int channel, int *val, int *val2) > +{ > + switch (channel) { > + case AXP20X_ACIN_V: > + case AXP20X_VBUS_V: > + *val = 1; > + *val2 = 700000; > + return IIO_VAL_INT_PLUS_MICRO; > + > + case AXP20X_ACIN_I: > + *val = 0; > + *val2 = 625000; > + return IIO_VAL_INT_PLUS_MICRO; > + > + case AXP20X_VBUS_I: > + *val = 0; > + *val2 = 375000; > + return IIO_VAL_INT_PLUS_MICRO; > + > + case AXP20X_TEMP_ADC: > + *val = 100; > + return IIO_VAL_INT; > + > + case AXP20X_GPIO0_V: > + case AXP20X_GPIO1_V: > + *val = 0; > + *val2 = 500000; > + return IIO_VAL_INT_PLUS_MICRO; > + > + case AXP20X_BATT_V: > + *val = 1; > + *val2 = 100000; > + return IIO_VAL_INT_PLUS_MICRO; > + > + case AXP20X_BATT_DISCHRG_I: > + case AXP20X_BATT_CHRG_I: > + *val = 0; > + *val2 = 500000; > + return IIO_VAL_INT_PLUS_MICRO; > + > + case AXP20X_IPSOUT_V: > + *val = 1; > + *val2 = 400000; > + return IIO_VAL_INT_PLUS_MICRO; > + > + default: > + return -EINVAL; > + } > + > + return -EINVAL; > +} > + > +static int axp22x_adc_scale(int channel, int *val, int *val2) > +{ > + switch (channel) { > + case AXP22X_TEMP_ADC: > + *val = 100; > + return IIO_VAL_INT; > + > + case AXP22X_BATT_V: > + *val = 1; > + *val2 = 100000; > + return IIO_VAL_INT_PLUS_MICRO; > + > + case AXP22X_BATT_DISCHRG_I: > + case AXP22X_BATT_CHRG_I: > + *val = 0; > + *val2 = 500000; > + return IIO_VAL_INT_PLUS_MICRO; > + > + default: > + return -EINVAL; > + } > + > + return -EINVAL; > +} > + > +static int axp20x_adc_offset(struct iio_dev *indio_dev, int channel, int *val) > +{ > + struct axp20x_adc_iio *info = iio_priv(indio_dev); > + int ret, reg; > + > + switch (channel) { > + case AXP20X_TEMP_ADC: > + *val = -1447; > + return IIO_VAL_INT; > + > + case AXP20X_GPIO0_V: > + case AXP20X_GPIO1_V: > + ret = regmap_read(info->regmap, AXP20X_GPIO10_IN_RANGE, ®); > + if (ret < 0) > + return ret; > + > + if (channel == AXP20X_GPIO0_V) > + *val = reg & AXP20X_GPIO10_IN_RANGE_GPIO0; > + else > + *val = reg & AXP20X_GPIO10_IN_RANGE_GPIO1; > + > + *val = !!(*val) * 700000; > + > + return IIO_VAL_INT; > + > + default: > + return -EINVAL; > + } > + > + return -EINVAL; > +} > + > +static int axp20x_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, int *val, > + int *val2, long mask) > +{ > + switch (mask) { > + case IIO_CHAN_INFO_OFFSET: > + return axp20x_adc_offset(indio_dev, chan->channel, val); > + > + case IIO_CHAN_INFO_SCALE: > + return axp20x_adc_scale(chan->channel, val, val2); > + > + case IIO_CHAN_INFO_RAW: > + return axp20x_adc_read_raw(indio_dev, chan, val, val2); > + > + default: > + return -EINVAL; > + } > + > + return -EINVAL; > +} > + > +static int axp22x_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, int *val, > + int *val2, long mask) > +{ > + switch (mask) { > + case IIO_CHAN_INFO_OFFSET: > + *val = -2667; > + return IIO_VAL_INT; > + > + case IIO_CHAN_INFO_SCALE: > + return axp22x_adc_scale(chan->channel, val, val2); > + > + case IIO_CHAN_INFO_RAW: > + return axp22x_adc_read_raw(indio_dev, chan, val, val2); > + > + default: > + return -EINVAL; > + } > + > + return -EINVAL; > +} > + > +static int axp20x_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, int val, int val2, > + long mask) > +{ > + struct axp20x_adc_iio *info = iio_priv(indio_dev); > + > + /* > + * The AXP20X PMIC allows the user to choose between 0V and 0.7V offsets > + * for (independently) GPIO0 and GPIO1 when in ADC mode. > + */ > + if (mask != IIO_CHAN_INFO_OFFSET) > + return -EINVAL; > + > + if (chan->channel != AXP20X_GPIO0_V && chan->channel != AXP20X_GPIO1_V) > + return -EINVAL; > + > + if (val != 0 && val != 700000) > + return -EINVAL; > + > + if (chan->channel == AXP20X_GPIO0_V) > + return regmap_update_bits(info->regmap, AXP20X_GPIO10_IN_RANGE, > + AXP20X_GPIO10_IN_RANGE_GPIO0, > + AXP20X_GPIO10_IN_RANGE_GPIO0_VAL(!!val)); > + > + return regmap_update_bits(info->regmap, AXP20X_GPIO10_IN_RANGE, > + AXP20X_GPIO10_IN_RANGE_GPIO1, > + AXP20X_GPIO10_IN_RANGE_GPIO1_VAL(!!val)); > +} > + > +static const struct iio_info axp20x_adc_iio_info = { > + .read_raw = axp20x_read_raw, > + .write_raw = axp20x_write_raw, > + .driver_module = THIS_MODULE, > +}; > + > +static const struct iio_info axp22x_adc_iio_info = { > + .read_raw = axp22x_read_raw, > + .driver_module = THIS_MODULE, > +}; > + > +static const struct of_device_id axp20x_adc_of_match[] = { > + { .compatible = "x-powers,axp209-adc", .data = (void *)AXP209_ID, }, > + { .compatible = "x-powers,axp221-adc", .data = (void *)AXP221_ID, }, > + { /* sentinel */ }, > +}; > + > +static int axp20x_probe(struct platform_device *pdev) > +{ > + struct axp20x_adc_iio *info; > + struct iio_dev *indio_dev; > + struct axp20x_dev *axp20x_dev; > + int ret, axp20x_id; > + > + axp20x_dev = dev_get_drvdata(pdev->dev.parent); > + > + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info)); > + if (!indio_dev) > + return -ENOMEM; > + > + info = iio_priv(indio_dev); > + platform_set_drvdata(pdev, indio_dev); > + > + info->regmap = axp20x_dev->regmap; > + info->indio_dev = indio_dev; > + indio_dev->name = dev_name(&pdev->dev); > + indio_dev->dev.parent = &pdev->dev; > + indio_dev->dev.of_node = pdev->dev.of_node; > + indio_dev->modes = INDIO_DIRECT_MODE; > + > + axp20x_id = (int)of_device_get_match_data(&pdev->dev); > + > + switch (axp20x_id) { > + case AXP209_ID: > + indio_dev->info = &axp20x_adc_iio_info; > + indio_dev->num_channels = ARRAY_SIZE(axp20x_adc_channels); > + indio_dev->channels = axp20x_adc_channels; > + > + /* Enable the ADCs on IP */ > + regmap_write(info->regmap, AXP20X_ADC_EN1, AXP20X_ADC_EN1_MASK); > + > + /* Enable GPIO0/1 and internal temperature ADCs */ > + regmap_update_bits(info->regmap, AXP20X_ADC_EN2, > + AXP20X_ADC_EN2_MASK, AXP20X_ADC_EN2_MASK); > + > + /* Configure ADCs rate */ > + regmap_update_bits(info->regmap, AXP20X_ADC_RATE, > + AXP20X_ADC_RATE_MASK, AXP20X_ADC_RATE_50HZ); > + break; > + > + case AXP221_ID: > + indio_dev->info = &axp22x_adc_iio_info; > + indio_dev->num_channels = ARRAY_SIZE(axp22x_adc_channels); > + indio_dev->channels = axp22x_adc_channels; > + > + /* Enable the ADCs on IP */ > + regmap_write(info->regmap, AXP20X_ADC_EN1, AXP22X_ADC_EN1_MASK); > + > + /* Configure ADCs rate */ > + regmap_update_bits(info->regmap, AXP20X_ADC_RATE, > + AXP20X_ADC_RATE_MASK, AXP22X_ADC_RATE_200HZ); > + break; > + > + default: > + return -EINVAL; > + } > + > + ret = devm_iio_device_register(&pdev->dev, indio_dev); > + if (ret < 0) { > + dev_err(&pdev->dev, "could not register the device\n"); > + regmap_write(info->regmap, AXP20X_ADC_EN1, 0); > + regmap_write(info->regmap, AXP20X_ADC_EN2, 0); > + return ret; > + } > + > + return 0; > +} > + > +static int axp20x_remove(struct platform_device *pdev) > +{ > + struct axp20x_adc_iio *info; > + struct iio_dev *indio_dev = platform_get_drvdata(pdev); > + > + info = iio_priv(indio_dev); > + regmap_write(info->regmap, AXP20X_ADC_EN1, 0); > + regmap_write(info->regmap, AXP20X_ADC_EN2, 0); > + > + return 0; > +} > + > +static struct platform_driver axp20x_adc_driver = { > + .driver = { > + .name = "axp20x-adc", > + .of_match_table = axp20x_adc_of_match, > + }, > + .probe = axp20x_probe, > + .remove = axp20x_remove, > +}; > + > +module_platform_driver(axp20x_adc_driver); > + > +MODULE_DESCRIPTION("ADC driver for AXP20X and AXP22X PMICs"); > +MODULE_AUTHOR("Quentin Schulz <quentin.schulz@free-electrons.com>"); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h > index a4860bc..650c6f6 100644 > --- a/include/linux/mfd/axp20x.h > +++ b/include/linux/mfd/axp20x.h > @@ -150,6 +150,10 @@ enum { > #define AXP20X_VBUS_I_ADC_L 0x5d > #define AXP20X_TEMP_ADC_H 0x5e > #define AXP20X_TEMP_ADC_L 0x5f > + > +#define AXP22X_TEMP_ADC_H 0x56 > +#define AXP22X_TEMP_ADC_L 0x57 > + > #define AXP20X_TS_IN_H 0x62 > #define AXP20X_TS_IN_L 0x63 > #define AXP20X_GPIO0_V_ADC_H 0x64 >
On 05/01/17 04:50, Quentin Schulz wrote: > On 05/01/2017 09:27, Chen-Yu Tsai wrote: >> On Thu, Jan 5, 2017 at 4:06 PM, Quentin Schulz >> <quentin.schulz@free-electrons.com> wrote: >>> Hi Chen-Yu, >>> >>> On 05/01/2017 06:42, Chen-Yu Tsai wrote: >>>> On Tue, Jan 3, 2017 at 12:37 AM, Quentin Schulz >>>> <quentin.schulz@free-electrons.com> wrote: >>> [...] >>>>> + >>>>> +#define AXP20X_ADC_RATE_MASK (3 << 6) >>>>> +#define AXP20X_ADC_RATE_25HZ (0 << 6) >>>>> +#define AXP20X_ADC_RATE_50HZ BIT(6) >>>> >>>> Please be consistent with the format. >>>> >>>>> +#define AXP20X_ADC_RATE_100HZ (2 << 6) >>>>> +#define AXP20X_ADC_RATE_200HZ (3 << 6) >>>>> + >>>>> +#define AXP22X_ADC_RATE_100HZ (0 << 6) >>>>> +#define AXP22X_ADC_RATE_200HZ BIT(6) >>>>> +#define AXP22X_ADC_RATE_400HZ (2 << 6) >>>>> +#define AXP22X_ADC_RATE_800HZ (3 << 6) >>>> >>>> These are power-of-2 multiples of some base rate. May I suggest >>>> a formula macro instead. Either way, you seem to be using only >>>> one value. Will this be made configurable in the future? >>>> >>> >>> Yes, I could use a formula macro instead. No plan to make it >>> configurable, should I make it configurable? >> >> I don't see a use case for that atm. >> >>>>> + >>>>> +#define AXP20X_ADC_CHANNEL(_channel, _name, _type, _reg) \ >>>>> + { \ >>>>> + .type = _type, \ >>>>> + .indexed = 1, \ >>>>> + .channel = _channel, \ >>>>> + .address = _reg, \ >>>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ >>>>> + BIT(IIO_CHAN_INFO_SCALE), \ >>>>> + .datasheet_name = _name, \ >>>>> + } >>>>> + >>>>> +#define AXP20X_ADC_CHANNEL_OFFSET(_channel, _name, _type, _reg) \ >>>>> + { \ >>>>> + .type = _type, \ >>>>> + .indexed = 1, \ >>>>> + .channel = _channel, \ >>>>> + .address = _reg, \ >>>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ >>>>> + BIT(IIO_CHAN_INFO_SCALE) |\ >>>>> + BIT(IIO_CHAN_INFO_OFFSET),\ >>>>> + .datasheet_name = _name, \ >>>>> + } >>>>> + >>>>> +struct axp20x_adc_iio { >>>>> + struct iio_dev *indio_dev; >>>>> + struct regmap *regmap; >>>>> +}; >>>>> + >>>>> +enum axp20x_adc_channel { >>>>> + AXP20X_ACIN_V = 0, >>>>> + AXP20X_ACIN_I, >>>>> + AXP20X_VBUS_V, >>>>> + AXP20X_VBUS_I, >>>>> + AXP20X_TEMP_ADC, >>>> >>>> PMIC_TEMP would be better. And please save a slot for TS input. >>>> >>> >>> ACK. >>> >>> Hum.. I'm wondering what should be the IIO type of the TS input channel >>> then? The TS Pin can be used in two modes: either to monitor the >>> temperature of the battery or as an external ADC, at least that's what I >>> understand from the datasheet. >> >> AFAIK the battery charge/discharge high/low temperature threshold >> registers take values in terms of voltage, not actual temperature. >> And the temperature readout kind of depends on the thermoresistor >> one is using. So I think "voltage" would be the proper type. >> > > ACK. Should I just add TS_IN in axp20x_adc_channel enum but not add it > in the exposed IIO channels ("saving" the slot but not using it)? Ideally we'd put an IIO consumer driver on the channel that handles the thermoresistor explicitly. Would need consumer support for events though to be much use. > >>> >>>>> + AXP20X_GPIO0_V, >>>>> + AXP20X_GPIO1_V, >>>> >>>> Please skip a slot for "battery instantaneous power". >>>> >>>>> + AXP20X_BATT_V, >>>>> + AXP20X_BATT_CHRG_I, >>>>> + AXP20X_BATT_DISCHRG_I, >>>>> + AXP20X_IPSOUT_V, >>>>> +}; >>>>> + >>>>> +enum axp22x_adc_channel { >>>>> + AXP22X_TEMP_ADC = 0, >>>> >>>> Same comments as AXP20X_TEMP_ADC. >>>> >>>>> + AXP22X_BATT_V, >>>>> + AXP22X_BATT_CHRG_I, >>>>> + AXP22X_BATT_DISCHRG_I, >>>>> +}; >>>> >>>> Shouldn't these channel numbers be exported as part of the device tree >>>> bindings? At the very least, they shouldn't be changed. >>>> >>> >>> I don't understand what you mean by that. Do you mean you want a >>> consistent numbering between the AXP20X and the AXP22X, so that >>> AXP22X_BATT_V would have the same channel number than AXP20X_BATT_V? >>> >>> Could you explain a bit more your thoughts on the channel numbers being >>> exported as part of the device tree bindings? >> >> What I meant was that, since you are referencing the channels in the >> device tree, the numbering scheme would be part of the device tree >> binding, and should never be changed. So either these would be macros >> in include/dt-bindings/, or a big warning should be put before it. >> > > ACK. > >> But see my reply on patch 7, about do we actually need to expose this >> in the device tree. >> > > I don't know what's the best. > >>>> Also please add a comment saying that the channels are numbered >>>> in the order of their respective registers, and not the table >>>> describing the ADCs in the datasheet (9.7 Signal Capture for AXP209 >>>> and 9.5 E-Gauge for AXP221). >>>> >>> >>> Yes I can. >>> >>> What about Rob wanting channel numbers to start at zero for each >>> different IIO type (i.e., today we have AXP22X_BATT_CHRG_I being >>> exported as in_current1_raw whereas he wants in_current0_raw). >> >> Hmm... I missed this. Are you talking about IIO or hwmon? IIRC >> hwmon numbers things starting at 1. >> > > About IIO. > > Today, we have exposed: > in_voltage0_raw for acin_v > in_current1_raw for acin_i > in_voltage2_raw for vbus_v > in_current3_raw for vbus_i > in_temp4_raw for adc_temp > in_voltage5_raw for gpio0_v > in_voltage6_raw for gpio1_v > in_voltage7_raw for batt_v > in_current8_raw for batt_chrg_i > in_current9_raw for batt_dischrg_i > in_voltage10_raw for ipsout_v > > but I think what Rob wants is: > > in_voltage0_raw acin_v > in_current0_raw for acin_i > in_voltage1_raw for vbus_v > in_current1_raw for vbus_i > in_temp_raw for adc_temp > in_voltage2_raw for gpio0_v > in_voltage3_raw for gpio1_v > in_voltage4_raw for batt_v > in_current2_raw for batt_chrg_i > in_current3_raw for batt_dischrg_i > in_voltage5_raw for ipsout_v That would be standard choice. It's not disastrous to skip numbers but it tends to not be what userspace expects. > >>> [...] >>>>> +static int axp22x_adc_read_raw(struct iio_dev *indio_dev, >>>>> + struct iio_chan_spec const *channel, int *val, >>>>> + int *val2) >>>>> +{ >>>>> + struct axp20x_adc_iio *info = iio_priv(indio_dev); >>>>> + int size = 12, ret; >>>>> + >>>>> + switch (channel->channel) { >>>>> + case AXP22X_BATT_DISCHRG_I: >>>>> + size = 13; >>>>> + case AXP22X_TEMP_ADC: >>>>> + case AXP22X_BATT_V: >>>>> + case AXP22X_BATT_CHRG_I: >>>> >>>> According to the datasheet, AXP22X_BATT_CHRG_I is also 13 bits wide. >>>> >>> >>> Where did you get that? >>> >>> Also, the datasheet is inconsistent: >>> - 9.5 E-Gauge Fuel Gauge system => the min value is at 0x0 and the max >>> value at 0xfff for all channels, that's 12 bits. >>> - 10.1.4 ADC Data => all channels except battery discharge current are >>> on 12 bits (8 high, 4 low). >> >> My datasheets (AXP221 v1.6, AXP221s v1.2, AXP223 v1.1, all Chinese) say >> in 10.1.4: >> >> - 7A: battery charge current high 5 bits >> - 7B: battery charge current low 8 bits >> - 7C: battery discharge current high 5 bits >> - 7D: battery discharge current low 8 bits >> > > AXP223 v1.1 in English in 10.1.4[1]: > - 7A: battery charge current high 8 bits > - 7B: battery charge current low 4 bits > - 7C: battery discharge current high 8 bits > - 7D: battery discharge current low 5 bits > > Note that it's 8 bits for high and 4/5 bits for low while you wrote 4/5 > bits high and low 8 bits (typos or actually what's written in the > datasheet?). > > Hum.. from the reg reading function[2] I would say that the correct > formula is high on 8 bits and low on 4/5 bits. > > So hum.. what do we do? > > [1] http://dl.linux-sunxi.org/AXP/AXP223-en.pdf > [2] http://lxr.free-electrons.com/source/include/linux/mfd/axp20x.h#L564 > >>> >>> [...] >>>>> +static int axp22x_read_raw(struct iio_dev *indio_dev, >>>>> + struct iio_chan_spec const *chan, int *val, >>>>> + int *val2, long mask) >>>>> +{ >>>>> + switch (mask) { >>>>> + case IIO_CHAN_INFO_OFFSET: >>>>> + *val = -2667; >>>> >>>> Datasheet says -267.7 C, or -2677 here. >>>> >>> >>> The formula in the datasheet is (in milli Celsius): >>> processed = raw * 100 - 266700; >>> >>> while the IIO framework asks for a scale and an offset which are then >>> applied as: >>> processed = (raw + offset) * scale; >>> >>> Thus by factorizing, we get: >>> processed = (raw - 2667) * 100; >> >> What I meant was that your lower end value is off by one degree, >> -266.7 in your code vs -267.7 in the datasheet. >> > > Indeed. Thanks. > >>> >>> [...] >>>>> +static int axp20x_remove(struct platform_device *pdev) >>>>> +{ >>>>> + struct axp20x_adc_iio *info; >>>>> + struct iio_dev *indio_dev = platform_get_drvdata(pdev); >>>>> + >>>>> + info = iio_priv(indio_dev); >>>> >>>> Nit: you could just reverse the 2 declarations above and join this >>>> line after struct axp20x_adc_iio *info; >>>> >>>>> + regmap_write(info->regmap, AXP20X_ADC_EN1, 0); >>>>> + regmap_write(info->regmap, AXP20X_ADC_EN2, 0); >>>> >>>> The existing VBUS power supply driver enables the VBUS ADC bits itself, >>>> and does not check them later on. This means if one were to remove this >>>> axp20x-adc module, the voltage/current readings in the VBUS power supply >>>> would be invalid. Some sort of workaround would be needed here in this >>>> driver of the VBUS driver. >>>> >>> >>> That would be one reason to migrate the VBUS driver to use the IIO >>> channels, wouldn't it? >> >> It is, preferably without changing the device tree. >> > > Yes, of course. > > Thanks, > Quentin >
On 05/01/17 05:28, Chen-Yu Tsai wrote: > On Thu, Jan 5, 2017 at 5:50 PM, Quentin Schulz > <quentin.schulz@free-electrons.com> wrote: >> On 05/01/2017 09:27, Chen-Yu Tsai wrote: >>> On Thu, Jan 5, 2017 at 4:06 PM, Quentin Schulz >>> <quentin.schulz@free-electrons.com> wrote: >>>> Hi Chen-Yu, >>>> >>>> On 05/01/2017 06:42, Chen-Yu Tsai wrote: >>>>> On Tue, Jan 3, 2017 at 12:37 AM, Quentin Schulz >>>>> <quentin.schulz@free-electrons.com> wrote: >>>> [...] >>>>>> + >>>>>> +#define AXP20X_ADC_RATE_MASK (3 << 6) >>>>>> +#define AXP20X_ADC_RATE_25HZ (0 << 6) >>>>>> +#define AXP20X_ADC_RATE_50HZ BIT(6) >>>>> >>>>> Please be consistent with the format. >>>>> >>>>>> +#define AXP20X_ADC_RATE_100HZ (2 << 6) >>>>>> +#define AXP20X_ADC_RATE_200HZ (3 << 6) >>>>>> + >>>>>> +#define AXP22X_ADC_RATE_100HZ (0 << 6) >>>>>> +#define AXP22X_ADC_RATE_200HZ BIT(6) >>>>>> +#define AXP22X_ADC_RATE_400HZ (2 << 6) >>>>>> +#define AXP22X_ADC_RATE_800HZ (3 << 6) >>>>> >>>>> These are power-of-2 multiples of some base rate. May I suggest >>>>> a formula macro instead. Either way, you seem to be using only >>>>> one value. Will this be made configurable in the future? >>>>> >>>> >>>> Yes, I could use a formula macro instead. No plan to make it >>>> configurable, should I make it configurable? >>> >>> I don't see a use case for that atm. >>> >>>>>> + >>>>>> +#define AXP20X_ADC_CHANNEL(_channel, _name, _type, _reg) \ >>>>>> + { \ >>>>>> + .type = _type, \ >>>>>> + .indexed = 1, \ >>>>>> + .channel = _channel, \ >>>>>> + .address = _reg, \ >>>>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ >>>>>> + BIT(IIO_CHAN_INFO_SCALE), \ >>>>>> + .datasheet_name = _name, \ >>>>>> + } >>>>>> + >>>>>> +#define AXP20X_ADC_CHANNEL_OFFSET(_channel, _name, _type, _reg) \ >>>>>> + { \ >>>>>> + .type = _type, \ >>>>>> + .indexed = 1, \ >>>>>> + .channel = _channel, \ >>>>>> + .address = _reg, \ >>>>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ >>>>>> + BIT(IIO_CHAN_INFO_SCALE) |\ >>>>>> + BIT(IIO_CHAN_INFO_OFFSET),\ >>>>>> + .datasheet_name = _name, \ >>>>>> + } >>>>>> + >>>>>> +struct axp20x_adc_iio { >>>>>> + struct iio_dev *indio_dev; >>>>>> + struct regmap *regmap; >>>>>> +}; >>>>>> + >>>>>> +enum axp20x_adc_channel { >>>>>> + AXP20X_ACIN_V = 0, >>>>>> + AXP20X_ACIN_I, >>>>>> + AXP20X_VBUS_V, >>>>>> + AXP20X_VBUS_I, >>>>>> + AXP20X_TEMP_ADC, >>>>> >>>>> PMIC_TEMP would be better. And please save a slot for TS input. >>>>> >>>> >>>> ACK. >>>> >>>> Hum.. I'm wondering what should be the IIO type of the TS input channel >>>> then? The TS Pin can be used in two modes: either to monitor the >>>> temperature of the battery or as an external ADC, at least that's what I >>>> understand from the datasheet. >>> >>> AFAIK the battery charge/discharge high/low temperature threshold >>> registers take values in terms of voltage, not actual temperature. >>> And the temperature readout kind of depends on the thermoresistor >>> one is using. So I think "voltage" would be the proper type. >>> >> >> ACK. Should I just add TS_IN in axp20x_adc_channel enum but not add it >> in the exposed IIO channels ("saving" the slot but not using it)? > > Sure. Or you could skip the number with > > AXP20X_GPIO0_V = 6, > >>>> >>>>>> + AXP20X_GPIO0_V, >>>>>> + AXP20X_GPIO1_V, >>>>> >>>>> Please skip a slot for "battery instantaneous power". >>>>> >>>>>> + AXP20X_BATT_V, >>>>>> + AXP20X_BATT_CHRG_I, >>>>>> + AXP20X_BATT_DISCHRG_I, >>>>>> + AXP20X_IPSOUT_V, >>>>>> +}; >>>>>> + >>>>>> +enum axp22x_adc_channel { >>>>>> + AXP22X_TEMP_ADC = 0, >>>>> >>>>> Same comments as AXP20X_TEMP_ADC. >>>>> >>>>>> + AXP22X_BATT_V, >>>>>> + AXP22X_BATT_CHRG_I, >>>>>> + AXP22X_BATT_DISCHRG_I, >>>>>> +}; >>>>> >>>>> Shouldn't these channel numbers be exported as part of the device tree >>>>> bindings? At the very least, they shouldn't be changed. >>>>> >>>> >>>> I don't understand what you mean by that. Do you mean you want a >>>> consistent numbering between the AXP20X and the AXP22X, so that >>>> AXP22X_BATT_V would have the same channel number than AXP20X_BATT_V? >>>> >>>> Could you explain a bit more your thoughts on the channel numbers being >>>> exported as part of the device tree bindings? >>> >>> What I meant was that, since you are referencing the channels in the >>> device tree, the numbering scheme would be part of the device tree >>> binding, and should never be changed. So either these would be macros >>> in include/dt-bindings/, or a big warning should be put before it. >>> >> >> ACK. >> >>> But see my reply on patch 7, about do we actually need to expose this >>> in the device tree. >>> >> >> I don't know what's the best. > > Then let's not expose it in the device tree for now. It's easier to > add it later than to remove it. > >> >>>>> Also please add a comment saying that the channels are numbered >>>>> in the order of their respective registers, and not the table >>>>> describing the ADCs in the datasheet (9.7 Signal Capture for AXP209 >>>>> and 9.5 E-Gauge for AXP221). >>>>> >>>> >>>> Yes I can. >>>> >>>> What about Rob wanting channel numbers to start at zero for each >>>> different IIO type (i.e., today we have AXP22X_BATT_CHRG_I being >>>> exported as in_current1_raw whereas he wants in_current0_raw). >>> >>> Hmm... I missed this. Are you talking about IIO or hwmon? IIRC >>> hwmon numbers things starting at 1. >>> >> >> About IIO. >> >> Today, we have exposed: >> in_voltage0_raw for acin_v >> in_current1_raw for acin_i >> in_voltage2_raw for vbus_v >> in_current3_raw for vbus_i >> in_temp4_raw for adc_temp >> in_voltage5_raw for gpio0_v >> in_voltage6_raw for gpio1_v >> in_voltage7_raw for batt_v >> in_current8_raw for batt_chrg_i >> in_current9_raw for batt_dischrg_i >> in_voltage10_raw for ipsout_v >> >> but I think what Rob wants is: >> >> in_voltage0_raw acin_v >> in_current0_raw for acin_i >> in_voltage1_raw for vbus_v >> in_current1_raw for vbus_i >> in_temp_raw for adc_temp >> in_voltage2_raw for gpio0_v >> in_voltage3_raw for gpio1_v >> in_voltage4_raw for batt_v >> in_current2_raw for batt_chrg_i >> in_current3_raw for batt_dischrg_i >> in_voltage5_raw for ipsout_v > > I think that's doable. If I understand IIO code correctly, the channel > number is not used anywhere in the core code for dereferencing. It's > used for sysfs entries (when .indexed is set) and events. However > the twl4030 and twl6030 drivers use our current exposed scheme. > I suppose its best to get some input from the IIO maintainer. If we are doing it to superficially group channels that are measuring different things about the same physical system then I'm fine with mappings with holes in them. It's not actually specified anywhere that we can't allow this and IIRC there are a few drivers doing exactly this. > > So if we want, we could have the following channel mapping: > > 0 -> acin > 1 -> vbus > 2 -> pmic > 3 -> gpio0 > 4 -> gpio1 > 5 -> ipsout > 6 -> battery > > Each channel could have mulitple entries in axp20x_adc_channels[], > one for each type of reading. You might need multiple channels for > the battery to cover charge and discharge currents. > > Your callback functions would get a bit messier though. > >> >>>> [...] >>>>>> +static int axp22x_adc_read_raw(struct iio_dev *indio_dev, >>>>>> + struct iio_chan_spec const *channel, int *val, >>>>>> + int *val2) >>>>>> +{ >>>>>> + struct axp20x_adc_iio *info = iio_priv(indio_dev); >>>>>> + int size = 12, ret; >>>>>> + >>>>>> + switch (channel->channel) { >>>>>> + case AXP22X_BATT_DISCHRG_I: >>>>>> + size = 13; >>>>>> + case AXP22X_TEMP_ADC: >>>>>> + case AXP22X_BATT_V: >>>>>> + case AXP22X_BATT_CHRG_I: >>>>> >>>>> According to the datasheet, AXP22X_BATT_CHRG_I is also 13 bits wide. >>>>> >>>> >>>> Where did you get that? >>>> >>>> Also, the datasheet is inconsistent: >>>> - 9.5 E-Gauge Fuel Gauge system => the min value is at 0x0 and the max >>>> value at 0xfff for all channels, that's 12 bits. >>>> - 10.1.4 ADC Data => all channels except battery discharge current are >>>> on 12 bits (8 high, 4 low). >>> >>> My datasheets (AXP221 v1.6, AXP221s v1.2, AXP223 v1.1, all Chinese) say >>> in 10.1.4: >>> >>> - 7A: battery charge current high 5 bits >>> - 7B: battery charge current low 8 bits >>> - 7C: battery discharge current high 5 bits >>> - 7D: battery discharge current low 8 bits >>> >> >> AXP223 v1.1 in English in 10.1.4[1]: >> - 7A: battery charge current high 8 bits >> - 7B: battery charge current low 4 bits >> - 7C: battery discharge current high 8 bits >> - 7D: battery discharge current low 5 bits >> >> Note that it's 8 bits for high and 4/5 bits for low while you wrote 4/5 >> bits high and low 8 bits (typos or actually what's written in the >> datasheet?). > > Typo on my end, sorry. It's high 8bits and low 5/4 bits. > > Apart from that, the Chinese and English versions don't match for the > battery charge current. :( > > Would it be possible for you to test this? As in, have the module running, > and charging a battery, and have a multimeter in series with the battery > to verify the readings. > > Regards > ChenYu > >> >> Hum.. from the reg reading function[2] I would say that the correct >> formula is high on 8 bits and low on 4/5 bits. >> >> So hum.. what do we do? >> >> [1] http://dl.linux-sunxi.org/AXP/AXP223-en.pdf >> [2] http://lxr.free-electrons.com/source/include/linux/mfd/axp20x.h#L564 >> >>>> >>>> [...] >>>>>> +static int axp22x_read_raw(struct iio_dev *indio_dev, >>>>>> + struct iio_chan_spec const *chan, int *val, >>>>>> + int *val2, long mask) >>>>>> +{ >>>>>> + switch (mask) { >>>>>> + case IIO_CHAN_INFO_OFFSET: >>>>>> + *val = -2667; >>>>> >>>>> Datasheet says -267.7 C, or -2677 here. >>>>> >>>> >>>> The formula in the datasheet is (in milli Celsius): >>>> processed = raw * 100 - 266700; >>>> >>>> while the IIO framework asks for a scale and an offset which are then >>>> applied as: >>>> processed = (raw + offset) * scale; >>>> >>>> Thus by factorizing, we get: >>>> processed = (raw - 2667) * 100; >>> >>> What I meant was that your lower end value is off by one degree, >>> -266.7 in your code vs -267.7 in the datasheet. >>> >> >> Indeed. Thanks. >> >>>> >>>> [...] >>>>>> +static int axp20x_remove(struct platform_device *pdev) >>>>>> +{ >>>>>> + struct axp20x_adc_iio *info; >>>>>> + struct iio_dev *indio_dev = platform_get_drvdata(pdev); >>>>>> + >>>>>> + info = iio_priv(indio_dev); >>>>> >>>>> Nit: you could just reverse the 2 declarations above and join this >>>>> line after struct axp20x_adc_iio *info; >>>>> >>>>>> + regmap_write(info->regmap, AXP20X_ADC_EN1, 0); >>>>>> + regmap_write(info->regmap, AXP20X_ADC_EN2, 0); >>>>> >>>>> The existing VBUS power supply driver enables the VBUS ADC bits itself, >>>>> and does not check them later on. This means if one were to remove this >>>>> axp20x-adc module, the voltage/current readings in the VBUS power supply >>>>> would be invalid. Some sort of workaround would be needed here in this >>>>> driver of the VBUS driver. >>>>> >>>> >>>> That would be one reason to migrate the VBUS driver to use the IIO >>>> channels, wouldn't it? >>> >>> It is, preferably without changing the device tree. >>> >> >> Yes, of course. >> >> Thanks, >> Quentin >> >> -- >> Quentin Schulz, Free Electrons >> Embedded Linux and Kernel engineering >> http://free-electrons.com > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index 38bc319..5c5b51f 100644 --- a/drivers/iio/adc/Kconfig +++ b/drivers/iio/adc/Kconfig @@ -154,6 +154,16 @@ config AT91_SAMA5D2_ADC To compile this driver as a module, choose M here: the module will be called at91-sama5d2_adc. +config AXP20X_ADC + tristate "X-Powers AXP20X and AXP22X ADC driver" + depends on MFD_AXP20X + help + Say yes here to have support for X-Powers power management IC (PMIC) + AXP20X and AXP22X ADC devices. + + To compile this driver as a module, choose M here: the module will be + called axp20x_adc. + config AXP288_ADC tristate "X-Powers AXP288 ADC driver" depends on MFD_AXP20X diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile index d36c4be..f5c28a5 100644 --- a/drivers/iio/adc/Makefile +++ b/drivers/iio/adc/Makefile @@ -16,6 +16,7 @@ obj-$(CONFIG_AD7887) += ad7887.o obj-$(CONFIG_AD799X) += ad799x.o obj-$(CONFIG_AT91_ADC) += at91_adc.o obj-$(CONFIG_AT91_SAMA5D2_ADC) += at91-sama5d2_adc.o +obj-$(CONFIG_AXP20X_ADC) += axp20x_adc.o obj-$(CONFIG_AXP288_ADC) += axp288_adc.o obj-$(CONFIG_BCM_IPROC_ADC) += bcm_iproc_adc.o obj-$(CONFIG_BERLIN2_ADC) += berlin2-adc.o diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c new file mode 100644 index 0000000..8df972a --- /dev/null +++ b/drivers/iio/adc/axp20x_adc.c @@ -0,0 +1,490 @@ +/* ADC driver for AXP20X and AXP22X PMICs + * + * Copyright (c) 2016 Free Electrons NextThing Co. + * Quentin Schulz <quentin.schulz@free-electrons.com> + * + * This program is free software; you can redistribute it and/or modify it under + * the terms of the GNU General Public License version 2 as published by the + * Free Software Foundation. + * + */ + +#include <linux/completion.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h> +#include <linux/regmap.h> +#include <linux/thermal.h> + +#include <linux/iio/iio.h> +#include <linux/mfd/axp20x.h> + +#define AXP20X_ADC_EN1_MASK GENMASK(7, 0) + +#define AXP20X_ADC_EN2_MASK (GENMASK(3, 2) | BIT(7)) +#define AXP22X_ADC_EN1_MASK (GENMASK(7, 5) | BIT(0)) +#define AXP20X_ADC_EN2_TEMP_ADC BIT(7) +#define AXP20X_ADC_EN2_GPIO0_ADC BIT(3) +#define AXP20X_ADC_EN2_GPIO1_ADC BIT(2) + +#define AXP20X_GPIO10_IN_RANGE_GPIO0 BIT(0) +#define AXP20X_GPIO10_IN_RANGE_GPIO1 BIT(1) +#define AXP20X_GPIO10_IN_RANGE_GPIO0_VAL(x) ((x) & BIT(0)) +#define AXP20X_GPIO10_IN_RANGE_GPIO1_VAL(x) (((x) & BIT(0)) << 1) + +#define AXP20X_ADC_RATE_MASK (3 << 6) +#define AXP20X_ADC_RATE_25HZ (0 << 6) +#define AXP20X_ADC_RATE_50HZ BIT(6) +#define AXP20X_ADC_RATE_100HZ (2 << 6) +#define AXP20X_ADC_RATE_200HZ (3 << 6) + +#define AXP22X_ADC_RATE_100HZ (0 << 6) +#define AXP22X_ADC_RATE_200HZ BIT(6) +#define AXP22X_ADC_RATE_400HZ (2 << 6) +#define AXP22X_ADC_RATE_800HZ (3 << 6) + +#define AXP20X_ADC_CHANNEL(_channel, _name, _type, _reg) \ + { \ + .type = _type, \ + .indexed = 1, \ + .channel = _channel, \ + .address = _reg, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ + BIT(IIO_CHAN_INFO_SCALE), \ + .datasheet_name = _name, \ + } + +#define AXP20X_ADC_CHANNEL_OFFSET(_channel, _name, _type, _reg) \ + { \ + .type = _type, \ + .indexed = 1, \ + .channel = _channel, \ + .address = _reg, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ + BIT(IIO_CHAN_INFO_SCALE) |\ + BIT(IIO_CHAN_INFO_OFFSET),\ + .datasheet_name = _name, \ + } + +struct axp20x_adc_iio { + struct iio_dev *indio_dev; + struct regmap *regmap; +}; + +enum axp20x_adc_channel { + AXP20X_ACIN_V = 0, + AXP20X_ACIN_I, + AXP20X_VBUS_V, + AXP20X_VBUS_I, + AXP20X_TEMP_ADC, + AXP20X_GPIO0_V, + AXP20X_GPIO1_V, + AXP20X_BATT_V, + AXP20X_BATT_CHRG_I, + AXP20X_BATT_DISCHRG_I, + AXP20X_IPSOUT_V, +}; + +enum axp22x_adc_channel { + AXP22X_TEMP_ADC = 0, + AXP22X_BATT_V, + AXP22X_BATT_CHRG_I, + AXP22X_BATT_DISCHRG_I, +}; + +static const struct iio_chan_spec axp20x_adc_channels[] = { + AXP20X_ADC_CHANNEL(AXP20X_ACIN_V, "acin_v", IIO_VOLTAGE, + AXP20X_ACIN_V_ADC_H), + AXP20X_ADC_CHANNEL(AXP20X_ACIN_I, "acin_i", IIO_CURRENT, + AXP20X_ACIN_I_ADC_H), + AXP20X_ADC_CHANNEL(AXP20X_VBUS_V, "vbus_v", IIO_VOLTAGE, + AXP20X_VBUS_V_ADC_H), + AXP20X_ADC_CHANNEL(AXP20X_VBUS_I, "vbus_i", IIO_CURRENT, + AXP20X_VBUS_I_ADC_H), + AXP20X_ADC_CHANNEL_OFFSET(AXP20X_TEMP_ADC, "temp_adc", IIO_TEMP, + AXP20X_TEMP_ADC_H), + AXP20X_ADC_CHANNEL_OFFSET(AXP20X_GPIO0_V, "gpio0_v", IIO_VOLTAGE, + AXP20X_GPIO0_V_ADC_H), + AXP20X_ADC_CHANNEL_OFFSET(AXP20X_GPIO1_V, "gpio1_v", IIO_VOLTAGE, + AXP20X_GPIO1_V_ADC_H), + AXP20X_ADC_CHANNEL(AXP20X_BATT_V, "batt_v", IIO_VOLTAGE, + AXP20X_BATT_V_H), + AXP20X_ADC_CHANNEL(AXP20X_BATT_CHRG_I, "batt_chrg_i", IIO_CURRENT, + AXP20X_BATT_CHRG_I_H), + AXP20X_ADC_CHANNEL(AXP20X_BATT_DISCHRG_I, "batt_dischrg_i", IIO_CURRENT, + AXP20X_BATT_DISCHRG_I_H), + AXP20X_ADC_CHANNEL(AXP20X_IPSOUT_V, "ipsout_v", IIO_VOLTAGE, + AXP20X_IPSOUT_V_HIGH_H), +}; + +static const struct iio_chan_spec axp22x_adc_channels[] = { + AXP20X_ADC_CHANNEL_OFFSET(AXP22X_TEMP_ADC, "temp_adc", IIO_TEMP, + AXP22X_TEMP_ADC_H), + AXP20X_ADC_CHANNEL(AXP22X_BATT_V, "batt_v", IIO_VOLTAGE, + AXP20X_BATT_V_H), + AXP20X_ADC_CHANNEL(AXP22X_BATT_CHRG_I, "batt_chrg_i", IIO_CURRENT, + AXP20X_BATT_CHRG_I_H), + AXP20X_ADC_CHANNEL(AXP22X_BATT_DISCHRG_I, "batt_dischrg_i", IIO_CURRENT, + AXP20X_BATT_DISCHRG_I_H), +}; + +static int axp20x_adc_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *channel, int *val, + int *val2) +{ + struct axp20x_adc_iio *info = iio_priv(indio_dev); + int size = 12, ret; + + switch (channel->channel) { + case AXP20X_BATT_DISCHRG_I: + size = 13; + case AXP20X_ACIN_V: + case AXP20X_ACIN_I: + case AXP20X_VBUS_V: + case AXP20X_VBUS_I: + case AXP20X_TEMP_ADC: + case AXP20X_BATT_V: + case AXP20X_BATT_CHRG_I: + case AXP20X_IPSOUT_V: + case AXP20X_GPIO0_V: + case AXP20X_GPIO1_V: + ret = axp20x_read_variable_width(info->regmap, channel->address, + size); + if (ret < 0) + return ret; + *val = ret; + return IIO_VAL_INT; + + default: + return -EINVAL; + } + + return -EINVAL; +} + +static int axp22x_adc_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *channel, int *val, + int *val2) +{ + struct axp20x_adc_iio *info = iio_priv(indio_dev); + int size = 12, ret; + + switch (channel->channel) { + case AXP22X_BATT_DISCHRG_I: + size = 13; + case AXP22X_TEMP_ADC: + case AXP22X_BATT_V: + case AXP22X_BATT_CHRG_I: + ret = axp20x_read_variable_width(info->regmap, channel->address, + size); + if (ret < 0) + return ret; + *val = ret; + return IIO_VAL_INT; + + default: + return -EINVAL; + } + + return -EINVAL; +} + +static int axp20x_adc_scale(int channel, int *val, int *val2) +{ + switch (channel) { + case AXP20X_ACIN_V: + case AXP20X_VBUS_V: + *val = 1; + *val2 = 700000; + return IIO_VAL_INT_PLUS_MICRO; + + case AXP20X_ACIN_I: + *val = 0; + *val2 = 625000; + return IIO_VAL_INT_PLUS_MICRO; + + case AXP20X_VBUS_I: + *val = 0; + *val2 = 375000; + return IIO_VAL_INT_PLUS_MICRO; + + case AXP20X_TEMP_ADC: + *val = 100; + return IIO_VAL_INT; + + case AXP20X_GPIO0_V: + case AXP20X_GPIO1_V: + *val = 0; + *val2 = 500000; + return IIO_VAL_INT_PLUS_MICRO; + + case AXP20X_BATT_V: + *val = 1; + *val2 = 100000; + return IIO_VAL_INT_PLUS_MICRO; + + case AXP20X_BATT_DISCHRG_I: + case AXP20X_BATT_CHRG_I: + *val = 0; + *val2 = 500000; + return IIO_VAL_INT_PLUS_MICRO; + + case AXP20X_IPSOUT_V: + *val = 1; + *val2 = 400000; + return IIO_VAL_INT_PLUS_MICRO; + + default: + return -EINVAL; + } + + return -EINVAL; +} + +static int axp22x_adc_scale(int channel, int *val, int *val2) +{ + switch (channel) { + case AXP22X_TEMP_ADC: + *val = 100; + return IIO_VAL_INT; + + case AXP22X_BATT_V: + *val = 1; + *val2 = 100000; + return IIO_VAL_INT_PLUS_MICRO; + + case AXP22X_BATT_DISCHRG_I: + case AXP22X_BATT_CHRG_I: + *val = 0; + *val2 = 500000; + return IIO_VAL_INT_PLUS_MICRO; + + default: + return -EINVAL; + } + + return -EINVAL; +} + +static int axp20x_adc_offset(struct iio_dev *indio_dev, int channel, int *val) +{ + struct axp20x_adc_iio *info = iio_priv(indio_dev); + int ret, reg; + + switch (channel) { + case AXP20X_TEMP_ADC: + *val = -1447; + return IIO_VAL_INT; + + case AXP20X_GPIO0_V: + case AXP20X_GPIO1_V: + ret = regmap_read(info->regmap, AXP20X_GPIO10_IN_RANGE, ®); + if (ret < 0) + return ret; + + if (channel == AXP20X_GPIO0_V) + *val = reg & AXP20X_GPIO10_IN_RANGE_GPIO0; + else + *val = reg & AXP20X_GPIO10_IN_RANGE_GPIO1; + + *val = !!(*val) * 700000; + + return IIO_VAL_INT; + + default: + return -EINVAL; + } + + return -EINVAL; +} + +static int axp20x_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, int *val, + int *val2, long mask) +{ + switch (mask) { + case IIO_CHAN_INFO_OFFSET: + return axp20x_adc_offset(indio_dev, chan->channel, val); + + case IIO_CHAN_INFO_SCALE: + return axp20x_adc_scale(chan->channel, val, val2); + + case IIO_CHAN_INFO_RAW: + return axp20x_adc_read_raw(indio_dev, chan, val, val2); + + default: + return -EINVAL; + } + + return -EINVAL; +} + +static int axp22x_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, int *val, + int *val2, long mask) +{ + switch (mask) { + case IIO_CHAN_INFO_OFFSET: + *val = -2667; + return IIO_VAL_INT; + + case IIO_CHAN_INFO_SCALE: + return axp22x_adc_scale(chan->channel, val, val2); + + case IIO_CHAN_INFO_RAW: + return axp22x_adc_read_raw(indio_dev, chan, val, val2); + + default: + return -EINVAL; + } + + return -EINVAL; +} + +static int axp20x_write_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, int val, int val2, + long mask) +{ + struct axp20x_adc_iio *info = iio_priv(indio_dev); + + /* + * The AXP20X PMIC allows the user to choose between 0V and 0.7V offsets + * for (independently) GPIO0 and GPIO1 when in ADC mode. + */ + if (mask != IIO_CHAN_INFO_OFFSET) + return -EINVAL; + + if (chan->channel != AXP20X_GPIO0_V && chan->channel != AXP20X_GPIO1_V) + return -EINVAL; + + if (val != 0 && val != 700000) + return -EINVAL; + + if (chan->channel == AXP20X_GPIO0_V) + return regmap_update_bits(info->regmap, AXP20X_GPIO10_IN_RANGE, + AXP20X_GPIO10_IN_RANGE_GPIO0, + AXP20X_GPIO10_IN_RANGE_GPIO0_VAL(!!val)); + + return regmap_update_bits(info->regmap, AXP20X_GPIO10_IN_RANGE, + AXP20X_GPIO10_IN_RANGE_GPIO1, + AXP20X_GPIO10_IN_RANGE_GPIO1_VAL(!!val)); +} + +static const struct iio_info axp20x_adc_iio_info = { + .read_raw = axp20x_read_raw, + .write_raw = axp20x_write_raw, + .driver_module = THIS_MODULE, +}; + +static const struct iio_info axp22x_adc_iio_info = { + .read_raw = axp22x_read_raw, + .driver_module = THIS_MODULE, +}; + +static const struct of_device_id axp20x_adc_of_match[] = { + { .compatible = "x-powers,axp209-adc", .data = (void *)AXP209_ID, }, + { .compatible = "x-powers,axp221-adc", .data = (void *)AXP221_ID, }, + { /* sentinel */ }, +}; + +static int axp20x_probe(struct platform_device *pdev) +{ + struct axp20x_adc_iio *info; + struct iio_dev *indio_dev; + struct axp20x_dev *axp20x_dev; + int ret, axp20x_id; + + axp20x_dev = dev_get_drvdata(pdev->dev.parent); + + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info)); + if (!indio_dev) + return -ENOMEM; + + info = iio_priv(indio_dev); + platform_set_drvdata(pdev, indio_dev); + + info->regmap = axp20x_dev->regmap; + info->indio_dev = indio_dev; + indio_dev->name = dev_name(&pdev->dev); + indio_dev->dev.parent = &pdev->dev; + indio_dev->dev.of_node = pdev->dev.of_node; + indio_dev->modes = INDIO_DIRECT_MODE; + + axp20x_id = (int)of_device_get_match_data(&pdev->dev); + + switch (axp20x_id) { + case AXP209_ID: + indio_dev->info = &axp20x_adc_iio_info; + indio_dev->num_channels = ARRAY_SIZE(axp20x_adc_channels); + indio_dev->channels = axp20x_adc_channels; + + /* Enable the ADCs on IP */ + regmap_write(info->regmap, AXP20X_ADC_EN1, AXP20X_ADC_EN1_MASK); + + /* Enable GPIO0/1 and internal temperature ADCs */ + regmap_update_bits(info->regmap, AXP20X_ADC_EN2, + AXP20X_ADC_EN2_MASK, AXP20X_ADC_EN2_MASK); + + /* Configure ADCs rate */ + regmap_update_bits(info->regmap, AXP20X_ADC_RATE, + AXP20X_ADC_RATE_MASK, AXP20X_ADC_RATE_50HZ); + break; + + case AXP221_ID: + indio_dev->info = &axp22x_adc_iio_info; + indio_dev->num_channels = ARRAY_SIZE(axp22x_adc_channels); + indio_dev->channels = axp22x_adc_channels; + + /* Enable the ADCs on IP */ + regmap_write(info->regmap, AXP20X_ADC_EN1, AXP22X_ADC_EN1_MASK); + + /* Configure ADCs rate */ + regmap_update_bits(info->regmap, AXP20X_ADC_RATE, + AXP20X_ADC_RATE_MASK, AXP22X_ADC_RATE_200HZ); + break; + + default: + return -EINVAL; + } + + ret = devm_iio_device_register(&pdev->dev, indio_dev); + if (ret < 0) { + dev_err(&pdev->dev, "could not register the device\n"); + regmap_write(info->regmap, AXP20X_ADC_EN1, 0); + regmap_write(info->regmap, AXP20X_ADC_EN2, 0); + return ret; + } + + return 0; +} + +static int axp20x_remove(struct platform_device *pdev) +{ + struct axp20x_adc_iio *info; + struct iio_dev *indio_dev = platform_get_drvdata(pdev); + + info = iio_priv(indio_dev); + regmap_write(info->regmap, AXP20X_ADC_EN1, 0); + regmap_write(info->regmap, AXP20X_ADC_EN2, 0); + + return 0; +} + +static struct platform_driver axp20x_adc_driver = { + .driver = { + .name = "axp20x-adc", + .of_match_table = axp20x_adc_of_match, + }, + .probe = axp20x_probe, + .remove = axp20x_remove, +}; + +module_platform_driver(axp20x_adc_driver); + +MODULE_DESCRIPTION("ADC driver for AXP20X and AXP22X PMICs"); +MODULE_AUTHOR("Quentin Schulz <quentin.schulz@free-electrons.com>"); +MODULE_LICENSE("GPL"); diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h index a4860bc..650c6f6 100644 --- a/include/linux/mfd/axp20x.h +++ b/include/linux/mfd/axp20x.h @@ -150,6 +150,10 @@ enum { #define AXP20X_VBUS_I_ADC_L 0x5d #define AXP20X_TEMP_ADC_H 0x5e #define AXP20X_TEMP_ADC_L 0x5f + +#define AXP22X_TEMP_ADC_H 0x56 +#define AXP22X_TEMP_ADC_L 0x57 + #define AXP20X_TS_IN_H 0x62 #define AXP20X_TS_IN_L 0x63 #define AXP20X_GPIO0_V_ADC_H 0x64
The X-Powers AXP20X and AXP22X PMICs have multiple ADCs. They expose the battery voltage, battery charge and discharge currents, AC-in and VBUS voltages and currents, 2 GPIOs muxable in ADC mode and PMIC temperature. This adds support for most of AXP20X and AXP22X ADCs. Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> --- drivers/iio/adc/Kconfig | 10 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/axp20x_adc.c | 490 +++++++++++++++++++++++++++++++++++++++++++ include/linux/mfd/axp20x.h | 4 + 4 files changed, 505 insertions(+) create mode 100644 drivers/iio/adc/axp20x_adc.c