| Message ID | 1447075704-4605-2-git-send-email-haibo.chen@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/09/2015 02:28 PM, Haibo Chen wrote: > Freescale i.MX7D soc contains a new ADC IP. This patch add this ADC > driver support, and the driver only support ADC software trigger. > > Signed-off-by: Haibo Chen <haibo.chen@freescale.com> Looks pretty good, a few comments inline. [...] > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/interrupt.h> > +#include <linux/delay.h> > +#include <linux/kernel.h> > +#include <linux/slab.h> > +#include <linux/io.h> > +#include <linux/clk.h> > +#include <linux/completion.h> > +#include <linux/of.h> > +#include <linux/of_irq.h> > +#include <linux/regulator/consumer.h> > +#include <linux/of_platform.h> > +#include <linux/err.h> I don't think you need all of these. [...] > +static void imx7d_adc_feature_config(struct imx7d_adc *info) > +{ > + info->adc_feature.clk_pre_div = IMX7D_ADC_ANALOG_CLK_PRE_DIV_4; > + info->adc_feature.avg_num = IMX7D_ADC_AVERAGE_NUM_32; > + info->adc_feature.core_time_unit = 1; > + info->adc_feature.average_en = true; What's the plan for these? Right now they are always initialized to the same static value. > +} [...] > +static int imx7d_adc_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, > + int *val2, > + long mask) > +{ > + struct imx7d_adc *info = iio_priv(indio_dev); > + > + u32 channel; > + long ret; > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + mutex_lock(&indio_dev->mlock); > + reinit_completion(&info->completion); > + > + channel = (chan->channel) & 0x0f; > + info->channel = channel; > + imx7d_adc_channel_set(info); How about just passing channel directy adc_channel_set() instead of doing it implicitly through the info struct? [...] > +static irqreturn_t imx7d_adc_isr(int irq, void *dev_id) > +{ > + struct imx7d_adc *info = (struct imx7d_adc *)dev_id; > + int status; > + > + status = readl(info->regs + IMX7D_REG_ADC_INT_STATUS); > + if (status & IMX7D_REG_ADC_INT_STATUS_CHANNEL_INT_STATUS) { > + info->value = imx7d_adc_read_data(info); > + complete(&info->completion); > + } > + writel(0, info->regs + IMX7D_REG_ADC_INT_STATUS); Is the hardware really this broken? If the interrupt happens between reading the status register and clearing it here it will be missed. > + > + return IRQ_HANDLED; You should only return IRQ_HANDLED if you actually handled are interrupt. > +} [...] > + > +static int imx7d_adc_probe(struct platform_device *pdev) > +{ > + struct imx7d_adc *info; > + struct iio_dev *indio_dev; > + struct resource *mem; > + int irq; > + int ret; > + u32 channels; > + > + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(struct imx7d_adc)); > + if (!indio_dev) { > + dev_err(&pdev->dev, "Failed allocating iio device\n"); > + return -ENOMEM; > + } > + > + info = iio_priv(indio_dev); > + info->dev = &pdev->dev; > + > + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + info->regs = devm_ioremap_resource(&pdev->dev, mem); > + if (IS_ERR(info->regs)) { > + ret = PTR_ERR(info->regs); > + dev_err(&pdev->dev, "failed to remap adc memory: %d\n", ret); > + return ret; > + } > + > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) { > + dev_err(&pdev->dev, "no irq resource?\n"); > + return irq; > + } > + > + ret = devm_request_irq(info->dev, irq, > + imx7d_adc_isr, 0, > + dev_name(&pdev->dev), info); This is too early. Completion is not initialized, clocks are not enabled, etc... > + if (ret < 0) { > + dev_err(&pdev->dev, "failed requesting irq, irq = %d\n", irq); > + return ret; > + } > + [...] > + ret = of_property_read_u32(pdev->dev.of_node, Extra space. > + "num-channels", &channels); What decides how many channels are in use? > + if (ret) > + channels = ARRAY_SIZE(imx7d_adc_iio_channels); > + > + 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 = &imx7d_adc_iio_info; > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->channels = imx7d_adc_iio_channels; > + indio_dev->num_channels = (int)channels; I don't think you need the case here. >[...]
On Tue, Nov 10, 2015 at 03:58:14PM +0100, Lars-Peter Clausen wrote: > On 11/09/2015 02:28 PM, Haibo Chen wrote: > > Freescale i.MX7D soc contains a new ADC IP. This patch add this ADC > > driver support, and the driver only support ADC software trigger. > > > > Signed-off-by: Haibo Chen <haibo.chen@freescale.com> > > Looks pretty good, a few comments inline. > > [...] > > +#include <linux/module.h> > > +#include <linux/platform_device.h> > > +#include <linux/interrupt.h> > > +#include <linux/delay.h> > > +#include <linux/kernel.h> > > +#include <linux/slab.h> > > +#include <linux/io.h> > > +#include <linux/clk.h> > > +#include <linux/completion.h> > > +#include <linux/of.h> > > +#include <linux/of_irq.h> > > +#include <linux/regulator/consumer.h> > > +#include <linux/of_platform.h> > > +#include <linux/err.h> > > I don't think you need all of these. > > [...] Yes, I will remove delay.h slab.h of_irq.h and of_platform.h > > +static void imx7d_adc_feature_config(struct imx7d_adc *info) > > +{ > > + info->adc_feature.clk_pre_div = IMX7D_ADC_ANALOG_CLK_PRE_DIV_4; > > + info->adc_feature.avg_num = IMX7D_ADC_AVERAGE_NUM_32; > > + info->adc_feature.core_time_unit = 1; > > + info->adc_feature.average_en = true; > > What's the plan for these? Right now they are always initialized to the same > static value. > In future, we can get these values from dts file, currently we just use the static value. > > > +} > [...] > > +static int imx7d_adc_read_raw(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, > > + int *val, > > + int *val2, > > + long mask) > > +{ > > + struct imx7d_adc *info = iio_priv(indio_dev); > > + > > + u32 channel; > > + long ret; > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + mutex_lock(&indio_dev->mlock); > > + reinit_completion(&info->completion); > > + > > + channel = (chan->channel) & 0x0f; > > + info->channel = channel; > > + imx7d_adc_channel_set(info); > > How about just passing channel directy adc_channel_set() instead of doing it > implicitly through the info struct? > I think there is no difference, besides, using this parameter info struct can keep align with other functions. eg. imx7d_adc_sample_set(), imx7d_adc_hw_init(), imx7d_adc_get_sample_rate(), all these functions have the same parameter. > [...] > > +static irqreturn_t imx7d_adc_isr(int irq, void *dev_id) > > +{ > > + struct imx7d_adc *info = (struct imx7d_adc *)dev_id; > > + int status; > > + > > + status = readl(info->regs + IMX7D_REG_ADC_INT_STATUS); > > + if (status & IMX7D_REG_ADC_INT_STATUS_CHANNEL_INT_STATUS) { > > + info->value = imx7d_adc_read_data(info); > > + complete(&info->completion); > > + } > > + writel(0, info->regs + IMX7D_REG_ADC_INT_STATUS); > > Is the hardware really this broken? If the interrupt happens between reading > the status register and clearing it here it will be missed. > I think interrupt can't happen between reading the status register and clearing it. Because in function imx7d_adc_read_raw(), we call the function imx7d_adc_channel_set(info), in this function, we config the register REG_ADC_CH_A\B\C\D_CFG1 and REG_ADC_CH_A\B\C\D_CFG2, only when these registers is configed, ADC start a conversion. Once the conversion complete, ADC trigger an interrupt, and call the imx7d_adc_isr(). > > + > > + return IRQ_HANDLED; > > You should only return IRQ_HANDLED if you actually handled are interrupt. > Here in the interrupt, we just handle the channel conversion finished flag, for other flag, ignore them this time, Will add other flag in future. > > +} > [...] > > + > > +static int imx7d_adc_probe(struct platform_device *pdev) > > +{ > > + struct imx7d_adc *info; > > + struct iio_dev *indio_dev; > > + struct resource *mem; > > + int irq; > > + int ret; > > + u32 channels; > > + > > + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(struct imx7d_adc)); > > + if (!indio_dev) { > > + dev_err(&pdev->dev, "Failed allocating iio device\n"); > > + return -ENOMEM; > > + } > > + > > + info = iio_priv(indio_dev); > > + info->dev = &pdev->dev; > > + > > + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + info->regs = devm_ioremap_resource(&pdev->dev, mem); > > + if (IS_ERR(info->regs)) { > > + ret = PTR_ERR(info->regs); > > + dev_err(&pdev->dev, "failed to remap adc memory: %d\n", ret); > > + return ret; > > + } > > + > > + irq = platform_get_irq(pdev, 0); > > + if (irq < 0) { > > + dev_err(&pdev->dev, "no irq resource?\n"); > > + return irq; > > + } > > + > > + ret = devm_request_irq(info->dev, irq, > > + imx7d_adc_isr, 0, > > + dev_name(&pdev->dev), info); > > This is too early. Completion is not initialized, clocks are not enabled, etc... > You are right, I will move the request irq function down. > > + if (ret < 0) { > > + dev_err(&pdev->dev, "failed requesting irq, irq = %d\n", irq); > > + return ret; > > + } > > + > [...] > > + ret = of_property_read_u32(pdev->dev.of_node, > > Extra space. > > > + "num-channels", &channels); > > What decides how many channels are in use? > The board decides the channel number. In dts file, there is a line: num-channels = <4>; > > + if (ret) > > + channels = ARRAY_SIZE(imx7d_adc_iio_channels); > > + > > + 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 = &imx7d_adc_iio_info; > > + indio_dev->modes = INDIO_DIRECT_MODE; > > + indio_dev->channels = imx7d_adc_iio_channels; > > + indio_dev->num_channels = (int)channels; > > I don't think you need the case here. > Sorry, can't get your point, you mean I should not indio_dev-> ? > >[...]
On 11/13/2015 10:39 AM, Haibo Chen wrote: [...] >>> +static void imx7d_adc_feature_config(struct imx7d_adc *info) >>> +{ >>> + info->adc_feature.clk_pre_div = IMX7D_ADC_ANALOG_CLK_PRE_DIV_4; >>> + info->adc_feature.avg_num = IMX7D_ADC_AVERAGE_NUM_32; >>> + info->adc_feature.core_time_unit = 1; >>> + info->adc_feature.average_en = true; >> >> What's the plan for these? Right now they are always initialized to the same >> static value. >> > > In future, we can get these values from dts file, currently we just use the static value. Those seem to be software configuration values though, not part of hardware description. > >> >>> +} >> [...] >>> +static int imx7d_adc_read_raw(struct iio_dev *indio_dev, >>> + struct iio_chan_spec const *chan, >>> + int *val, >>> + int *val2, >>> + long mask) >>> +{ >>> + struct imx7d_adc *info = iio_priv(indio_dev); >>> + >>> + u32 channel; >>> + long ret; >>> + >>> + switch (mask) { >>> + case IIO_CHAN_INFO_RAW: >>> + mutex_lock(&indio_dev->mlock); >>> + reinit_completion(&info->completion); >>> + >>> + channel = (chan->channel) & 0x0f; >>> + info->channel = channel; >>> + imx7d_adc_channel_set(info); >> >> How about just passing channel directy adc_channel_set() instead of doing it >> implicitly through the info struct? >> > > I think there is no difference, besides, using this parameter info struct can keep align with other functions. > eg. imx7d_adc_sample_set(), imx7d_adc_hw_init(), imx7d_adc_get_sample_rate(), all these functions have the same parameter. > >> [...] >>> +static irqreturn_t imx7d_adc_isr(int irq, void *dev_id) >>> +{ >>> + struct imx7d_adc *info = (struct imx7d_adc *)dev_id; >>> + int status; >>> + >>> + status = readl(info->regs + IMX7D_REG_ADC_INT_STATUS); >>> + if (status & IMX7D_REG_ADC_INT_STATUS_CHANNEL_INT_STATUS) { >>> + info->value = imx7d_adc_read_data(info); >>> + complete(&info->completion); >>> + } >>> + writel(0, info->regs + IMX7D_REG_ADC_INT_STATUS); >> >> Is the hardware really this broken? If the interrupt happens between reading >> the status register and clearing it here it will be missed. >> > > I think interrupt can't happen between reading the status register and clearing it. > Because in function imx7d_adc_read_raw(), we call the function > imx7d_adc_channel_set(info), in this function, we config the register > REG_ADC_CH_A\B\C\D_CFG1 and REG_ADC_CH_A\B\C\D_CFG2, only when these registers > is configed, ADC start a conversion. Once the conversion complete, ADC trigger an > interrupt, and call the imx7d_adc_isr(). Well an interrupt can always happen, its running fully asynchronous to the CPU. If you have multiple interrupt sources enabled and the first one fires and you run then start the irq handler, read the status register, now the second irq sources fires, and then you clear the status interrupt register. This means the driver will not see the irq from the second source. > >>> + >>> + return IRQ_HANDLED; >> >> You should only return IRQ_HANDLED if you actually handled are interrupt. >> > > Here in the interrupt, we just handle the channel conversion finished flag, for > other flag, ignore them this time, Will add other flag in future. Yeah, but if you don't handle any interrupt you should return IRQ_NONE. This will make sure that the system can recover in case the hardware (or the driver) is broken and generates unexpected interrupts. If there are a 1000 or so IRQ_NONEs in a row in a short time frame the kernel will disable the IRQ. >> [...] >>> + ret = of_property_read_u32(pdev->dev.of_node, >> >> Extra space. >> >>> + "num-channels", &channels); >> >> What decides how many channels are in use? >> > > The board decides the channel number. > In dts file, there is a line: num-channels = <4>; Yes, but what decides how this property should be configured? > > >>> + if (ret) >>> + channels = ARRAY_SIZE(imx7d_adc_iio_channels); >>> + >>> + 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 = &imx7d_adc_iio_info; >>> + indio_dev->modes = INDIO_DIRECT_MODE; >>> + indio_dev->channels = imx7d_adc_iio_channels; >>> + indio_dev->num_channels = (int)channels; >> >> I don't think you need the case here. >> > > Sorry, can't get your point, you mean I should not indio_dev-> ? Sorry, I meant the (int) cast. > >>> [...] >
On 09/11/15 13:28, Haibo Chen wrote: > Freescale i.MX7D soc contains a new ADC IP. This patch add this ADC > driver support, and the driver only support ADC software trigger. > > Signed-off-by: Haibo Chen <haibo.chen@freescale.com> Hi Haibo, A few comments inline in addition to Lars' ones. One small point is that whilst the 80 char limit should be ignored when obeying it significantly hurts readability, there are cases in here where it wouldn't hurt to keep the lines shorter, so that would be preferred. Mostly looking good. Jonathan > --- > drivers/iio/adc/Kconfig | 9 + > drivers/iio/adc/Makefile | 1 + > drivers/iio/adc/imx7d_adc.c | 571 ++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 581 insertions(+) > create mode 100644 drivers/iio/adc/imx7d_adc.c > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index 7868c74..bf0611c 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -194,6 +194,15 @@ config HI8435 > This driver can also be built as a module. If so, the module will be > called hi8435. > > +config IMX7D_ADC > + tristate "IMX7D ADC driver" > + depends on OF > + help > + Say yes here to build support for IMX7D ADC. > + > + This driver can also be built as a module. If so, the module will be > + called imx7d_adc. > + > config LP8788_ADC > tristate "LP8788 ADC driver" > depends on MFD_LP8788 > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile > index 99b37a9..282ffc01 100644 > --- a/drivers/iio/adc/Makefile > +++ b/drivers/iio/adc/Makefile > @@ -20,6 +20,7 @@ obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o > obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o > obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o > obj-$(CONFIG_HI8435) += hi8435.o > +obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o > obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o > obj-$(CONFIG_MAX1027) += max1027.o > obj-$(CONFIG_MAX1363) += max1363.o > diff --git a/drivers/iio/adc/imx7d_adc.c b/drivers/iio/adc/imx7d_adc.c > new file mode 100644 > index 0000000..7e25e4e > --- /dev/null > +++ b/drivers/iio/adc/imx7d_adc.c > @@ -0,0 +1,571 @@ > +/* > + * Freescale i.MX7D ADC driver > + * > + * Copyright (C) 2015 Freescale Semiconductor, Inc. > + * > + * 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/module.h> > +#include <linux/platform_device.h> > +#include <linux/interrupt.h> > +#include <linux/delay.h> > +#include <linux/kernel.h> > +#include <linux/slab.h> > +#include <linux/io.h> > +#include <linux/clk.h> > +#include <linux/completion.h> > +#include <linux/of.h> > +#include <linux/of_irq.h> > +#include <linux/regulator/consumer.h> > +#include <linux/of_platform.h> > +#include <linux/err.h> > + > +#include <linux/iio/iio.h> > +#include <linux/iio/sysfs.h> > +#include <linux/iio/driver.h> > + > +/* This will be the driver name the kernel reports */ > +#define DRIVER_NAME "imx7d_adc" It's only used in one place so don't bother with the define. Use the string inline and it's immediately obvious what it is for without the comment. > + > +/* ADC register */ > +#define IMX7D_REG_ADC_CH_A_CFG1 0x00 > +#define IMX7D_REG_ADC_CH_A_CFG2 0x10 > +#define IMX7D_REG_ADC_CH_B_CFG1 0x20 > +#define IMX7D_REG_ADC_CH_B_CFG2 0x30 > +#define IMX7D_REG_ADC_CH_C_CFG1 0x40 > +#define IMX7D_REG_ADC_CH_C_CFG2 0x50 > +#define IMX7D_REG_ADC_CH_D_CFG1 0x60 > +#define IMX7D_REG_ADC_CH_D_CFG2 0x70 > +#define IMX7D_REG_ADC_CH_SW_CFG 0x80 > +#define IMX7D_REG_ADC_TIMER_UNIT 0x90 > +#define IMX7D_REG_ADC_DMA_FIFO 0xa0 > +#define IMX7D_REG_ADC_FIFO_STATUS 0xb0 > +#define IMX7D_REG_ADC_INT_SIG_EN 0xc0 > +#define IMX7D_REG_ADC_INT_EN 0xd0 > +#define IMX7D_REG_ADC_INT_STATUS 0xe0 > +#define IMX7D_REG_ADC_CHA_B_CNV_RSLT 0xf0 > +#define IMX7D_REG_ADC_CHC_D_CNV_RSLT 0x100 > +#define IMX7D_REG_ADC_CH_SW_CNV_RSLT 0x110 > +#define IMX7D_REG_ADC_DMA_FIFO_DAT 0x120 > +#define IMX7D_REG_ADC_ADC_CFG 0x130 > + > +#define IMX7D_EACH_CHANNEL_REG_SHIF 0x20 > + > +#define IMX7D_REG_ADC_CH_CFG1_CHANNEL_EN (0x1 << 31) > +#define IMX7D_REG_ADC_CH_CFG1_CHANNEL_DISABLE (0x0 << 31) > +#define IMX7D_REG_ADC_CH_CFG1_CHANNEL_SINGLE BIT(30) > +#define IMX7D_REG_ADC_CH_CFG1_CHANNEL_AVG_EN BIT(29) > +#define IMX7D_REG_ADC_CH_CFG1_CHANNEL_SEL_SHIF 24 > + > +#define IMX7D_REG_ADC_CH_CFG2_AVG_NUM_4 (0x0 << 12) > +#define IMX7D_REG_ADC_CH_CFG2_AVG_NUM_8 (0x1 << 12) > +#define IMX7D_REG_ADC_CH_CFG2_AVG_NUM_16 (0x2 << 12) > +#define IMX7D_REG_ADC_CH_CFG2_AVG_NUM_32 (0x3 << 12) > + > +#define IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_4 (0x0 << 29) > +#define IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_8 (0x1 << 29) > +#define IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_16 (0x2 << 29) > +#define IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_32 (0x3 << 29) > +#define IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_64 (0x4 << 29) > +#define IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_128 (0x5 << 29) > + > +#define IMX7D_REG_ADC_ADC_CFG_ADC_CLK_DOWN BIT(31) > +#define IMX7D_REG_ADC_ADC_CFG_ADC_POWER_DOWN BIT(1) > +#define IMX7D_REG_ADC_ADC_CFG_ADC_EN BIT(0) > + > +#define IMX7D_REG_ADC_INT_CHA_COV_INT_EN BIT(8) > +#define IMX7D_REG_ADC_INT_CHB_COV_INT_EN BIT(9) > +#define IMX7D_REG_ADC_INT_CHC_COV_INT_EN BIT(10) > +#define IMX7D_REG_ADC_INT_CHD_COV_INT_EN BIT(11) > +#define IMX7D_REG_ADC_INT_CHANNEL_INT_EN (IMX7D_REG_ADC_INT_CHA_COV_INT_EN | \ > + IMX7D_REG_ADC_INT_CHB_COV_INT_EN | \ > + IMX7D_REG_ADC_INT_CHC_COV_INT_EN | \ > + IMX7D_REG_ADC_INT_CHD_COV_INT_EN) > +#define IMX7D_REG_ADC_INT_STATUS_CHANNEL_INT_STATUS 0xf00 > + > +#define IMX7D_ADC_TIMEOUT msecs_to_jiffies(100) > + > +enum clk_pre_div { > + IMX7D_ADC_ANALOG_CLK_PRE_DIV_4, > + IMX7D_ADC_ANALOG_CLK_PRE_DIV_8, > + IMX7D_ADC_ANALOG_CLK_PRE_DIV_16, > + IMX7D_ADC_ANALOG_CLK_PRE_DIV_32, > + IMX7D_ADC_ANALOG_CLK_PRE_DIV_64, > + IMX7D_ADC_ANALOG_CLK_PRE_DIV_128, > +}; > + I'd prefer prefixes on these enum defs e.g. imx7d_adc_average_num The naming is generic enough it might bit us in future otherwise! > +enum average_num { > + IMX7D_ADC_AVERAGE_NUM_4, > + IMX7D_ADC_AVERAGE_NUM_8, > + IMX7D_ADC_AVERAGE_NUM_16, > + IMX7D_ADC_AVERAGE_NUM_32, > +}; > + > +struct imx7d_adc_feature { > + enum clk_pre_div clk_pre_div; > + enum average_num avg_num; > + > + u32 core_time_unit; /* impact the sample rate */ > + > + bool average_en; > +}; > + > +struct imx7d_adc { > + struct device *dev; > + void __iomem *regs; > + struct clk *clk; > + > + u32 vref_uv; > + u32 value; > + u32 channel; > + u32 pre_div_num; > + > + struct regulator *vref; > + struct imx7d_adc_feature adc_feature; > + > + struct completion completion; > +}; > + > +struct imx7d_adc_analogue_core_clk { > + u32 pre_div; > + u32 reg_config; > +}; > + > +#define IMX7D_ADC_ALALOGUE_CLK_CONFIG(_pre_div, _reg_conf) { \ > + .pre_div = (_pre_div), \ > + .reg_config = (_reg_conf), \ > +} > + > +static const struct imx7d_adc_analogue_core_clk imx7d_adc_analogue_clk[] = { > + IMX7D_ADC_ALALOGUE_CLK_CONFIG(4, IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_4), > + IMX7D_ADC_ALALOGUE_CLK_CONFIG(8, IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_8), > + IMX7D_ADC_ALALOGUE_CLK_CONFIG(16, IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_16), > + IMX7D_ADC_ALALOGUE_CLK_CONFIG(32, IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_32), > + IMX7D_ADC_ALALOGUE_CLK_CONFIG(64, IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_64), > + IMX7D_ADC_ALALOGUE_CLK_CONFIG(128, IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_128), > +}; > + > +#define IMX7D_ADC_CHAN(_idx, _chan_type) { \ > + .type = (_chan_type), \ > + .indexed = 1, \ > + .channel = (_idx), \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \ > + BIT(IIO_CHAN_INFO_SAMP_FREQ), \ > +} > + > +static const struct iio_chan_spec imx7d_adc_iio_channels[] = { > + IMX7D_ADC_CHAN(0, IIO_VOLTAGE), > + IMX7D_ADC_CHAN(1, IIO_VOLTAGE), > + IMX7D_ADC_CHAN(2, IIO_VOLTAGE), > + IMX7D_ADC_CHAN(3, IIO_VOLTAGE), > + IMX7D_ADC_CHAN(4, IIO_VOLTAGE), > + IMX7D_ADC_CHAN(5, IIO_VOLTAGE), > + IMX7D_ADC_CHAN(6, IIO_VOLTAGE), > + IMX7D_ADC_CHAN(7, IIO_VOLTAGE), > + IMX7D_ADC_CHAN(8, IIO_VOLTAGE), > + IMX7D_ADC_CHAN(9, IIO_VOLTAGE), > + IMX7D_ADC_CHAN(10, IIO_VOLTAGE), > + IMX7D_ADC_CHAN(11, IIO_VOLTAGE), > + IMX7D_ADC_CHAN(12, IIO_VOLTAGE), > + IMX7D_ADC_CHAN(13, IIO_VOLTAGE), > + IMX7D_ADC_CHAN(14, IIO_VOLTAGE), > + IMX7D_ADC_CHAN(15, IIO_VOLTAGE), > +}; > + > +static const u32 imx7d_adc_average_num[] = { > + IMX7D_REG_ADC_CH_CFG2_AVG_NUM_4, > + IMX7D_REG_ADC_CH_CFG2_AVG_NUM_8, > + IMX7D_REG_ADC_CH_CFG2_AVG_NUM_16, > + IMX7D_REG_ADC_CH_CFG2_AVG_NUM_32, > +}; > + > +static void imx7d_adc_feature_config(struct imx7d_adc *info) > +{ > + info->adc_feature.clk_pre_div = IMX7D_ADC_ANALOG_CLK_PRE_DIV_4; > + info->adc_feature.avg_num = IMX7D_ADC_AVERAGE_NUM_32; > + info->adc_feature.core_time_unit = 1; > + info->adc_feature.average_en = true; > +} > + > +static void imx7d_adc_sample_set(struct imx7d_adc *info) > +{ > + struct imx7d_adc_feature *adc_feature = &info->adc_feature; > + struct imx7d_adc_analogue_core_clk adc_analogure_clk; > + u32 i; > + u32 sample_rate = 0; > + > + /* > + * Before sample set, disable channel A,B,C,D. Here we > + * clear the bit 31 of register REG_ADC_CH_A\B\C\D_CFG1. > + */ > + for (i = 0; i < 4; i++) > + writel(IMX7D_REG_ADC_CH_CFG1_CHANNEL_DISABLE, > + info->regs + i * IMX7D_EACH_CHANNEL_REG_SHIF); > + > + adc_analogure_clk = imx7d_adc_analogue_clk[adc_feature->clk_pre_div]; > + sample_rate |= adc_analogure_clk.reg_config; > + info->pre_div_num = adc_analogure_clk.pre_div; > + > + sample_rate |= adc_feature->core_time_unit; > + writel(sample_rate, info->regs + IMX7D_REG_ADC_TIMER_UNIT); > +} > + > +static void imx7d_adc_hw_init(struct imx7d_adc *info) > +{ > + u32 cfg; > + /* power up and enable adc analogue core */ > + cfg = readl(info->regs + IMX7D_REG_ADC_ADC_CFG); > + cfg &= ~(IMX7D_REG_ADC_ADC_CFG_ADC_CLK_DOWN | IMX7D_REG_ADC_ADC_CFG_ADC_POWER_DOWN); > + cfg |= IMX7D_REG_ADC_ADC_CFG_ADC_EN; > + writel(cfg, info->regs + IMX7D_REG_ADC_ADC_CFG); > + > + /* enable channel A,B,C,D interrupt */ > + writel(IMX7D_REG_ADC_INT_CHANNEL_INT_EN, info->regs + IMX7D_REG_ADC_INT_SIG_EN); > + writel(IMX7D_REG_ADC_INT_CHANNEL_INT_EN, info->regs + IMX7D_REG_ADC_INT_EN); > + > + imx7d_adc_sample_set(info); > +} > + > +static void imx7d_adc_channel_set(struct imx7d_adc *info) > +{ > + u32 cfg1 = 0; > + u32 cfg2; > + u32 channel; > + > + channel = info->channel; > + > + /* the channel choose single conversion, and enable average mode */ > + cfg1 |= (IMX7D_REG_ADC_CH_CFG1_CHANNEL_EN | IMX7D_REG_ADC_CH_CFG1_CHANNEL_SINGLE); I don't think readability would be hurt by breaking the above line to keep it shorter. > + if (info->adc_feature.average_en) > + cfg1 |= IMX7D_REG_ADC_CH_CFG1_CHANNEL_AVG_EN; > + > + /* > + * physical channel 0 chose logical channel A > + * physical channel 1 chose logical channel B > + * physical channel 2 chose logical channel C > + * physical channel 3 chose logical channel D > + */ > + cfg1 |= (channel << IMX7D_REG_ADC_CH_CFG1_CHANNEL_SEL_SHIF); > + > + /* read register REG_ADC_CH_A\B\C\D_CFG2, according to the channel chosen */ > + cfg2 = readl(info->regs + IMX7D_EACH_CHANNEL_REG_SHIF * channel + 0x10); > + > + cfg2 |= imx7d_adc_average_num[info->adc_feature.avg_num]; > + > + /* write the register REG_ADC_CH_A\B\C\D_CFG2, according to the channel chosen */ > + writel(cfg2, info->regs + IMX7D_EACH_CHANNEL_REG_SHIF * channel + 0x10); > + writel(cfg1, info->regs + IMX7D_EACH_CHANNEL_REG_SHIF * channel); > +} > + > +static u32 imx7d_adc_get_sample_rate(struct imx7d_adc *info) > +{ > + /* input clock is always 24MHz */ > + u32 input_clk = 24000000; > + u32 analogue_core_clk; > + u32 core_time_unit = info->adc_feature.core_time_unit; > + u32 sample_clk; > + u32 tmp; > + > + analogue_core_clk = input_clk / info->pre_div_num; > + tmp = (core_time_unit + 1) * 6; > + sample_clk = analogue_core_clk / tmp; > + > + return sample_clk; > +} > + > +static int imx7d_adc_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, > + int *val2, > + long mask) > +{ > + struct imx7d_adc *info = iio_priv(indio_dev); > + > + u32 channel; > + long ret; > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + mutex_lock(&indio_dev->mlock); > + reinit_completion(&info->completion); > + > + channel = (chan->channel) & 0x0f; > + info->channel = channel; > + imx7d_adc_channel_set(info); > + > + ret = wait_for_completion_interruptible_timeout > + (&info->completion, IMX7D_ADC_TIMEOUT); > + if (ret == 0) { > + mutex_unlock(&indio_dev->mlock); > + return -ETIMEDOUT; > + } > + if (ret < 0) { > + mutex_unlock(&indio_dev->mlock); > + return ret; > + } > + > + *val = info->value; > + mutex_unlock(&indio_dev->mlock); > + return IIO_VAL_INT; > + > + case IIO_CHAN_INFO_SCALE: > + *val = info->vref_uv / 1000; > + *val2 = 12; > + return IIO_VAL_FRACTIONAL_LOG2; > + > + case IIO_CHAN_INFO_SAMP_FREQ: > + *val = imx7d_adc_get_sample_rate(info); > + *val2 = 0; > + return IIO_VAL_INT; > + > + default: > + break; > + } > + > + return -EINVAL; > +} > + > +static int imx7d_adc_read_data(struct imx7d_adc *info) > +{ > + u32 channel; > + u32 value; > + > + channel = info->channel; > + > + /* > + * channel A and B conversion result share one register, > + * bit[27~16] is the channel B conversion result, > + * bit[11~0] is the channel A conversion result. > + * channel C and D is the same. > + */ > + if (channel < 2) > + value = readl(info->regs + IMX7D_REG_ADC_CHA_B_CNV_RSLT); > + else > + value = readl(info->regs + IMX7D_REG_ADC_CHC_D_CNV_RSLT); > + if (channel & 0x1) /* channel B or D */ > + value = (value >> 16) & 0xFFF; > + else /* channel A or C */ > + value &= 0xFFF; > + > + return value; > +} > + > +static irqreturn_t imx7d_adc_isr(int irq, void *dev_id) > +{ > + struct imx7d_adc *info = (struct imx7d_adc *)dev_id; > + int status; > + > + status = readl(info->regs + IMX7D_REG_ADC_INT_STATUS); > + if (status & IMX7D_REG_ADC_INT_STATUS_CHANNEL_INT_STATUS) { > + info->value = imx7d_adc_read_data(info); > + complete(&info->completion); > + } > + writel(0, info->regs + IMX7D_REG_ADC_INT_STATUS); > + > + return IRQ_HANDLED; > +} > + > +static int imx7d_adc_reg_access(struct iio_dev *indio_dev, > + unsigned reg, unsigned writeval, > + unsigned *readval) > +{ > + struct imx7d_adc *info = iio_priv(indio_dev); > + > + if ((readval == NULL) || > + ((reg % 4) || (reg > IMX7D_REG_ADC_ADC_CFG))) > + return -EINVAL; > + > + *readval = readl(info->regs + reg); > + > + return 0; > +} > + > +static const struct iio_info imx7d_adc_iio_info = { > + .driver_module = THIS_MODULE, > + .read_raw = &imx7d_adc_read_raw, > + .debugfs_reg_access = &imx7d_adc_reg_access, > +}; > + > +static const struct of_device_id imx7d_adc_match[] = { > + { .compatible = "fsl,imx7d-adc", }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, imx7d_adc_match); > + > +static int imx7d_adc_probe(struct platform_device *pdev) > +{ > + struct imx7d_adc *info; > + struct iio_dev *indio_dev; > + struct resource *mem; > + int irq; > + int ret; > + u32 channels; > + > + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(struct imx7d_adc)); > + if (!indio_dev) { > + dev_err(&pdev->dev, "Failed allocating iio device\n"); > + return -ENOMEM; > + } > + > + info = iio_priv(indio_dev); > + info->dev = &pdev->dev; > + > + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + info->regs = devm_ioremap_resource(&pdev->dev, mem); > + if (IS_ERR(info->regs)) { > + ret = PTR_ERR(info->regs); > + dev_err(&pdev->dev, "failed to remap adc memory: %d\n", ret); > + return ret; > + } > + > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) { > + dev_err(&pdev->dev, "no irq resource?\n"); > + return irq; > + } > + > + ret = devm_request_irq(info->dev, irq, > + imx7d_adc_isr, 0, > + dev_name(&pdev->dev), info); > + if (ret < 0) { > + dev_err(&pdev->dev, "failed requesting irq, irq = %d\n", irq); > + return ret; > + } > + > + info->clk = devm_clk_get(&pdev->dev, "adc"); > + if (IS_ERR(info->clk)) { > + ret = PTR_ERR(info->clk); > + dev_err(&pdev->dev, "failed getting clock, err = %d\n", ret); > + return ret; > + } > + > + info->vref = devm_regulator_get(&pdev->dev, "vref"); > + if (IS_ERR(info->vref)) { > + ret = PTR_ERR(info->vref); > + dev_err(&pdev->dev, "failed getting reference voltage: %d\n", ret); > + return ret; > + } > + > + ret = regulator_enable(info->vref); > + if (ret) > + return ret; > + > + info->vref_uv = regulator_get_voltage(info->vref); Given the use of this isn't typically on a fast path, I'd be tempted to query the regulator when you need to know (i.e. in the scale read). That avoids the tiny possibility that the regulator voltage has changed (which would be odd for a reference voltage source!) > + > + platform_set_drvdata(pdev, indio_dev); > + > + init_completion(&info->completion); > + > + ret = of_property_read_u32(pdev->dev.of_node, > + "num-channels", &channels); > + if (ret) > + channels = ARRAY_SIZE(imx7d_adc_iio_channels); > + > + 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 = &imx7d_adc_iio_info; > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->channels = imx7d_adc_iio_channels; > + indio_dev->num_channels = (int)channels; > + > + ret = clk_prepare_enable(info->clk); > + if (ret) { > + dev_err(&pdev->dev, > + "Could not prepare or enable the clock.\n"); > + goto error_adc_clk_enable; > + } > + > + imx7d_adc_feature_config(info); > + imx7d_adc_hw_init(info); > + > + ret = iio_device_register(indio_dev); > + if (ret) { > + dev_err(&pdev->dev, "Couldn't register the device.\n"); > + goto error_iio_device_register; > + } > + > + return 0; > + > +error_iio_device_register: > + clk_disable_unprepare(info->clk); > +error_adc_clk_enable: > + regulator_disable(info->vref); > + > + return ret; > +} > + > +static int imx7d_adc_remove(struct platform_device *pdev) > +{ > + struct iio_dev *indio_dev = platform_get_drvdata(pdev); > + struct imx7d_adc *info = iio_priv(indio_dev); > + > + iio_device_unregister(indio_dev); > + regulator_disable(info->vref); > + clk_disable_unprepare(info->clk); > + > + return 0; > +} > + > +static int __maybe_unused imx7d_adc_suspend(struct device *dev) > +{ > + struct iio_dev *indio_dev = dev_get_drvdata(dev); > + struct imx7d_adc *info = iio_priv(indio_dev); > + u32 adc_cfg; > + > + adc_cfg = readl(info->regs + IMX7D_REG_ADC_ADC_CFG); > + adc_cfg |= IMX7D_REG_ADC_ADC_CFG_ADC_CLK_DOWN | IMX7D_REG_ADC_ADC_CFG_ADC_POWER_DOWN; Readability isn't going to be greatly reduced by breaking the line above, so in this case I'd keep to the 80 char limit. > + adc_cfg &= ~IMX7D_REG_ADC_ADC_CFG_ADC_EN; > + writel(adc_cfg, info->regs + IMX7D_REG_ADC_ADC_CFG); > + > + clk_disable_unprepare(info->clk); > + regulator_disable(info->vref); > + > + return 0; > +} > + > +static int __maybe_unused imx7d_adc_resume(struct device *dev) > +{ > + struct iio_dev *indio_dev = dev_get_drvdata(dev); > + struct imx7d_adc *info = iio_priv(indio_dev); > + int ret; > + > + ret = regulator_enable(info->vref); > + if (ret) > + return ret; > + > + ret = clk_prepare_enable(info->clk); > + if (ret) { > + dev_err(info->dev, > + "Could not prepare or enable clock.\n"); > + regulator_disable(info->vref); > + return ret; > + } > + > + imx7d_adc_hw_init(info); > + > + return 0; > +} > + > +static SIMPLE_DEV_PM_OPS(imx7d_adc_pm_ops, imx7d_adc_suspend, imx7d_adc_resume); > + > +static struct platform_driver imx7d_adc_driver = { > + .probe = imx7d_adc_probe, > + .remove = imx7d_adc_remove, > + .driver = { > + .name = DRIVER_NAME, > + .of_match_table = imx7d_adc_match, > + .pm = &imx7d_adc_pm_ops, > + }, > +}; > + > +module_platform_driver(imx7d_adc_driver); > + > +MODULE_AUTHOR("Haibo Chen <haibo.chen@freescale.com>"); > +MODULE_DESCRIPTION("Freeacale IMX7D ADC driver"); > +MODULE_LICENSE("GPL v2"); >
On 13/11/15 09:52, Lars-Peter Clausen wrote: > On 11/13/2015 10:39 AM, Haibo Chen wrote: > [...] >>>> +static void imx7d_adc_feature_config(struct imx7d_adc *info) >>>> +{ >>>> + info->adc_feature.clk_pre_div = IMX7D_ADC_ANALOG_CLK_PRE_DIV_4; >>>> + info->adc_feature.avg_num = IMX7D_ADC_AVERAGE_NUM_32; >>>> + info->adc_feature.core_time_unit = 1; >>>> + info->adc_feature.average_en = true; >>> >>> What's the plan for these? Right now they are always initialized to the same >>> static value. >>> >> >> In future, we can get these values from dts file, currently we just use the static value. > > Those seem to be software configuration values though, not part of hardware > description. > >> >>> >>>> +} >>> [...] >>>> +static int imx7d_adc_read_raw(struct iio_dev *indio_dev, >>>> + struct iio_chan_spec const *chan, >>>> + int *val, >>>> + int *val2, >>>> + long mask) >>>> +{ >>>> + struct imx7d_adc *info = iio_priv(indio_dev); >>>> + >>>> + u32 channel; >>>> + long ret; >>>> + >>>> + switch (mask) { >>>> + case IIO_CHAN_INFO_RAW: >>>> + mutex_lock(&indio_dev->mlock); >>>> + reinit_completion(&info->completion); >>>> + >>>> + channel = (chan->channel) & 0x0f; >>>> + info->channel = channel; >>>> + imx7d_adc_channel_set(info); >>> >>> How about just passing channel directy adc_channel_set() instead of doing it >>> implicitly through the info struct? >>> >> >> I think there is no difference, besides, using this parameter info struct can keep align with other functions. >> eg. imx7d_adc_sample_set(), imx7d_adc_hw_init(), imx7d_adc_get_sample_rate(), all these functions have the same parameter. >> >>> [...] >>>> +static irqreturn_t imx7d_adc_isr(int irq, void *dev_id) >>>> +{ >>>> + struct imx7d_adc *info = (struct imx7d_adc *)dev_id; >>>> + int status; >>>> + >>>> + status = readl(info->regs + IMX7D_REG_ADC_INT_STATUS); >>>> + if (status & IMX7D_REG_ADC_INT_STATUS_CHANNEL_INT_STATUS) { >>>> + info->value = imx7d_adc_read_data(info); >>>> + complete(&info->completion); >>>> + } >>>> + writel(0, info->regs + IMX7D_REG_ADC_INT_STATUS); >>> >>> Is the hardware really this broken? If the interrupt happens between reading >>> the status register and clearing it here it will be missed. >>> >> >> I think interrupt can't happen between reading the status register and clearing it. >> Because in function imx7d_adc_read_raw(), we call the function >> imx7d_adc_channel_set(info), in this function, we config the register >> REG_ADC_CH_A\B\C\D_CFG1 and REG_ADC_CH_A\B\C\D_CFG2, only when these registers >> is configed, ADC start a conversion. Once the conversion complete, ADC trigger an >> interrupt, and call the imx7d_adc_isr(). > > Well an interrupt can always happen, its running fully asynchronous to the > CPU. If you have multiple interrupt sources enabled and the first one fires > and you run then start the irq handler, read the status register, now the > second irq sources fires, and then you clear the status interrupt register. > This means the driver will not see the irq from the second source. I'd agree with this, would normally expect to see only the relevant parts of the register 'cleared'. Usually this is done by explicitly picking those bits out (normally writing 1 to the bit to clear the interrupt source). If there is anything else of interest in that register this is nasty! Hmm. looking at the bits you have defined and the way it is used you only ever have one interrupt at a time possibly occurring at the moment? If this is actually the case, then why read the register at all? If there is anything in the other bits of that register it would be interesting to know what is there! > >> >>>> + >>>> + return IRQ_HANDLED; >>> >>> You should only return IRQ_HANDLED if you actually handled are interrupt. >>> >> >> Here in the interrupt, we just handle the channel conversion finished flag, for >> other flag, ignore them this time, Will add other flag in future. > > Yeah, but if you don't handle any interrupt you should return IRQ_NONE. This > will make sure that the system can recover in case the hardware (or the > driver) is broken and generates unexpected interrupts. If there are a 1000 > or so IRQ_NONEs in a row in a short time frame the kernel will disable the IRQ. > >>> [...] >>>> + ret = of_property_read_u32(pdev->dev.of_node, >>> >>> Extra space. >>> >>>> + "num-channels", &channels); >>> >>> What decides how many channels are in use? >>> >> >> The board decides the channel number. >> In dts file, there is a line: num-channels = <4>; > > Yes, but what decides how this property should be configured? > >> >> >>>> + if (ret) >>>> + channels = ARRAY_SIZE(imx7d_adc_iio_channels); >>>> + >>>> + 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 = &imx7d_adc_iio_info; >>>> + indio_dev->modes = INDIO_DIRECT_MODE; >>>> + indio_dev->channels = imx7d_adc_iio_channels; >>>> + indio_dev->num_channels = (int)channels; >>> >>> I don't think you need the case here. >>> >> >> Sorry, can't get your point, you mean I should not indio_dev-> ? > > Sorry, I meant the (int) cast. > >> >>>> [...] >> >
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index 7868c74..bf0611c 100644 --- a/drivers/iio/adc/Kconfig +++ b/drivers/iio/adc/Kconfig @@ -194,6 +194,15 @@ config HI8435 This driver can also be built as a module. If so, the module will be called hi8435. +config IMX7D_ADC + tristate "IMX7D ADC driver" + depends on OF + help + Say yes here to build support for IMX7D ADC. + + This driver can also be built as a module. If so, the module will be + called imx7d_adc. + config LP8788_ADC tristate "LP8788 ADC driver" depends on MFD_LP8788 diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile index 99b37a9..282ffc01 100644 --- a/drivers/iio/adc/Makefile +++ b/drivers/iio/adc/Makefile @@ -20,6 +20,7 @@ obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o obj-$(CONFIG_HI8435) += hi8435.o +obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o obj-$(CONFIG_MAX1027) += max1027.o obj-$(CONFIG_MAX1363) += max1363.o diff --git a/drivers/iio/adc/imx7d_adc.c b/drivers/iio/adc/imx7d_adc.c new file mode 100644 index 0000000..7e25e4e --- /dev/null +++ b/drivers/iio/adc/imx7d_adc.c @@ -0,0 +1,571 @@ +/* + * Freescale i.MX7D ADC driver + * + * Copyright (C) 2015 Freescale Semiconductor, Inc. + * + * 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/module.h> +#include <linux/platform_device.h> +#include <linux/interrupt.h> +#include <linux/delay.h> +#include <linux/kernel.h> +#include <linux/slab.h> +#include <linux/io.h> +#include <linux/clk.h> +#include <linux/completion.h> +#include <linux/of.h> +#include <linux/of_irq.h> +#include <linux/regulator/consumer.h> +#include <linux/of_platform.h> +#include <linux/err.h> + +#include <linux/iio/iio.h> +#include <linux/iio/sysfs.h> +#include <linux/iio/driver.h> + +/* This will be the driver name the kernel reports */ +#define DRIVER_NAME "imx7d_adc" + +/* ADC register */ +#define IMX7D_REG_ADC_CH_A_CFG1 0x00 +#define IMX7D_REG_ADC_CH_A_CFG2 0x10 +#define IMX7D_REG_ADC_CH_B_CFG1 0x20 +#define IMX7D_REG_ADC_CH_B_CFG2 0x30 +#define IMX7D_REG_ADC_CH_C_CFG1 0x40 +#define IMX7D_REG_ADC_CH_C_CFG2 0x50 +#define IMX7D_REG_ADC_CH_D_CFG1 0x60 +#define IMX7D_REG_ADC_CH_D_CFG2 0x70 +#define IMX7D_REG_ADC_CH_SW_CFG 0x80 +#define IMX7D_REG_ADC_TIMER_UNIT 0x90 +#define IMX7D_REG_ADC_DMA_FIFO 0xa0 +#define IMX7D_REG_ADC_FIFO_STATUS 0xb0 +#define IMX7D_REG_ADC_INT_SIG_EN 0xc0 +#define IMX7D_REG_ADC_INT_EN 0xd0 +#define IMX7D_REG_ADC_INT_STATUS 0xe0 +#define IMX7D_REG_ADC_CHA_B_CNV_RSLT 0xf0 +#define IMX7D_REG_ADC_CHC_D_CNV_RSLT 0x100 +#define IMX7D_REG_ADC_CH_SW_CNV_RSLT 0x110 +#define IMX7D_REG_ADC_DMA_FIFO_DAT 0x120 +#define IMX7D_REG_ADC_ADC_CFG 0x130 + +#define IMX7D_EACH_CHANNEL_REG_SHIF 0x20 + +#define IMX7D_REG_ADC_CH_CFG1_CHANNEL_EN (0x1 << 31) +#define IMX7D_REG_ADC_CH_CFG1_CHANNEL_DISABLE (0x0 << 31) +#define IMX7D_REG_ADC_CH_CFG1_CHANNEL_SINGLE BIT(30) +#define IMX7D_REG_ADC_CH_CFG1_CHANNEL_AVG_EN BIT(29) +#define IMX7D_REG_ADC_CH_CFG1_CHANNEL_SEL_SHIF 24 + +#define IMX7D_REG_ADC_CH_CFG2_AVG_NUM_4 (0x0 << 12) +#define IMX7D_REG_ADC_CH_CFG2_AVG_NUM_8 (0x1 << 12) +#define IMX7D_REG_ADC_CH_CFG2_AVG_NUM_16 (0x2 << 12) +#define IMX7D_REG_ADC_CH_CFG2_AVG_NUM_32 (0x3 << 12) + +#define IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_4 (0x0 << 29) +#define IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_8 (0x1 << 29) +#define IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_16 (0x2 << 29) +#define IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_32 (0x3 << 29) +#define IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_64 (0x4 << 29) +#define IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_128 (0x5 << 29) + +#define IMX7D_REG_ADC_ADC_CFG_ADC_CLK_DOWN BIT(31) +#define IMX7D_REG_ADC_ADC_CFG_ADC_POWER_DOWN BIT(1) +#define IMX7D_REG_ADC_ADC_CFG_ADC_EN BIT(0) + +#define IMX7D_REG_ADC_INT_CHA_COV_INT_EN BIT(8) +#define IMX7D_REG_ADC_INT_CHB_COV_INT_EN BIT(9) +#define IMX7D_REG_ADC_INT_CHC_COV_INT_EN BIT(10) +#define IMX7D_REG_ADC_INT_CHD_COV_INT_EN BIT(11) +#define IMX7D_REG_ADC_INT_CHANNEL_INT_EN (IMX7D_REG_ADC_INT_CHA_COV_INT_EN | \ + IMX7D_REG_ADC_INT_CHB_COV_INT_EN | \ + IMX7D_REG_ADC_INT_CHC_COV_INT_EN | \ + IMX7D_REG_ADC_INT_CHD_COV_INT_EN) +#define IMX7D_REG_ADC_INT_STATUS_CHANNEL_INT_STATUS 0xf00 + +#define IMX7D_ADC_TIMEOUT msecs_to_jiffies(100) + +enum clk_pre_div { + IMX7D_ADC_ANALOG_CLK_PRE_DIV_4, + IMX7D_ADC_ANALOG_CLK_PRE_DIV_8, + IMX7D_ADC_ANALOG_CLK_PRE_DIV_16, + IMX7D_ADC_ANALOG_CLK_PRE_DIV_32, + IMX7D_ADC_ANALOG_CLK_PRE_DIV_64, + IMX7D_ADC_ANALOG_CLK_PRE_DIV_128, +}; + +enum average_num { + IMX7D_ADC_AVERAGE_NUM_4, + IMX7D_ADC_AVERAGE_NUM_8, + IMX7D_ADC_AVERAGE_NUM_16, + IMX7D_ADC_AVERAGE_NUM_32, +}; + +struct imx7d_adc_feature { + enum clk_pre_div clk_pre_div; + enum average_num avg_num; + + u32 core_time_unit; /* impact the sample rate */ + + bool average_en; +}; + +struct imx7d_adc { + struct device *dev; + void __iomem *regs; + struct clk *clk; + + u32 vref_uv; + u32 value; + u32 channel; + u32 pre_div_num; + + struct regulator *vref; + struct imx7d_adc_feature adc_feature; + + struct completion completion; +}; + +struct imx7d_adc_analogue_core_clk { + u32 pre_div; + u32 reg_config; +}; + +#define IMX7D_ADC_ALALOGUE_CLK_CONFIG(_pre_div, _reg_conf) { \ + .pre_div = (_pre_div), \ + .reg_config = (_reg_conf), \ +} + +static const struct imx7d_adc_analogue_core_clk imx7d_adc_analogue_clk[] = { + IMX7D_ADC_ALALOGUE_CLK_CONFIG(4, IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_4), + IMX7D_ADC_ALALOGUE_CLK_CONFIG(8, IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_8), + IMX7D_ADC_ALALOGUE_CLK_CONFIG(16, IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_16), + IMX7D_ADC_ALALOGUE_CLK_CONFIG(32, IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_32), + IMX7D_ADC_ALALOGUE_CLK_CONFIG(64, IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_64), + IMX7D_ADC_ALALOGUE_CLK_CONFIG(128, IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_128), +}; + +#define IMX7D_ADC_CHAN(_idx, _chan_type) { \ + .type = (_chan_type), \ + .indexed = 1, \ + .channel = (_idx), \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \ + BIT(IIO_CHAN_INFO_SAMP_FREQ), \ +} + +static const struct iio_chan_spec imx7d_adc_iio_channels[] = { + IMX7D_ADC_CHAN(0, IIO_VOLTAGE), + IMX7D_ADC_CHAN(1, IIO_VOLTAGE), + IMX7D_ADC_CHAN(2, IIO_VOLTAGE), + IMX7D_ADC_CHAN(3, IIO_VOLTAGE), + IMX7D_ADC_CHAN(4, IIO_VOLTAGE), + IMX7D_ADC_CHAN(5, IIO_VOLTAGE), + IMX7D_ADC_CHAN(6, IIO_VOLTAGE), + IMX7D_ADC_CHAN(7, IIO_VOLTAGE), + IMX7D_ADC_CHAN(8, IIO_VOLTAGE), + IMX7D_ADC_CHAN(9, IIO_VOLTAGE), + IMX7D_ADC_CHAN(10, IIO_VOLTAGE), + IMX7D_ADC_CHAN(11, IIO_VOLTAGE), + IMX7D_ADC_CHAN(12, IIO_VOLTAGE), + IMX7D_ADC_CHAN(13, IIO_VOLTAGE), + IMX7D_ADC_CHAN(14, IIO_VOLTAGE), + IMX7D_ADC_CHAN(15, IIO_VOLTAGE), +}; + +static const u32 imx7d_adc_average_num[] = { + IMX7D_REG_ADC_CH_CFG2_AVG_NUM_4, + IMX7D_REG_ADC_CH_CFG2_AVG_NUM_8, + IMX7D_REG_ADC_CH_CFG2_AVG_NUM_16, + IMX7D_REG_ADC_CH_CFG2_AVG_NUM_32, +}; + +static void imx7d_adc_feature_config(struct imx7d_adc *info) +{ + info->adc_feature.clk_pre_div = IMX7D_ADC_ANALOG_CLK_PRE_DIV_4; + info->adc_feature.avg_num = IMX7D_ADC_AVERAGE_NUM_32; + info->adc_feature.core_time_unit = 1; + info->adc_feature.average_en = true; +} + +static void imx7d_adc_sample_set(struct imx7d_adc *info) +{ + struct imx7d_adc_feature *adc_feature = &info->adc_feature; + struct imx7d_adc_analogue_core_clk adc_analogure_clk; + u32 i; + u32 sample_rate = 0; + + /* + * Before sample set, disable channel A,B,C,D. Here we + * clear the bit 31 of register REG_ADC_CH_A\B\C\D_CFG1. + */ + for (i = 0; i < 4; i++) + writel(IMX7D_REG_ADC_CH_CFG1_CHANNEL_DISABLE, + info->regs + i * IMX7D_EACH_CHANNEL_REG_SHIF); + + adc_analogure_clk = imx7d_adc_analogue_clk[adc_feature->clk_pre_div]; + sample_rate |= adc_analogure_clk.reg_config; + info->pre_div_num = adc_analogure_clk.pre_div; + + sample_rate |= adc_feature->core_time_unit; + writel(sample_rate, info->regs + IMX7D_REG_ADC_TIMER_UNIT); +} + +static void imx7d_adc_hw_init(struct imx7d_adc *info) +{ + u32 cfg; + /* power up and enable adc analogue core */ + cfg = readl(info->regs + IMX7D_REG_ADC_ADC_CFG); + cfg &= ~(IMX7D_REG_ADC_ADC_CFG_ADC_CLK_DOWN | IMX7D_REG_ADC_ADC_CFG_ADC_POWER_DOWN); + cfg |= IMX7D_REG_ADC_ADC_CFG_ADC_EN; + writel(cfg, info->regs + IMX7D_REG_ADC_ADC_CFG); + + /* enable channel A,B,C,D interrupt */ + writel(IMX7D_REG_ADC_INT_CHANNEL_INT_EN, info->regs + IMX7D_REG_ADC_INT_SIG_EN); + writel(IMX7D_REG_ADC_INT_CHANNEL_INT_EN, info->regs + IMX7D_REG_ADC_INT_EN); + + imx7d_adc_sample_set(info); +} + +static void imx7d_adc_channel_set(struct imx7d_adc *info) +{ + u32 cfg1 = 0; + u32 cfg2; + u32 channel; + + channel = info->channel; + + /* the channel choose single conversion, and enable average mode */ + cfg1 |= (IMX7D_REG_ADC_CH_CFG1_CHANNEL_EN | IMX7D_REG_ADC_CH_CFG1_CHANNEL_SINGLE); + if (info->adc_feature.average_en) + cfg1 |= IMX7D_REG_ADC_CH_CFG1_CHANNEL_AVG_EN; + + /* + * physical channel 0 chose logical channel A + * physical channel 1 chose logical channel B + * physical channel 2 chose logical channel C + * physical channel 3 chose logical channel D + */ + cfg1 |= (channel << IMX7D_REG_ADC_CH_CFG1_CHANNEL_SEL_SHIF); + + /* read register REG_ADC_CH_A\B\C\D_CFG2, according to the channel chosen */ + cfg2 = readl(info->regs + IMX7D_EACH_CHANNEL_REG_SHIF * channel + 0x10); + + cfg2 |= imx7d_adc_average_num[info->adc_feature.avg_num]; + + /* write the register REG_ADC_CH_A\B\C\D_CFG2, according to the channel chosen */ + writel(cfg2, info->regs + IMX7D_EACH_CHANNEL_REG_SHIF * channel + 0x10); + writel(cfg1, info->regs + IMX7D_EACH_CHANNEL_REG_SHIF * channel); +} + +static u32 imx7d_adc_get_sample_rate(struct imx7d_adc *info) +{ + /* input clock is always 24MHz */ + u32 input_clk = 24000000; + u32 analogue_core_clk; + u32 core_time_unit = info->adc_feature.core_time_unit; + u32 sample_clk; + u32 tmp; + + analogue_core_clk = input_clk / info->pre_div_num; + tmp = (core_time_unit + 1) * 6; + sample_clk = analogue_core_clk / tmp; + + return sample_clk; +} + +static int imx7d_adc_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int *val, + int *val2, + long mask) +{ + struct imx7d_adc *info = iio_priv(indio_dev); + + u32 channel; + long ret; + + switch (mask) { + case IIO_CHAN_INFO_RAW: + mutex_lock(&indio_dev->mlock); + reinit_completion(&info->completion); + + channel = (chan->channel) & 0x0f; + info->channel = channel; + imx7d_adc_channel_set(info); + + ret = wait_for_completion_interruptible_timeout + (&info->completion, IMX7D_ADC_TIMEOUT); + if (ret == 0) { + mutex_unlock(&indio_dev->mlock); + return -ETIMEDOUT; + } + if (ret < 0) { + mutex_unlock(&indio_dev->mlock); + return ret; + } + + *val = info->value; + mutex_unlock(&indio_dev->mlock); + return IIO_VAL_INT; + + case IIO_CHAN_INFO_SCALE: + *val = info->vref_uv / 1000; + *val2 = 12; + return IIO_VAL_FRACTIONAL_LOG2; + + case IIO_CHAN_INFO_SAMP_FREQ: + *val = imx7d_adc_get_sample_rate(info); + *val2 = 0; + return IIO_VAL_INT; + + default: + break; + } + + return -EINVAL; +} + +static int imx7d_adc_read_data(struct imx7d_adc *info) +{ + u32 channel; + u32 value; + + channel = info->channel; + + /* + * channel A and B conversion result share one register, + * bit[27~16] is the channel B conversion result, + * bit[11~0] is the channel A conversion result. + * channel C and D is the same. + */ + if (channel < 2) + value = readl(info->regs + IMX7D_REG_ADC_CHA_B_CNV_RSLT); + else + value = readl(info->regs + IMX7D_REG_ADC_CHC_D_CNV_RSLT); + if (channel & 0x1) /* channel B or D */ + value = (value >> 16) & 0xFFF; + else /* channel A or C */ + value &= 0xFFF; + + return value; +} + +static irqreturn_t imx7d_adc_isr(int irq, void *dev_id) +{ + struct imx7d_adc *info = (struct imx7d_adc *)dev_id; + int status; + + status = readl(info->regs + IMX7D_REG_ADC_INT_STATUS); + if (status & IMX7D_REG_ADC_INT_STATUS_CHANNEL_INT_STATUS) { + info->value = imx7d_adc_read_data(info); + complete(&info->completion); + } + writel(0, info->regs + IMX7D_REG_ADC_INT_STATUS); + + return IRQ_HANDLED; +} + +static int imx7d_adc_reg_access(struct iio_dev *indio_dev, + unsigned reg, unsigned writeval, + unsigned *readval) +{ + struct imx7d_adc *info = iio_priv(indio_dev); + + if ((readval == NULL) || + ((reg % 4) || (reg > IMX7D_REG_ADC_ADC_CFG))) + return -EINVAL; + + *readval = readl(info->regs + reg); + + return 0; +} + +static const struct iio_info imx7d_adc_iio_info = { + .driver_module = THIS_MODULE, + .read_raw = &imx7d_adc_read_raw, + .debugfs_reg_access = &imx7d_adc_reg_access, +}; + +static const struct of_device_id imx7d_adc_match[] = { + { .compatible = "fsl,imx7d-adc", }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, imx7d_adc_match); + +static int imx7d_adc_probe(struct platform_device *pdev) +{ + struct imx7d_adc *info; + struct iio_dev *indio_dev; + struct resource *mem; + int irq; + int ret; + u32 channels; + + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(struct imx7d_adc)); + if (!indio_dev) { + dev_err(&pdev->dev, "Failed allocating iio device\n"); + return -ENOMEM; + } + + info = iio_priv(indio_dev); + info->dev = &pdev->dev; + + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); + info->regs = devm_ioremap_resource(&pdev->dev, mem); + if (IS_ERR(info->regs)) { + ret = PTR_ERR(info->regs); + dev_err(&pdev->dev, "failed to remap adc memory: %d\n", ret); + return ret; + } + + irq = platform_get_irq(pdev, 0); + if (irq < 0) { + dev_err(&pdev->dev, "no irq resource?\n"); + return irq; + } + + ret = devm_request_irq(info->dev, irq, + imx7d_adc_isr, 0, + dev_name(&pdev->dev), info); + if (ret < 0) { + dev_err(&pdev->dev, "failed requesting irq, irq = %d\n", irq); + return ret; + } + + info->clk = devm_clk_get(&pdev->dev, "adc"); + if (IS_ERR(info->clk)) { + ret = PTR_ERR(info->clk); + dev_err(&pdev->dev, "failed getting clock, err = %d\n", ret); + return ret; + } + + info->vref = devm_regulator_get(&pdev->dev, "vref"); + if (IS_ERR(info->vref)) { + ret = PTR_ERR(info->vref); + dev_err(&pdev->dev, "failed getting reference voltage: %d\n", ret); + return ret; + } + + ret = regulator_enable(info->vref); + if (ret) + return ret; + + info->vref_uv = regulator_get_voltage(info->vref); + + platform_set_drvdata(pdev, indio_dev); + + init_completion(&info->completion); + + ret = of_property_read_u32(pdev->dev.of_node, + "num-channels", &channels); + if (ret) + channels = ARRAY_SIZE(imx7d_adc_iio_channels); + + 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 = &imx7d_adc_iio_info; + indio_dev->modes = INDIO_DIRECT_MODE; + indio_dev->channels = imx7d_adc_iio_channels; + indio_dev->num_channels = (int)channels; + + ret = clk_prepare_enable(info->clk); + if (ret) { + dev_err(&pdev->dev, + "Could not prepare or enable the clock.\n"); + goto error_adc_clk_enable; + } + + imx7d_adc_feature_config(info); + imx7d_adc_hw_init(info); + + ret = iio_device_register(indio_dev); + if (ret) { + dev_err(&pdev->dev, "Couldn't register the device.\n"); + goto error_iio_device_register; + } + + return 0; + +error_iio_device_register: + clk_disable_unprepare(info->clk); +error_adc_clk_enable: + regulator_disable(info->vref); + + return ret; +} + +static int imx7d_adc_remove(struct platform_device *pdev) +{ + struct iio_dev *indio_dev = platform_get_drvdata(pdev); + struct imx7d_adc *info = iio_priv(indio_dev); + + iio_device_unregister(indio_dev); + regulator_disable(info->vref); + clk_disable_unprepare(info->clk); + + return 0; +} + +static int __maybe_unused imx7d_adc_suspend(struct device *dev) +{ + struct iio_dev *indio_dev = dev_get_drvdata(dev); + struct imx7d_adc *info = iio_priv(indio_dev); + u32 adc_cfg; + + adc_cfg = readl(info->regs + IMX7D_REG_ADC_ADC_CFG); + adc_cfg |= IMX7D_REG_ADC_ADC_CFG_ADC_CLK_DOWN | IMX7D_REG_ADC_ADC_CFG_ADC_POWER_DOWN; + adc_cfg &= ~IMX7D_REG_ADC_ADC_CFG_ADC_EN; + writel(adc_cfg, info->regs + IMX7D_REG_ADC_ADC_CFG); + + clk_disable_unprepare(info->clk); + regulator_disable(info->vref); + + return 0; +} + +static int __maybe_unused imx7d_adc_resume(struct device *dev) +{ + struct iio_dev *indio_dev = dev_get_drvdata(dev); + struct imx7d_adc *info = iio_priv(indio_dev); + int ret; + + ret = regulator_enable(info->vref); + if (ret) + return ret; + + ret = clk_prepare_enable(info->clk); + if (ret) { + dev_err(info->dev, + "Could not prepare or enable clock.\n"); + regulator_disable(info->vref); + return ret; + } + + imx7d_adc_hw_init(info); + + return 0; +} + +static SIMPLE_DEV_PM_OPS(imx7d_adc_pm_ops, imx7d_adc_suspend, imx7d_adc_resume); + +static struct platform_driver imx7d_adc_driver = { + .probe = imx7d_adc_probe, + .remove = imx7d_adc_remove, + .driver = { + .name = DRIVER_NAME, + .of_match_table = imx7d_adc_match, + .pm = &imx7d_adc_pm_ops, + }, +}; + +module_platform_driver(imx7d_adc_driver); + +MODULE_AUTHOR("Haibo Chen <haibo.chen@freescale.com>"); +MODULE_DESCRIPTION("Freeacale IMX7D ADC driver"); +MODULE_LICENSE("GPL v2");
Freescale i.MX7D soc contains a new ADC IP. This patch add this ADC driver support, and the driver only support ADC software trigger. Signed-off-by: Haibo Chen <haibo.chen@freescale.com> --- drivers/iio/adc/Kconfig | 9 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/imx7d_adc.c | 571 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 581 insertions(+) create mode 100644 drivers/iio/adc/imx7d_adc.c