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

From: Quentin Schulz
Date: Thu Jan 05 2017 - 04:55:24 EST

Next message: Lorenzo Pieralisi: "Re: [PATCH v6 03/14] ACPI: ARM64: IORT: add missing comment for iort_dev_find_its_id()"
Previous message: Gregory CLEMENT: "Re: [PATCH] cpufreq: Remove CONFIG_CPU_FREQ_STAT_DETAILS config option"
In reply to: Chen-Yu Tsai: "Re: [PATCH 03/22] iio: adc: add support for X-Powers AXP20X and AXP22X PMICs ADCs"
Next in thread: Chen-Yu Tsai: "Re: [PATCH 03/22] iio: adc: add support for X-Powers AXP20X and AXP22X PMICs ADCs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 05/01/2017 09:27, Chen-Yu Tsai wrote:
> On Thu, Jan 5, 2017 at 4:06 PM, Quentin Schulz
> <quentin.schulz@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx> 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

Next message: Lorenzo Pieralisi: "Re: [PATCH v6 03/14] ACPI: ARM64: IORT: add missing comment for iort_dev_find_its_id()"
Previous message: Gregory CLEMENT: "Re: [PATCH] cpufreq: Remove CONFIG_CPU_FREQ_STAT_DETAILS config option"
In reply to: Chen-Yu Tsai: "Re: [PATCH 03/22] iio: adc: add support for X-Powers AXP20X and AXP22X PMICs ADCs"
Next in thread: Chen-Yu Tsai: "Re: [PATCH 03/22] iio: adc: add support for X-Powers AXP20X and AXP22X PMICs ADCs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]