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?
>> +
>> +#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@xxxxxxxxxxxxxxxxxx>");
>> +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
--
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
