On 04/07/2016 18:29, Guenter Roeck wrote: > On 07/04/2016 12:26 AM, Quentin Schulz wrote: >> On 03/07/2016 17:43, Guenter Roeck wrote: >>> On 07/03/2016 04:54 AM, Jonathan Cameron wrote: >>>> On 28/06/16 09:18, Quentin Schulz wrote: >>>>> The Allwinner SoCs all have an ADC that can also act as a touchscreen >>>>> controller and a thermal sensor. This patch adds the ADC driver >>>>> which is >>>>> based on the MFD for the same SoCs ADC. >>>>> >>>>> This also registers the thermal adc channel in the iio map array so >>>>> iio_hwmon could use it without modifying the Device Tree. >>>>> >>>>> This driver probes on three different platform_device_id to take into >>>>> account slight differences between Allwinner SoCs ADCs. >>>>> >>>>> Signed-off-by: Quentin Schulz <quentin.schulz@xxxxxxxxxxxxxxxxxx> >>>> Hi Quentin. >>>> >>>> I'm a bit in two minds about some of this. That temperature sensor is >>>> so obviously meant for hwmon purposes, I'm tempted to suggest it might >>>> actually make sense to put it directly in hwmon rather than using the >>>> bridge. That obviously makes it less flexible in some ways (i.e. for >>>> use within the thermal subsystem at some point). >>>> >>>> Guenter, what do you think? >>>> >>> >>> With the upcoming new hwmon API, thermal registration is handled in the >>> hwmon core, so that should not be an issue. Besides, other hwmon sensors >>> already register with the thermal subsystem as well. >>> >>> This is difficult to evaluate without datasheet; I am not sure if >>> the chip supports limits or trip points. If it supports trip points, >>> thermal may be a better target. >>> >>> Overall it does look like the temperature sensor would warrant >>> a separate driver. Only question is thermal or hwmon. >>> >>> Guenter >> >> Actually, the publicly available documentation for the temperature >> sensor is almost inexistent. We only have the registers for it. For most >> of the work on temperature sensor, it has been taken from the current >> driver (sun4i-ts in input/touschreen) from Hans de Goede. >> > Does this mean that the temperature data will be exported twice, once > through > iio and once through the touchscreen driver ? > > Guenter Currently, the temperature data is exported once in thermal framework from the sun4i-ts input driver. In the proposed patch, the temperature is exported twice: once in iio framework from the IIO driver and once in hwmon framework from iio_hwmon driver (which takes data from a channel of the IIO driver). Quentin >> You can find the documentation here: >> http://dl.linux-sunxi.org/A10/A10%20User%20manual%20V1.50.pdf >> http://dl.linux-sunxi.org/A13/A13%20User%20Manual%20V1.30.pdf >> http://dl.linux-sunxi.org/A31/A31%20User%20Manual%20V1.20.pdf >> >> The ADC is either called TP controller, Touch Panel controller or GPADC. >> The temperature sensor's registers are only defined in the User Manual >> of the A10 (first link). >> >> Nothing to be found for limits or trip points I think. >> >> Quentin >> >>>> I'm guessing detailed docs for this part aren't avaiable publicly? :( >>>> >>>> So the rest of my comments are kind of predicated on me having roughtly >>>> understood how this device works from the structure of the driver. >>>> >>>> The temperature sensor is really effectively as separate ADC? >>>> The main interest in this is for key detection? >>>> >>>> Anyhow, if the data flow for the temperatures sensor is not synced with >>>> the other ADC channels, adding buffered (pushed) output from the >>>> driver in >>>> future will be fiddly and with a 250Hz device you'll probably want it. >>>> Basically IIO buffered supports assumes each iio device will sample >>>> data >>>> at a particular frequency. If channels are not synchronized in that >>>> fashion >>>> then you have to register multiple devices or only pick a subset of >>>> channels >>>> to export. >>>> >>>> For the key detection you have already observed that IIO needs some >>>> additions to be able to have consumers of what we term 'events' e.g. >>>> threshold >>>> interrupts. >>>> >>>> Looking at the lradc-keys driver in tree, it looks like we only really >>>> have >>>> really simple threshold interrups - configured to detect a very low >>>> voltage? >>>> + only one per channel. >>>> >>>> So not too nasty a case, but you are right some work is needed in >>>> IIO as >>>> we simply don't have a means of passing these on as yet or configuring >>>> them >>>> from in kernel consumers. >>>> If we take the easy route and don't demux incoming events then it >>>> shouldn't >>>> be too hard to add (demux can follow later). Hence any client device >>>> can try >>>> to enable events it wants, but may get events that other client >>>> devices wanted >>>> as well. >>>> >>>> Config interface should be much the same as the write support for >>>> channels. >>>> Data flow marginally harder, but pretty much a list of callbacks within >>>> iio_push_event. >>>> >>>> Not trivial, but not too tricky either. >>>> >>>> The events subsystem has a few 'limitations' we need to address long >>>> term >>>> but as this is in kernel interface only, we can do this now and fix >>>> stuff >>>> up in future without any ABI breakage. (limitations are things like >>>> only >>>> one event of a given type and direction per channel - main challenge on >>>> that is finding a way of doing it without abi breakage). >>>> >>>> Anyhow, sounds fun - wish I had the time to do it myself! >>>> >>>> Otherwise, your remove is never going to work as indio_dev is always >>>> NULL. >>>> >>>> Jonathan >>>> >>>>> --- >>>>> drivers/iio/adc/Kconfig | 12 ++ >>>>> drivers/iio/adc/Makefile | 1 + >>>>> drivers/iio/adc/sunxi-gpadc-iio.c | 371 >>>>> ++++++++++++++++++++++++++++++++++++++ >>>>> 3 files changed, 384 insertions(+) >>>>> create mode 100644 drivers/iio/adc/sunxi-gpadc-iio.c >>>>> >>>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig >>>>> index 82c718c..b7b566a 100644 >>>>> --- a/drivers/iio/adc/Kconfig >>>>> +++ b/drivers/iio/adc/Kconfig >>>>> @@ -328,6 +328,18 @@ config NAU7802 >>>>> To compile this driver as a module, choose M here: the >>>>> module will be called nau7802. >>>>> >>>>> +config SUNXI_ADC >>>>> + tristate "ADC driver for sunxi platforms" >>>>> + depends on IIO >>>>> + depends on MFD_SUNXI_ADC >>>>> + help >>>>> + Say yes here to build support for Allwinner SoCs (A10, A13 and >>>>> A31) >>>>> + SoCs ADC. This ADC provides 4 channels which can be used as an >>>>> ADC or >>>>> + as a touchscreen input and one channel for thermal sensor. >>>>> + >>>>> + To compile this driver as a module, choose M here: the >>>>> + module will be called sunxi-gpadc-iio. >>>>> + >>>>> config PALMAS_GPADC >>>>> tristate "TI Palmas General Purpose ADC" >>>>> depends on MFD_PALMAS >>>>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile >>>>> index 0cb7921..2996a5b 100644 >>>>> --- a/drivers/iio/adc/Makefile >>>>> +++ b/drivers/iio/adc/Makefile >>>>> @@ -32,6 +32,7 @@ obj-$(CONFIG_MCP3422) += mcp3422.o >>>>> obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o >>>>> obj-$(CONFIG_MXS_LRADC) += mxs-lradc.o >>>>> obj-$(CONFIG_NAU7802) += nau7802.o >>>>> +obj-$(CONFIG_SUNXI_ADC) += sunxi-gpadc-iio.o >>>>> obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o >>>>> obj-$(CONFIG_QCOM_SPMI_IADC) += qcom-spmi-iadc.o >>>>> obj-$(CONFIG_QCOM_SPMI_VADC) += qcom-spmi-vadc.o >>>>> diff --git a/drivers/iio/adc/sunxi-gpadc-iio.c >>>>> b/drivers/iio/adc/sunxi-gpadc-iio.c >>>>> new file mode 100644 >>>>> index 0000000..5840f43 >>>>> --- /dev/null >>>>> +++ b/drivers/iio/adc/sunxi-gpadc-iio.c >>>>> @@ -0,0 +1,371 @@ >>>>> +#include <linux/module.h> >>>>> +#include <linux/platform_device.h> >>>>> +#include <linux/io.h> >>>>> +#include <linux/of.h> >>>>> +#include <linux/of_device.h> >>>>> +#include <linux/clk.h> >>>>> +#include <linux/completion.h> >>>>> +#include <linux/iio/iio.h> >>>>> +#include <linux/iio/machine.h> >>>>> +#include <linux/iio/driver.h> >>>>> +#include <linux/interrupt.h> >>>>> +#include <linux/mfd/sunxi-gpadc-mfd.h> >>>>> +#include <linux/regmap.h> >>>>> + >>>> I'm fussy about this: Please prefix all defines as some of these >>>> are generic enough they may well end up defined in an included header >>>> sometime in the future. >>>> >>>>> +#define TP_CTRL0 0x00 >>>>> +#define TP_CTRL1 0x04 >>>>> +#define TP_CTRL2 0x08 >>>>> +#define TP_CTRL3 0x0c >>>>> +#define TP_TPR 0x18 >>>>> +#define TP_CDAT 0x1c >>>>> +#define TEMP_DATA 0x20 >>>>> +#define TP_DATA 0x24 >>>>> + >>>>> +/* TP_CTRL0 bits */ >>>>> +#define ADC_FIRST_DLY(x) ((x) << 24) /* 8 bits */ >>>>> +#define ADC_FIRST_DLY_MODE BIT(23) >>>>> +#define ADC_CLK_SELECT BIT(22) >>>>> +#define ADC_CLK_DIVIDER(x) ((x) << 20) /* 2 bits */ >>>>> +#define FS_DIV(x) ((x) << 16) /* 4 bits */ >>>>> +#define T_ACQ(x) ((x) << 0) /* 16 bits*/ >>>>> + >>>>> +/* TP_CTRL1 bits */ >>>>> +#define STYLUS_UP_DEBOUNCE(x) ((x) << 12) /* 8 bits */ >>>>> +#define STYLUS_UP_DEBOUNCE_EN BIT(9) >>>>> +#define TOUCH_PAN_CALI_EN BIT(6) >>>>> +#define TP_DUAL_EN BIT(5) >>>>> +#define TP_MODE_EN BIT(4) >>>>> +#define TP_ADC_SELECT BIT(3) >>>>> +#define ADC_CHAN_SELECT(x) ((x) << 0) /* 3 bits */ >>>>> + >>>>> +/* TP_CTRL1 bits for sun6i SOCs */ >>>>> +#define SUN6I_TOUCH_PAN_CALI_EN BIT(7) >>>>> +#define SUN6I_TP_DUAL_EN BIT(6) >>>>> +#define SUN6I_TP_MODE_EN BIT(5) >>>>> +#define SUN6I_TP_ADC_SELECT BIT(4) >>>>> +#define SUN6I_ADC_CHAN_SELECT(x) BIT(x) /* 4 bits */ >>>>> + >>>>> +/* TP_CTRL2 bits */ >>>>> +#define TP_SENSITIVE_ADJUST(x) ((x) << 28) /* 4 bits */ >>>>> +#define TP_MODE_SELECT(x) ((x) << 26) /* 2 bits */ >>>>> +#define PRE_MEA_EN BIT(24) >>>>> +#define PRE_MEA_THRE_CNT(x) ((x) << 0) /* 24 bits*/ >>>>> + >>>>> +/* TP_CTRL3 bits */ >>>>> +#define FILTER_EN BIT(2) >>>>> +#define FILTER_TYPE(x) ((x) << 0) /* 2 bits */ >>>>> + >>>>> +/* TP_INT_FIFOC irq and fifo mask / control bits */ >>>>> +#define TEMP_IRQ_EN BIT(18) >>>>> +#define TP_OVERRUN_IRQ_EN BIT(17) >>>>> +#define TP_DATA_IRQ_EN BIT(16) >>>>> +#define TP_DATA_XY_CHANGE BIT(13) >>>>> +#define TP_FIFO_TRIG_LEVEL(x) ((x) << 8) /* 5 bits */ >>>>> +#define TP_DATA_DRQ_EN BIT(7) >>>>> +#define TP_FIFO_FLUSH BIT(4) >>>>> +#define TP_UP_IRQ_EN BIT(1) >>>>> +#define TP_DOWN_IRQ_EN BIT(0) >>>>> + >>>>> +/* TP_INT_FIFOS irq and fifo status bits */ >>>>> +#define TEMP_DATA_PENDING BIT(18) >>>>> +#define FIFO_OVERRUN_PENDING BIT(17) >>>>> +#define FIFO_DATA_PENDING BIT(16) >>>>> +#define TP_IDLE_FLG BIT(2) >>>>> +#define TP_UP_PENDING BIT(1) >>>>> +#define TP_DOWN_PENDING BIT(0) >>>>> + >>>>> +/* TP_TPR bits */ >>>>> +#define TEMP_ENABLE(x) ((x) << 16) >>>>> +#define TEMP_PERIOD(x) ((x) << 0) /*t = x * 256 * 16 / >>>>> clkin*/ >>>>> + >>>>> +#define ARCH_SUN4I BIT(0) >>>>> +#define ARCH_SUN5I BIT(1) >>>>> +#define ARCH_SUN6I BIT(2) >>>>> + >>>>> +struct sunxi_gpadc_dev { >>>>> + void __iomem *regs; >>>>> + struct completion completion; >>>>> + int temp_data; >>>>> + u32 adc_data; >>>>> + struct regmap *regmap; >>>>> + unsigned int fifo_data_irq; >>>>> + unsigned int temp_data_irq; >>>>> + unsigned int flags; >>>>> +}; >>>>> + >>>>> +#define ADC_CHANNEL(_channel, _name) { \ >>>>> + .type = IIO_VOLTAGE, \ >>>>> + .indexed = 1, \ >>>>> + .channel = _channel, \ >>>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ >>>>> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ >>>>> + .datasheet_name = _name, \ >>>>> +} >>>>> + >>>>> +static struct iio_map sunxi_gpadc_hwmon_maps[] = { >>>>> + { >>>>> + .adc_channel_label = "temp_adc", >>>>> + .consumer_dev_name = "iio_hwmon.0", >>>>> + }, >>>>> + {}, >>>>> +}; >>>>> + >>>>> +static const struct iio_chan_spec sunxi_gpadc_channels[] = { >>>>> + ADC_CHANNEL(0, "adc_chan0"), >>>>> + ADC_CHANNEL(1, "adc_chan1"), >>>>> + ADC_CHANNEL(2, "adc_chan2"), >>>>> + ADC_CHANNEL(3, "adc_chan3"), >>>>> + { >>>>> + .type = IIO_TEMP, >>>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), >>>>> + .datasheet_name = "temp_adc", >>>>> + }, >>>>> +}; >>>>> + >>>>> +static int sunxi_gpadc_adc_read(struct iio_dev *indio_dev, int >>>>> channel) >>>>> +{ >>>>> + struct sunxi_gpadc_dev *info = iio_priv(indio_dev); >>>>> + int val = 0; >>>>> + >>>>> + mutex_lock(&indio_dev->mlock); >>>>> + >>>>> + reinit_completion(&info->completion); >>>>> + regmap_write(info->regmap, TP_CTRL1, SUN6I_TP_MODE_EN | >>>>> + SUN6I_TP_ADC_SELECT | >>>>> + SUN6I_ADC_CHAN_SELECT(channel)); >>>>> + regmap_write(info->regmap, TP_INT_FIFOC, TP_FIFO_TRIG_LEVEL(1) | >>>>> + TP_FIFO_FLUSH); >>>>> + enable_irq(info->fifo_data_irq); >>>>> + >>>>> + if (!wait_for_completion_timeout(&info->completion, >>>>> + msecs_to_jiffies(100))) { >>>>> + disable_irq(info->fifo_data_irq); >>>>> + mutex_unlock(&indio_dev->mlock); >>>>> + return -ETIMEDOUT; >>>>> + } >>>>> + >>>>> + val = info->adc_data; >>>>> + disable_irq(info->fifo_data_irq); >>>>> + mutex_unlock(&indio_dev->mlock); >>>>> + >>>>> + return val; >>>>> +} >>>>> + >>>>> +static int sunxi_gpadc_temp_read(struct iio_dev *indio_dev) >>>>> +{ >>>>> + struct sunxi_gpadc_dev *info = iio_priv(indio_dev); >>>>> + int val = 0; >>>>> + >>>>> + mutex_lock(&indio_dev->mlock); >>>>> + >>>>> + reinit_completion(&info->completion); >>>>> + >>>>> + regmap_write(info->regmap, TP_INT_FIFOC, TP_FIFO_TRIG_LEVEL(1) | >>>>> + TP_FIFO_FLUSH); >>>>> + regmap_write(info->regmap, TP_CTRL1, SUN6I_TP_MODE_EN); >>>>> + enable_irq(info->temp_data_irq); >>>>> + >>>>> + if (!wait_for_completion_timeout(&info->completion, >>>>> + msecs_to_jiffies(100))) { >>>>> + disable_irq(info->temp_data_irq); >>>>> + mutex_unlock(&indio_dev->mlock); >>>>> + return -ETIMEDOUT; >>>>> + } >>>>> + >>>>> + if (info->flags & ARCH_SUN4I) >>>>> + val = info->temp_data * 133 - 257000; >>>>> + else if (info->flags & ARCH_SUN5I) >>>>> + val = info->temp_data * 100 - 144700; >>>>> + else if (info->flags & ARCH_SUN6I) >>>>> + val = info->temp_data * 167 - 271000; >>>>> + >>>>> + disable_irq(info->temp_data_irq); >>>>> + mutex_unlock(&indio_dev->mlock); >>>>> + return val; >>>>> +} >>>>> + >>>>> +static int sunxi_gpadc_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_PROCESSED: >>>>> + *val = sunxi_gpadc_temp_read(indio_dev); >>>>> + if (*val < 0) >>>>> + return *val; >>>>> + >>>>> + return IIO_VAL_INT; >>>>> + case IIO_CHAN_INFO_RAW: >>>>> + *val = sunxi_gpadc_adc_read(indio_dev, chan->channel); >>>>> + if (*val < 0) >>>>> + return *val; >>>>> + >>>>> + return IIO_VAL_INT; >>>>> + default: >>>>> + break; >>>>> + } >>>>> + >>>>> + return -EINVAL; >>>>> +} >>>>> + >>>>> +static const struct iio_info sunxi_gpadc_iio_info = { >>>>> + .read_raw = sunxi_gpadc_read_raw, >>>>> + .driver_module = THIS_MODULE, >>>>> +}; >>>>> + >>>>> +static irqreturn_t sunxi_gpadc_temp_data_irq_handler(int irq, void >>>>> *dev_id) >>>>> +{ >>>>> + struct sunxi_gpadc_dev *info = dev_id; >>>>> + int ret; >>>>> + >>>>> + ret = regmap_read(info->regmap, TEMP_DATA, &info->temp_data); >>>>> + if (ret == 0) >>>>> + complete(&info->completion); >>>>> + >>>>> + return IRQ_HANDLED; >>>>> +} >>>> Interesting that it's a separate data flow for the temperature sensor. >>>> That means that if you add buffered (pushed data) support in future >>>> either >>>> you'll need to split the device in two or not allow the temperature >>>> channel >>>> to go through the buffer interface. >>>>> + >>>>> +static irqreturn_t sunxi_gpadc_fifo_data_irq_handler(int irq, void >>>>> *dev_id) >>>>> +{ >>>>> + struct sunxi_gpadc_dev *info = dev_id; >>>>> + int ret; >>>>> + >>>>> + ret = regmap_read(info->regmap, TP_DATA, &info->adc_data); >>>>> + if (ret == 0) >>>>> + complete(&info->completion); >>>>> + >>>>> + return IRQ_HANDLED; >>>>> +} >>>>> + >>>>> +static int sunxi_gpadc_probe(struct platform_device *pdev) >>>>> +{ >>>>> + struct sunxi_gpadc_dev *info = NULL; >>>>> + struct iio_dev *indio_dev = NULL; >>>>> + int ret = 0; >>>>> + unsigned int irq; >>>>> + struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev; >>>>> + >>>>> + sunxi_gpadc_mfd_dev = dev_get_drvdata(pdev->dev.parent); >>>>> + >>>>> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info)); >>>>> + if (!indio_dev) { >>>>> + dev_err(&pdev->dev, "failed to allocate iio device.\n"); >>>>> + return -ENOMEM; >>>>> + } >>>>> + info = iio_priv(indio_dev); >>>>> + >>>>> + info->regmap = sunxi_gpadc_mfd_dev->regmap; >>>>> + init_completion(&info->completion); >>>>> + indio_dev->name = dev_name(&pdev->dev); >>>>> + indio_dev->dev.parent = &pdev->dev; >>>>> + indio_dev->dev.of_node = pdev->dev.of_node; >>>>> + indio_dev->info = &sunxi_gpadc_iio_info; >>>>> + indio_dev->modes = INDIO_DIRECT_MODE; >>>>> + indio_dev->num_channels = ARRAY_SIZE(sunxi_gpadc_channels); >>>>> + indio_dev->channels = sunxi_gpadc_channels; >>>>> + >>>>> + info->flags = platform_get_device_id(pdev)->driver_data; >>>>> + >>>>> + regmap_write(info->regmap, TP_CTRL0, ADC_CLK_DIVIDER(2) | >>>>> + FS_DIV(7) | >>>>> + T_ACQ(63)); >>>>> + regmap_write(info->regmap, TP_CTRL1, SUN6I_TP_MODE_EN); >>>>> + regmap_write(info->regmap, TP_CTRL3, FILTER_EN | FILTER_TYPE(1)); >>>>> + regmap_write(info->regmap, TP_TPR, TEMP_ENABLE(1) | >>>>> TEMP_PERIOD(1953)); >>>>> + >>>>> + irq = platform_get_irq_byname(pdev, "TEMP_DATA_PENDING"); >>>>> + if (irq < 0) { >>>>> + dev_err(&pdev->dev, >>>>> + "no TEMP_DATA_PENDING interrupt registered\n"); >>>>> + return irq; >>>>> + } >>>>> + >>>>> + irq = regmap_irq_get_virq(sunxi_gpadc_mfd_dev->regmap_irqc, irq); >>>>> + ret = devm_request_any_context_irq(&pdev->dev, irq, >>>>> + sunxi_gpadc_temp_data_irq_handler, >>>>> + 0, "temp_data", info); >>>>> + if (ret < 0) { >>>>> + dev_err(&pdev->dev, >>>>> + "could not request TEMP_DATA_PENDING interrupt: %d\n", >>>>> + ret); >>>>> + return ret; >>>>> + } >>>>> + >>>>> + info->temp_data_irq = irq; >>>>> + disable_irq(irq); >>>>> + >>>>> + irq = platform_get_irq_byname(pdev, "FIFO_DATA_PENDING"); >>>> hohum. A fifo? This part is getting more interesting ;) I'll have to >>>> dig out the datasheet at some point (if public). >>>>> + if (irq < 0) { >>>>> + dev_err(&pdev->dev, >>>>> + "no FIFO_DATA_PENDING interrupt registered\n"); >>>>> + return irq; >>>>> + } >>>>> + >>>>> + irq = regmap_irq_get_virq(sunxi_gpadc_mfd_dev->regmap_irqc, irq); >>>>> + ret = devm_request_any_context_irq(&pdev->dev, irq, >>>>> + sunxi_gpadc_fifo_data_irq_handler, >>>>> + 0, "fifo_data", info); >>>>> + if (ret < 0) { >>>>> + dev_err(&pdev->dev, >>>>> + "could not request FIFO_DATA_PENDING interrupt: %d\n", >>>>> + ret); >>>>> + return ret; >>>>> + } >>>>> + >>>>> + info->fifo_data_irq = irq; >>>>> + disable_irq(irq); >>>>> + >>>>> + ret = iio_map_array_register(indio_dev, sunxi_gpadc_hwmon_maps); >>>> As mentioned for previous patch I think this should be described >>>> externally. >>>> Chances are that some of those other adc channels are also going to be >>>> in reality used for hwmon anyway so doing it in the device tree will >>>> give >>>> you more flexibility. >>>> >>>>> + if (ret < 0) { >>>>> + dev_err(&pdev->dev, "failed to register iio map array\n"); >>>>> + return ret; >>>>> + } >>>>> + >>>>> + platform_set_drvdata(pdev, indio_dev); >>>>> + >>>>> + ret = iio_device_register(indio_dev); >>>>> + if (ret < 0) { >>>>> + dev_err(&pdev->dev, "could not register the device\n"); >>>>> + iio_map_array_unregister(indio_dev); >>>>> + return ret; >>>>> + } >>>>> + >>>> This is kind of self evident when the device turns up so I'd not bother >>>> cluttering up the logs with it as no additional information is given. >>>>> + dev_info(&pdev->dev, "successfully loaded\n"); >>>>> + >>>>> + return ret; >>>>> +} >>>>> + >>>>> +static int sunxi_gpadc_remove(struct platform_device *pdev) >>>>> +{ >>>>> + struct iio_dev *indio_dev = NULL; >>>> ? If it's null the below isn't going to work as intended. >>>> Missing a platform_get_drvdata which I'd just roll into the above >>>> local variable definition. >>>> >>>>> + struct sunxi_gpadc_dev *info = NULL; >>>>> + >>>>> + iio_device_unregister(indio_dev); >>>>> + iio_map_array_unregister(indio_dev); >>>>> + info = iio_priv(indio_dev); >>>> I'd roll this into the local variable declaration above >>>> (it's only a trivial bit of pointer arithemetic after all. >>>> struct sunxi_gpadc_dev *info = iio_priv(indio_dev); >>>> >>>>> + regmap_write(info->regmap, TP_INT_FIFOC, 0); >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static const struct platform_device_id sunxi_gpadc_id[] = { >>>>> + { "sun4i-a10-gpadc-iio", ARCH_SUN4I }, >>>>> + { "sun5i-a13-gpadc-iio", ARCH_SUN5I }, >>>>> + { "sun6i-a31-gpadc-iio", ARCH_SUN6I }, >>>>> + { /*sentinel*/ }, >>>>> +}; >>>>> + >>>>> +static struct platform_driver sunxi_gpadc_driver = { >>>>> + .driver = { >>>>> + .name = "sunxi-gpadc-iio", >>>>> + }, >>>>> + .id_table = sunxi_gpadc_id, >>>>> + .probe = sunxi_gpadc_probe, >>>>> + .remove = sunxi_gpadc_remove, >>>>> +}; >>>>> + >>>>> +module_platform_driver(sunxi_gpadc_driver); >>>>> + >>>>> +MODULE_DESCRIPTION("ADC driver for sunxi platforms"); >>>>> +MODULE_AUTHOR("Quentin Schulz <quentin.schulz@xxxxxxxxxxxxxxxxxx>"); >>>>> +MODULE_LICENSE("GPL v2"); >>>>> >>>> >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe >>>> linux-hwmon" 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
