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

[Quentin Schulz <quentin.schulz@free-electrons.com>]

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