Jacob Pan schrieb, Am 16.09.2014 20:21:
> On Sun, 14 Sep 2014 15:09:06 +0200
> Hartmut Knaack <knaack.h@xxxxxx> wrote:
> Thanks for the review, most points are taken, please comments inline.
>
>> Jonathan Cameron schrieb, Am 13.09.2014 21:52:
>>> On 12/09/14 13:44, Peter Meerwald wrote:
>>>>
>>>>> Platform driver for XPowers AXP288 ADC, which is a sub-device of
>>>>> the customized PMIC for Intel Baytrail-CR platforms. GPADC device
>>>>> enumerates as one of the MFD cell devices. It uses IIO
>>>>> infrastructure to communicate with userspace and consumer drivers.
>>>>>
>>>>> Usages of ADC channels include battery charging and thermal
>>>>> sensors.
>>>>
>>>> minor nitpicking below
>>> A few more bits and pieces from me.
>> And even some more.
>>>
>>> Jonathan
>>>>
>>>>> Based on initial work by:
>>>>> Ramakrishna Pallala <ramakrishna.pallala@xxxxxxxxx>
>>>>>
>>>>> Signed-off-by: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
>>>>> ---
>>>>> drivers/iio/adc/Kconfig | 8 ++
>>>>> drivers/iio/adc/Makefile | 1 +
>>>>> drivers/iio/adc/axp288_gpadc.c | 238
>>>>> +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 247
>>>>> insertions(+) create mode 100644 drivers/iio/adc/axp288_gpadc.c
>>>>>
>>>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>>>> index 11b048a..d02a08e 100644
>>>>> --- a/drivers/iio/adc/Kconfig
>>>>> +++ b/drivers/iio/adc/Kconfig
>>>>> @@ -127,6 +127,14 @@ config AT91_ADC
>>>>> help
>>>>> Say yes here to build support for Atmel AT91 ADC.
>>>>>
>>>>> +config AXP288_GPADC
>>>>> + tristate "X-Power AXP288 GPADC driver"
>>>>> + depends on MFD_AXP2XX
>>>>> + help
>>>>> + Say yes here to have support for X-Power power
>>>>> management IC (PMIC) ADC
>>>>> + device. Depending on platform configuration, this
>>>>> general purpose ADC can
>>>>> + be used for sampling sensors such as thermal resistors.
>>>>> +
>>>>> config EXYNOS_ADC
>>>>> tristate "Exynos ADC driver support"
>>>>> depends on ARCH_EXYNOS || (OF && COMPILE_TEST)
>>>>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>>>>> index ad81b51..8bf0104 100644
>>>>> --- a/drivers/iio/adc/Makefile
>>>>> +++ b/drivers/iio/adc/Makefile
>>>>> @@ -30,3 +30,4 @@ obj-$(CONFIG_VF610_ADC) += vf610_adc.o
>>>>> obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
>>>>> xilinx-xadc-y := xilinx-xadc-core.o xilinx-xadc-events.o
>>>>> obj-$(CONFIG_XILINX_XADC) += xilinx-xadc.o
>>>>> +obj-$(CONFIG_AXP288_GPADC) += axp288_gpadc.o
>> Insert in alphabetic order.
>>>>> diff --git a/drivers/iio/adc/axp288_gpadc.c
>>>>> b/drivers/iio/adc/axp288_gpadc.c new file mode 100644
>>>>> index 0000000..aaa24b0
>>>>> --- /dev/null
>>>>> +++ b/drivers/iio/adc/axp288_gpadc.c
>>>>> @@ -0,0 +1,238 @@
>>>>> +/*
>>>>> + * axp288_gpadc.c - Xpower AXP288 PMIC GPADC Driver
>>>>
>>>> X-Power
>>>>
>>>>> + *
>>>>> + * Copyright (C) 2014 Intel Corporation
>>>>> + *
>>>>> + *
>>>>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>> + *
>>>>> + * This program is free software; you can redistribute it and/or
>>>>> modify
>>>>> + * it under the terms of the GNU General Public License as
>>>>> published by
>>>>> + * the Free Software Foundation; version 2 of the License.
>>>>> + *
>>>>> + * This program is distributed in the hope that it will be
>>>>> useful, but
>>>>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
>>>>> See the GNU
>>>>> + * General Public License for more details.
>>>>> + *
>>>>> + */
>>>>> +
>>>>> +#include <linux/module.h>
>>>>> +#include <linux/kernel.h>
>>>>> +#include <linux/device.h>
>>>>> +#include <linux/regmap.h>
>>>>> +#include <linux/mfd/axp2xx.h>
>>>>> +#include <linux/platform_device.h>
>>>>> +
>>>>> +#include <linux/iio/iio.h>
>>>>> +#include <linux/iio/machine.h>
>>>>> +#include <linux/iio/driver.h>
>>>>> +
>>>>> +#define AXP288_ADC_EN_MASK 0xF1
>>>>> +#define AXP288_ADC_TS_PIN_GPADC 0xF2
>>>>> +#define AXP288_ADC_TS_PIN_ON 0xF3
>> Use tabs instead of whitespaces for alignment.
>>>>> +
>>>>> +enum axp288_adc_id {
>>>>> + AXP288_ADC_TS,
>>>>> + AXP288_ADC_PMIC,
>>>>> + AXP288_ADC_GP,
>>>>> + AXP288_ADC_BATT_CHRG_I,
>>>>> + AXP288_ADC_BATT_DISCHRG_I,
>>>>> + AXP288_ADC_BATT_V,
>>>>> + AXP288_ADC_NR_CHAN,
>>>>> +};
>>>>> +
>>>>> +static const int axp288_scale[AXP288_ADC_NR_CHAN] = {
>>>>
>>>> axp288_adc_scale
>>>>
>>>>> + [AXP288_ADC_BATT_CHRG_I] = 1,
>>>>> + [AXP288_ADC_BATT_DISCHRG_I] = 1,
>>>>> + [AXP288_ADC_BATT_V] = 1,
>>>>> +};
>>>>> +
>>>>> +struct gpadc_info {
>>>>
>>>> axp288_adc_info
>>>>
>>>>> + int irq;
>>>>> + struct device *dev;
>>>>> + struct regmap *regmap;
>>>>> +};
>>>>> +
>>>>> +#define ADC_CHANNEL(_type, _channel, _address,
>>>>> _info_mask) \
>>>>
>>>> AXP288_ADC_CHANNEL
>>>>
>>>>> +
>>>>> { \
>>>>> + .indexed =
>>>>> 1, \
>>>>> + .type =
>>>>> _type, \
>>>>> + .channel =
>>>>> _channel, \
>>>>> + .address =
>>>>> _address, \
>>>>> + .datasheet_name =
>>>>> "CH"#_channel, \
>>>>> + .info_mask_separate =
>>>>> _info_mask, \
>>>>> + }
>>> I'm not entirely convinced this little macro actually helps seeing
>>> as there is only one fixed element and a little saving on the
>>> datasheet name. Would be tempted to just list it long hand below
>>> for simplicity.
>>>
>>>>> +
>>>>> +static const struct iio_chan_spec const axp288_adc_channels[] = {
>>>>> + ADC_CHANNEL(IIO_TEMP, 0, AXP288_TS_ADC_H, 0),
>>>>> + ADC_CHANNEL(IIO_TEMP, 1, AXP288_PMIC_ADC_H, 0),
>>>>> + ADC_CHANNEL(IIO_TEMP, 2, AXP288_GP_ADC_H, 0),
>>> So these first three have no known scale or offset? Fair enough if
>>> true.
>>>
>>>>> + ADC_CHANNEL(IIO_CURRENT, 3, AXP20X_BATT_CHRG_I_H,
>>>>> + (BIT(IIO_CHAN_INFO_RAW) |
>>>>> BIT(IIO_CHAN_INFO_SCALE))),
>>> So the output of these (raw*scale) is in milliamps as per the ABI?
>>> (it would be unusual if they are!)
>>>
>>>>> + ADC_CHANNEL(IIO_CURRENT, 4, AXP20X_BATT_DISCHRG_I_H,
>>>>> + (BIT(IIO_CHAN_INFO_RAW) |
>>>>> BIT(IIO_CHAN_INFO_SCALE))),
>>>>> + ADC_CHANNEL(IIO_VOLTAGE, 5, AXP20X_BATT_V_H,
>>>>> + (BIT(IIO_CHAN_INFO_RAW) |
>>>>> BIT(IIO_CHAN_INFO_SCALE))), +};
>>>>> +
>>>>> +#define
>>>>> ADC_MAP(_adc_channel_label, \
>>>>
>>>> AXP288_ADC_MAP
>>>>
>>>>> + _consumer_dev_name, \
>>>>> +
>>>>> _consumer_channel) \
>> Why not put all parameters in first line?
> it is over 80 chars.
Right, including Jonathans suggestion, it does. Haven't checked on that - my bad.
>>>>> + {
>>>>> \
>>>>> + .adc_channel_label = _adc_channel_label, \
>>>>> + .consumer_dev_name = _consumer_dev_name, \
>>>>> + .consumer_channel =
>>>>> _consumer_channel, \
>>>>> + }
>>>>> +
>>>>> +/* for consumer drivers */
>>>>> +static struct iio_map axp288_iio_default_maps[] = {
>>>>
>>>> axp288_adc_iio_default_maps
>>>>
>>>>> + ADC_MAP("TS_PIN", "axp288-batt", "axp288-batt-temp"),
>>>>> + ADC_MAP("PMIC_TEMP", "axp288-pmic", "axp288-pmic-temp"),
>>>>> + ADC_MAP("GPADC", "axp288-gpadc", "axp288-system-temp"),
>>>>> + ADC_MAP("BATT_CHG_I", "axp288-chrg", "axp288-chrg-curr"),
>>>>> + ADC_MAP("BATT_DISCHRG_I", "axp288-chrg",
>>>>> "axp288-chrg-d-curr"),
>>>>> + ADC_MAP("BATT_V", "axp288-batt", "axp288-batt-volt"),
>>>>> + {},
>>>>> +};
>>>>> +
>>>>> +static int axp288_adc_read_raw(struct iio_dev *indio_dev,
>>>>> + struct iio_chan_spec const *chan,
>>>>> + int *val, int *val2, long mask)
>> Improve indentation of parameters.
>>>>> +{
>>>>> + int ret;
>>>>> + struct gpadc_info *info = iio_priv(indio_dev);
>>>>> + unsigned int th, tl;
>>>>> +
>>>>> + mutex_lock(&indio_dev->mlock);
>>>>> +
>>>>> + switch (mask) {
>>>>> + case IIO_CHAN_INFO_RAW:
>>>>> + /* special case for GPADC sample */
>>>>> + if (chan->address == AXP288_GP_ADC_H)
>>>>> + regmap_write(info->regmap,
>>>>> AXP288_ADC_TS_PIN_CTRL,
>>>>> + AXP288_ADC_TS_PIN_GPADC);
>> Improve indentation. Also check for error on regmap_write.
>>>>> + ret = regmap_read(info->regmap, chan->address,
>>>>> &th);
>> Better use regmap_bulk_read?
> good idea. will do.
>>>>> + if (ret) {
>>>>> + dev_err(&indio_dev->dev, "sample raw
>>>>> data high failed\n");
>>>>> + break;
>>>>> + }
>>>>> + ret = regmap_read(info->regmap, chan->address +
>>>>> 1, &tl);
>>>>> + if (ret) {
>>>>> + dev_err(&indio_dev->dev, "sample raw
>>>>> data low failed\n");
>>>>> + break;
>>>>> + }
>>>>> +
>>>>> + *val = (th << 4) + ((tl >> 4) & 0x0F);
>>>>> + ret = IIO_VAL_INT;
>>>>> + break;
>>>>> + case IIO_CHAN_INFO_SCALE:
>>>>> + *val = axp288_scale[chan->channel];
>>>>
>>>> either set
>>>> *val2 = 0 or
>>>> ret = IIO_VAL_INT;
>>> This one. No point in misleading userspace into thinking their might
>>> be a decimal part sometimes. If they are as it appears all 1, then
>>> those channels can use IIO_CHAN_INFO_PROCESSED and skip exporting
>>> this at all.
>>>>
>>>>> + ret = IIO_VAL_INT_PLUS_MICRO;
>>>>> + break;
>>>>> + default:
>>>>> + ret = -EINVAL;
>>>>> + }
>>>>> +
>>>>> + if (IIO_CHAN_INFO_RAW && chan->address ==
>>>>> AXP288_GP_ADC_H)
>>>
>>> Well, IIO_CHAN_INFO_RAW is always going to be false, so I'm
>>> guessing this isn't what was intended...
>>> mask == IIO_CHAN_INFO_RAW I would imagine.
>>>>> + regmap_write(info->regmap,
>>>>> AXP288_ADC_TS_PIN_CTRL,
>>>>> + AXP288_ADC_TS_PIN_ON);
>> Improve indentation.
>>>>> +
>>>>> + mutex_unlock(&indio_dev->mlock);
>>>>> +
>>>>> + return ret;
>>>>> +}
>>>>> +
>>>>> +static int axp288_gpadc_enable(struct regmap *regmap, bool
>>>>> enable)
>>>>
>>>> axp288_adc_enable()
>>>>
>>>>> +{
>>>>> + unsigned int pin_on, adc_en;
>>>>> +
>>>>> + if (enable) {
>>>>> + pin_on = AXP288_ADC_TS_PIN_ON;
>>>>> + adc_en = AXP288_ADC_EN_MASK;
>>>>> + } else {
>>>>> + pin_on = ~AXP288_ADC_TS_PIN_ON;
>>>>> + adc_en = ~AXP288_ADC_EN_MASK;
>>>>> + }
>>>>> + if (regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL, pin_on))
>>>>> + return -EIO;
>> Better return real error value of regmap_write().
>>>>> +
>>>>> + return regmap_write(regmap, AXP20X_ADC_EN1,
>>>>> ~AXP288_ADC_EN_MASK);
>> Didn't you mean to write adc_en?
> No, EN1 is the register.
I think you got me wrong here. You set up adc_en depending on the state of enable, but then you discard adc_en. So, I would expect that your intention was to do regmap_write(regmap, AXP20X_ADC_EN1, adc_en)?
>>>>> +}
>>>>> +
>>>>> +static const struct iio_info axp288_iio_info = {
>>>>
>>>> axp288_adc_iio_info
>>> or axp288_adc_info would do.
>>>>
>>>>> + .read_raw = &axp288_adc_read_raw,
>>>>> + .driver_module = THIS_MODULE,
>>>>> +};
>>>>> +
>>>>> +static int axp288_gpadc_probe(struct platform_device *pdev)
>>>>
>>>> axp288_adc_probe()
>>>>
>>>>> +{
>>>>> + int err;
>>>>> + struct gpadc_info *info;
>>>>> + struct iio_dev *indio_dev;
>>>>> + struct axp2xx_dev *axp2xx =
>>>>> dev_get_drvdata(pdev->dev.parent);
>>>>> + int irq;
>> Move irq in one line with err, or use err instead of irq (and better
>> call it ret), or as Peter suggested below.
> yes, removed local irq.
>>>>> +
>>>>> + indio_dev = devm_iio_device_alloc(&pdev->dev,
>>>>> sizeof(*info));
>>>>> + if (!indio_dev)
>>>>> + return -ENOMEM;
>>>>> +
>>>>> + info = iio_priv(indio_dev);
>>>>> + irq = platform_get_irq(pdev, 0);
>>>>
>>>> could save local irq variable and directly assign to info->irq
>>>>
>>>>> + if (irq < 0) {
>>>>> + dev_err(&pdev->dev, "no irq resource?\n");
>>>>> + return irq;
>>>>> + }
>>>>> + info->irq = irq;
>>>>> + platform_set_drvdata(pdev, indio_dev);
>>>>> + info->regmap = axp2xx->regmap;
>>>>> + axp288_gpadc_enable(axp2xx->regmap, true);
>> This function can return errors, check for them.
>>>>> +
>>>>> + indio_dev->dev.parent = &pdev->dev;
>>>>> + indio_dev->name = pdev->name;
>>>>> + indio_dev->channels = axp288_adc_channels;
>>>>> + indio_dev->num_channels =
>>>>> ARRAY_SIZE(axp288_adc_channels);
>>>>> + indio_dev->info = &axp288_iio_info;
>>>>> + indio_dev->modes = INDIO_DIRECT_MODE;
>>>>> + /* REVISIT: override default map with platform data */
>>>>> + err = iio_map_array_register(indio_dev,
>>>>> axp288_iio_default_maps);
>>>>> + if (err)
>>>>
>>>> maybe err < 0, just to be consistent with below
>>>>
>>>>> + goto err_disable_dev;
>>>>> +
>>>>> + err = iio_device_register(indio_dev);
>>>>> + if (err < 0) {
>>>>> + dev_err(&pdev->dev, "unable to register iio
>>>>> device\n");
>>>>> + goto err_array_unregister;
>>>>> + }
>>>>> + return 0;
>>>>> +
>>>>> +err_array_unregister:
>>>>> + iio_map_array_unregister(indio_dev);
>>>>> +err_disable_dev:
>>>>> + axp288_gpadc_enable(axp2xx->regmap, false);
>>>>> + return err;
>>>>> +}
>>>>> +
>>>>> +static int axp288_gpadc_remove(struct platform_device *pdev)
>>>>> +{
>>>>> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>>>>> +
>>>>> + iio_device_unregister(indio_dev);
>>>>> + iio_map_array_unregister(indio_dev);
>>>>
>>>> axp288_gpadc_enable(axp2xx->regmap, false);
>>>>
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static struct platform_driver axp288_gpadc_driver = {
>>>>> + .probe = axp288_gpadc_probe,
>>>>> + .remove = axp288_gpadc_remove,
>>>>> + .driver = {
>>>>> + .name = "axp288_adc",
>>>>> + .owner = THIS_MODULE,
>>>>> + },
>>>>> +};
>>>>> +
>>>>> +module_platform_driver(axp288_gpadc_driver);
>>>>> +
>>>>> +MODULE_AUTHOR("Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>");
>>>>> +MODULE_DESCRIPTION("Dollar Cove Xpower AXP288 General Purpose
>>>>> ADC Driver");
>>>>
>>>> X-Power
>>>>
>>>>> +MODULE_LICENSE("GPL");
>>>>>
>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe
>>> linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>
>
> [Jacob Pan]
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
