Re: [PATCH v3 4/5] iio/adc/axp288: add support for axp288 gpadc

[Jacob Pan <jacob.jun.pan@linux.intel.com>]

On Sun, 14 Sep 2014 15:09:06 +0200
Hartmut Knaack <knaack.h@gmx.de> 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@intel.com>
> >>>
> >>> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> >>> ---
> >>>  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.
> >>> +	{
> >>> \
> >>> +		.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.
> >>> +}
> >>> +
> >>> +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@linux.intel.com>");
> >>> +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@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 

[Jacob Pan]