[03/22] iio: adc: add support for X-Powers AXP20X and AXP22X PMICs ADCs

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

-- 
2.9.3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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, &reg);

> +               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

>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

-- 
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

-- 
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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, &reg);

> +		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

> 



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

> 



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

> 



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel