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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux