lkml.org 
[lkml]   [2017]   [Jan]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    SubjectRe: [PATCH 03/22] iio: adc: add support for X-Powers AXP20X and AXP22X PMICs ADCs
    From
    Date
    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

    \
     
     \ /
      Last update: 2017-01-05 10:52    [W:2.860 / U:0.228 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site