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. > >>> + { > >>> \ > >>> + .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@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