On 09/09/14 11:53, Opensource [Adam Thomson] wrote:
> On August 30, 2014 21:01, Jonathan Cameron wrote:
>
>> On 28/08/14 12:28, Peter Meerwald wrote:
>>>
>>> some minor comments inline
>> A few more from me + make sure all your units match the ABI and in
>> particular that everything you use is documented in
>> Documentation/ABI/testing/syfs-bus-iio. Some stuff that only
>> exists in staging drivers isn't documented in there as yet
>> (current measurements for example) so the docs will need additions
>> alongside this driver.
>>
>> Thanks,
>>
>> Jonathan
>>>
>>>> This patch adds support for DA9150 Charger & Fuel-Gauge IC GPADC.
>>>>
>>>> Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@xxxxxxxxxxx>
>>>> ---
>>>> drivers/iio/adc/Kconfig | 9 +
>>>> drivers/iio/adc/Makefile | 1 +
>>>> drivers/iio/adc/da9150-gpadc.c | 430
>> +++++++++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 440 insertions(+)
>>>> create mode 100644 drivers/iio/adc/da9150-gpadc.c
>>>>
>>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>>> index 11b048a..8041347 100644
>>>> --- a/drivers/iio/adc/Kconfig
>>>> +++ b/drivers/iio/adc/Kconfig
>>>> @@ -127,6 +127,15 @@ config AT91_ADC
>>>> help
>>>> Say yes here to build support for Atmel AT91 ADC.
>>>>
>>>> +config DA9150_GPADC
>>>> + tristate "Dialog DA9150 GPADC driver support"
>>>> + depends on MFD_DA9150
>>>> + help
>>>> + Say yes here to build support for Dialog DA9150 GPADC.
>>>> +
>>>> + This driver can also be built as a module. If chosen, the module name
>>>> + will be da9150-gpadc.
>>>> +
>>>> 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..48413d2 100644
>>>> --- a/drivers/iio/adc/Makefile
>>>> +++ b/drivers/iio/adc/Makefile
>>>> @@ -14,6 +14,7 @@ obj-$(CONFIG_AD7793) += ad7793.o
>>>> obj-$(CONFIG_AD7887) += ad7887.o
>>>> obj-$(CONFIG_AD799X) += ad799x.o
>>>> obj-$(CONFIG_AT91_ADC) += at91_adc.o
>>>> +obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o
>>>> obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
>>>> obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>>>> obj-$(CONFIG_MAX1027) += max1027.o
>>>> diff --git a/drivers/iio/adc/da9150-gpadc.c b/drivers/iio/adc/da9150-gpadc.c
>>>> new file mode 100644
>>>> index 0000000..21a21a9
>>>> --- /dev/null
>>>> +++ b/drivers/iio/adc/da9150-gpadc.c
>>>> @@ -0,0 +1,430 @@
>>>> +/*
>>>> + * DA9150 GPADC Driver
>>>> + *
>>>> + * Copyright (c) 2014 Dialog Semiconductor
>>>> + *
>>>> + * Author: Adam Thomson <Adam.Thomson.Opensource@xxxxxxxxxxx>
>>>> + *
>>>> + * 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; either version 2 of the License, or (at your
>>>> + * option) any later version.
>>>> + */
>>>> +
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/slab.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/interrupt.h>
>>>> +
>>>> +#include <linux/mfd/da9150/core.h>
>>>> +#include <linux/mfd/da9150/pdata.h>
>>>> +#include <linux/mfd/da9150/registers.h>
>>>> +
>>>> +#include <linux/iio/iio.h>
>>>> +#include <linux/iio/machine.h>
>>>> +#include <linux/iio/driver.h>
>>>> +
>>>> +
>>>> +/* Channels */
>>>> +enum da9150_gpadc_hw_channel {
>>>> + DA9150_GPADC_HW_CHAN_GPIOA_2V = 0,
>>>> + DA9150_GPADC_HW_CHAN_GPIOA_2V_,
>>>
>>> why the underscore_ notation?
>>> couldn't you use e.g. DA9150_GPADC_HW_CHAN_GPIOB_2V = 2?
>>>
>>>> + DA9150_GPADC_HW_CHAN_GPIOB_2V,
>>>> + DA9150_GPADC_HW_CHAN_GPIOB_2V_,
>>>> + DA9150_GPADC_HW_CHAN_GPIOC_2V,
>>>> + DA9150_GPADC_HW_CHAN_GPIOC_2V_,
>>>> + DA9150_GPADC_HW_CHAN_GPIOD_2V,
>>>> + DA9150_GPADC_HW_CHAN_GPIOD_2V_,
>>>> + DA9150_GPADC_HW_CHAN_IBUS_SENSE,
>>>> + DA9150_GPADC_HW_CHAN_IBUS_SENSE_,
>>>> + DA9150_GPADC_HW_CHAN_VBUS_DIV,
>>>> + DA9150_GPADC_HW_CHAN_VBUS_DIV_,
>>>> + DA9150_GPADC_HW_CHAN_ID,
>>>> + DA9150_GPADC_HW_CHAN_ID_,
>>>> + DA9150_GPADC_HW_CHAN_VSYS,
>>>> + DA9150_GPADC_HW_CHAN_VSYS_,
>>>> + DA9150_GPADC_HW_CHAN_GPIOA_5V,
>>>> + DA9150_GPADC_HW_CHAN_GPIOA_5V_,
>>>> + DA9150_GPADC_HW_CHAN_GPIOB_5V,
>>>> + DA9150_GPADC_HW_CHAN_GPIOB_5V_,
>>>> + DA9150_GPADC_HW_CHAN_GPIOC_5V,
>>>> + DA9150_GPADC_HW_CHAN_GPIOC_5V_,
>>>> + DA9150_GPADC_HW_CHAN_GPIOD_5V,
>>>> + DA9150_GPADC_HW_CHAN_GPIOD_5V_,
>>>> + DA9150_GPADC_HW_CHAN_VBAT,
>>>> + DA9150_GPADC_HW_CHAN_VBAT_,
>>>> + DA9150_GPADC_HW_CHAN_TBAT,
>>>> + DA9150_GPADC_HW_CHAN_TBAT_,
>>>> + DA9150_GPADC_HW_CHAN_TJUNC_CORE,
>>>> + DA9150_GPADC_HW_CHAN_TJUNC_CORE_,
>>>> + DA9150_GPADC_HW_CHAN_TJUNC_OVP,
>>>> + DA9150_GPADC_HW_CHAN_TJUNC_OVP_,
>>>> +};
>>>> +
>>>> +enum da9150_gpadc_channel {
>>>> + DA9150_GPADC_CHAN_GPIOA = 0,
>>>> + DA9150_GPADC_CHAN_GPIOB,
>>>> + DA9150_GPADC_CHAN_GPIOC,
>>>> + DA9150_GPADC_CHAN_GPIOD,
>>>> + DA9150_GPADC_CHAN_IBUS,
>>>> + DA9150_GPADC_CHAN_VBUS,
>>>> + DA9150_GPADC_CHAN_ID,
>>>> + DA9150_GPADC_CHAN_VSYS,
>>>> + DA9150_GPADC_CHAN_VBAT,
>>>> + DA9150_GPADC_CHAN_TBAT,
>>>> + DA9150_GPADC_CHAN_TJUNC_CORE,
>>>> + DA9150_GPADC_CHAN_TJUNC_OVP,
>>>> +};
>>>> +
>>>> +/* Private data */
>>>> +struct da9150_gpadc {
>>>> + struct da9150 *da9150;
>>>> + struct device *dev;
>>>> +
>>>> + struct mutex lock;
>>>> + struct completion complete;
>>>> +};
>>>> +
>>>> +
>>>> +static irqreturn_t da9150_gpadc_irq(int irq, void *data)
>>>> +{
>>>> +
>>>> + struct da9150_gpadc *gpadc = data;
>>>> +
>>>> + complete(&gpadc->complete);
>>>> +
>>>> + return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +int da9150_gpadc_read_adc(struct da9150_gpadc *gpadc, int hw_chan)
>>>
>>> static?
>>>
>>>> +{
>>>> + u8 result_regs[2];
>>>> + int result;
>>>> +
>>>> + mutex_lock(&gpadc->lock);
>>>> +
>>>> + /* Set channel & enable measurement */
>>>> + da9150_reg_write(gpadc->da9150, DA9150_GPADC_MAN,
>>>> + (DA9150_GPADC_EN_MASK |
>>>> + hw_chan << DA9150_GPADC_MUX_SHIFT));
>>>> +
>>>> + /* Consume left-over completion from a previous timeout */
>>>> + try_wait_for_completion(&gpadc->complete);
>>>> +
>>>> + /* Check for actual completion */
>>>> + wait_for_completion_timeout(&gpadc->complete, msecs_to_jiffies(5));
>>>> +
>>>> + /* Read result and status from device */
>>>> + da9150_bulk_read(gpadc->da9150, DA9150_GPADC_RES_A, 2, result_regs);
>>>> +
>>>> + mutex_unlock(&gpadc->lock);
>>>> +
>>>> + /* Check to make sure device really has completed reading */
>>>> + if (result_regs[1] & DA9150_GPADC_RUN_MASK) {
>>>> + dev_err(gpadc->dev, "Timeout on channel %d of GP-ADC\n",
>>>
>>> GPADC everywhere else
>>>
>>>> + hw_chan);
>>>> + return -ETIMEDOUT;
>>>> + }
>>>> +
>>>> + /* LSBs - 2 bits */
>>>> + result = (result_regs[1] & DA9150_GPADC_RES_L_MASK) >>
>>>> + DA9150_GPADC_RES_L_SHIFT;
>>>> + /* MSBs - 8 bits */
>>>> + result |= result_regs[0] << DA9150_GPADC_RES_L_BITS;
>>>
>>> can't the read be done with a 16bit read?
>>>
>>>> +
>>>> + return result;
>>>> +}
>>>> +
>>>> +static inline int da9150_gpadc_gpio_5v_voltage_now(int raw_val)
>>>> +{
>>>> + /* Convert to uV */
>>>> + return (((6 * ((raw_val * 1000) + 500)) / 1024) * 1000);
>> Base units of voltage are millivolts. See Documentation/ABI/testing/sysfs-bus-iio.
>
> Right. Ok.
>
>>>
>>> outer parenthesis not needed, here and below
>>>
>>>> +}
>>>> +
>>>> +static inline int da9150_gpadc_ibus_current_avg(int raw_val)
>>>> +{
>>>> + /* Convert to uA */
>>>> + return (((4 * ((raw_val * 1000) + 500)) / 2048) * 1000);
>> interestingly this is actually our first current channel outside staging.
>> As such the docs don't cover it. Please add, keeping inline with the
>> units used in hwmon. mA, IIRC
>
> Right, ok.
>
>>>> +}
>>>> +
>>>> +static inline int da9150_gpadc_vbus_21v_voltage_now(int raw_val)
>>>> +{
>>>> + /* Convert to uV */
>>>> + return (((21 * ((raw_val * 1000) + 500)) / 1024) * 1000);
>>>> +}
>>>> +
>>>> +static inline int da9150_gpadc_vsys_6v_voltage_now(int raw_val)
>>>> +{
>>>> + /* Convert to uV */
>>>> + return (((3 * ((raw_val * 1000) + 500)) / 512) * 1000);
>>>> +}
>>>> +
>>>> +static inline int da9150_gpadc_tjunc_temp(int raw_val)
>>>> +{
>>>> + /* Convert to 0.1 degrees C */
>>>
>>> IIO wants mille degrees C
>> Exactly. Please check all of these correspond to the base units required
>> by the documentation. Most of these might be better handled as raw
>> with scale and offset provided, letting the maths be doing in userspace using
>> floating point.
>
> Some are needed by the charger driver. The intention of adding the conversions
> here was so this knowledge was not needed in the charger driver. Seems wrong to
> move the calculations out of the IIO driver. Most of these are not simple linear
> so scale and offset will not work, unless I'm missing something.
Umm. They all appear to be simple linear conversions to me - be it written a form
that slightly obscures that fact. But I'm not that fussed if you really want to
do them in kernel.
>
>>>
>>>> + return (((879 - (1023 - raw_val)) * 10000) / 4420);
>>>> +}
>>>> +
>>>> +static inline int da9150_gpadc_vbat_voltage_now(int raw_val)
>>>> +{
>>>> + /* Convert to uV */
>>>> + return ((2932 * raw_val) + 1500000);
>>>> +}
>>>> +
>>>> +int da9150_gpadc_read_processed(struct da9150_gpadc *gpadc, int channel,
>>>> + int hw_chan)
>>>
>>> static?
>>>
>>>> +{
>>>> + int raw_val, ret;
>>>> +
>>>> + raw_val = da9150_gpadc_read_adc(gpadc, hw_chan);
>>>> + if (raw_val < 0)
>>>> + return raw_val;
>>>> +
>>>> + switch (channel) {
>>>> + case DA9150_GPADC_CHAN_GPIOA:
>>>> + case DA9150_GPADC_CHAN_GPIOB:
>>>> + case DA9150_GPADC_CHAN_GPIOC:
>>>> + case DA9150_GPADC_CHAN_GPIOD:
>>>> + ret = da9150_gpadc_gpio_5v_voltage_now(raw_val);
>>>
>>> could return directly ...
>>>
>>>> + break;
>>>> + case DA9150_GPADC_CHAN_IBUS:
>>>> + ret = da9150_gpadc_ibus_current_avg(raw_val);
>>>> + break;
>>>> + case DA9150_GPADC_CHAN_VBUS:
>>>> + ret = da9150_gpadc_vbus_21v_voltage_now(raw_val);
>>>> + break;
>>>> + case DA9150_GPADC_CHAN_VSYS:
>>>> + ret = da9150_gpadc_vsys_6v_voltage_now(raw_val);
>>>> + break;
>>>> + case DA9150_GPADC_CHAN_TJUNC_CORE:
>>>> + case DA9150_GPADC_CHAN_TJUNC_OVP:
>>>> + ret = da9150_gpadc_tjunc_temp(raw_val);
>>>> + break;
>>>> + default:
>>>> + /* No processing for other channels so return raw value */
>>>> + ret = raw_val;
>>>> + break;
>>>> + }
>>>
>>> and save the final return
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +int da9150_gpadc_read_scale(int channel)
>>>
>>> static?
>>>
>>>> +{
>>>> + int ret;
>>>> +
>>>> + switch (channel) {
>>>> + case DA9150_GPADC_CHAN_VBAT:
>>>> + ret = 2932;
>>>
>>> return directly?
>>>
>>>> + break;
>>>> + default:
>>>> + ret = -EINVAL;
>>>> + }
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +int da9150_gpadc_read_offset(int channel)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + switch (channel) {
>>>> + case DA9150_GPADC_CHAN_VBAT:
>>>> + ret = 1500000 / 2932;
>>>
>>> return directly?
>>>
>>>> + break;
>>>> + default:
>>>> + ret = -EINVAL;
>>>> + }
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +int da9150_gpadc_read_raw(struct iio_dev *indio_dev,
>>>> + struct iio_chan_spec const *chan,
>>>> + int *val, int *val2, long mask)
>>>
>>> static?
>>>
>>>> +{
>>>> + struct da9150_gpadc *gpadc = iio_priv(indio_dev);
>>>> + int ret;
>>>> +
>>>> + if ((chan->channel < DA9150_GPADC_CHAN_GPIOA) ||
>>>> + (chan->channel > DA9150_GPADC_CHAN_TJUNC_OVP))
>>>> + return -EINVAL;
>>>> +
>>>> + switch (mask) {
>>>> + case IIO_CHAN_INFO_RAW:
>>>> + case IIO_CHAN_INFO_PROCESSED:
>> Would be cleaner to have each of these functions use ret for return code
>> including IIO_VAL_INT and pass val as an arguement. Would get rid
>> of the fiddly handling at the end of this function and allow direct
>> returns from each of the case statements.
>>
>> Also, are the base units of all of these channels really giving us
>> an integer result? E.g. mV, mA etc? Not impossible, but seems unlikely!
>
> Ok, I can make that change. Wouldn't say what was there now is fiddly, but am
> happy to update.
>
> In terms of base units, they are actually in V or A, rather than say mV or mA,
> according to the datasheet, but as there's no floating point support in the
> kernel the sensible option was to provide them in smaller units and provide
> greater accuracy.
No it really isn't. There is no floating point in kernel, so we simulate the
effect using the IIO_VAL types and a pair of 32bit integers. It's not ideal
but it does allow us to cover any plausible range. The reason other drivers need to
match it is that we need a clean ABI to userspace. Hence they must all be the same.
This is why we try to move the conversions into userspace where possible. It can
be done in kernel but is admittedly rather fiddly!
>
>>
>>>> + ret = da9150_gpadc_read_processed(gpadc, chan->channel,
>>>> + chan->address);
>>>> + break;
>>>> + case IIO_CHAN_INFO_SCALE:
>>>> + ret = da9150_gpadc_read_scale(chan->channel);
>>>> + break;
>>>> + case IIO_CHAN_INFO_OFFSET:
>>>> + ret = da9150_gpadc_read_offset(chan->channel);
>>>> + break;
>>>> + default:
>>>> + ret = -EINVAL;
>>>> + break;
>>>> + }
>>>> +
>>>> + if (ret < 0)
>>>> + return ret;
>>>> +
>>>> + *val = ret;
>>>> +
>>>> + return IIO_VAL_INT;
>>>> +}
>>>> +
>>>> +static const struct iio_info da9150_gpadc_info = {
>>>> + .read_raw = &da9150_gpadc_read_raw,
>>>> + .driver_module = THIS_MODULE,
>>>> +};
>>>> +
>>>> +#define GPADC_CHANNEL(_id, _hw_id, _type, chan_info, _ext_name) { \
>>>> + .type = _type, \
>>>> + .indexed = 1, \
>>>
>>> there is only one current channel, it should not be indexed
>> Optional. Either way userspace should cope fine. At one point I thought
>> about insisting everything was indexed, to reduce userspace complexity
>> (ever so slightly) but was a bit late by the time I thought of it ;)
>
> If it's ok, I will leave as is as to keep this macro common.
>
>>>
>>>> + .channel = DA9150_GPADC_CHAN_##_id, \
>>>> + .address = DA9150_GPADC_HW_CHAN_##_hw_id, \
>>>> + .info_mask_separate = chan_info, \
>>>> + .extend_name = _ext_name, \
>>>> + .datasheet_name = #_id, \
>>>> +}
>>>> +
>>>> +#define GPADC_CHANNEL_RAW(_id, _hw_id, _type, _ext_name) \
>>>> + GPADC_CHANNEL(_id, _hw_id, _type, BIT(IIO_CHAN_INFO_RAW),
>> _ext_name)
>>>> +
>>>> +#define GPADC_CHANNEL_SCALED(_id, _hw_id, _type, _ext_name) \
>>>> + GPADC_CHANNEL(_id, _hw_id, _type, \
>>>> + BIT(IIO_CHAN_INFO_RAW) | \
>>>> + BIT(IIO_CHAN_INFO_SCALE) | \
>>>> + BIT(IIO_CHAN_INFO_OFFSET), \
>>>> + _ext_name)
>>>> +
>>>> +#define GPADC_CHANNEL_PROCESSED(_id, _hw_id, _type, _ext_name)
>> \
>>>> + GPADC_CHANNEL(_id, _hw_id, _type, BIT(IIO_CHAN_INFO_PROCESSED), \
>>>> + _ext_name)
>> This macro and similar need a DA9150 prefix to avoid likely name clashes in
>> future.
>
> Fair point. Will update.
>
>>>> +
>>>> +/* Supported channels */
>>>> +static const struct iio_chan_spec da9150_gpadc_channels[] = {
>>>> + GPADC_CHANNEL_PROCESSED(GPIOA, GPIOA_5V, IIO_VOLTAGE, "GPIOA"),
>>>> + GPADC_CHANNEL_PROCESSED(GPIOB, GPIOB_5V, IIO_VOLTAGE, "GPIOB"),
>>>> + GPADC_CHANNEL_PROCESSED(GPIOC, GPIOC_5V, IIO_VOLTAGE, "GPIOC"),
>>>> + GPADC_CHANNEL_PROCESSED(GPIOD, GPIOD_5V, IIO_VOLTAGE, "GPIOD"),
>>>> + GPADC_CHANNEL_PROCESSED(IBUS, IBUS_SENSE, IIO_CURRENT, "IBUS"),
>>>> + GPADC_CHANNEL_PROCESSED(VBUS, VBUS_DIV_, IIO_VOLTAGE, "VBUS"),
>>>> + GPADC_CHANNEL_RAW(ID, ID, IIO_VOLTAGE, "ID"),
>>>> + GPADC_CHANNEL_PROCESSED(VSYS, VSYS, IIO_VOLTAGE, "VSYS"),
>>>> + GPADC_CHANNEL_SCALED(VBAT, VBAT, IIO_VOLTAGE, "VBAT"),
>>>> + GPADC_CHANNEL_RAW(TBAT, TBAT, IIO_VOLTAGE, "TBAT"),
>>>> + GPADC_CHANNEL_PROCESSED(TJUNC_CORE, TJUNC_CORE, IIO_TEMP,
>> "TJUNC_CORE"),
>>>> + GPADC_CHANNEL_PROCESSED(TJUNC_OVP, TJUNC_OVP, IIO_TEMP,
>> "TJUNC_OVP"),
>>>> +};
>>>> +
>>>> +/* Default maps used by da9150-charger */
>>>> +static struct iio_map da9150_gpadc_default_maps[] = {
>>>> + {
>>>> + .consumer_dev_name = "da9150-charger",
>>>> + .consumer_channel = "CHAN_IBUS",
>>>> + .adc_channel_label = "IBUS",
>>>> + },
>>>> + {
>>>> + .consumer_dev_name = "da9150-charger",
>>>> + .consumer_channel = "CHAN_VBUS",
>>>> + .adc_channel_label = "VBUS",
>>>> + },
>>>> + {
>>>> + .consumer_dev_name = "da9150-charger",
>>>> + .consumer_channel = "CHAN_TJUNC",
>>>> + .adc_channel_label = "TJUNC_CORE",
>>>> + },
>>>> + {
>>>> + .consumer_dev_name = "da9150-charger",
>>>> + .consumer_channel = "CHAN_VBAT",
>>>> + .adc_channel_label = "VBAT",
>>>> + },
>>>> + {},
>>>> +};
>>>> +
>>>> +static int da9150_gpadc_probe(struct platform_device *pdev)
>>>> +{
>>>> + struct device *dev = &pdev->dev;
>>>> + struct da9150 *da9150 = dev_get_drvdata(dev->parent);
>>>> + struct da9150_gpadc *gpadc;
>>>> + struct iio_dev *indio_dev;
>>>> + int ret;
>>>> +
>>>> + indio_dev = devm_iio_device_alloc(&pdev->dev,
>>>> + sizeof(struct da9150_gpadc));
>>>> + if (!indio_dev) {
>>>> + dev_err(&pdev->dev, "Failed to allocate IIO device\n");
>>>> + return -ENOMEM;
>>>> + }
>>>> + gpadc = iio_priv(indio_dev);
>>>> +
>>>> + platform_set_drvdata(pdev, indio_dev);
>>>> + gpadc->da9150 = da9150;
>>>> + gpadc->dev = dev;
>>>> +
>>>> + ret = iio_map_array_register(indio_dev, da9150_gpadc_default_maps);
>>>> + if (ret) {
>>>> + dev_err(dev, "Failed to register IIO maps: %d\n", ret);
>>>> + goto iio_map_fail;
>>>
>>> just return here?
>>>
>>>> + }
>>>> +
>>>> + mutex_init(&gpadc->lock);
>>>> + init_completion(&gpadc->complete);
>>>> +
>>>> + /* Register IRQ */
>>>> + ret = da9150_register_irq(pdev, gpadc, da9150_gpadc_irq, "GPADC");
>> This function wants renaming to indicate that it is doing a managed
>> irq request... devm_da9150 etc would be conventional ;)
>
> Yep, it's a good point. Will update.
>
>>>> + if (ret < 0)
>>>> + goto irq_fail;
>>>> +
>>>> + indio_dev->name = dev_name(dev);
>>>> + indio_dev->dev.parent = dev;
>>>> + indio_dev->dev.of_node = pdev->dev.of_node;
>>>> + indio_dev->info = &da9150_gpadc_info;
>>>> + indio_dev->modes = INDIO_DIRECT_MODE;
>>>> + indio_dev->channels = da9150_gpadc_channels;
>>>> + indio_dev->num_channels = ARRAY_SIZE(da9150_gpadc_channels);
>>>> +
>>>> + ret = iio_device_register(indio_dev);
>>>> + if (ret) {
>>>> + dev_err(dev, "Failed to register IIO device: %d\n", ret);
>>>> + goto iio_dev_fail;
>>>> + }
>>>> +
>>>> + return ret;
>>>> +
>>>> +iio_dev_fail:
>>>> +irq_fail:
>>>
>>> why two labels?
>>>
>>>> + iio_map_array_unregister(indio_dev);
>>>> +
>>>> +iio_map_fail:
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static int da9150_gpadc_remove(struct platform_device *pdev)
>>>> +{
>>>> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>>>> +
>>>> + iio_map_array_unregister(indio_dev);
>>>
>>> unregister irq?
>> Not actually needed as it was a devm register - having said that
>> the function should indicate that it's naming.
>
> Agreed.
>
>>>
>>>> + iio_device_unregister(indio_dev);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static struct platform_driver da9150_gpadc_driver = {
>>>> + .driver = {
>>>> + .name = "da9150-gpadc",
>>>> + .owner = THIS_MODULE,
>>>> + },
>>>> + .probe = da9150_gpadc_probe,
>>>> + .remove = da9150_gpadc_remove,
>>>> +};
>>>> +
>>>> +module_platform_driver(da9150_gpadc_driver);
>>>> +
>>>> +MODULE_DESCRIPTION("GPADC Driver for DA9150");
>>>> +MODULE_AUTHOR("Adam Thomson
>> <Adam.Thomson.Opensource@xxxxxxxxxxx");
>>>> +MODULE_LICENSE("GPL");
>>>> --
>>>> 1.9.3
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>>>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>> Please read the FAQ at http://www.tux.org/lkml/
>>>>
>>>
--
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
