| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <alpine.DEB.2.02.1512011105210.14849@pmeerw.net>
Date: Tue, 1 Dec 2015 11:17:00 +0100 (CET)
From: Peter Meerwald-Stadler <pmeerw@...erw.net>
To: Chen Bough <Haibo.Chen@...escale.com>
cc: "jic23@...nel.org" <jic23@...nel.org>,
"knaack.h@....de" <knaack.h@....de>,
"lars@...afoo.de" <lars@...afoo.de>,
"shawnguo@...nel.org" <shawnguo@...nel.org>,
"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v4 1/4] iio: adc: add IMX7D ADC driver support
> > -----Original Message-----
> > From: Peter Meerwald-Stadler [mailto:pmeerw@...erw.net]
> > Sent: Tuesday, December 01, 2015 4:51 PM
> > To: Chen Haibo-B51421
> > Cc: jic23@...nel.org; knaack.h@....de; lars@...afoo.de;
> > shawnguo@...nel.org; linux-iio@...r.kernel.org; linux-
> > kernel@...r.kernel.org
> > Subject: Re: [PATCH v4 1/4] iio: adc: add IMX7D ADC driver support
> >
> >
> > > Freescale i.MX7D soc contains a new ADC IP. This patch add this ADC
> > > driver support, and the driver only support ADC software trigger.
> >
> > some extreme nitpicking below :)
> >
> > > Signed-off-by: Haibo Chen <haibo.chen@...escale.com>
> > > ---
> > > drivers/iio/adc/Kconfig | 9 +
> > > drivers/iio/adc/Makefile | 1 +
> > > drivers/iio/adc/imx7d_adc.c | 588
> > > ++++++++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 598 insertions(+)
> > > create mode 100644 drivers/iio/adc/imx7d_adc.c
> > >
> > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index
> > > 7868c74..3493a46 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 ARCH_MXC || COMPILE_TEST
> > > + 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..4780595
> > > --- /dev/null
> > > +++ b/drivers/iio/adc/imx7d_adc.c
> > > @@ -0,0 +1,588 @@
> > > +/*
> > > + * 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/clk.h>
> > > +#include <linux/completion.h>
> > > +#include <linux/err.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/io.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/regulator/consumer.h>
> > > +
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/iio/driver.h>
> > > +#include <linux/iio/sysfs.h>
> > > +
> > > +/* 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_REG_ADC_CHANNEL_CFG2_BASE 0x10
> > > +#define IMX7D_EACH_CHANNEL_REG_OFFSET 0x20
> > > +
> > > +#define IMX7D_REG_ADC_CH_CFG1_CHANNEL_EN (0x1 << 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(x) ((x) << 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 imx7d_adc_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 imx7d_adc_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 imx7d_adc_clk_pre_div clk_pre_div;
> > > + enum imx7d_adc_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_ANALOGUE_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_ANALOGUE_CLK_CONFIG(4,
> > IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_4),
> > > + IMX7D_ADC_ANALOGUE_CLK_CONFIG(8,
> > IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_8),
> > > + IMX7D_ADC_ANALOGUE_CLK_CONFIG(16,
> > IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_16),
> > > + IMX7D_ADC_ANALOGUE_CLK_CONFIG(32,
> > IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_32),
> > > + IMX7D_ADC_ANALOGUE_CLK_CONFIG(64,
> > IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_64),
> > > + IMX7D_ADC_ANALOGUE_CLK_CONFIG(128,
> > > +IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_128),
> > > +};
> > > +
> > > +#define IMX7D_ADC_CHAN(_idx) { \
> > > + .type = IIO_VOLTAGE, \
> > > + .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),
> > > + IMX7D_ADC_CHAN(1),
> > > + IMX7D_ADC_CHAN(2),
> > > + IMX7D_ADC_CHAN(3),
> > > + IMX7D_ADC_CHAN(4),
> > > + IMX7D_ADC_CHAN(5),
> > > + IMX7D_ADC_CHAN(6),
> > > + IMX7D_ADC_CHAN(7),
> > > + IMX7D_ADC_CHAN(8),
> > > + IMX7D_ADC_CHAN(9),
> > > + IMX7D_ADC_CHAN(10),
> > > + IMX7D_ADC_CHAN(11),
> > > + IMX7D_ADC_CHAN(12),
> > > + IMX7D_ADC_CHAN(13),
> > > + IMX7D_ADC_CHAN(14),
> > > + IMX7D_ADC_CHAN(15),
> > > +};
> > > +
> > > +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_rate_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 tmp_cfg1;
> > > + 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++) {
> > > + tmp_cfg1 = readl(info->regs + i *
> > IMX7D_EACH_CHANNEL_REG_OFFSET);
> > > + tmp_cfg1 &= ~IMX7D_REG_ADC_CH_CFG1_CHANNEL_EN;
> > > + writel(tmp_cfg1, info->regs + i *
> > IMX7D_EACH_CHANNEL_REG_OFFSET);
> > > + }
> > > +
> > > + 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_rate_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 |= IMX7D_REG_ADC_CH_CFG1_CHANNEL_SEL(channel);
> > > +
> > > + /* read register REG_ADC_CH_A\B\C\D_CFG2, according to the channel
> > chosen */
> > > + cfg2 = readl(info->regs + IMX7D_EACH_CHANNEL_REG_OFFSET * channel +
> > > +IMX7D_REG_ADC_CHANNEL_CFG2_BASE);
> > > +
> > > + 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_OFFSET * channel +
> > IMX7D_REG_ADC_CHANNEL_CFG2_BASE);
> > > + writel(cfg1, info->regs + IMX7D_EACH_CHANNEL_REG_OFFSET * 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 tmp;
> > > +
> > > + analogue_core_clk = input_clk / info->pre_div_num;
> > > + tmp = (core_time_unit + 1) * 6;
> > > +
> > > + return analogue_core_clk / tmp;
> >
> > extra space after return
> >
> > > +}
> > > +
> > > +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 & 0x03;
> > > + info->channel = channel;
> > > + imx7d_adc_channel_set(info);
> > > +
> > > + ret = wait_for_completion_interruptible_timeout
> > > + (&info->completion, IMX7D_ADC_TIMEOUT);
> >
> > all mutex_unlock() calls could be moved here
>
> [haibo] Sorry, I can't get your point, you mean we don't need to use mutex_unlock?
I was aiming for code simplification (reduce the number of mutex_unlock()
calls); the mutex protects info->value so it is difficult to rearrange
please ignore my comment
one could do
ret = wait_for_completion(...);
if (ret > 0)
*val = info->value;
mutex_unlock();
if (ret == 0)
return -ETIMEDOUT;
else if (ret < 0)
return ret;
return IIO_VAL_INT;
this saves two mutex_unlock() but this is not really any better
> > > + 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:
> > > + info->vref_uv = regulator_get_voltage(info->vref);
> > > + *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);
> > > + return IIO_VAL_INT;
> > > +
> > > + default:
> > > + break;
> >
> > just
> > return -EINVAL
> > here
> >
> > > + }
> > > +
> > > + return -EINVAL;
> > > +}
> > > +
> > > +static int imx7d_adc_read_data(struct imx7d_adc *info) {
> > > + u32 channel;
> > > + u32 value;
> > > +
> > > + channel = info->channel & 0x03;
> > > +
> > > + /*
> > > + * 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);
> > > +
> > > + /*
> > > + * The register IMX7D_REG_ADC_INT_STATUS can't clear
> > > + * itself after read operation, need software to write
> > > + * 0 to the related bit. Here we clear the channel A/B/C/D
> > > + * conversion finished flag.
> > > + */
> > > + status &= ~IMX7D_REG_ADC_INT_STATUS_CHANNEL_INT_STATUS;
> > > + writel(status, info->regs + IMX7D_REG_ADC_INT_STATUS);
> > > + return IRQ_HANDLED;
> > > + }
> > > +
> > > + /*
> > > + * Currently, driver only enable channel A/B/C/D conversion
> > interrupt,
> > > + * so logically for other bits of IMX7D_REG_ADC_INT_STATUS, they
> > should
> > > + * always be zero. But due to this register can't clear itself
> > after the
> > > + * read operation, if any accident happen and cause the other bits
> > > +of this
> >
> > happens and causes
> >
> > > + * register set, then interrupt will be triggered constantly. To
> > make the
> > > + * code robust, here clear this register for all the accident.
> >
> > clear this register here .. accidents
> >
> > > + */
> > > + writel(0, info->regs + IMX7D_REG_ADC_INT_STATUS);
> > > + return IRQ_NONE;
> > > +}
> > > +
> > > +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 || 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;
> > > +
> > > + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
> > > + 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, err = %d\n",
> > ret);
> > > + return ret;
> > > + }
> > > +
> > > + irq = platform_get_irq(pdev, 0);
> > > + if (irq < 0) {
> > > + dev_err(&pdev->dev, "No irq resource?\n");
> > > + return irq;
> > > + }
> > > +
> > > + 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, err
> > = %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + ret = regulator_enable(info->vref);
> > > + if (ret) {
> > > + dev_err(&pdev->dev, "Can't enable adc reference top voltage,
> > err = %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + platform_set_drvdata(pdev, indio_dev);
> > > +
> > > + init_completion(&info->completion);
> > > +
> > > + indio_dev->name = dev_name(&pdev->dev);
> > > + indio_dev->dev.parent = &pdev->dev;
> > > + indio_dev->info = &imx7d_adc_iio_info;
> > > + indio_dev->modes = INDIO_DIRECT_MODE;
> > > + indio_dev->channels = imx7d_adc_iio_channels;
> > > + indio_dev->num_channels = ARRAY_SIZE(imx7d_adc_iio_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;
> > > + }
> > > +
> > > + 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);
> > > + goto error_iio_device_register;
> > > + }
> > > +
> > > + 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");
> >
> > probably power down
>
> [haibo] Yes, you are right! I will add this.
>
> Thanks for your careful review!
>
> Best Regards
> Haibo chen
>
> >
> > > + 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 void imx7d_adc_power_down(struct imx7d_adc *info) {
> > > + 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); }
> > > +
> > > +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);
> > > +
> > > + imx7d_adc_power_down(info);
> > > +
> > > + iio_device_unregister(indio_dev);
> > > + clk_disable_unprepare(info->clk);
> > > + regulator_disable(info->vref);
> > > +
> > > + 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);
> > > +
> > > + imx7d_adc_power_down(info);
> > > +
> > > + 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) {
> > > + dev_err(info->dev,
> > > + "Can't enable adc reference top voltage, err = %d\n",
> > 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 = "imx7d_adc",
> > > + .of_match_table = imx7d_adc_match,
> > > + .pm = &imx7d_adc_pm_ops,
> > > + },
> > > +};
> > > +
> > > +module_platform_driver(imx7d_adc_driver);
> > > +
> > > +MODULE_AUTHOR("Haibo Chen <haibo.chen@...escale.com>");
> > > +MODULE_DESCRIPTION("Freeacale IMX7D ADC driver"); MODULE_LICENSE("GPL
> > > +v2");
> > >
> >
> > --
> >
> > Peter Meerwald-Stadler
> > +43-664-2444418 (mobile)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
--
Peter Meerwald-Stadler
+43-664-2444418 (mobile)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists