* [PATCHv4 0/4] iio: adc: exynos_adc: Support Exynos3250 ADC and code clean
@ 2014-06-18 2:20 Chanwoo Choi
2014-06-18 2:20 ` [PATCHv4 1/4] iio: adc: exynos_adc: Add exynos_adc_ops structure to improve readability Chanwoo Choi
` (3 more replies)
0 siblings, 4 replies; 23+ messages in thread
From: Chanwoo Choi @ 2014-06-18 2:20 UTC (permalink / raw)
To: jic23, ch.naveen, t.figa, kgene.kim
Cc: robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak,
rdunlap, sachin.kamat, linux-iio, linux-samsung-soc,
linux-kernel, devicetree, linux-doc, Chanwoo Choi
This patchset support Exynos3250 ADC (Analog Digital Converter) because
Exynos3250 has additional special clock for ADC IP and add 'exynos_adc_ops'
structure to improve readability.
Changes from v3:
- Add new 'exynos_adc_ops' structure to improve readability according to
Tomasz Figa comment[1]
[1] https://lkml.org/lkml/2014/4/16/238
- Add new 'exynos3250-adc-v2' compatible string to support Exynos3250 ADC
- Fix wrong compaitlbe string of ADC in Exynos3250 dtsi file
Changes from v2:
- Check return value of clock function to deal with error exception
- Fix minor coding style to improve readability
Changes from v1:
- Add new "samsung,exynos-adc-v3" compatible to support Exynos3250 ADC
- Add a patch about DT binding documentation
Chanwoo Choi (4):
iio: adc: exynos_adc: Add exynos_adc_ops structure to improve readability
iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC
iio: devicetree: Add DT binding documentation for Exynos3250 ADC
ARM: dts: Fix wrong compatible string for Exynos3250 ADC
.../devicetree/bindings/arm/samsung/exynos-adc.txt | 20 ++
arch/arm/boot/dts/exynos3250.dtsi | 4 +-
drivers/iio/adc/exynos_adc.c | 267 ++++++++++++++++-----
3 files changed, 223 insertions(+), 68 deletions(-)
--
1.8.0
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCHv4 1/4] iio: adc: exynos_adc: Add exynos_adc_ops structure to improve readability
2014-06-18 2:20 [PATCHv4 0/4] iio: adc: exynos_adc: Support Exynos3250 ADC and code clean Chanwoo Choi
@ 2014-06-18 2:20 ` Chanwoo Choi
2014-06-18 5:27 ` Naveen Krishna Ch
2014-06-18 7:55 ` Tomasz Figa
2014-06-18 2:20 ` [PATCHv4 2/4] iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC Chanwoo Choi
` (2 subsequent siblings)
3 siblings, 2 replies; 23+ messages in thread
From: Chanwoo Choi @ 2014-06-18 2:20 UTC (permalink / raw)
To: jic23, ch.naveen, t.figa, kgene.kim
Cc: robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak,
rdunlap, sachin.kamat, linux-iio, linux-samsung-soc,
linux-kernel, devicetree, linux-doc, Chanwoo Choi
This patchset add 'exynos_adc_ops' structure which includes some functions
to control ADC operation according to ADC version (v1 or v2).
Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
drivers/iio/adc/exynos_adc.c | 174 +++++++++++++++++++++++++++++--------------
1 file changed, 120 insertions(+), 54 deletions(-)
diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
index 010578f..c30def6 100644
--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -90,6 +90,7 @@ struct exynos_adc {
struct clk *clk;
unsigned int irq;
struct regulator *vdd;
+ struct exynos_adc_ops *ops;
struct completion completion;
@@ -97,6 +98,13 @@ struct exynos_adc {
unsigned int version;
};
+struct exynos_adc_ops {
+ void (*init_hw)(struct exynos_adc *info);
+ void (*clear_irq)(struct exynos_adc *info);
+ void (*start_conv)(struct exynos_adc *info, unsigned long addr);
+ void (*stop_conv)(struct exynos_adc *info);
+};
+
static const struct of_device_id exynos_adc_match[] = {
{ .compatible = "samsung,exynos-adc-v1", .data = (void *)ADC_V1 },
{ .compatible = "samsung,exynos-adc-v2", .data = (void *)ADC_V2 },
@@ -112,30 +120,98 @@ static inline unsigned int exynos_adc_get_version(struct platform_device *pdev)
return (unsigned int)match->data;
}
-static void exynos_adc_hw_init(struct exynos_adc *info)
+static void exynos_adc_v1_init_hw(struct exynos_adc *info)
+{
+ u32 con1;
+
+ /* set default prescaler values and Enable prescaler */
+ con1 = ADC_V1_CON_PRSCLV(49) | ADC_V1_CON_PRSCEN;
+
+ /* Enable 12-bit ADC resolution */
+ con1 |= ADC_V1_CON_RES;
+ writel(con1, ADC_V1_CON(info->regs));
+}
+
+static void exynos_adc_v1_start_conv(struct exynos_adc *info, unsigned long addr)
+{
+ u32 con1;
+
+ writel(addr, ADC_V1_MUX(info->regs));
+
+ con1 = readl(ADC_V1_CON(info->regs));
+ writel(con1 | ADC_CON_EN_START, ADC_V1_CON(info->regs));
+}
+
+static void exynos_adc_v1_clear_irq(struct exynos_adc *info)
+{
+ writel(1, ADC_V1_INTCLR(info->regs));
+}
+
+static void exynos_adc_v1_stop_conv(struct exynos_adc *info)
+{
+ u32 con;
+
+ con = readl(ADC_V1_CON(info->regs));
+ con |= ADC_V1_CON_STANDBY;
+ writel(con, ADC_V1_CON(info->regs));
+}
+
+static struct exynos_adc_ops exynos_adc_v1_ops = {
+ .init_hw = exynos_adc_v1_init_hw,
+ .clear_irq = exynos_adc_v1_clear_irq,
+ .start_conv = exynos_adc_v1_start_conv,
+ .stop_conv = exynos_adc_v1_stop_conv,
+};
+
+static void exynos_adc_v2_init_hw(struct exynos_adc *info)
{
u32 con1, con2;
- if (info->version == ADC_V2) {
- con1 = ADC_V2_CON1_SOFT_RESET;
- writel(con1, ADC_V2_CON1(info->regs));
+ con1 = ADC_V2_CON1_SOFT_RESET;
+ writel(con1, ADC_V2_CON1(info->regs));
- con2 = ADC_V2_CON2_OSEL | ADC_V2_CON2_ESEL |
- ADC_V2_CON2_HIGHF | ADC_V2_CON2_C_TIME(0);
- writel(con2, ADC_V2_CON2(info->regs));
+ con2 = ADC_V2_CON2_OSEL | ADC_V2_CON2_ESEL |
+ ADC_V2_CON2_HIGHF | ADC_V2_CON2_C_TIME(0);
+ writel(con2, ADC_V2_CON2(info->regs));
- /* Enable interrupts */
- writel(1, ADC_V2_INT_EN(info->regs));
- } else {
- /* set default prescaler values and Enable prescaler */
- con1 = ADC_V1_CON_PRSCLV(49) | ADC_V1_CON_PRSCEN;
+ /* Enable interrupts */
+ writel(1, ADC_V2_INT_EN(info->regs));
+}
- /* Enable 12-bit ADC resolution */
- con1 |= ADC_V1_CON_RES;
- writel(con1, ADC_V1_CON(info->regs));
- }
+static void exynos_adc_v2_start_conv(struct exynos_adc *info, unsigned long addr)
+{
+ u32 con1, con2;
+
+ con2 = readl(ADC_V2_CON2(info->regs));
+ con2 &= ~ADC_V2_CON2_ACH_MASK;
+ con2 |= ADC_V2_CON2_ACH_SEL(addr);
+ writel(con2, ADC_V2_CON2(info->regs));
+
+ con1 = readl(ADC_V2_CON1(info->regs));
+ writel(con1 | ADC_CON_EN_START, ADC_V2_CON1(info->regs));
+}
+
+static void exynos_adc_v2_clear_irq(struct exynos_adc *info)
+{
+ writel(1, ADC_V2_INT_ST(info->regs));
+}
+
+static void exynos_adc_v2_stop_conv(struct exynos_adc *info)
+{
+ u32 con;
+
+ con = readl(ADC_V2_CON1(info->regs));
+ con &= ~ADC_CON_EN_START;
+ writel(con, ADC_V2_CON1(info->regs));
}
+static struct exynos_adc_ops exynos_adc_v2_ops = {
+ .init_hw = exynos_adc_v2_init_hw,
+ .start_conv = exynos_adc_v2_start_conv,
+ .clear_irq = exynos_adc_v2_clear_irq,
+ .stop_conv = exynos_adc_v2_stop_conv,
+};
+
static int exynos_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int *val,
@@ -144,7 +220,6 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
{
struct exynos_adc *info = iio_priv(indio_dev);
unsigned long timeout;
- u32 con1, con2;
int ret;
if (mask != IIO_CHAN_INFO_RAW)
@@ -154,28 +229,15 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
reinit_completion(&info->completion);
/* Select the channel to be used and Trigger conversion */
- if (info->version == ADC_V2) {
- con2 = readl(ADC_V2_CON2(info->regs));
- con2 &= ~ADC_V2_CON2_ACH_MASK;
- con2 |= ADC_V2_CON2_ACH_SEL(chan->address);
- writel(con2, ADC_V2_CON2(info->regs));
-
- con1 = readl(ADC_V2_CON1(info->regs));
- writel(con1 | ADC_CON_EN_START,
- ADC_V2_CON1(info->regs));
- } else {
- writel(chan->address, ADC_V1_MUX(info->regs));
-
- con1 = readl(ADC_V1_CON(info->regs));
- writel(con1 | ADC_CON_EN_START,
- ADC_V1_CON(info->regs));
- }
+ if (info->ops->start_conv)
+ info->ops->start_conv(info, chan->address);
timeout = wait_for_completion_timeout
(&info->completion, EXYNOS_ADC_TIMEOUT);
if (timeout == 0) {
dev_warn(&indio_dev->dev, "Conversion timed out! Resetting\n");
- exynos_adc_hw_init(info);
+ if (info->ops->init_hw)
+ info->ops->init_hw(info);
ret = -ETIMEDOUT;
} else {
*val = info->value;
@@ -193,13 +255,11 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
struct exynos_adc *info = (struct exynos_adc *)dev_id;
/* Read value */
- info->value = readl(ADC_V1_DATX(info->regs)) &
- ADC_DATX_MASK;
+ info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
+
/* clear irq */
- if (info->version == ADC_V2)
- writel(1, ADC_V2_INT_ST(info->regs));
- else
- writel(1, ADC_V1_INTCLR(info->regs));
+ if (info->ops->clear_irq)
+ info->ops->clear_irq(info);
complete(&info->completion);
@@ -277,6 +337,19 @@ static int exynos_adc_probe(struct platform_device *pdev)
info = iio_priv(indio_dev);
+ info->version = exynos_adc_get_version(pdev);
+ switch (info->version) {
+ case ADC_V1:
+ info->ops = &exynos_adc_v1_ops;
+ break;
+ case ADC_V2:
+ info->ops = &exynos_adc_v2_ops;
+ break;
+ default:
+ dev_err(&pdev->dev, "failed to identify ADC version\n");
+ return -EINVAL;
+ };
+
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
info->regs = devm_ioremap_resource(&pdev->dev, mem);
if (IS_ERR(info->regs))
@@ -321,8 +394,6 @@ static int exynos_adc_probe(struct platform_device *pdev)
writel(1, info->enable_reg);
- info->version = exynos_adc_get_version(pdev);
-
platform_set_drvdata(pdev, indio_dev);
indio_dev->name = dev_name(&pdev->dev);
@@ -349,7 +420,8 @@ static int exynos_adc_probe(struct platform_device *pdev)
if (ret)
goto err_irq;
- exynos_adc_hw_init(info);
+ if (info->ops->init_hw)
+ info->ops->init_hw(info);
ret = of_platform_populate(np, exynos_adc_match, NULL, &indio_dev->dev);
if (ret < 0) {
@@ -394,17 +466,9 @@ static int exynos_adc_suspend(struct device *dev)
{
struct iio_dev *indio_dev = dev_get_drvdata(dev);
struct exynos_adc *info = iio_priv(indio_dev);
- u32 con;
- if (info->version == ADC_V2) {
- con = readl(ADC_V2_CON1(info->regs));
- con &= ~ADC_CON_EN_START;
- writel(con, ADC_V2_CON1(info->regs));
- } else {
- con = readl(ADC_V1_CON(info->regs));
- con |= ADC_V1_CON_STANDBY;
- writel(con, ADC_V1_CON(info->regs));
- }
+ if (info->ops->stop_conv)
+ info->ops->stop_conv(info);
writel(0, info->enable_reg);
clk_disable_unprepare(info->clk);
@@ -428,7 +492,9 @@ static int exynos_adc_resume(struct device *dev)
return ret;
writel(1, info->enable_reg);
- exynos_adc_hw_init(info);
+
+ if (info->ops->init_hw)
+ info->ops->init_hw(info);
return 0;
}
--
1.8.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCHv4 2/4] iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC
2014-06-18 2:20 [PATCHv4 0/4] iio: adc: exynos_adc: Support Exynos3250 ADC and code clean Chanwoo Choi
2014-06-18 2:20 ` [PATCHv4 1/4] iio: adc: exynos_adc: Add exynos_adc_ops structure to improve readability Chanwoo Choi
@ 2014-06-18 2:20 ` Chanwoo Choi
2014-06-18 7:58 ` Tomasz Figa
2014-06-18 2:21 ` [PATCHv4 3/4] iio: devicetree: Add DT binding documentation for " Chanwoo Choi
2014-06-18 2:21 ` [PATCHv4 4/4] ARM: dts: Fix wrong compatible string " Chanwoo Choi
3 siblings, 1 reply; 23+ messages in thread
From: Chanwoo Choi @ 2014-06-18 2:20 UTC (permalink / raw)
To: jic23, ch.naveen, t.figa, kgene.kim
Cc: robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak,
rdunlap, sachin.kamat, linux-iio, linux-samsung-soc,
linux-kernel, devicetree, linux-doc, Chanwoo Choi
This patch control special clock for ADC in Exynos series's FSYS block.
If special clock of ADC is registerd on clock list of common clk framework,
Exynos ADC drvier have to control this clock.
Exynos3250/Exynos4/Exynos5 has 'adc' clock as following:
- 'adc' clock: bus clock for ADC
Exynos3250 has additional 'sclk_adc' clock as following:
- 'sclk_adc' clock: special clock for ADC which provide clock to internal ADC
Exynos 4210/4212/4412 and Exynos5250/5420 has not included 'sclk_adc' clock
in FSYS_BLK. But, Exynos3250 based on Cortex-A7 has only included 'sclk_adc'
clock in FSYS_BLK.
Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
drivers/iio/adc/exynos_adc.c | 93 ++++++++++++++++++++++++++++++++++++++------
1 file changed, 81 insertions(+), 12 deletions(-)
diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
index c30def6..6b026ac 100644
--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -41,7 +41,8 @@
enum adc_version {
ADC_V1,
- ADC_V2
+ ADC_V2,
+ ADC_V2_EXYNOS3250,
};
/* EXYNOS4412/5250 ADC_V1 registers definitions */
@@ -85,9 +86,11 @@ enum adc_version {
#define EXYNOS_ADC_TIMEOUT (msecs_to_jiffies(100))
struct exynos_adc {
+ struct device *dev;
void __iomem *regs;
void __iomem *enable_reg;
struct clk *clk;
+ struct clk *sclk;
unsigned int irq;
struct regulator *vdd;
struct exynos_adc_ops *ops;
@@ -96,6 +99,7 @@ struct exynos_adc {
u32 value;
unsigned int version;
+ bool needs_sclk;
};
struct exynos_adc_ops {
@@ -103,11 +107,21 @@ struct exynos_adc_ops {
void (*clear_irq)(struct exynos_adc *info);
void (*start_conv)(struct exynos_adc *info, unsigned long addr);
void (*stop_conv)(struct exynos_adc *info);
+ void (*disable_clk)(struct exynos_adc *info);
+ int (*enable_clk)(struct exynos_adc *info);
};
static const struct of_device_id exynos_adc_match[] = {
- { .compatible = "samsung,exynos-adc-v1", .data = (void *)ADC_V1 },
- { .compatible = "samsung,exynos-adc-v2", .data = (void *)ADC_V2 },
+ {
+ .compatible = "samsung,exynos-adc-v1",
+ .data = (void *)ADC_V1,
+ }, {
+ .compatible = "samsung,exynos-adc-v2",
+ .data = (void *)ADC_V2,
+ }, {
+ .compatible = "samsung,exynos3250-adc-v2",
+ .data = (void *)ADC_V2_EXYNOS3250,
+ },
{},
};
MODULE_DEVICE_TABLE(of, exynos_adc_match);
@@ -156,11 +170,42 @@ static void exynos_adc_v1_stop_conv(struct exynos_adc *info)
writel(con, ADC_V1_CON(info->regs));
}
+static void exynos_adc_disable_clk(struct exynos_adc *info)
+{
+ if (info->needs_sclk)
+ clk_disable_unprepare(info->sclk);
+ clk_disable_unprepare(info->clk);
+}
+
+static int exynos_adc_enable_clk(struct exynos_adc *info)
+{
+ int ret;
+
+ ret = clk_prepare_enable(info->clk);
+ if (ret) {
+ dev_err(info->dev, "failed enabling adc clock: %d\n", ret);
+ return ret;
+ }
+
+ if (info->needs_sclk) {
+ ret = clk_prepare_enable(info->sclk);
+ if (ret) {
+ clk_disable_unprepare(info->clk);
+ dev_err(info->dev,
+ "failed enabling sclk_tsadc clock: %d\n", ret);
+ }
+ }
+
+ return 0;
+}
+
static struct exynos_adc_ops exynos_adc_v1_ops = {
.init_hw = exynos_adc_v1_init_hw,
.clear_irq = exynos_adc_v1_clear_irq,
.start_conv = exynos_adc_v1_start_conv,
.stop_conv = exynos_adc_v1_stop_conv,
+ .disable_clk = exynos_adc_disable_clk,
+ .enable_clk = exynos_adc_enable_clk,
};
static void exynos_adc_v2_init_hw(struct exynos_adc *info)
@@ -210,6 +255,8 @@ static struct exynos_adc_ops exynos_adc_v2_ops = {
.start_conv = exynos_adc_v2_start_conv,
.clear_irq = exynos_adc_v2_clear_irq,
.stop_conv = exynos_adc_v2_stop_conv,
+ .disable_clk = exynos_adc_disable_clk,
+ .enable_clk = exynos_adc_enable_clk,
};
static int exynos_read_raw(struct iio_dev *indio_dev,
@@ -345,6 +392,10 @@ static int exynos_adc_probe(struct platform_device *pdev)
case ADC_V2:
info->ops = &exynos_adc_v2_ops;
break;
+ case ADC_V2_EXYNOS3250:
+ info->ops = &exynos_adc_v2_ops;
+ info->needs_sclk = true;
+ break;
default:
dev_err(&pdev->dev, "failed to identify ADC version\n");
return -EINVAL;
@@ -367,6 +418,7 @@ static int exynos_adc_probe(struct platform_device *pdev)
}
info->irq = irq;
+ info->dev = &pdev->dev;
init_completion(&info->completion);
@@ -377,6 +429,16 @@ static int exynos_adc_probe(struct platform_device *pdev)
return PTR_ERR(info->clk);
}
+ if (info->needs_sclk) {
+ info->sclk = devm_clk_get(&pdev->dev, "sclk_adc");
+ if (IS_ERR(info->sclk)) {
+ dev_err(&pdev->dev,
+ "failed getting sclk_tsadc, err = %ld\n",
+ PTR_ERR(info->sclk));
+ return PTR_ERR(info->sclk);
+ }
+ }
+
info->vdd = devm_regulator_get(&pdev->dev, "vdd");
if (IS_ERR(info->vdd)) {
dev_err(&pdev->dev, "failed getting regulator, err = %ld\n",
@@ -388,9 +450,11 @@ static int exynos_adc_probe(struct platform_device *pdev)
if (ret)
return ret;
- ret = clk_prepare_enable(info->clk);
- if (ret)
- goto err_disable_reg;
+ if (info->ops->enable_clk) {
+ ret = info->ops->enable_clk(info);
+ if (ret)
+ goto err_disable_reg;
+ }
writel(1, info->enable_reg);
@@ -439,7 +503,8 @@ err_irq:
free_irq(info->irq, info);
err_disable_clk:
writel(0, info->enable_reg);
- clk_disable_unprepare(info->clk);
+ if (info->ops->disable_clk)
+ info->ops->disable_clk(info);
err_disable_reg:
regulator_disable(info->vdd);
return ret;
@@ -455,7 +520,8 @@ static int exynos_adc_remove(struct platform_device *pdev)
iio_device_unregister(indio_dev);
free_irq(info->irq, info);
writel(0, info->enable_reg);
- clk_disable_unprepare(info->clk);
+ if (info->ops->disable_clk)
+ info->ops->disable_clk(info);
regulator_disable(info->vdd);
return 0;
@@ -471,7 +537,8 @@ static int exynos_adc_suspend(struct device *dev)
info->ops->stop_conv(info);
writel(0, info->enable_reg);
- clk_disable_unprepare(info->clk);
+ if (info->ops->disable_clk)
+ info->ops->disable_clk(info);
regulator_disable(info->vdd);
return 0;
@@ -487,9 +554,11 @@ static int exynos_adc_resume(struct device *dev)
if (ret)
return ret;
- ret = clk_prepare_enable(info->clk);
- if (ret)
- return ret;
+ if (info->ops->enable_clk) {
+ ret = info->ops->enable_clk(info);
+ if (ret)
+ return ret;
+ }
writel(1, info->enable_reg);
--
1.8.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCHv4 3/4] iio: devicetree: Add DT binding documentation for Exynos3250 ADC
2014-06-18 2:20 [PATCHv4 0/4] iio: adc: exynos_adc: Support Exynos3250 ADC and code clean Chanwoo Choi
2014-06-18 2:20 ` [PATCHv4 1/4] iio: adc: exynos_adc: Add exynos_adc_ops structure to improve readability Chanwoo Choi
2014-06-18 2:20 ` [PATCHv4 2/4] iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC Chanwoo Choi
@ 2014-06-18 2:21 ` Chanwoo Choi
2014-06-18 6:12 ` Naveen Krishna Ch
2014-06-18 8:35 ` Tomasz Figa
2014-06-18 2:21 ` [PATCHv4 4/4] ARM: dts: Fix wrong compatible string " Chanwoo Choi
3 siblings, 2 replies; 23+ messages in thread
From: Chanwoo Choi @ 2014-06-18 2:21 UTC (permalink / raw)
To: jic23, ch.naveen, t.figa, kgene.kim
Cc: robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak,
rdunlap, sachin.kamat, linux-iio, linux-samsung-soc,
linux-kernel, devicetree, linux-doc, Chanwoo Choi
This patch add DT binding documentation for Exynos3250 ADC IP. Exynos3250 has
special clock ('sclk_tsadc') for ADC which provide clock to internal ADC.
Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
.../devicetree/bindings/arm/samsung/exynos-adc.txt | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
index 5d49f2b..3a5af82 100644
--- a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
+++ b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
@@ -14,6 +14,8 @@ Required properties:
for exynos4412/5250 controllers.
Must be "samsung,exynos-adc-v2" for
future controllers.
+ Must be "samsung,exynos3250-adc-v2" for
+ for exynos3250 controllers.
- reg: Contains ADC register address range (base address and
length) and the address of the phy enable register.
- interrupts: Contains the interrupt information for the timer. The
@@ -21,7 +23,11 @@ Required properties:
the Samsung device uses.
- #io-channel-cells = <1>; As ADC has multiple outputs
- clocks From common clock binding: handle to adc clock.
+ From common clock binding: handle to sclk_tsadc clock
+ if using Exynos3250.
- clock-names From common clock binding: Shall be "adc".
+ From common clock binding: Shall be "sclk_tsadc"
+ if using Exynos3250.
- vdd-supply VDD input supply.
Note: child nodes can be added for auto probing from device tree.
@@ -41,6 +47,20 @@ adc: adc@12D10000 {
vdd-supply = <&buck5_reg>;
};
+Example: adding device info in dtsi file for Exynos3250 with additional sclk
+
+adc: adc@126C0000 {
+ compatible = "samsung,exynos3250-adc-v2";
+ reg = <0x126C0000 0x100>, <0x10020718 0x4>;
+ interrupts = <0 137 0>;
+ #io-channel-cells = <1>;
+ io-channel-ranges;
+
+ clocks = <&cmu CLK_TSADC>, <&cmu CLK_SCLK_TSADC>;
+ clock-names = "adc", "sclk_adc";
+
+ vdd-supply = <&buck5_reg>;
+};
Example: Adding child nodes in dts file
--
1.8.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCHv4 4/4] ARM: dts: Fix wrong compatible string for Exynos3250 ADC
2014-06-18 2:20 [PATCHv4 0/4] iio: adc: exynos_adc: Support Exynos3250 ADC and code clean Chanwoo Choi
` (2 preceding siblings ...)
2014-06-18 2:21 ` [PATCHv4 3/4] iio: devicetree: Add DT binding documentation for " Chanwoo Choi
@ 2014-06-18 2:21 ` Chanwoo Choi
2014-06-18 8:37 ` Tomasz Figa
3 siblings, 1 reply; 23+ messages in thread
From: Chanwoo Choi @ 2014-06-18 2:21 UTC (permalink / raw)
To: jic23, ch.naveen, t.figa, kgene.kim
Cc: robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak,
rdunlap, sachin.kamat, linux-iio, linux-samsung-soc,
linux-kernel, devicetree, linux-doc, Chanwoo Choi
This patchset fix wrong compatible string for Exynos3250 ADC. Exynos3250 SoC
need to control only special clock for ADC. Exynos SoC except for Exynos3250
has not included special clock for ADC. The exynos ADC driver can control
special clock if compatible string is 'exynos3250-adc-v2'.
Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
arch/arm/boot/dts/exynos3250.dtsi | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/boot/dts/exynos3250.dtsi b/arch/arm/boot/dts/exynos3250.dtsi
index 6c1fb67..107dc44 100644
--- a/arch/arm/boot/dts/exynos3250.dtsi
+++ b/arch/arm/boot/dts/exynos3250.dtsi
@@ -414,10 +414,10 @@
};
adc: adc@126C0000 {
- compatible = "samsung,exynos-adc-v3";
+ compatible = "samsung,exynos3250-adc-v2";
reg = <0x126C0000 0x100>, <0x10020718 0x4>;
interrupts = <0 137 0>;
- clock-names = "adc", "sclk_tsadc";
+ clock-names = "adc", "sclk_adc";
clocks = <&cmu CLK_TSADC>, <&cmu CLK_SCLK_TSADC>;
#io-channel-cells = <1>;
io-channel-ranges;
--
1.8.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCHv4 1/4] iio: adc: exynos_adc: Add exynos_adc_ops structure to improve readability
2014-06-18 2:20 ` [PATCHv4 1/4] iio: adc: exynos_adc: Add exynos_adc_ops structure to improve readability Chanwoo Choi
@ 2014-06-18 5:27 ` Naveen Krishna Ch
2014-06-18 5:56 ` Chanwoo Choi
2014-06-18 7:55 ` Tomasz Figa
1 sibling, 1 reply; 23+ messages in thread
From: Naveen Krishna Ch @ 2014-06-18 5:27 UTC (permalink / raw)
To: Chanwoo Choi
Cc: jic23, My self, t.figa, Kukjin Kim, robh+dt, pawel.moll,
mark.rutland, ijc+devicetree, galak, rdunlap, sachin.kamat,
linux-iio, linux-samsung-soc, linux-kernel, devicetree,
linux-doc
Hello Chanwoo,
On 18 June 2014 07:50, Chanwoo Choi <cw00.choi@samsung.com> wrote:
> This patchset add 'exynos_adc_ops' structure which includes some functions
> to control ADC operation according to ADC version (v1 or v2).
>
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
This is a good piece of change, Thanks.
Reviewed-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
> ---
> drivers/iio/adc/exynos_adc.c | 174 +++++++++++++++++++++++++++++--------------
> 1 file changed, 120 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
> index 010578f..c30def6 100644
> --- a/drivers/iio/adc/exynos_adc.c
> +++ b/drivers/iio/adc/exynos_adc.c
> @@ -90,6 +90,7 @@ struct exynos_adc {
> struct clk *clk;
> unsigned int irq;
> struct regulator *vdd;
> + struct exynos_adc_ops *ops;
>
> struct completion completion;
>
> @@ -97,6 +98,13 @@ struct exynos_adc {
> unsigned int version;
> };
>
> +struct exynos_adc_ops {
> + void (*init_hw)(struct exynos_adc *info);
> + void (*clear_irq)(struct exynos_adc *info);
> + void (*start_conv)(struct exynos_adc *info, unsigned long addr);
> + void (*stop_conv)(struct exynos_adc *info);
> +};
> +
> static const struct of_device_id exynos_adc_match[] = {
> { .compatible = "samsung,exynos-adc-v1", .data = (void *)ADC_V1 },
> { .compatible = "samsung,exynos-adc-v2", .data = (void *)ADC_V2 },
> @@ -112,30 +120,98 @@ static inline unsigned int exynos_adc_get_version(struct platform_device *pdev)
> return (unsigned int)match->data;
> }
>
> -static void exynos_adc_hw_init(struct exynos_adc *info)
> +static void exynos_adc_v1_init_hw(struct exynos_adc *info)
> +{
> + u32 con1;
> +
> + /* set default prescaler values and Enable prescaler */
> + con1 = ADC_V1_CON_PRSCLV(49) | ADC_V1_CON_PRSCEN;
> +
> + /* Enable 12-bit ADC resolution */
> + con1 |= ADC_V1_CON_RES;
> + writel(con1, ADC_V1_CON(info->regs));
> +}
> +
> +static void exynos_adc_v1_start_conv(struct exynos_adc *info, unsigned long addr)
> +{
> + u32 con1;
> +
> + writel(addr, ADC_V1_MUX(info->regs));
> +
> + con1 = readl(ADC_V1_CON(info->regs));
> + writel(con1 | ADC_CON_EN_START, ADC_V1_CON(info->regs));
> +}
> +
> +static void exynos_adc_v1_clear_irq(struct exynos_adc *info)
> +{
> + writel(1, ADC_V1_INTCLR(info->regs));
> +}
> +
> +static void exynos_adc_v1_stop_conv(struct exynos_adc *info)
> +{
> + u32 con;
> +
> + con = readl(ADC_V1_CON(info->regs));
> + con |= ADC_V1_CON_STANDBY;
> + writel(con, ADC_V1_CON(info->regs));
> +}
> +
> +static struct exynos_adc_ops exynos_adc_v1_ops = {
> + .init_hw = exynos_adc_v1_init_hw,
> + .clear_irq = exynos_adc_v1_clear_irq,
> + .start_conv = exynos_adc_v1_start_conv,
> + .stop_conv = exynos_adc_v1_stop_conv,
> +};
> +
> +static void exynos_adc_v2_init_hw(struct exynos_adc *info)
> {
> u32 con1, con2;
>
> - if (info->version == ADC_V2) {
> - con1 = ADC_V2_CON1_SOFT_RESET;
> - writel(con1, ADC_V2_CON1(info->regs));
> + con1 = ADC_V2_CON1_SOFT_RESET;
> + writel(con1, ADC_V2_CON1(info->regs));
>
> - con2 = ADC_V2_CON2_OSEL | ADC_V2_CON2_ESEL |
> - ADC_V2_CON2_HIGHF | ADC_V2_CON2_C_TIME(0);
> - writel(con2, ADC_V2_CON2(info->regs));
> + con2 = ADC_V2_CON2_OSEL | ADC_V2_CON2_ESEL |
> + ADC_V2_CON2_HIGHF | ADC_V2_CON2_C_TIME(0);
> + writel(con2, ADC_V2_CON2(info->regs));
>
> - /* Enable interrupts */
> - writel(1, ADC_V2_INT_EN(info->regs));
> - } else {
> - /* set default prescaler values and Enable prescaler */
> - con1 = ADC_V1_CON_PRSCLV(49) | ADC_V1_CON_PRSCEN;
> + /* Enable interrupts */
> + writel(1, ADC_V2_INT_EN(info->regs));
> +}
>
> - /* Enable 12-bit ADC resolution */
> - con1 |= ADC_V1_CON_RES;
> - writel(con1, ADC_V1_CON(info->regs));
> - }
> +static void exynos_adc_v2_start_conv(struct exynos_adc *info, unsigned long addr)
> +{
> + u32 con1, con2;
> +
> + con2 = readl(ADC_V2_CON2(info->regs));
> + con2 &= ~ADC_V2_CON2_ACH_MASK;
> + con2 |= ADC_V2_CON2_ACH_SEL(addr);
> + writel(con2, ADC_V2_CON2(info->regs));
> +
> + con1 = readl(ADC_V2_CON1(info->regs));
> + writel(con1 | ADC_CON_EN_START, ADC_V2_CON1(info->regs));
> +}
> +
> +static void exynos_adc_v2_clear_irq(struct exynos_adc *info)
> +{
> + writel(1, ADC_V2_INT_ST(info->regs));
> +}
> +
> +static void exynos_adc_v2_stop_conv(struct exynos_adc *info)
> +{
> + u32 con;
> +
> + con = readl(ADC_V2_CON1(info->regs));
> + con &= ~ADC_CON_EN_START;
> + writel(con, ADC_V2_CON1(info->regs));
> }
>
> +static struct exynos_adc_ops exynos_adc_v2_ops = {
> + .init_hw = exynos_adc_v2_init_hw,
> + .start_conv = exynos_adc_v2_start_conv,
> + .clear_irq = exynos_adc_v2_clear_irq,
> + .stop_conv = exynos_adc_v2_stop_conv,
> +};
> +
> static int exynos_read_raw(struct iio_dev *indio_dev,
> struct iio_chan_spec const *chan,
> int *val,
> @@ -144,7 +220,6 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
> {
> struct exynos_adc *info = iio_priv(indio_dev);
> unsigned long timeout;
> - u32 con1, con2;
> int ret;
>
> if (mask != IIO_CHAN_INFO_RAW)
> @@ -154,28 +229,15 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
> reinit_completion(&info->completion);
>
> /* Select the channel to be used and Trigger conversion */
> - if (info->version == ADC_V2) {
> - con2 = readl(ADC_V2_CON2(info->regs));
> - con2 &= ~ADC_V2_CON2_ACH_MASK;
> - con2 |= ADC_V2_CON2_ACH_SEL(chan->address);
> - writel(con2, ADC_V2_CON2(info->regs));
> -
> - con1 = readl(ADC_V2_CON1(info->regs));
> - writel(con1 | ADC_CON_EN_START,
> - ADC_V2_CON1(info->regs));
> - } else {
> - writel(chan->address, ADC_V1_MUX(info->regs));
> -
> - con1 = readl(ADC_V1_CON(info->regs));
> - writel(con1 | ADC_CON_EN_START,
> - ADC_V1_CON(info->regs));
> - }
> + if (info->ops->start_conv)
> + info->ops->start_conv(info, chan->address);
>
> timeout = wait_for_completion_timeout
> (&info->completion, EXYNOS_ADC_TIMEOUT);
> if (timeout == 0) {
> dev_warn(&indio_dev->dev, "Conversion timed out! Resetting\n");
> - exynos_adc_hw_init(info);
> + if (info->ops->init_hw)
> + info->ops->init_hw(info);
> ret = -ETIMEDOUT;
> } else {
> *val = info->value;
> @@ -193,13 +255,11 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
> struct exynos_adc *info = (struct exynos_adc *)dev_id;
>
> /* Read value */
> - info->value = readl(ADC_V1_DATX(info->regs)) &
> - ADC_DATX_MASK;
> + info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
> +
> /* clear irq */
> - if (info->version == ADC_V2)
> - writel(1, ADC_V2_INT_ST(info->regs));
> - else
> - writel(1, ADC_V1_INTCLR(info->regs));
> + if (info->ops->clear_irq)
> + info->ops->clear_irq(info);
>
> complete(&info->completion);
>
> @@ -277,6 +337,19 @@ static int exynos_adc_probe(struct platform_device *pdev)
>
> info = iio_priv(indio_dev);
>
> + info->version = exynos_adc_get_version(pdev);
> + switch (info->version) {
> + case ADC_V1:
> + info->ops = &exynos_adc_v1_ops;
> + break;
> + case ADC_V2:
> + info->ops = &exynos_adc_v2_ops;
> + break;
> + default:
> + dev_err(&pdev->dev, "failed to identify ADC version\n");
> + return -EINVAL;
> + };
> +
> mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> info->regs = devm_ioremap_resource(&pdev->dev, mem);
> if (IS_ERR(info->regs))
> @@ -321,8 +394,6 @@ static int exynos_adc_probe(struct platform_device *pdev)
>
> writel(1, info->enable_reg);
>
> - info->version = exynos_adc_get_version(pdev);
> -
> platform_set_drvdata(pdev, indio_dev);
>
> indio_dev->name = dev_name(&pdev->dev);
> @@ -349,7 +420,8 @@ static int exynos_adc_probe(struct platform_device *pdev)
> if (ret)
> goto err_irq;
>
> - exynos_adc_hw_init(info);
> + if (info->ops->init_hw)
> + info->ops->init_hw(info);
>
> ret = of_platform_populate(np, exynos_adc_match, NULL, &indio_dev->dev);
> if (ret < 0) {
> @@ -394,17 +466,9 @@ static int exynos_adc_suspend(struct device *dev)
> {
> struct iio_dev *indio_dev = dev_get_drvdata(dev);
> struct exynos_adc *info = iio_priv(indio_dev);
> - u32 con;
>
> - if (info->version == ADC_V2) {
> - con = readl(ADC_V2_CON1(info->regs));
> - con &= ~ADC_CON_EN_START;
> - writel(con, ADC_V2_CON1(info->regs));
> - } else {
> - con = readl(ADC_V1_CON(info->regs));
> - con |= ADC_V1_CON_STANDBY;
> - writel(con, ADC_V1_CON(info->regs));
> - }
> + if (info->ops->stop_conv)
> + info->ops->stop_conv(info);
>
> writel(0, info->enable_reg);
> clk_disable_unprepare(info->clk);
> @@ -428,7 +492,9 @@ static int exynos_adc_resume(struct device *dev)
> return ret;
>
> writel(1, info->enable_reg);
> - exynos_adc_hw_init(info);
> +
> + if (info->ops->init_hw)
> + info->ops->init_hw(info);
>
> return 0;
> }
> --
> 1.8.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Shine bright,
(: Nav :)
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCHv4 1/4] iio: adc: exynos_adc: Add exynos_adc_ops structure to improve readability
2014-06-18 5:27 ` Naveen Krishna Ch
@ 2014-06-18 5:56 ` Chanwoo Choi
0 siblings, 0 replies; 23+ messages in thread
From: Chanwoo Choi @ 2014-06-18 5:56 UTC (permalink / raw)
To: Naveen Krishna Ch
Cc: jic23, My self, t.figa, Kukjin Kim, robh+dt, pawel.moll,
mark.rutland, ijc+devicetree, galak, rdunlap, sachin.kamat,
linux-iio, linux-samsung-soc, linux-kernel, devicetree,
linux-doc
Hi Naveen,
On 06/18/2014 02:27 PM, Naveen Krishna Ch wrote:
> Hello Chanwoo,
>
> On 18 June 2014 07:50, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>> This patchset add 'exynos_adc_ops' structure which includes some functions
>> to control ADC operation according to ADC version (v1 or v2).
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>
> This is a good piece of change, Thanks.
>
> Reviewed-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
Thanks for your review.
Best Regards,
Chanwoo Choi
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCHv4 3/4] iio: devicetree: Add DT binding documentation for Exynos3250 ADC
2014-06-18 2:21 ` [PATCHv4 3/4] iio: devicetree: Add DT binding documentation for " Chanwoo Choi
@ 2014-06-18 6:12 ` Naveen Krishna Ch
2014-06-18 6:20 ` Chanwoo Choi
2014-06-18 8:35 ` Tomasz Figa
1 sibling, 1 reply; 23+ messages in thread
From: Naveen Krishna Ch @ 2014-06-18 6:12 UTC (permalink / raw)
To: Chanwoo Choi
Cc: jic23, My self, t.figa, Kukjin Kim, robh+dt, pawel.moll,
mark.rutland, ijc+devicetree, galak, rdunlap, sachin.kamat,
linux-iio, linux-samsung-soc, linux-kernel, devicetree,
linux-doc
Hello Chanwoo,
On 18 June 2014 07:51, Chanwoo Choi <cw00.choi@samsung.com> wrote:
> This patch add DT binding documentation for Exynos3250 ADC IP. Exynos3250 has
> special clock ('sclk_tsadc') for ADC which provide clock to internal ADC.
>
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
Changes look good to me.
Reviewed-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
> ---
> .../devicetree/bindings/arm/samsung/exynos-adc.txt | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
> index 5d49f2b..3a5af82 100644
> --- a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
> +++ b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
> @@ -14,6 +14,8 @@ Required properties:
> for exynos4412/5250 controllers.
> Must be "samsung,exynos-adc-v2" for
> future controllers.
> + Must be "samsung,exynos3250-adc-v2" for
> + for exynos3250 controllers.
> - reg: Contains ADC register address range (base address and
> length) and the address of the phy enable register.
> - interrupts: Contains the interrupt information for the timer. The
> @@ -21,7 +23,11 @@ Required properties:
> the Samsung device uses.
> - #io-channel-cells = <1>; As ADC has multiple outputs
> - clocks From common clock binding: handle to adc clock.
> + From common clock binding: handle to sclk_tsadc clock
> + if using Exynos3250.
> - clock-names From common clock binding: Shall be "adc".
> + From common clock binding: Shall be "sclk_tsadc"
> + if using Exynos3250.
> - vdd-supply VDD input supply.
>
> Note: child nodes can be added for auto probing from device tree.
> @@ -41,6 +47,20 @@ adc: adc@12D10000 {
> vdd-supply = <&buck5_reg>;
> };
>
> +Example: adding device info in dtsi file for Exynos3250 with additional sclk
> +
> +adc: adc@126C0000 {
> + compatible = "samsung,exynos3250-adc-v2";
> + reg = <0x126C0000 0x100>, <0x10020718 0x4>;
> + interrupts = <0 137 0>;
> + #io-channel-cells = <1>;
> + io-channel-ranges;
> +
> + clocks = <&cmu CLK_TSADC>, <&cmu CLK_SCLK_TSADC>;
> + clock-names = "adc", "sclk_adc";
> +
> + vdd-supply = <&buck5_reg>;
> +};
>
> Example: Adding child nodes in dts file
>
> --
> 1.8.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Shine bright,
(: Nav :)
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCHv4 3/4] iio: devicetree: Add DT binding documentation for Exynos3250 ADC
2014-06-18 6:12 ` Naveen Krishna Ch
@ 2014-06-18 6:20 ` Chanwoo Choi
0 siblings, 0 replies; 23+ messages in thread
From: Chanwoo Choi @ 2014-06-18 6:20 UTC (permalink / raw)
To: Naveen Krishna Ch
Cc: jic23, My self, t.figa, Kukjin Kim, robh+dt, pawel.moll,
mark.rutland, ijc+devicetree, galak, rdunlap, sachin.kamat,
linux-iio, linux-samsung-soc, linux-kernel, devicetree,
linux-doc
Hi Naveen,
On 06/18/2014 03:12 PM, Naveen Krishna Ch wrote:
> Hello Chanwoo,
>
> On 18 June 2014 07:51, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>> This patch add DT binding documentation for Exynos3250 ADC IP. Exynos3250 has
>> special clock ('sclk_tsadc') for ADC which provide clock to internal ADC.
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>
> Changes look good to me.
> Reviewed-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
Thanks for your review.
Best Regards,
Chanwoo Choi
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCHv4 1/4] iio: adc: exynos_adc: Add exynos_adc_ops structure to improve readability
2014-06-18 2:20 ` [PATCHv4 1/4] iio: adc: exynos_adc: Add exynos_adc_ops structure to improve readability Chanwoo Choi
2014-06-18 5:27 ` Naveen Krishna Ch
@ 2014-06-18 7:55 ` Tomasz Figa
2014-06-20 0:20 ` Chanwoo Choi
1 sibling, 1 reply; 23+ messages in thread
From: Tomasz Figa @ 2014-06-18 7:55 UTC (permalink / raw)
To: Chanwoo Choi, jic23, ch.naveen, t.figa, kgene.kim
Cc: robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak,
rdunlap, sachin.kamat, linux-iio, linux-samsung-soc,
linux-kernel, devicetree, linux-doc
Hi Chanwoo,
On 18.06.2014 04:20, Chanwoo Choi wrote:
> This patchset add 'exynos_adc_ops' structure which includes some functions
> to control ADC operation according to ADC version (v1 or v2).
>
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> drivers/iio/adc/exynos_adc.c | 174 +++++++++++++++++++++++++++++--------------
> 1 file changed, 120 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
> index 010578f..c30def6 100644
> --- a/drivers/iio/adc/exynos_adc.c
> +++ b/drivers/iio/adc/exynos_adc.c
> @@ -90,6 +90,7 @@ struct exynos_adc {
> struct clk *clk;
> unsigned int irq;
> struct regulator *vdd;
> + struct exynos_adc_ops *ops;
>
> struct completion completion;
>
> @@ -97,6 +98,13 @@ struct exynos_adc {
> unsigned int version;
> };
>
> +struct exynos_adc_ops {
> + void (*init_hw)(struct exynos_adc *info);
> + void (*clear_irq)(struct exynos_adc *info);
> + void (*start_conv)(struct exynos_adc *info, unsigned long addr);
> + void (*stop_conv)(struct exynos_adc *info);
> +};
> +
> static const struct of_device_id exynos_adc_match[] = {
> { .compatible = "samsung,exynos-adc-v1", .data = (void *)ADC_V1 },
> { .compatible = "samsung,exynos-adc-v2", .data = (void *)ADC_V2 },
> @@ -112,30 +120,98 @@ static inline unsigned int exynos_adc_get_version(struct platform_device *pdev)
> return (unsigned int)match->data;
> }
>
> -static void exynos_adc_hw_init(struct exynos_adc *info)
> +static void exynos_adc_v1_init_hw(struct exynos_adc *info)
> +{
> + u32 con1;
> +
> + /* set default prescaler values and Enable prescaler */
> + con1 = ADC_V1_CON_PRSCLV(49) | ADC_V1_CON_PRSCEN;
> +
> + /* Enable 12-bit ADC resolution */
> + con1 |= ADC_V1_CON_RES;
> + writel(con1, ADC_V1_CON(info->regs));
> +}
> +
> +static void exynos_adc_v1_start_conv(struct exynos_adc *info, unsigned long addr)
> +{
> + u32 con1;
> +
> + writel(addr, ADC_V1_MUX(info->regs));
> +
> + con1 = readl(ADC_V1_CON(info->regs));
> + writel(con1 | ADC_CON_EN_START, ADC_V1_CON(info->regs));
> +}
> +
> +static void exynos_adc_v1_clear_irq(struct exynos_adc *info)
> +{
> + writel(1, ADC_V1_INTCLR(info->regs));
> +}
> +
> +static void exynos_adc_v1_stop_conv(struct exynos_adc *info)
> +{
> + u32 con;
> +
> + con = readl(ADC_V1_CON(info->regs));
> + con |= ADC_V1_CON_STANDBY;
> + writel(con, ADC_V1_CON(info->regs));
> +}
> +
> +static struct exynos_adc_ops exynos_adc_v1_ops = {
> + .init_hw = exynos_adc_v1_init_hw,
> + .clear_irq = exynos_adc_v1_clear_irq,
> + .start_conv = exynos_adc_v1_start_conv,
> + .stop_conv = exynos_adc_v1_stop_conv,
> +};
> +
> +static void exynos_adc_v2_init_hw(struct exynos_adc *info)
> {
> u32 con1, con2;
>
> - if (info->version == ADC_V2) {
> - con1 = ADC_V2_CON1_SOFT_RESET;
> - writel(con1, ADC_V2_CON1(info->regs));
> + con1 = ADC_V2_CON1_SOFT_RESET;
> + writel(con1, ADC_V2_CON1(info->regs));
>
> - con2 = ADC_V2_CON2_OSEL | ADC_V2_CON2_ESEL |
> - ADC_V2_CON2_HIGHF | ADC_V2_CON2_C_TIME(0);
> - writel(con2, ADC_V2_CON2(info->regs));
> + con2 = ADC_V2_CON2_OSEL | ADC_V2_CON2_ESEL |
> + ADC_V2_CON2_HIGHF | ADC_V2_CON2_C_TIME(0);
> + writel(con2, ADC_V2_CON2(info->regs));
>
> - /* Enable interrupts */
> - writel(1, ADC_V2_INT_EN(info->regs));
> - } else {
> - /* set default prescaler values and Enable prescaler */
> - con1 = ADC_V1_CON_PRSCLV(49) | ADC_V1_CON_PRSCEN;
> + /* Enable interrupts */
> + writel(1, ADC_V2_INT_EN(info->regs));
> +}
>
> - /* Enable 12-bit ADC resolution */
> - con1 |= ADC_V1_CON_RES;
> - writel(con1, ADC_V1_CON(info->regs));
> - }
> +static void exynos_adc_v2_start_conv(struct exynos_adc *info, unsigned long addr)
> +{
> + u32 con1, con2;
> +
> + con2 = readl(ADC_V2_CON2(info->regs));
> + con2 &= ~ADC_V2_CON2_ACH_MASK;
> + con2 |= ADC_V2_CON2_ACH_SEL(addr);
> + writel(con2, ADC_V2_CON2(info->regs));
> +
> + con1 = readl(ADC_V2_CON1(info->regs));
> + writel(con1 | ADC_CON_EN_START, ADC_V2_CON1(info->regs));
> +}
> +
> +static void exynos_adc_v2_clear_irq(struct exynos_adc *info)
> +{
> + writel(1, ADC_V2_INT_ST(info->regs));
> +}
> +
> +static void exynos_adc_v2_stop_conv(struct exynos_adc *info)
> +{
> + u32 con;
> +
> + con = readl(ADC_V2_CON1(info->regs));
> + con &= ~ADC_CON_EN_START;
> + writel(con, ADC_V2_CON1(info->regs));
> }
>
> +static struct exynos_adc_ops exynos_adc_v2_ops = {
> + .init_hw = exynos_adc_v2_init_hw,
> + .start_conv = exynos_adc_v2_start_conv,
> + .clear_irq = exynos_adc_v2_clear_irq,
> + .stop_conv = exynos_adc_v2_stop_conv,
> +};
> +
> static int exynos_read_raw(struct iio_dev *indio_dev,
> struct iio_chan_spec const *chan,
> int *val,
> @@ -144,7 +220,6 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
> {
> struct exynos_adc *info = iio_priv(indio_dev);
> unsigned long timeout;
> - u32 con1, con2;
> int ret;
>
> if (mask != IIO_CHAN_INFO_RAW)
> @@ -154,28 +229,15 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
> reinit_completion(&info->completion);
>
> /* Select the channel to be used and Trigger conversion */
> - if (info->version == ADC_V2) {
> - con2 = readl(ADC_V2_CON2(info->regs));
> - con2 &= ~ADC_V2_CON2_ACH_MASK;
> - con2 |= ADC_V2_CON2_ACH_SEL(chan->address);
> - writel(con2, ADC_V2_CON2(info->regs));
> -
> - con1 = readl(ADC_V2_CON1(info->regs));
> - writel(con1 | ADC_CON_EN_START,
> - ADC_V2_CON1(info->regs));
> - } else {
> - writel(chan->address, ADC_V1_MUX(info->regs));
> -
> - con1 = readl(ADC_V1_CON(info->regs));
> - writel(con1 | ADC_CON_EN_START,
> - ADC_V1_CON(info->regs));
> - }
> + if (info->ops->start_conv)
> + info->ops->start_conv(info, chan->address);
>
> timeout = wait_for_completion_timeout
> (&info->completion, EXYNOS_ADC_TIMEOUT);
> if (timeout == 0) {
> dev_warn(&indio_dev->dev, "Conversion timed out! Resetting\n");
> - exynos_adc_hw_init(info);
> + if (info->ops->init_hw)
> + info->ops->init_hw(info);
> ret = -ETIMEDOUT;
> } else {
> *val = info->value;
> @@ -193,13 +255,11 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
> struct exynos_adc *info = (struct exynos_adc *)dev_id;
>
> /* Read value */
> - info->value = readl(ADC_V1_DATX(info->regs)) &
> - ADC_DATX_MASK;
> + info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
> +
> /* clear irq */
> - if (info->version == ADC_V2)
> - writel(1, ADC_V2_INT_ST(info->regs));
> - else
> - writel(1, ADC_V1_INTCLR(info->regs));
> + if (info->ops->clear_irq)
> + info->ops->clear_irq(info);
>
> complete(&info->completion);
>
> @@ -277,6 +337,19 @@ static int exynos_adc_probe(struct platform_device *pdev)
>
> info = iio_priv(indio_dev);
>
> + info->version = exynos_adc_get_version(pdev);
> + switch (info->version) {
> + case ADC_V1:
> + info->ops = &exynos_adc_v1_ops;
> + break;
> + case ADC_V2:
> + info->ops = &exynos_adc_v2_ops;
> + break;
> + default:
> + dev_err(&pdev->dev, "failed to identify ADC version\n");
> + return -EINVAL;
> + };
Just drop the enum completely and assign the struct pointer directly to
driver data fields in match tables. I also suspect that the struct
should be "variant" not "ops", because it will also require other data
than function pointers, e.g. "needs_sclk".
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCHv4 2/4] iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC
2014-06-18 2:20 ` [PATCHv4 2/4] iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC Chanwoo Choi
@ 2014-06-18 7:58 ` Tomasz Figa
2014-06-20 0:22 ` Chanwoo Choi
0 siblings, 1 reply; 23+ messages in thread
From: Tomasz Figa @ 2014-06-18 7:58 UTC (permalink / raw)
To: Chanwoo Choi, jic23, ch.naveen, t.figa, kgene.kim
Cc: robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak,
rdunlap, sachin.kamat, linux-iio, linux-samsung-soc,
linux-kernel, devicetree, linux-doc
Hi Chanwoo,
On 18.06.2014 04:20, Chanwoo Choi wrote:
> This patch control special clock for ADC in Exynos series's FSYS block.
> If special clock of ADC is registerd on clock list of common clk framework,
> Exynos ADC drvier have to control this clock.
>
> Exynos3250/Exynos4/Exynos5 has 'adc' clock as following:
> - 'adc' clock: bus clock for ADC
>
> Exynos3250 has additional 'sclk_adc' clock as following:
> - 'sclk_adc' clock: special clock for ADC which provide clock to internal ADC
>
> Exynos 4210/4212/4412 and Exynos5250/5420 has not included 'sclk_adc' clock
> in FSYS_BLK. But, Exynos3250 based on Cortex-A7 has only included 'sclk_adc'
> clock in FSYS_BLK.
>
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> drivers/iio/adc/exynos_adc.c | 93 ++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 81 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
> index c30def6..6b026ac 100644
> --- a/drivers/iio/adc/exynos_adc.c
> +++ b/drivers/iio/adc/exynos_adc.c
> @@ -41,7 +41,8 @@
>
> enum adc_version {
> ADC_V1,
> - ADC_V2
> + ADC_V2,
> + ADC_V2_EXYNOS3250,
> };
>
> /* EXYNOS4412/5250 ADC_V1 registers definitions */
> @@ -85,9 +86,11 @@ enum adc_version {
> #define EXYNOS_ADC_TIMEOUT (msecs_to_jiffies(100))
>
> struct exynos_adc {
> + struct device *dev;
> void __iomem *regs;
> void __iomem *enable_reg;
> struct clk *clk;
> + struct clk *sclk;
> unsigned int irq;
> struct regulator *vdd;
> struct exynos_adc_ops *ops;
> @@ -96,6 +99,7 @@ struct exynos_adc {
>
> u32 value;
> unsigned int version;
> + bool needs_sclk;
This should be rather a part of the variant struct. See my comments to
patch 1/4.
> };
>
> struct exynos_adc_ops {
> @@ -103,11 +107,21 @@ struct exynos_adc_ops {
> void (*clear_irq)(struct exynos_adc *info);
> void (*start_conv)(struct exynos_adc *info, unsigned long addr);
> void (*stop_conv)(struct exynos_adc *info);
> + void (*disable_clk)(struct exynos_adc *info);
> + int (*enable_clk)(struct exynos_adc *info);
> };
>
> static const struct of_device_id exynos_adc_match[] = {
> - { .compatible = "samsung,exynos-adc-v1", .data = (void *)ADC_V1 },
> - { .compatible = "samsung,exynos-adc-v2", .data = (void *)ADC_V2 },
> + {
> + .compatible = "samsung,exynos-adc-v1",
> + .data = (void *)ADC_V1,
> + }, {
> + .compatible = "samsung,exynos-adc-v2",
> + .data = (void *)ADC_V2,
> + }, {
> + .compatible = "samsung,exynos3250-adc-v2",
> + .data = (void *)ADC_V2_EXYNOS3250,
> + },
> {},
> };
> MODULE_DEVICE_TABLE(of, exynos_adc_match);
> @@ -156,11 +170,42 @@ static void exynos_adc_v1_stop_conv(struct exynos_adc *info)
> writel(con, ADC_V1_CON(info->regs));
> }
>
> +static void exynos_adc_disable_clk(struct exynos_adc *info)
> +{
> + if (info->needs_sclk)
> + clk_disable_unprepare(info->sclk);
> + clk_disable_unprepare(info->clk);
> +}
> +
> +static int exynos_adc_enable_clk(struct exynos_adc *info)
> +{
> + int ret;
> +
> + ret = clk_prepare_enable(info->clk);
> + if (ret) {
> + dev_err(info->dev, "failed enabling adc clock: %d\n", ret);
> + return ret;
> + }
> +
> + if (info->needs_sclk) {
> + ret = clk_prepare_enable(info->sclk);
> + if (ret) {
> + clk_disable_unprepare(info->clk);
> + dev_err(info->dev,
> + "failed enabling sclk_tsadc clock: %d\n", ret);
> + }
> + }
> +
> + return 0;
> +}
> +
> static struct exynos_adc_ops exynos_adc_v1_ops = {
> .init_hw = exynos_adc_v1_init_hw,
> .clear_irq = exynos_adc_v1_clear_irq,
> .start_conv = exynos_adc_v1_start_conv,
> .stop_conv = exynos_adc_v1_stop_conv,
> + .disable_clk = exynos_adc_disable_clk,
> + .enable_clk = exynos_adc_enable_clk,
> };
>
> static void exynos_adc_v2_init_hw(struct exynos_adc *info)
> @@ -210,6 +255,8 @@ static struct exynos_adc_ops exynos_adc_v2_ops = {
> .start_conv = exynos_adc_v2_start_conv,
> .clear_irq = exynos_adc_v2_clear_irq,
> .stop_conv = exynos_adc_v2_stop_conv,
> + .disable_clk = exynos_adc_disable_clk,
> + .enable_clk = exynos_adc_enable_clk,
Based on the fact that all variants use the same function, I don't think
there is a reason to add .{disable,enable}_clk in the ops struct. If
they diverge in future, they could be added later, but right now it
doesn't have any value.
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCHv4 3/4] iio: devicetree: Add DT binding documentation for Exynos3250 ADC
2014-06-18 2:21 ` [PATCHv4 3/4] iio: devicetree: Add DT binding documentation for " Chanwoo Choi
2014-06-18 6:12 ` Naveen Krishna Ch
@ 2014-06-18 8:35 ` Tomasz Figa
2014-06-18 8:54 ` Chanwoo Choi
1 sibling, 1 reply; 23+ messages in thread
From: Tomasz Figa @ 2014-06-18 8:35 UTC (permalink / raw)
To: Chanwoo Choi, jic23, ch.naveen, t.figa, kgene.kim
Cc: robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak,
rdunlap, sachin.kamat, linux-iio, linux-samsung-soc,
linux-kernel, devicetree, linux-doc
Hi Chanwoo,
On 18.06.2014 04:21, Chanwoo Choi wrote:
> This patch add DT binding documentation for Exynos3250 ADC IP. Exynos3250 has
> special clock ('sclk_tsadc') for ADC which provide clock to internal ADC.
>
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> .../devicetree/bindings/arm/samsung/exynos-adc.txt | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
> index 5d49f2b..3a5af82 100644
> --- a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
> +++ b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
> @@ -14,6 +14,8 @@ Required properties:
> for exynos4412/5250 controllers.
> Must be "samsung,exynos-adc-v2" for
> future controllers.
> + Must be "samsung,exynos3250-adc-v2" for
> + for exynos3250 controllers.
You might change the last line for:
for controllers compatible with ADC of Exynos3250.
This is to make it also account for possible future SoCs which need
exactly the same handling.
> - reg: Contains ADC register address range (base address and
> length) and the address of the phy enable register.
> - interrupts: Contains the interrupt information for the timer. The
> @@ -21,7 +23,11 @@ Required properties:
> the Samsung device uses.
> - #io-channel-cells = <1>; As ADC has multiple outputs
> - clocks From common clock binding: handle to adc clock.
> + From common clock binding: handle to sclk_tsadc clock
> + if using Exynos3250.
This is not clear. It might sound like the "sclk_tsadc" clock is used on
Exynos3250 and "adc" on remaining SoCs. I'd write this simply as:
>From common clock bindings: handles to clocks specified in "clock-names"
property, in the same order.
> - clock-names From common clock binding: Shall be "adc".
> + From common clock binding: Shall be "sclk_tsadc"
> + if using Exynos3250.
This is also not clear. I'd recommend something like:
>From common clock bindings: list of clock input names used by ADC block:
- "adc" : ADC bus clock,
- "sclk_tsadc" : ADC special clock (only for Exynos3250 and
compatible ADC blocks).
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCHv4 4/4] ARM: dts: Fix wrong compatible string for Exynos3250 ADC
2014-06-18 2:21 ` [PATCHv4 4/4] ARM: dts: Fix wrong compatible string " Chanwoo Choi
@ 2014-06-18 8:37 ` Tomasz Figa
2014-06-18 8:51 ` Chanwoo Choi
0 siblings, 1 reply; 23+ messages in thread
From: Tomasz Figa @ 2014-06-18 8:37 UTC (permalink / raw)
To: Chanwoo Choi, jic23, ch.naveen, t.figa, kgene.kim
Cc: robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak,
rdunlap, sachin.kamat, linux-iio, linux-samsung-soc,
linux-kernel, devicetree, linux-doc
Hi Chanwoo,
On 18.06.2014 04:21, Chanwoo Choi wrote:
> This patchset fix wrong compatible string for Exynos3250 ADC. Exynos3250 SoC
> need to control only special clock for ADC. Exynos SoC except for Exynos3250
> has not included special clock for ADC. The exynos ADC driver can control
> special clock if compatible string is 'exynos3250-adc-v2'.
>
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> arch/arm/boot/dts/exynos3250.dtsi | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/boot/dts/exynos3250.dtsi b/arch/arm/boot/dts/exynos3250.dtsi
> index 6c1fb67..107dc44 100644
> --- a/arch/arm/boot/dts/exynos3250.dtsi
> +++ b/arch/arm/boot/dts/exynos3250.dtsi
> @@ -414,10 +414,10 @@
> };
>
> adc: adc@126C0000 {
> - compatible = "samsung,exynos-adc-v3";
> + compatible = "samsung,exynos3250-adc-v2";
> reg = <0x126C0000 0x100>, <0x10020718 0x4>;
> interrupts = <0 137 0>;
> - clock-names = "adc", "sclk_tsadc";
> + clock-names = "adc", "sclk_adc";
So, is it "sclk_adc" or "sclk_tsadc"? The code uses the former, while
the documentation mentions the latter. Please fix this.
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCHv4 4/4] ARM: dts: Fix wrong compatible string for Exynos3250 ADC
2014-06-18 8:37 ` Tomasz Figa
@ 2014-06-18 8:51 ` Chanwoo Choi
0 siblings, 0 replies; 23+ messages in thread
From: Chanwoo Choi @ 2014-06-18 8:51 UTC (permalink / raw)
To: Tomasz Figa
Cc: jic23, ch.naveen, t.figa, kgene.kim, robh+dt, pawel.moll,
mark.rutland, ijc+devicetree, galak, rdunlap, sachin.kamat,
linux-iio, linux-samsung-soc, linux-kernel, devicetree,
linux-doc
Hi Tomasz,
On 06/18/2014 05:37 PM, Tomasz Figa wrote:
> Hi Chanwoo,
>
> On 18.06.2014 04:21, Chanwoo Choi wrote:
>> This patchset fix wrong compatible string for Exynos3250 ADC. Exynos3250 SoC
>> need to control only special clock for ADC. Exynos SoC except for Exynos3250
>> has not included special clock for ADC. The exynos ADC driver can control
>> special clock if compatible string is 'exynos3250-adc-v2'.
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>> arch/arm/boot/dts/exynos3250.dtsi | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/exynos3250.dtsi b/arch/arm/boot/dts/exynos3250.dtsi
>> index 6c1fb67..107dc44 100644
>> --- a/arch/arm/boot/dts/exynos3250.dtsi
>> +++ b/arch/arm/boot/dts/exynos3250.dtsi
>> @@ -414,10 +414,10 @@
>> };
>>
>> adc: adc@126C0000 {
>> - compatible = "samsung,exynos-adc-v3";
>> + compatible = "samsung,exynos3250-adc-v2";
>> reg = <0x126C0000 0x100>, <0x10020718 0x4>;
>> interrupts = <0 137 0>;
>> - clock-names = "adc", "sclk_tsadc";
>> + clock-names = "adc", "sclk_adc";
>
> So, is it "sclk_adc" or "sclk_tsadc"? The code uses the former, while
> the documentation mentions the latter. Please fix this.
OK, I'll fix it by using 'sclk_adc' clock name.
Best Regards,
Chanwoo Choi
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCHv4 3/4] iio: devicetree: Add DT binding documentation for Exynos3250 ADC
2014-06-18 8:35 ` Tomasz Figa
@ 2014-06-18 8:54 ` Chanwoo Choi
0 siblings, 0 replies; 23+ messages in thread
From: Chanwoo Choi @ 2014-06-18 8:54 UTC (permalink / raw)
To: Tomasz Figa
Cc: jic23, ch.naveen, t.figa, kgene.kim, robh+dt, pawel.moll,
mark.rutland, ijc+devicetree, galak, rdunlap, sachin.kamat,
linux-iio, linux-samsung-soc, linux-kernel, devicetree,
linux-doc
Hi Tomasz,
On 06/18/2014 05:35 PM, Tomasz Figa wrote:
> Hi Chanwoo,
>
> On 18.06.2014 04:21, Chanwoo Choi wrote:
>> This patch add DT binding documentation for Exynos3250 ADC IP. Exynos3250 has
>> special clock ('sclk_tsadc') for ADC which provide clock to internal ADC.
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>> .../devicetree/bindings/arm/samsung/exynos-adc.txt | 20 ++++++++++++++++++++
>> 1 file changed, 20 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
>> index 5d49f2b..3a5af82 100644
>> --- a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
>> +++ b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
>> @@ -14,6 +14,8 @@ Required properties:
>> for exynos4412/5250 controllers.
>> Must be "samsung,exynos-adc-v2" for
>> future controllers.
>> + Must be "samsung,exynos3250-adc-v2" for
>> + for exynos3250 controllers.
>
> You might change the last line for:
>
> for controllers compatible with ADC of Exynos3250.
>
> This is to make it also account for possible future SoCs which need
> exactly the same handling.
OK, I'll modify it as folloiwng according to your comment:
>> + Must be "samsung,exynos3250-adc-v2" for
>> + for controllers compatible with ADC of Exynos3250.
>
>
>> - reg: Contains ADC register address range (base address and
>> length) and the address of the phy enable register.
>> - interrupts: Contains the interrupt information for the timer. The
>> @@ -21,7 +23,11 @@ Required properties:
>> the Samsung device uses.
>> - #io-channel-cells = <1>; As ADC has multiple outputs
>> - clocks From common clock binding: handle to adc clock.
>> + From common clock binding: handle to sclk_tsadc clock
>> + if using Exynos3250.
>
> This is not clear. It might sound like the "sclk_tsadc" clock is used on
> Exynos3250 and "adc" on remaining SoCs. I'd write this simply as:
>
>>From common clock bindings: handles to clocks specified in "clock-names"
> property, in the same order.
I'll modify it.
>
>> - clock-names From common clock binding: Shall be "adc".
>> + From common clock binding: Shall be "sclk_tsadc"
>> + if using Exynos3250.
>
> This is also not clear. I'd recommend something like:
>
>>From common clock bindings: list of clock input names used by ADC block:
> - "adc" : ADC bus clock,
> - "sclk_tsadc" : ADC special clock (only for Exynos3250 and
> compatible ADC blocks).
I'll modify it.
Best Regards,
Chanwoo Choi
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCHv4 1/4] iio: adc: exynos_adc: Add exynos_adc_ops structure to improve readability
2014-06-18 7:55 ` Tomasz Figa
@ 2014-06-20 0:20 ` Chanwoo Choi
2014-06-20 0:24 ` Tomasz Figa
0 siblings, 1 reply; 23+ messages in thread
From: Chanwoo Choi @ 2014-06-20 0:20 UTC (permalink / raw)
To: Tomasz Figa
Cc: jic23, ch.naveen, t.figa, kgene.kim, robh+dt, pawel.moll,
mark.rutland, ijc+devicetree, galak, rdunlap, sachin.kamat,
linux-iio, linux-samsung-soc, linux-kernel, devicetree,
linux-doc
Hi Tomasz,
On 06/18/2014 04:55 PM, Tomasz Figa wrote:
> Hi Chanwoo,
>
> On 18.06.2014 04:20, Chanwoo Choi wrote:
>> This patchset add 'exynos_adc_ops' structure which includes some functions
>> to control ADC operation according to ADC version (v1 or v2).
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>> drivers/iio/adc/exynos_adc.c | 174 +++++++++++++++++++++++++++++--------------
>> 1 file changed, 120 insertions(+), 54 deletions(-)
>>
>> diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
>> index 010578f..c30def6 100644
>> --- a/drivers/iio/adc/exynos_adc.c
>> +++ b/drivers/iio/adc/exynos_adc.c
>> @@ -90,6 +90,7 @@ struct exynos_adc {
>> struct clk *clk;
>> unsigned int irq;
>> struct regulator *vdd;
>> + struct exynos_adc_ops *ops;
>>
>> struct completion completion;
>>
>> @@ -97,6 +98,13 @@ struct exynos_adc {
>> unsigned int version;
>> };
>>
>> +struct exynos_adc_ops {
>> + void (*init_hw)(struct exynos_adc *info);
>> + void (*clear_irq)(struct exynos_adc *info);
>> + void (*start_conv)(struct exynos_adc *info, unsigned long addr);
>> + void (*stop_conv)(struct exynos_adc *info);
>> +};
>> +
>> static const struct of_device_id exynos_adc_match[] = {
>> { .compatible = "samsung,exynos-adc-v1", .data = (void *)ADC_V1 },
>> { .compatible = "samsung,exynos-adc-v2", .data = (void *)ADC_V2 },
>> @@ -112,30 +120,98 @@ static inline unsigned int exynos_adc_get_version(struct platform_device *pdev)
>> return (unsigned int)match->data;
>> }
>>
>> -static void exynos_adc_hw_init(struct exynos_adc *info)
>> +static void exynos_adc_v1_init_hw(struct exynos_adc *info)
>> +{
>> + u32 con1;
>> +
>> + /* set default prescaler values and Enable prescaler */
>> + con1 = ADC_V1_CON_PRSCLV(49) | ADC_V1_CON_PRSCEN;
>> +
>> + /* Enable 12-bit ADC resolution */
>> + con1 |= ADC_V1_CON_RES;
>> + writel(con1, ADC_V1_CON(info->regs));
>> +}
>> +
>> +static void exynos_adc_v1_start_conv(struct exynos_adc *info, unsigned long addr)
>> +{
>> + u32 con1;
>> +
>> + writel(addr, ADC_V1_MUX(info->regs));
>> +
>> + con1 = readl(ADC_V1_CON(info->regs));
>> + writel(con1 | ADC_CON_EN_START, ADC_V1_CON(info->regs));
>> +}
>> +
>> +static void exynos_adc_v1_clear_irq(struct exynos_adc *info)
>> +{
>> + writel(1, ADC_V1_INTCLR(info->regs));
>> +}
>> +
>> +static void exynos_adc_v1_stop_conv(struct exynos_adc *info)
>> +{
>> + u32 con;
>> +
>> + con = readl(ADC_V1_CON(info->regs));
>> + con |= ADC_V1_CON_STANDBY;
>> + writel(con, ADC_V1_CON(info->regs));
>> +}
>> +
>> +static struct exynos_adc_ops exynos_adc_v1_ops = {
>> + .init_hw = exynos_adc_v1_init_hw,
>> + .clear_irq = exynos_adc_v1_clear_irq,
>> + .start_conv = exynos_adc_v1_start_conv,
>> + .stop_conv = exynos_adc_v1_stop_conv,
>> +};
>> +
>> +static void exynos_adc_v2_init_hw(struct exynos_adc *info)
>> {
>> u32 con1, con2;
>>
>> - if (info->version == ADC_V2) {
>> - con1 = ADC_V2_CON1_SOFT_RESET;
>> - writel(con1, ADC_V2_CON1(info->regs));
>> + con1 = ADC_V2_CON1_SOFT_RESET;
>> + writel(con1, ADC_V2_CON1(info->regs));
>>
>> - con2 = ADC_V2_CON2_OSEL | ADC_V2_CON2_ESEL |
>> - ADC_V2_CON2_HIGHF | ADC_V2_CON2_C_TIME(0);
>> - writel(con2, ADC_V2_CON2(info->regs));
>> + con2 = ADC_V2_CON2_OSEL | ADC_V2_CON2_ESEL |
>> + ADC_V2_CON2_HIGHF | ADC_V2_CON2_C_TIME(0);
>> + writel(con2, ADC_V2_CON2(info->regs));
>>
>> - /* Enable interrupts */
>> - writel(1, ADC_V2_INT_EN(info->regs));
>> - } else {
>> - /* set default prescaler values and Enable prescaler */
>> - con1 = ADC_V1_CON_PRSCLV(49) | ADC_V1_CON_PRSCEN;
>> + /* Enable interrupts */
>> + writel(1, ADC_V2_INT_EN(info->regs));
>> +}
>>
>> - /* Enable 12-bit ADC resolution */
>> - con1 |= ADC_V1_CON_RES;
>> - writel(con1, ADC_V1_CON(info->regs));
>> - }
>> +static void exynos_adc_v2_start_conv(struct exynos_adc *info, unsigned long addr)
>> +{
>> + u32 con1, con2;
>> +
>> + con2 = readl(ADC_V2_CON2(info->regs));
>> + con2 &= ~ADC_V2_CON2_ACH_MASK;
>> + con2 |= ADC_V2_CON2_ACH_SEL(addr);
>> + writel(con2, ADC_V2_CON2(info->regs));
>> +
>> + con1 = readl(ADC_V2_CON1(info->regs));
>> + writel(con1 | ADC_CON_EN_START, ADC_V2_CON1(info->regs));
>> +}
>> +
>> +static void exynos_adc_v2_clear_irq(struct exynos_adc *info)
>> +{
>> + writel(1, ADC_V2_INT_ST(info->regs));
>> +}
>> +
>> +static void exynos_adc_v2_stop_conv(struct exynos_adc *info)
>> +{
>> + u32 con;
>> +
>> + con = readl(ADC_V2_CON1(info->regs));
>> + con &= ~ADC_CON_EN_START;
>> + writel(con, ADC_V2_CON1(info->regs));
>> }
>>
>> +static struct exynos_adc_ops exynos_adc_v2_ops = {
>> + .init_hw = exynos_adc_v2_init_hw,
>> + .start_conv = exynos_adc_v2_start_conv,
>> + .clear_irq = exynos_adc_v2_clear_irq,
>> + .stop_conv = exynos_adc_v2_stop_conv,
>> +};
>> +
>> static int exynos_read_raw(struct iio_dev *indio_dev,
>> struct iio_chan_spec const *chan,
>> int *val,
>> @@ -144,7 +220,6 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
>> {
>> struct exynos_adc *info = iio_priv(indio_dev);
>> unsigned long timeout;
>> - u32 con1, con2;
>> int ret;
>>
>> if (mask != IIO_CHAN_INFO_RAW)
>> @@ -154,28 +229,15 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
>> reinit_completion(&info->completion);
>>
>> /* Select the channel to be used and Trigger conversion */
>> - if (info->version == ADC_V2) {
>> - con2 = readl(ADC_V2_CON2(info->regs));
>> - con2 &= ~ADC_V2_CON2_ACH_MASK;
>> - con2 |= ADC_V2_CON2_ACH_SEL(chan->address);
>> - writel(con2, ADC_V2_CON2(info->regs));
>> -
>> - con1 = readl(ADC_V2_CON1(info->regs));
>> - writel(con1 | ADC_CON_EN_START,
>> - ADC_V2_CON1(info->regs));
>> - } else {
>> - writel(chan->address, ADC_V1_MUX(info->regs));
>> -
>> - con1 = readl(ADC_V1_CON(info->regs));
>> - writel(con1 | ADC_CON_EN_START,
>> - ADC_V1_CON(info->regs));
>> - }
>> + if (info->ops->start_conv)
>> + info->ops->start_conv(info, chan->address);
>>
>> timeout = wait_for_completion_timeout
>> (&info->completion, EXYNOS_ADC_TIMEOUT);
>> if (timeout == 0) {
>> dev_warn(&indio_dev->dev, "Conversion timed out! Resetting\n");
>> - exynos_adc_hw_init(info);
>> + if (info->ops->init_hw)
>> + info->ops->init_hw(info);
>> ret = -ETIMEDOUT;
>> } else {
>> *val = info->value;
>> @@ -193,13 +255,11 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
>> struct exynos_adc *info = (struct exynos_adc *)dev_id;
>>
>> /* Read value */
>> - info->value = readl(ADC_V1_DATX(info->regs)) &
>> - ADC_DATX_MASK;
>> + info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
>> +
>> /* clear irq */
>> - if (info->version == ADC_V2)
>> - writel(1, ADC_V2_INT_ST(info->regs));
>> - else
>> - writel(1, ADC_V1_INTCLR(info->regs));
>> + if (info->ops->clear_irq)
>> + info->ops->clear_irq(info);
>>
>> complete(&info->completion);
>>
>> @@ -277,6 +337,19 @@ static int exynos_adc_probe(struct platform_device *pdev)
>>
>> info = iio_priv(indio_dev);
>>
>> + info->version = exynos_adc_get_version(pdev);
>> + switch (info->version) {
>> + case ADC_V1:
>> + info->ops = &exynos_adc_v1_ops;
>> + break;
>> + case ADC_V2:
>> + info->ops = &exynos_adc_v2_ops;
>> + break;
>> + default:
>> + dev_err(&pdev->dev, "failed to identify ADC version\n");
>> + return -EINVAL;
>> + };
>
> Just drop the enum completely and assign the struct pointer directly to
> driver data fields in match tables. I also suspect that the struct
> should be "variant" not "ops", because it will also require other data
> than function pointers, e.g. "needs_sclk".
OK, I will use "variant" structure instead of "ops" and drop enum vairable of ADC version.
Best Regards,
Chanwoo Choi
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCHv4 2/4] iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC
2014-06-18 7:58 ` Tomasz Figa
@ 2014-06-20 0:22 ` Chanwoo Choi
2014-06-20 0:24 ` Tomasz Figa
0 siblings, 1 reply; 23+ messages in thread
From: Chanwoo Choi @ 2014-06-20 0:22 UTC (permalink / raw)
To: Tomasz Figa
Cc: jic23, ch.naveen, t.figa, kgene.kim, robh+dt, pawel.moll,
mark.rutland, ijc+devicetree, galak, rdunlap, sachin.kamat,
linux-iio, linux-samsung-soc, linux-kernel, devicetree,
linux-doc
Hi Tomasz,
On 06/18/2014 04:58 PM, Tomasz Figa wrote:
> Hi Chanwoo,
>
> On 18.06.2014 04:20, Chanwoo Choi wrote:
>> This patch control special clock for ADC in Exynos series's FSYS block.
>> If special clock of ADC is registerd on clock list of common clk framework,
>> Exynos ADC drvier have to control this clock.
>>
>> Exynos3250/Exynos4/Exynos5 has 'adc' clock as following:
>> - 'adc' clock: bus clock for ADC
>>
>> Exynos3250 has additional 'sclk_adc' clock as following:
>> - 'sclk_adc' clock: special clock for ADC which provide clock to internal ADC
>>
>> Exynos 4210/4212/4412 and Exynos5250/5420 has not included 'sclk_adc' clock
>> in FSYS_BLK. But, Exynos3250 based on Cortex-A7 has only included 'sclk_adc'
>> clock in FSYS_BLK.
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>> drivers/iio/adc/exynos_adc.c | 93 ++++++++++++++++++++++++++++++++++++++------
>> 1 file changed, 81 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
>> index c30def6..6b026ac 100644
>> --- a/drivers/iio/adc/exynos_adc.c
>> +++ b/drivers/iio/adc/exynos_adc.c
>> @@ -41,7 +41,8 @@
>>
>> enum adc_version {
>> ADC_V1,
>> - ADC_V2
>> + ADC_V2,
>> + ADC_V2_EXYNOS3250,
>> };
>>
>> /* EXYNOS4412/5250 ADC_V1 registers definitions */
>> @@ -85,9 +86,11 @@ enum adc_version {
>> #define EXYNOS_ADC_TIMEOUT (msecs_to_jiffies(100))
>>
>> struct exynos_adc {
>> + struct device *dev;
>> void __iomem *regs;
>> void __iomem *enable_reg;
>> struct clk *clk;
>> + struct clk *sclk;
>> unsigned int irq;
>> struct regulator *vdd;
>> struct exynos_adc_ops *ops;
>> @@ -96,6 +99,7 @@ struct exynos_adc {
>>
>> u32 value;
>> unsigned int version;
>> + bool needs_sclk;
>
> This should be rather a part of the variant struct. See my comments to
> patch 1/4.
OK, I'll include 'needs_sclk' in "variant" structure.
>
>> };
>>
>> struct exynos_adc_ops {
>> @@ -103,11 +107,21 @@ struct exynos_adc_ops {
>> void (*clear_irq)(struct exynos_adc *info);
>> void (*start_conv)(struct exynos_adc *info, unsigned long addr);
>> void (*stop_conv)(struct exynos_adc *info);
>> + void (*disable_clk)(struct exynos_adc *info);
>> + int (*enable_clk)(struct exynos_adc *info);
>> };
>>
>> static const struct of_device_id exynos_adc_match[] = {
>> - { .compatible = "samsung,exynos-adc-v1", .data = (void *)ADC_V1 },
>> - { .compatible = "samsung,exynos-adc-v2", .data = (void *)ADC_V2 },
>> + {
>> + .compatible = "samsung,exynos-adc-v1",
>> + .data = (void *)ADC_V1,
>> + }, {
>> + .compatible = "samsung,exynos-adc-v2",
>> + .data = (void *)ADC_V2,
>> + }, {
>> + .compatible = "samsung,exynos3250-adc-v2",
>> + .data = (void *)ADC_V2_EXYNOS3250,
>> + },
>> {},
>> };
>> MODULE_DEVICE_TABLE(of, exynos_adc_match);
>> @@ -156,11 +170,42 @@ static void exynos_adc_v1_stop_conv(struct exynos_adc *info)
>> writel(con, ADC_V1_CON(info->regs));
>> }
>>
>> +static void exynos_adc_disable_clk(struct exynos_adc *info)
>> +{
>> + if (info->needs_sclk)
>> + clk_disable_unprepare(info->sclk);
>> + clk_disable_unprepare(info->clk);
>> +}
>> +
>> +static int exynos_adc_enable_clk(struct exynos_adc *info)
>> +{
>> + int ret;
>> +
>> + ret = clk_prepare_enable(info->clk);
>> + if (ret) {
>> + dev_err(info->dev, "failed enabling adc clock: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + if (info->needs_sclk) {
>> + ret = clk_prepare_enable(info->sclk);
>> + if (ret) {
>> + clk_disable_unprepare(info->clk);
>> + dev_err(info->dev,
>> + "failed enabling sclk_tsadc clock: %d\n", ret);
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static struct exynos_adc_ops exynos_adc_v1_ops = {
>> .init_hw = exynos_adc_v1_init_hw,
>> .clear_irq = exynos_adc_v1_clear_irq,
>> .start_conv = exynos_adc_v1_start_conv,
>> .stop_conv = exynos_adc_v1_stop_conv,
>> + .disable_clk = exynos_adc_disable_clk,
>> + .enable_clk = exynos_adc_enable_clk,
>> };
>>
>> static void exynos_adc_v2_init_hw(struct exynos_adc *info)
>> @@ -210,6 +255,8 @@ static struct exynos_adc_ops exynos_adc_v2_ops = {
>> .start_conv = exynos_adc_v2_start_conv,
>> .clear_irq = exynos_adc_v2_clear_irq,
>> .stop_conv = exynos_adc_v2_stop_conv,
>> + .disable_clk = exynos_adc_disable_clk,
>> + .enable_clk = exynos_adc_enable_clk,
>
> Based on the fact that all variants use the same function, I don't think
> there is a reason to add .{disable,enable}_clk in the ops struct. If
> they diverge in future, they could be added later, but right now it
> doesn't have any value.
OK, I'll not add .{disable,enable}_clk and then just use following functions for clock control:
- exynos_adc_prepare_clk() : once execute this function in _probe()
- exynos_adc_unprepare_clk() : once execute this function in _remove()
- exynos_adc_enable_clk()
- exynos_adc_disable_clk()
Best Regards,
Chanwoo Choi
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCHv4 2/4] iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC
2014-06-20 0:22 ` Chanwoo Choi
@ 2014-06-20 0:24 ` Tomasz Figa
2014-06-20 0:28 ` Chanwoo Choi
0 siblings, 1 reply; 23+ messages in thread
From: Tomasz Figa @ 2014-06-20 0:24 UTC (permalink / raw)
To: Chanwoo Choi
Cc: jic23, ch.naveen, t.figa, kgene.kim, robh+dt, pawel.moll,
mark.rutland, ijc+devicetree, galak, rdunlap, sachin.kamat,
linux-iio, linux-samsung-soc, linux-kernel, devicetree,
linux-doc
On 20.06.2014 02:22, Chanwoo Choi wrote:
> Hi Tomasz,
>
> On 06/18/2014 04:58 PM, Tomasz Figa wrote:
>> Hi Chanwoo,
>>
>> On 18.06.2014 04:20, Chanwoo Choi wrote:
>>> This patch control special clock for ADC in Exynos series's FSYS block.
>>> If special clock of ADC is registerd on clock list of common clk framework,
>>> Exynos ADC drvier have to control this clock.
>>>
>>> Exynos3250/Exynos4/Exynos5 has 'adc' clock as following:
>>> - 'adc' clock: bus clock for ADC
>>>
>>> Exynos3250 has additional 'sclk_adc' clock as following:
>>> - 'sclk_adc' clock: special clock for ADC which provide clock to internal ADC
>>>
>>> Exynos 4210/4212/4412 and Exynos5250/5420 has not included 'sclk_adc' clock
>>> in FSYS_BLK. But, Exynos3250 based on Cortex-A7 has only included 'sclk_adc'
>>> clock in FSYS_BLK.
>>>
>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>>> ---
>>> drivers/iio/adc/exynos_adc.c | 93 ++++++++++++++++++++++++++++++++++++++------
>>> 1 file changed, 81 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
>>> index c30def6..6b026ac 100644
>>> --- a/drivers/iio/adc/exynos_adc.c
>>> +++ b/drivers/iio/adc/exynos_adc.c
>>> @@ -41,7 +41,8 @@
>>>
>>> enum adc_version {
>>> ADC_V1,
>>> - ADC_V2
>>> + ADC_V2,
>>> + ADC_V2_EXYNOS3250,
>>> };
>>>
>>> /* EXYNOS4412/5250 ADC_V1 registers definitions */
>>> @@ -85,9 +86,11 @@ enum adc_version {
>>> #define EXYNOS_ADC_TIMEOUT (msecs_to_jiffies(100))
>>>
>>> struct exynos_adc {
>>> + struct device *dev;
>>> void __iomem *regs;
>>> void __iomem *enable_reg;
>>> struct clk *clk;
>>> + struct clk *sclk;
>>> unsigned int irq;
>>> struct regulator *vdd;
>>> struct exynos_adc_ops *ops;
>>> @@ -96,6 +99,7 @@ struct exynos_adc {
>>>
>>> u32 value;
>>> unsigned int version;
>>> + bool needs_sclk;
>>
>> This should be rather a part of the variant struct. See my comments to
>> patch 1/4.
>
> OK, I'll include 'needs_sclk' in "variant" structure.
>
>>
>>> };
>>>
>>> struct exynos_adc_ops {
>>> @@ -103,11 +107,21 @@ struct exynos_adc_ops {
>>> void (*clear_irq)(struct exynos_adc *info);
>>> void (*start_conv)(struct exynos_adc *info, unsigned long addr);
>>> void (*stop_conv)(struct exynos_adc *info);
>>> + void (*disable_clk)(struct exynos_adc *info);
>>> + int (*enable_clk)(struct exynos_adc *info);
>>> };
>>>
>>> static const struct of_device_id exynos_adc_match[] = {
>>> - { .compatible = "samsung,exynos-adc-v1", .data = (void *)ADC_V1 },
>>> - { .compatible = "samsung,exynos-adc-v2", .data = (void *)ADC_V2 },
>>> + {
>>> + .compatible = "samsung,exynos-adc-v1",
>>> + .data = (void *)ADC_V1,
>>> + }, {
>>> + .compatible = "samsung,exynos-adc-v2",
>>> + .data = (void *)ADC_V2,
>>> + }, {
>>> + .compatible = "samsung,exynos3250-adc-v2",
>>> + .data = (void *)ADC_V2_EXYNOS3250,
>>> + },
>>> {},
>>> };
>>> MODULE_DEVICE_TABLE(of, exynos_adc_match);
>>> @@ -156,11 +170,42 @@ static void exynos_adc_v1_stop_conv(struct exynos_adc *info)
>>> writel(con, ADC_V1_CON(info->regs));
>>> }
>>>
>>> +static void exynos_adc_disable_clk(struct exynos_adc *info)
>>> +{
>>> + if (info->needs_sclk)
>>> + clk_disable_unprepare(info->sclk);
>>> + clk_disable_unprepare(info->clk);
>>> +}
>>> +
>>> +static int exynos_adc_enable_clk(struct exynos_adc *info)
>>> +{
>>> + int ret;
>>> +
>>> + ret = clk_prepare_enable(info->clk);
>>> + if (ret) {
>>> + dev_err(info->dev, "failed enabling adc clock: %d\n", ret);
>>> + return ret;
>>> + }
>>> +
>>> + if (info->needs_sclk) {
>>> + ret = clk_prepare_enable(info->sclk);
>>> + if (ret) {
>>> + clk_disable_unprepare(info->clk);
>>> + dev_err(info->dev,
>>> + "failed enabling sclk_tsadc clock: %d\n", ret);
>>> + }
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> static struct exynos_adc_ops exynos_adc_v1_ops = {
>>> .init_hw = exynos_adc_v1_init_hw,
>>> .clear_irq = exynos_adc_v1_clear_irq,
>>> .start_conv = exynos_adc_v1_start_conv,
>>> .stop_conv = exynos_adc_v1_stop_conv,
>>> + .disable_clk = exynos_adc_disable_clk,
>>> + .enable_clk = exynos_adc_enable_clk,
>>> };
>>>
>>> static void exynos_adc_v2_init_hw(struct exynos_adc *info)
>>> @@ -210,6 +255,8 @@ static struct exynos_adc_ops exynos_adc_v2_ops = {
>>> .start_conv = exynos_adc_v2_start_conv,
>>> .clear_irq = exynos_adc_v2_clear_irq,
>>> .stop_conv = exynos_adc_v2_stop_conv,
>>> + .disable_clk = exynos_adc_disable_clk,
>>> + .enable_clk = exynos_adc_enable_clk,
>>
>> Based on the fact that all variants use the same function, I don't think
>> there is a reason to add .{disable,enable}_clk in the ops struct. If
>> they diverge in future, they could be added later, but right now it
>> doesn't have any value.
>
> OK, I'll not add .{disable,enable}_clk and then just use following functions for clock control:
> - exynos_adc_prepare_clk() : once execute this function in _probe()
> - exynos_adc_unprepare_clk() : once execute this function in _remove()
> - exynos_adc_enable_clk()
> - exynos_adc_disable_clk()
Is there any need to separate prepare/unprepare from enable/disable?
Otherwise sounds good, thanks.
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCHv4 1/4] iio: adc: exynos_adc: Add exynos_adc_ops structure to improve readability
2014-06-20 0:20 ` Chanwoo Choi
@ 2014-06-20 0:24 ` Tomasz Figa
0 siblings, 0 replies; 23+ messages in thread
From: Tomasz Figa @ 2014-06-20 0:24 UTC (permalink / raw)
To: Chanwoo Choi
Cc: jic23, ch.naveen, t.figa, kgene.kim, robh+dt, pawel.moll,
mark.rutland, ijc+devicetree, galak, rdunlap, sachin.kamat,
linux-iio, linux-samsung-soc, linux-kernel, devicetree,
linux-doc
On 20.06.2014 02:20, Chanwoo Choi wrote:
> Hi Tomasz,
>
> On 06/18/2014 04:55 PM, Tomasz Figa wrote:
>> Hi Chanwoo,
>>
>> On 18.06.2014 04:20, Chanwoo Choi wrote:
>>> This patchset add 'exynos_adc_ops' structure which includes some functions
>>> to control ADC operation according to ADC version (v1 or v2).
>>>
>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>>> ---
>>> drivers/iio/adc/exynos_adc.c | 174 +++++++++++++++++++++++++++++--------------
>>> 1 file changed, 120 insertions(+), 54 deletions(-)
>>>
>>> diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
>>> index 010578f..c30def6 100644
>>> --- a/drivers/iio/adc/exynos_adc.c
>>> +++ b/drivers/iio/adc/exynos_adc.c
>>> @@ -90,6 +90,7 @@ struct exynos_adc {
>>> struct clk *clk;
>>> unsigned int irq;
>>> struct regulator *vdd;
>>> + struct exynos_adc_ops *ops;
>>>
>>> struct completion completion;
>>>
>>> @@ -97,6 +98,13 @@ struct exynos_adc {
>>> unsigned int version;
>>> };
>>>
>>> +struct exynos_adc_ops {
>>> + void (*init_hw)(struct exynos_adc *info);
>>> + void (*clear_irq)(struct exynos_adc *info);
>>> + void (*start_conv)(struct exynos_adc *info, unsigned long addr);
>>> + void (*stop_conv)(struct exynos_adc *info);
>>> +};
>>> +
>>> static const struct of_device_id exynos_adc_match[] = {
>>> { .compatible = "samsung,exynos-adc-v1", .data = (void *)ADC_V1 },
>>> { .compatible = "samsung,exynos-adc-v2", .data = (void *)ADC_V2 },
>>> @@ -112,30 +120,98 @@ static inline unsigned int exynos_adc_get_version(struct platform_device *pdev)
>>> return (unsigned int)match->data;
>>> }
>>>
>>> -static void exynos_adc_hw_init(struct exynos_adc *info)
>>> +static void exynos_adc_v1_init_hw(struct exynos_adc *info)
>>> +{
>>> + u32 con1;
>>> +
>>> + /* set default prescaler values and Enable prescaler */
>>> + con1 = ADC_V1_CON_PRSCLV(49) | ADC_V1_CON_PRSCEN;
>>> +
>>> + /* Enable 12-bit ADC resolution */
>>> + con1 |= ADC_V1_CON_RES;
>>> + writel(con1, ADC_V1_CON(info->regs));
>>> +}
>>> +
>>> +static void exynos_adc_v1_start_conv(struct exynos_adc *info, unsigned long addr)
>>> +{
>>> + u32 con1;
>>> +
>>> + writel(addr, ADC_V1_MUX(info->regs));
>>> +
>>> + con1 = readl(ADC_V1_CON(info->regs));
>>> + writel(con1 | ADC_CON_EN_START, ADC_V1_CON(info->regs));
>>> +}
>>> +
>>> +static void exynos_adc_v1_clear_irq(struct exynos_adc *info)
>>> +{
>>> + writel(1, ADC_V1_INTCLR(info->regs));
>>> +}
>>> +
>>> +static void exynos_adc_v1_stop_conv(struct exynos_adc *info)
>>> +{
>>> + u32 con;
>>> +
>>> + con = readl(ADC_V1_CON(info->regs));
>>> + con |= ADC_V1_CON_STANDBY;
>>> + writel(con, ADC_V1_CON(info->regs));
>>> +}
>>> +
>>> +static struct exynos_adc_ops exynos_adc_v1_ops = {
>>> + .init_hw = exynos_adc_v1_init_hw,
>>> + .clear_irq = exynos_adc_v1_clear_irq,
>>> + .start_conv = exynos_adc_v1_start_conv,
>>> + .stop_conv = exynos_adc_v1_stop_conv,
>>> +};
>>> +
>>> +static void exynos_adc_v2_init_hw(struct exynos_adc *info)
>>> {
>>> u32 con1, con2;
>>>
>>> - if (info->version == ADC_V2) {
>>> - con1 = ADC_V2_CON1_SOFT_RESET;
>>> - writel(con1, ADC_V2_CON1(info->regs));
>>> + con1 = ADC_V2_CON1_SOFT_RESET;
>>> + writel(con1, ADC_V2_CON1(info->regs));
>>>
>>> - con2 = ADC_V2_CON2_OSEL | ADC_V2_CON2_ESEL |
>>> - ADC_V2_CON2_HIGHF | ADC_V2_CON2_C_TIME(0);
>>> - writel(con2, ADC_V2_CON2(info->regs));
>>> + con2 = ADC_V2_CON2_OSEL | ADC_V2_CON2_ESEL |
>>> + ADC_V2_CON2_HIGHF | ADC_V2_CON2_C_TIME(0);
>>> + writel(con2, ADC_V2_CON2(info->regs));
>>>
>>> - /* Enable interrupts */
>>> - writel(1, ADC_V2_INT_EN(info->regs));
>>> - } else {
>>> - /* set default prescaler values and Enable prescaler */
>>> - con1 = ADC_V1_CON_PRSCLV(49) | ADC_V1_CON_PRSCEN;
>>> + /* Enable interrupts */
>>> + writel(1, ADC_V2_INT_EN(info->regs));
>>> +}
>>>
>>> - /* Enable 12-bit ADC resolution */
>>> - con1 |= ADC_V1_CON_RES;
>>> - writel(con1, ADC_V1_CON(info->regs));
>>> - }
>>> +static void exynos_adc_v2_start_conv(struct exynos_adc *info, unsigned long addr)
>>> +{
>>> + u32 con1, con2;
>>> +
>>> + con2 = readl(ADC_V2_CON2(info->regs));
>>> + con2 &= ~ADC_V2_CON2_ACH_MASK;
>>> + con2 |= ADC_V2_CON2_ACH_SEL(addr);
>>> + writel(con2, ADC_V2_CON2(info->regs));
>>> +
>>> + con1 = readl(ADC_V2_CON1(info->regs));
>>> + writel(con1 | ADC_CON_EN_START, ADC_V2_CON1(info->regs));
>>> +}
>>> +
>>> +static void exynos_adc_v2_clear_irq(struct exynos_adc *info)
>>> +{
>>> + writel(1, ADC_V2_INT_ST(info->regs));
>>> +}
>>> +
>>> +static void exynos_adc_v2_stop_conv(struct exynos_adc *info)
>>> +{
>>> + u32 con;
>>> +
>>> + con = readl(ADC_V2_CON1(info->regs));
>>> + con &= ~ADC_CON_EN_START;
>>> + writel(con, ADC_V2_CON1(info->regs));
>>> }
>>>
>>> +static struct exynos_adc_ops exynos_adc_v2_ops = {
>>> + .init_hw = exynos_adc_v2_init_hw,
>>> + .start_conv = exynos_adc_v2_start_conv,
>>> + .clear_irq = exynos_adc_v2_clear_irq,
>>> + .stop_conv = exynos_adc_v2_stop_conv,
>>> +};
>>> +
>>> static int exynos_read_raw(struct iio_dev *indio_dev,
>>> struct iio_chan_spec const *chan,
>>> int *val,
>>> @@ -144,7 +220,6 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
>>> {
>>> struct exynos_adc *info = iio_priv(indio_dev);
>>> unsigned long timeout;
>>> - u32 con1, con2;
>>> int ret;
>>>
>>> if (mask != IIO_CHAN_INFO_RAW)
>>> @@ -154,28 +229,15 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
>>> reinit_completion(&info->completion);
>>>
>>> /* Select the channel to be used and Trigger conversion */
>>> - if (info->version == ADC_V2) {
>>> - con2 = readl(ADC_V2_CON2(info->regs));
>>> - con2 &= ~ADC_V2_CON2_ACH_MASK;
>>> - con2 |= ADC_V2_CON2_ACH_SEL(chan->address);
>>> - writel(con2, ADC_V2_CON2(info->regs));
>>> -
>>> - con1 = readl(ADC_V2_CON1(info->regs));
>>> - writel(con1 | ADC_CON_EN_START,
>>> - ADC_V2_CON1(info->regs));
>>> - } else {
>>> - writel(chan->address, ADC_V1_MUX(info->regs));
>>> -
>>> - con1 = readl(ADC_V1_CON(info->regs));
>>> - writel(con1 | ADC_CON_EN_START,
>>> - ADC_V1_CON(info->regs));
>>> - }
>>> + if (info->ops->start_conv)
>>> + info->ops->start_conv(info, chan->address);
>>>
>>> timeout = wait_for_completion_timeout
>>> (&info->completion, EXYNOS_ADC_TIMEOUT);
>>> if (timeout == 0) {
>>> dev_warn(&indio_dev->dev, "Conversion timed out! Resetting\n");
>>> - exynos_adc_hw_init(info);
>>> + if (info->ops->init_hw)
>>> + info->ops->init_hw(info);
>>> ret = -ETIMEDOUT;
>>> } else {
>>> *val = info->value;
>>> @@ -193,13 +255,11 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
>>> struct exynos_adc *info = (struct exynos_adc *)dev_id;
>>>
>>> /* Read value */
>>> - info->value = readl(ADC_V1_DATX(info->regs)) &
>>> - ADC_DATX_MASK;
>>> + info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
>>> +
>>> /* clear irq */
>>> - if (info->version == ADC_V2)
>>> - writel(1, ADC_V2_INT_ST(info->regs));
>>> - else
>>> - writel(1, ADC_V1_INTCLR(info->regs));
>>> + if (info->ops->clear_irq)
>>> + info->ops->clear_irq(info);
>>>
>>> complete(&info->completion);
>>>
>>> @@ -277,6 +337,19 @@ static int exynos_adc_probe(struct platform_device *pdev)
>>>
>>> info = iio_priv(indio_dev);
>>>
>>> + info->version = exynos_adc_get_version(pdev);
>>> + switch (info->version) {
>>> + case ADC_V1:
>>> + info->ops = &exynos_adc_v1_ops;
>>> + break;
>>> + case ADC_V2:
>>> + info->ops = &exynos_adc_v2_ops;
>>> + break;
>>> + default:
>>> + dev_err(&pdev->dev, "failed to identify ADC version\n");
>>> + return -EINVAL;
>>> + };
>>
>> Just drop the enum completely and assign the struct pointer directly to
>> driver data fields in match tables. I also suspect that the struct
>> should be "variant" not "ops", because it will also require other data
>> than function pointers, e.g. "needs_sclk".
>
> OK, I will use "variant" structure instead of "ops" and drop enum vairable of ADC version.
Sounds good, thanks.
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCHv4 2/4] iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC
2014-06-20 0:24 ` Tomasz Figa
@ 2014-06-20 0:28 ` Chanwoo Choi
2014-06-20 0:30 ` Tomasz Figa
0 siblings, 1 reply; 23+ messages in thread
From: Chanwoo Choi @ 2014-06-20 0:28 UTC (permalink / raw)
To: Tomasz Figa
Cc: jic23, ch.naveen, t.figa, kgene.kim, robh+dt, pawel.moll,
mark.rutland, ijc+devicetree, galak, rdunlap, sachin.kamat,
linux-iio, linux-samsung-soc, linux-kernel, devicetree,
linux-doc
On 06/20/2014 09:24 AM, Tomasz Figa wrote:
> On 20.06.2014 02:22, Chanwoo Choi wrote:
>> Hi Tomasz,
>>
>> On 06/18/2014 04:58 PM, Tomasz Figa wrote:
>>> Hi Chanwoo,
>>>
>>> On 18.06.2014 04:20, Chanwoo Choi wrote:
>>>> This patch control special clock for ADC in Exynos series's FSYS block.
>>>> If special clock of ADC is registerd on clock list of common clk framework,
>>>> Exynos ADC drvier have to control this clock.
>>>>
>>>> Exynos3250/Exynos4/Exynos5 has 'adc' clock as following:
>>>> - 'adc' clock: bus clock for ADC
>>>>
>>>> Exynos3250 has additional 'sclk_adc' clock as following:
>>>> - 'sclk_adc' clock: special clock for ADC which provide clock to internal ADC
>>>>
>>>> Exynos 4210/4212/4412 and Exynos5250/5420 has not included 'sclk_adc' clock
>>>> in FSYS_BLK. But, Exynos3250 based on Cortex-A7 has only included 'sclk_adc'
>>>> clock in FSYS_BLK.
>>>>
>>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>>>> ---
>>>> drivers/iio/adc/exynos_adc.c | 93 ++++++++++++++++++++++++++++++++++++++------
>>>> 1 file changed, 81 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
>>>> index c30def6..6b026ac 100644
>>>> --- a/drivers/iio/adc/exynos_adc.c
>>>> +++ b/drivers/iio/adc/exynos_adc.c
>>>> @@ -41,7 +41,8 @@
>>>>
>>>> enum adc_version {
>>>> ADC_V1,
>>>> - ADC_V2
>>>> + ADC_V2,
>>>> + ADC_V2_EXYNOS3250,
>>>> };
>>>>
>>>> /* EXYNOS4412/5250 ADC_V1 registers definitions */
>>>> @@ -85,9 +86,11 @@ enum adc_version {
>>>> #define EXYNOS_ADC_TIMEOUT (msecs_to_jiffies(100))
>>>>
>>>> struct exynos_adc {
>>>> + struct device *dev;
>>>> void __iomem *regs;
>>>> void __iomem *enable_reg;
>>>> struct clk *clk;
>>>> + struct clk *sclk;
>>>> unsigned int irq;
>>>> struct regulator *vdd;
>>>> struct exynos_adc_ops *ops;
>>>> @@ -96,6 +99,7 @@ struct exynos_adc {
>>>>
>>>> u32 value;
>>>> unsigned int version;
>>>> + bool needs_sclk;
>>>
>>> This should be rather a part of the variant struct. See my comments to
>>> patch 1/4.
>>
>> OK, I'll include 'needs_sclk' in "variant" structure.
>>
>>>
>>>> };
>>>>
>>>> struct exynos_adc_ops {
>>>> @@ -103,11 +107,21 @@ struct exynos_adc_ops {
>>>> void (*clear_irq)(struct exynos_adc *info);
>>>> void (*start_conv)(struct exynos_adc *info, unsigned long addr);
>>>> void (*stop_conv)(struct exynos_adc *info);
>>>> + void (*disable_clk)(struct exynos_adc *info);
>>>> + int (*enable_clk)(struct exynos_adc *info);
>>>> };
>>>>
>>>> static const struct of_device_id exynos_adc_match[] = {
>>>> - { .compatible = "samsung,exynos-adc-v1", .data = (void *)ADC_V1 },
>>>> - { .compatible = "samsung,exynos-adc-v2", .data = (void *)ADC_V2 },
>>>> + {
>>>> + .compatible = "samsung,exynos-adc-v1",
>>>> + .data = (void *)ADC_V1,
>>>> + }, {
>>>> + .compatible = "samsung,exynos-adc-v2",
>>>> + .data = (void *)ADC_V2,
>>>> + }, {
>>>> + .compatible = "samsung,exynos3250-adc-v2",
>>>> + .data = (void *)ADC_V2_EXYNOS3250,
>>>> + },
>>>> {},
>>>> };
>>>> MODULE_DEVICE_TABLE(of, exynos_adc_match);
>>>> @@ -156,11 +170,42 @@ static void exynos_adc_v1_stop_conv(struct exynos_adc *info)
>>>> writel(con, ADC_V1_CON(info->regs));
>>>> }
>>>>
>>>> +static void exynos_adc_disable_clk(struct exynos_adc *info)
>>>> +{
>>>> + if (info->needs_sclk)
>>>> + clk_disable_unprepare(info->sclk);
>>>> + clk_disable_unprepare(info->clk);
>>>> +}
>>>> +
>>>> +static int exynos_adc_enable_clk(struct exynos_adc *info)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + ret = clk_prepare_enable(info->clk);
>>>> + if (ret) {
>>>> + dev_err(info->dev, "failed enabling adc clock: %d\n", ret);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + if (info->needs_sclk) {
>>>> + ret = clk_prepare_enable(info->sclk);
>>>> + if (ret) {
>>>> + clk_disable_unprepare(info->clk);
>>>> + dev_err(info->dev,
>>>> + "failed enabling sclk_tsadc clock: %d\n", ret);
>>>> + }
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> static struct exynos_adc_ops exynos_adc_v1_ops = {
>>>> .init_hw = exynos_adc_v1_init_hw,
>>>> .clear_irq = exynos_adc_v1_clear_irq,
>>>> .start_conv = exynos_adc_v1_start_conv,
>>>> .stop_conv = exynos_adc_v1_stop_conv,
>>>> + .disable_clk = exynos_adc_disable_clk,
>>>> + .enable_clk = exynos_adc_enable_clk,
>>>> };
>>>>
>>>> static void exynos_adc_v2_init_hw(struct exynos_adc *info)
>>>> @@ -210,6 +255,8 @@ static struct exynos_adc_ops exynos_adc_v2_ops = {
>>>> .start_conv = exynos_adc_v2_start_conv,
>>>> .clear_irq = exynos_adc_v2_clear_irq,
>>>> .stop_conv = exynos_adc_v2_stop_conv,
>>>> + .disable_clk = exynos_adc_disable_clk,
>>>> + .enable_clk = exynos_adc_enable_clk,
>>>
>>> Based on the fact that all variants use the same function, I don't think
>>> there is a reason to add .{disable,enable}_clk in the ops struct. If
>>> they diverge in future, they could be added later, but right now it
>>> doesn't have any value.
>>
>> OK, I'll not add .{disable,enable}_clk and then just use following functions for clock control:
>> - exynos_adc_prepare_clk() : once execute this function in _probe()
>> - exynos_adc_unprepare_clk() : once execute this function in _remove()
>> - exynos_adc_enable_clk()
>> - exynos_adc_disable_clk()
>
> Is there any need to separate prepare/unprepare from enable/disable?
> Otherwise sounds good, thanks.
Naveen Krishna Chatradhi want to execute once prepare/unpreare in probe/remove function.
- Following comment of Naveen Krishna Chatradhi
> +static void exynos_adc_disable_clk(struct exynos_adc *info)
> +{
> + if (info->needs_sclk)
> + clk_disable_unprepare(info->sclk);
> + clk_disable_unprepare(info->clk);
(Just a nit pick) As a part of cleanup can we also change to use
clk_disable() here and clk_unprepare() once and for all at the end.
> +}
> +
> +static int exynos_adc_enable_clk(struct exynos_adc *info)
> +{
> + int ret;
> +
> + ret = clk_prepare_enable(info->clk);
> + if (ret) {
> + dev_err(info->dev, "failed enabling adc clock: %d\n", ret);
> + return ret;
> + }
> +
> + if (info->needs_sclk) {
> + ret = clk_prepare_enable(info->sclk);
Can we use clk_enable() here and clk_prepare() some where during the probe.
Best Regards,
Chanwoo Choi
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCHv4 2/4] iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC
2014-06-20 0:28 ` Chanwoo Choi
@ 2014-06-20 0:30 ` Tomasz Figa
2014-06-20 3:21 ` Naveen Krishna Ch
0 siblings, 1 reply; 23+ messages in thread
From: Tomasz Figa @ 2014-06-20 0:30 UTC (permalink / raw)
To: Chanwoo Choi
Cc: jic23, ch.naveen, t.figa, kgene.kim, robh+dt, pawel.moll,
mark.rutland, ijc+devicetree, galak, rdunlap, sachin.kamat,
linux-iio, linux-samsung-soc, linux-kernel, devicetree,
linux-doc
On 20.06.2014 02:28, Chanwoo Choi wrote:
> On 06/20/2014 09:24 AM, Tomasz Figa wrote:
>> On 20.06.2014 02:22, Chanwoo Choi wrote:
>>> Hi Tomasz,
>>>
>>> On 06/18/2014 04:58 PM, Tomasz Figa wrote:
>>>> Hi Chanwoo,
>>>>
>>>> On 18.06.2014 04:20, Chanwoo Choi wrote:
>>>>> This patch control special clock for ADC in Exynos series's FSYS block.
>>>>> If special clock of ADC is registerd on clock list of common clk framework,
>>>>> Exynos ADC drvier have to control this clock.
>>>>>
>>>>> Exynos3250/Exynos4/Exynos5 has 'adc' clock as following:
>>>>> - 'adc' clock: bus clock for ADC
>>>>>
>>>>> Exynos3250 has additional 'sclk_adc' clock as following:
>>>>> - 'sclk_adc' clock: special clock for ADC which provide clock to internal ADC
>>>>>
>>>>> Exynos 4210/4212/4412 and Exynos5250/5420 has not included 'sclk_adc' clock
>>>>> in FSYS_BLK. But, Exynos3250 based on Cortex-A7 has only included 'sclk_adc'
>>>>> clock in FSYS_BLK.
>>>>>
>>>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>>>>> ---
>>>>> drivers/iio/adc/exynos_adc.c | 93 ++++++++++++++++++++++++++++++++++++++------
>>>>> 1 file changed, 81 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
>>>>> index c30def6..6b026ac 100644
>>>>> --- a/drivers/iio/adc/exynos_adc.c
>>>>> +++ b/drivers/iio/adc/exynos_adc.c
>>>>> @@ -41,7 +41,8 @@
>>>>>
>>>>> enum adc_version {
>>>>> ADC_V1,
>>>>> - ADC_V2
>>>>> + ADC_V2,
>>>>> + ADC_V2_EXYNOS3250,
>>>>> };
>>>>>
>>>>> /* EXYNOS4412/5250 ADC_V1 registers definitions */
>>>>> @@ -85,9 +86,11 @@ enum adc_version {
>>>>> #define EXYNOS_ADC_TIMEOUT (msecs_to_jiffies(100))
>>>>>
>>>>> struct exynos_adc {
>>>>> + struct device *dev;
>>>>> void __iomem *regs;
>>>>> void __iomem *enable_reg;
>>>>> struct clk *clk;
>>>>> + struct clk *sclk;
>>>>> unsigned int irq;
>>>>> struct regulator *vdd;
>>>>> struct exynos_adc_ops *ops;
>>>>> @@ -96,6 +99,7 @@ struct exynos_adc {
>>>>>
>>>>> u32 value;
>>>>> unsigned int version;
>>>>> + bool needs_sclk;
>>>>
>>>> This should be rather a part of the variant struct. See my comments to
>>>> patch 1/4.
>>>
>>> OK, I'll include 'needs_sclk' in "variant" structure.
>>>
>>>>
>>>>> };
>>>>>
>>>>> struct exynos_adc_ops {
>>>>> @@ -103,11 +107,21 @@ struct exynos_adc_ops {
>>>>> void (*clear_irq)(struct exynos_adc *info);
>>>>> void (*start_conv)(struct exynos_adc *info, unsigned long addr);
>>>>> void (*stop_conv)(struct exynos_adc *info);
>>>>> + void (*disable_clk)(struct exynos_adc *info);
>>>>> + int (*enable_clk)(struct exynos_adc *info);
>>>>> };
>>>>>
>>>>> static const struct of_device_id exynos_adc_match[] = {
>>>>> - { .compatible = "samsung,exynos-adc-v1", .data = (void *)ADC_V1 },
>>>>> - { .compatible = "samsung,exynos-adc-v2", .data = (void *)ADC_V2 },
>>>>> + {
>>>>> + .compatible = "samsung,exynos-adc-v1",
>>>>> + .data = (void *)ADC_V1,
>>>>> + }, {
>>>>> + .compatible = "samsung,exynos-adc-v2",
>>>>> + .data = (void *)ADC_V2,
>>>>> + }, {
>>>>> + .compatible = "samsung,exynos3250-adc-v2",
>>>>> + .data = (void *)ADC_V2_EXYNOS3250,
>>>>> + },
>>>>> {},
>>>>> };
>>>>> MODULE_DEVICE_TABLE(of, exynos_adc_match);
>>>>> @@ -156,11 +170,42 @@ static void exynos_adc_v1_stop_conv(struct exynos_adc *info)
>>>>> writel(con, ADC_V1_CON(info->regs));
>>>>> }
>>>>>
>>>>> +static void exynos_adc_disable_clk(struct exynos_adc *info)
>>>>> +{
>>>>> + if (info->needs_sclk)
>>>>> + clk_disable_unprepare(info->sclk);
>>>>> + clk_disable_unprepare(info->clk);
>>>>> +}
>>>>> +
>>>>> +static int exynos_adc_enable_clk(struct exynos_adc *info)
>>>>> +{
>>>>> + int ret;
>>>>> +
>>>>> + ret = clk_prepare_enable(info->clk);
>>>>> + if (ret) {
>>>>> + dev_err(info->dev, "failed enabling adc clock: %d\n", ret);
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + if (info->needs_sclk) {
>>>>> + ret = clk_prepare_enable(info->sclk);
>>>>> + if (ret) {
>>>>> + clk_disable_unprepare(info->clk);
>>>>> + dev_err(info->dev,
>>>>> + "failed enabling sclk_tsadc clock: %d\n", ret);
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> static struct exynos_adc_ops exynos_adc_v1_ops = {
>>>>> .init_hw = exynos_adc_v1_init_hw,
>>>>> .clear_irq = exynos_adc_v1_clear_irq,
>>>>> .start_conv = exynos_adc_v1_start_conv,
>>>>> .stop_conv = exynos_adc_v1_stop_conv,
>>>>> + .disable_clk = exynos_adc_disable_clk,
>>>>> + .enable_clk = exynos_adc_enable_clk,
>>>>> };
>>>>>
>>>>> static void exynos_adc_v2_init_hw(struct exynos_adc *info)
>>>>> @@ -210,6 +255,8 @@ static struct exynos_adc_ops exynos_adc_v2_ops = {
>>>>> .start_conv = exynos_adc_v2_start_conv,
>>>>> .clear_irq = exynos_adc_v2_clear_irq,
>>>>> .stop_conv = exynos_adc_v2_stop_conv,
>>>>> + .disable_clk = exynos_adc_disable_clk,
>>>>> + .enable_clk = exynos_adc_enable_clk,
>>>>
>>>> Based on the fact that all variants use the same function, I don't think
>>>> there is a reason to add .{disable,enable}_clk in the ops struct. If
>>>> they diverge in future, they could be added later, but right now it
>>>> doesn't have any value.
>>>
>>> OK, I'll not add .{disable,enable}_clk and then just use following functions for clock control:
>>> - exynos_adc_prepare_clk() : once execute this function in _probe()
>>> - exynos_adc_unprepare_clk() : once execute this function in _remove()
>>> - exynos_adc_enable_clk()
>>> - exynos_adc_disable_clk()
>>
>> Is there any need to separate prepare/unprepare from enable/disable?
>> Otherwise sounds good, thanks.
>
> Naveen Krishna Chatradhi want to execute once prepare/unpreare in probe/remove function.
>
> - Following comment of Naveen Krishna Chatradhi
>> +static void exynos_adc_disable_clk(struct exynos_adc *info)
>> +{
>> + if (info->needs_sclk)
>> + clk_disable_unprepare(info->sclk);
>> + clk_disable_unprepare(info->clk);
>
> (Just a nit pick) As a part of cleanup can we also change to use
> clk_disable() here and clk_unprepare() once and for all at the end.
>
>> +}
>> +
>> +static int exynos_adc_enable_clk(struct exynos_adc *info)
>> +{
>> + int ret;
>> +
>> + ret = clk_prepare_enable(info->clk);
>> + if (ret) {
>> + dev_err(info->dev, "failed enabling adc clock: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + if (info->needs_sclk) {
>> + ret = clk_prepare_enable(info->sclk);
> Can we use clk_enable() here and clk_prepare() some where during the probe.
I still don't see any reason to do it. Naveen, what's the motivation for
this change? For me, it only complicates the code, without any added value.
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCHv4 2/4] iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC
2014-06-20 0:30 ` Tomasz Figa
@ 2014-06-20 3:21 ` Naveen Krishna Ch
2014-06-24 0:58 ` Chanwoo Choi
0 siblings, 1 reply; 23+ messages in thread
From: Naveen Krishna Ch @ 2014-06-20 3:21 UTC (permalink / raw)
To: Tomasz Figa
Cc: Chanwoo Choi, jic23, My self, t.figa, Kukjin Kim, robh+dt,
pawel.moll, mark.rutland, ijc+devicetree, galak, rdunlap,
sachin.kamat, linux-iio, linux-samsung-soc, linux-kernel,
devicetree, linux-doc
Hello Tomasz,
On 20 June 2014 06:00, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> On 20.06.2014 02:28, Chanwoo Choi wrote:
>> On 06/20/2014 09:24 AM, Tomasz Figa wrote:
>>> On 20.06.2014 02:22, Chanwoo Choi wrote:
>>>> Hi Tomasz,
>>>>
>>>> On 06/18/2014 04:58 PM, Tomasz Figa wrote:
>>>>> Hi Chanwoo,
>>>>>
>>>>> On 18.06.2014 04:20, Chanwoo Choi wrote:
>>>>>> This patch control special clock for ADC in Exynos series's FSYS block.
>>>>>> If special clock of ADC is registerd on clock list of common clk framework,
>>>>>> Exynos ADC drvier have to control this clock.
>>>>>>
>>>>>> Exynos3250/Exynos4/Exynos5 has 'adc' clock as following:
>>>>>> - 'adc' clock: bus clock for ADC
>>>>>>
>>>>>> Exynos3250 has additional 'sclk_adc' clock as following:
>>>>>> - 'sclk_adc' clock: special clock for ADC which provide clock to internal ADC
>>>>>>
>>>>>> Exynos 4210/4212/4412 and Exynos5250/5420 has not included 'sclk_adc' clock
>>>>>> in FSYS_BLK. But, Exynos3250 based on Cortex-A7 has only included 'sclk_adc'
>>>>>> clock in FSYS_BLK.
>>>>>>
>>>>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>>>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>>>>>> ---
>>>>>> drivers/iio/adc/exynos_adc.c | 93 ++++++++++++++++++++++++++++++++++++++------
>>>>>> 1 file changed, 81 insertions(+), 12 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
>>>>>> index c30def6..6b026ac 100644
>>>>>> --- a/drivers/iio/adc/exynos_adc.c
>>>>>> +++ b/drivers/iio/adc/exynos_adc.c
>>>>>> @@ -41,7 +41,8 @@
>>>>>>
>>>>>> enum adc_version {
>>>>>> ADC_V1,
>>>>>> - ADC_V2
>>>>>> + ADC_V2,
>>>>>> + ADC_V2_EXYNOS3250,
>>>>>> };
>>>>>>
>>>>>> /* EXYNOS4412/5250 ADC_V1 registers definitions */
>>>>>> @@ -85,9 +86,11 @@ enum adc_version {
>>>>>> #define EXYNOS_ADC_TIMEOUT (msecs_to_jiffies(100))
>>>>>>
>>>>>> struct exynos_adc {
>>>>>> + struct device *dev;
>>>>>> void __iomem *regs;
>>>>>> void __iomem *enable_reg;
>>>>>> struct clk *clk;
>>>>>> + struct clk *sclk;
>>>>>> unsigned int irq;
>>>>>> struct regulator *vdd;
>>>>>> struct exynos_adc_ops *ops;
>>>>>> @@ -96,6 +99,7 @@ struct exynos_adc {
>>>>>>
>>>>>> u32 value;
>>>>>> unsigned int version;
>>>>>> + bool needs_sclk;
>>>>>
>>>>> This should be rather a part of the variant struct. See my comments to
>>>>> patch 1/4.
>>>>
>>>> OK, I'll include 'needs_sclk' in "variant" structure.
>>>>
>>>>>
>>>>>> };
>>>>>>
>>>>>> struct exynos_adc_ops {
>>>>>> @@ -103,11 +107,21 @@ struct exynos_adc_ops {
>>>>>> void (*clear_irq)(struct exynos_adc *info);
>>>>>> void (*start_conv)(struct exynos_adc *info, unsigned long addr);
>>>>>> void (*stop_conv)(struct exynos_adc *info);
>>>>>> + void (*disable_clk)(struct exynos_adc *info);
>>>>>> + int (*enable_clk)(struct exynos_adc *info);
>>>>>> };
>>>>>>
>>>>>> static const struct of_device_id exynos_adc_match[] = {
>>>>>> - { .compatible = "samsung,exynos-adc-v1", .data = (void *)ADC_V1 },
>>>>>> - { .compatible = "samsung,exynos-adc-v2", .data = (void *)ADC_V2 },
>>>>>> + {
>>>>>> + .compatible = "samsung,exynos-adc-v1",
>>>>>> + .data = (void *)ADC_V1,
>>>>>> + }, {
>>>>>> + .compatible = "samsung,exynos-adc-v2",
>>>>>> + .data = (void *)ADC_V2,
>>>>>> + }, {
>>>>>> + .compatible = "samsung,exynos3250-adc-v2",
>>>>>> + .data = (void *)ADC_V2_EXYNOS3250,
>>>>>> + },
>>>>>> {},
>>>>>> };
>>>>>> MODULE_DEVICE_TABLE(of, exynos_adc_match);
>>>>>> @@ -156,11 +170,42 @@ static void exynos_adc_v1_stop_conv(struct exynos_adc *info)
>>>>>> writel(con, ADC_V1_CON(info->regs));
>>>>>> }
>>>>>>
>>>>>> +static void exynos_adc_disable_clk(struct exynos_adc *info)
>>>>>> +{
>>>>>> + if (info->needs_sclk)
>>>>>> + clk_disable_unprepare(info->sclk);
>>>>>> + clk_disable_unprepare(info->clk);
>>>>>> +}
>>>>>> +
>>>>>> +static int exynos_adc_enable_clk(struct exynos_adc *info)
>>>>>> +{
>>>>>> + int ret;
>>>>>> +
>>>>>> + ret = clk_prepare_enable(info->clk);
>>>>>> + if (ret) {
>>>>>> + dev_err(info->dev, "failed enabling adc clock: %d\n", ret);
>>>>>> + return ret;
>>>>>> + }
>>>>>> +
>>>>>> + if (info->needs_sclk) {
>>>>>> + ret = clk_prepare_enable(info->sclk);
>>>>>> + if (ret) {
>>>>>> + clk_disable_unprepare(info->clk);
>>>>>> + dev_err(info->dev,
>>>>>> + "failed enabling sclk_tsadc clock: %d\n", ret);
>>>>>> + }
>>>>>> + }
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> static struct exynos_adc_ops exynos_adc_v1_ops = {
>>>>>> .init_hw = exynos_adc_v1_init_hw,
>>>>>> .clear_irq = exynos_adc_v1_clear_irq,
>>>>>> .start_conv = exynos_adc_v1_start_conv,
>>>>>> .stop_conv = exynos_adc_v1_stop_conv,
>>>>>> + .disable_clk = exynos_adc_disable_clk,
>>>>>> + .enable_clk = exynos_adc_enable_clk,
>>>>>> };
>>>>>>
>>>>>> static void exynos_adc_v2_init_hw(struct exynos_adc *info)
>>>>>> @@ -210,6 +255,8 @@ static struct exynos_adc_ops exynos_adc_v2_ops = {
>>>>>> .start_conv = exynos_adc_v2_start_conv,
>>>>>> .clear_irq = exynos_adc_v2_clear_irq,
>>>>>> .stop_conv = exynos_adc_v2_stop_conv,
>>>>>> + .disable_clk = exynos_adc_disable_clk,
>>>>>> + .enable_clk = exynos_adc_enable_clk,
>>>>>
>>>>> Based on the fact that all variants use the same function, I don't think
>>>>> there is a reason to add .{disable,enable}_clk in the ops struct. If
>>>>> they diverge in future, they could be added later, but right now it
>>>>> doesn't have any value.
>>>>
>>>> OK, I'll not add .{disable,enable}_clk and then just use following functions for clock control:
>>>> - exynos_adc_prepare_clk() : once execute this function in _probe()
>>>> - exynos_adc_unprepare_clk() : once execute this function in _remove()
>>>> - exynos_adc_enable_clk()
>>>> - exynos_adc_disable_clk()
>>>
>>> Is there any need to separate prepare/unprepare from enable/disable?
>>> Otherwise sounds good, thanks.
>>
>> Naveen Krishna Chatradhi want to execute once prepare/unpreare in probe/remove function.
>>
>> - Following comment of Naveen Krishna Chatradhi
>>> +static void exynos_adc_disable_clk(struct exynos_adc *info)
>>> +{
>>> + if (info->needs_sclk)
>>> + clk_disable_unprepare(info->sclk);
>>> + clk_disable_unprepare(info->clk);
>>
>> (Just a nit pick) As a part of cleanup can we also change to use
>> clk_disable() here and clk_unprepare() once and for all at the end.
>>
>>> +}
>>> +
>>> +static int exynos_adc_enable_clk(struct exynos_adc *info)
>>> +{
>>> + int ret;
>>> +
>>> + ret = clk_prepare_enable(info->clk);
>>> + if (ret) {
>>> + dev_err(info->dev, "failed enabling adc clock: %d\n", ret);
>>> + return ret;
>>> + }
>>> +
>>> + if (info->needs_sclk) {
>>> + ret = clk_prepare_enable(info->sclk);
>> Can we use clk_enable() here and clk_prepare() some where during the probe.
>
> I still don't see any reason to do it. Naveen, what's the motivation for
> this change? For me, it only complicates the code, without any added value.
clk_prepare() and clk_unprepare() maintains the clk prepare count.
Which we may not need for every transaction.
We just need to clk_enable() and clk_disable() the clock carefully.
Thus, using clk_prepare() and clk_unprepare() once should reduce a set of
instructions for every transaction. Right ?
>
> Best regards,
> Tomasz
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Shine bright,
(: Nav :)
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCHv4 2/4] iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC
2014-06-20 3:21 ` Naveen Krishna Ch
@ 2014-06-24 0:58 ` Chanwoo Choi
0 siblings, 0 replies; 23+ messages in thread
From: Chanwoo Choi @ 2014-06-24 0:58 UTC (permalink / raw)
To: Naveen Krishna Ch
Cc: Tomasz Figa, jic23, My self, t.figa, Kukjin Kim, robh+dt,
pawel.moll, mark.rutland, ijc+devicetree, galak, rdunlap,
sachin.kamat, linux-iio, linux-samsung-soc, linux-kernel,
devicetree, linux-doc
Hi Tomasz,
On 06/20/2014 12:21 PM, Naveen Krishna Ch wrote:
> Hello Tomasz,
>
> On 20 June 2014 06:00, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> On 20.06.2014 02:28, Chanwoo Choi wrote:
>>> On 06/20/2014 09:24 AM, Tomasz Figa wrote:
>>>> On 20.06.2014 02:22, Chanwoo Choi wrote:
>>>>> Hi Tomasz,
>>>>>
>>>>> On 06/18/2014 04:58 PM, Tomasz Figa wrote:
>>>>>> Hi Chanwoo,
>>>>>>
>>>>>> On 18.06.2014 04:20, Chanwoo Choi wrote:
>>>>>>> This patch control special clock for ADC in Exynos series's FSYS block.
>>>>>>> If special clock of ADC is registerd on clock list of common clk framework,
>>>>>>> Exynos ADC drvier have to control this clock.
>>>>>>>
>>>>>>> Exynos3250/Exynos4/Exynos5 has 'adc' clock as following:
>>>>>>> - 'adc' clock: bus clock for ADC
>>>>>>>
>>>>>>> Exynos3250 has additional 'sclk_adc' clock as following:
>>>>>>> - 'sclk_adc' clock: special clock for ADC which provide clock to internal ADC
>>>>>>>
>>>>>>> Exynos 4210/4212/4412 and Exynos5250/5420 has not included 'sclk_adc' clock
>>>>>>> in FSYS_BLK. But, Exynos3250 based on Cortex-A7 has only included 'sclk_adc'
>>>>>>> clock in FSYS_BLK.
>>>>>>>
>>>>>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>>>>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>>>>>>> ---
>>>>>>> drivers/iio/adc/exynos_adc.c | 93 ++++++++++++++++++++++++++++++++++++++------
>>>>>>> 1 file changed, 81 insertions(+), 12 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
>>>>>>> index c30def6..6b026ac 100644
>>>>>>> --- a/drivers/iio/adc/exynos_adc.c
>>>>>>> +++ b/drivers/iio/adc/exynos_adc.c
>>>>>>> @@ -41,7 +41,8 @@
>>>>>>>
>>>>>>> enum adc_version {
>>>>>>> ADC_V1,
>>>>>>> - ADC_V2
>>>>>>> + ADC_V2,
>>>>>>> + ADC_V2_EXYNOS3250,
>>>>>>> };
>>>>>>>
>>>>>>> /* EXYNOS4412/5250 ADC_V1 registers definitions */
>>>>>>> @@ -85,9 +86,11 @@ enum adc_version {
>>>>>>> #define EXYNOS_ADC_TIMEOUT (msecs_to_jiffies(100))
>>>>>>>
>>>>>>> struct exynos_adc {
>>>>>>> + struct device *dev;
>>>>>>> void __iomem *regs;
>>>>>>> void __iomem *enable_reg;
>>>>>>> struct clk *clk;
>>>>>>> + struct clk *sclk;
>>>>>>> unsigned int irq;
>>>>>>> struct regulator *vdd;
>>>>>>> struct exynos_adc_ops *ops;
>>>>>>> @@ -96,6 +99,7 @@ struct exynos_adc {
>>>>>>>
>>>>>>> u32 value;
>>>>>>> unsigned int version;
>>>>>>> + bool needs_sclk;
>>>>>>
>>>>>> This should be rather a part of the variant struct. See my comments to
>>>>>> patch 1/4.
>>>>>
>>>>> OK, I'll include 'needs_sclk' in "variant" structure.
>>>>>
>>>>>>
>>>>>>> };
>>>>>>>
>>>>>>> struct exynos_adc_ops {
>>>>>>> @@ -103,11 +107,21 @@ struct exynos_adc_ops {
>>>>>>> void (*clear_irq)(struct exynos_adc *info);
>>>>>>> void (*start_conv)(struct exynos_adc *info, unsigned long addr);
>>>>>>> void (*stop_conv)(struct exynos_adc *info);
>>>>>>> + void (*disable_clk)(struct exynos_adc *info);
>>>>>>> + int (*enable_clk)(struct exynos_adc *info);
>>>>>>> };
>>>>>>>
>>>>>>> static const struct of_device_id exynos_adc_match[] = {
>>>>>>> - { .compatible = "samsung,exynos-adc-v1", .data = (void *)ADC_V1 },
>>>>>>> - { .compatible = "samsung,exynos-adc-v2", .data = (void *)ADC_V2 },
>>>>>>> + {
>>>>>>> + .compatible = "samsung,exynos-adc-v1",
>>>>>>> + .data = (void *)ADC_V1,
>>>>>>> + }, {
>>>>>>> + .compatible = "samsung,exynos-adc-v2",
>>>>>>> + .data = (void *)ADC_V2,
>>>>>>> + }, {
>>>>>>> + .compatible = "samsung,exynos3250-adc-v2",
>>>>>>> + .data = (void *)ADC_V2_EXYNOS3250,
>>>>>>> + },
>>>>>>> {},
>>>>>>> };
>>>>>>> MODULE_DEVICE_TABLE(of, exynos_adc_match);
>>>>>>> @@ -156,11 +170,42 @@ static void exynos_adc_v1_stop_conv(struct exynos_adc *info)
>>>>>>> writel(con, ADC_V1_CON(info->regs));
>>>>>>> }
>>>>>>>
>>>>>>> +static void exynos_adc_disable_clk(struct exynos_adc *info)
>>>>>>> +{
>>>>>>> + if (info->needs_sclk)
>>>>>>> + clk_disable_unprepare(info->sclk);
>>>>>>> + clk_disable_unprepare(info->clk);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static int exynos_adc_enable_clk(struct exynos_adc *info)
>>>>>>> +{
>>>>>>> + int ret;
>>>>>>> +
>>>>>>> + ret = clk_prepare_enable(info->clk);
>>>>>>> + if (ret) {
>>>>>>> + dev_err(info->dev, "failed enabling adc clock: %d\n", ret);
>>>>>>> + return ret;
>>>>>>> + }
>>>>>>> +
>>>>>>> + if (info->needs_sclk) {
>>>>>>> + ret = clk_prepare_enable(info->sclk);
>>>>>>> + if (ret) {
>>>>>>> + clk_disable_unprepare(info->clk);
>>>>>>> + dev_err(info->dev,
>>>>>>> + "failed enabling sclk_tsadc clock: %d\n", ret);
>>>>>>> + }
>>>>>>> + }
>>>>>>> +
>>>>>>> + return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> static struct exynos_adc_ops exynos_adc_v1_ops = {
>>>>>>> .init_hw = exynos_adc_v1_init_hw,
>>>>>>> .clear_irq = exynos_adc_v1_clear_irq,
>>>>>>> .start_conv = exynos_adc_v1_start_conv,
>>>>>>> .stop_conv = exynos_adc_v1_stop_conv,
>>>>>>> + .disable_clk = exynos_adc_disable_clk,
>>>>>>> + .enable_clk = exynos_adc_enable_clk,
>>>>>>> };
>>>>>>>
>>>>>>> static void exynos_adc_v2_init_hw(struct exynos_adc *info)
>>>>>>> @@ -210,6 +255,8 @@ static struct exynos_adc_ops exynos_adc_v2_ops = {
>>>>>>> .start_conv = exynos_adc_v2_start_conv,
>>>>>>> .clear_irq = exynos_adc_v2_clear_irq,
>>>>>>> .stop_conv = exynos_adc_v2_stop_conv,
>>>>>>> + .disable_clk = exynos_adc_disable_clk,
>>>>>>> + .enable_clk = exynos_adc_enable_clk,
>>>>>>
>>>>>> Based on the fact that all variants use the same function, I don't think
>>>>>> there is a reason to add .{disable,enable}_clk in the ops struct. If
>>>>>> they diverge in future, they could be added later, but right now it
>>>>>> doesn't have any value.
>>>>>
>>>>> OK, I'll not add .{disable,enable}_clk and then just use following functions for clock control:
>>>>> - exynos_adc_prepare_clk() : once execute this function in _probe()
>>>>> - exynos_adc_unprepare_clk() : once execute this function in _remove()
>>>>> - exynos_adc_enable_clk()
>>>>> - exynos_adc_disable_clk()
>>>>
>>>> Is there any need to separate prepare/unprepare from enable/disable?
>>>> Otherwise sounds good, thanks.
>>>
>>> Naveen Krishna Chatradhi want to execute once prepare/unpreare in probe/remove function.
>>>
>>> - Following comment of Naveen Krishna Chatradhi
>>>> +static void exynos_adc_disable_clk(struct exynos_adc *info)
>>>> +{
>>>> + if (info->needs_sclk)
>>>> + clk_disable_unprepare(info->sclk);
>>>> + clk_disable_unprepare(info->clk);
>>>
>>> (Just a nit pick) As a part of cleanup can we also change to use
>>> clk_disable() here and clk_unprepare() once and for all at the end.
>>>
>>>> +}
>>>> +
>>>> +static int exynos_adc_enable_clk(struct exynos_adc *info)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + ret = clk_prepare_enable(info->clk);
>>>> + if (ret) {
>>>> + dev_err(info->dev, "failed enabling adc clock: %d\n", ret);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + if (info->needs_sclk) {
>>>> + ret = clk_prepare_enable(info->sclk);
>>> Can we use clk_enable() here and clk_prepare() some where during the probe.
>>
>> I still don't see any reason to do it. Naveen, what's the motivation for
>> this change? For me, it only complicates the code, without any added value.
>
> clk_prepare() and clk_unprepare() maintains the clk prepare count.
> Which we may not need for every transaction.
>
> We just need to clk_enable() and clk_disable() the clock carefully.
>
> Thus, using clk_prepare() and clk_unprepare() once should reduce a set of
> instructions for every transaction. Right ?
Any other comment?
Best Regards,
Chanwoo Choi
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2014-06-24 0:58 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-18 2:20 [PATCHv4 0/4] iio: adc: exynos_adc: Support Exynos3250 ADC and code clean Chanwoo Choi
2014-06-18 2:20 ` [PATCHv4 1/4] iio: adc: exynos_adc: Add exynos_adc_ops structure to improve readability Chanwoo Choi
2014-06-18 5:27 ` Naveen Krishna Ch
2014-06-18 5:56 ` Chanwoo Choi
2014-06-18 7:55 ` Tomasz Figa
2014-06-20 0:20 ` Chanwoo Choi
2014-06-20 0:24 ` Tomasz Figa
2014-06-18 2:20 ` [PATCHv4 2/4] iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC Chanwoo Choi
2014-06-18 7:58 ` Tomasz Figa
2014-06-20 0:22 ` Chanwoo Choi
2014-06-20 0:24 ` Tomasz Figa
2014-06-20 0:28 ` Chanwoo Choi
2014-06-20 0:30 ` Tomasz Figa
2014-06-20 3:21 ` Naveen Krishna Ch
2014-06-24 0:58 ` Chanwoo Choi
2014-06-18 2:21 ` [PATCHv4 3/4] iio: devicetree: Add DT binding documentation for " Chanwoo Choi
2014-06-18 6:12 ` Naveen Krishna Ch
2014-06-18 6:20 ` Chanwoo Choi
2014-06-18 8:35 ` Tomasz Figa
2014-06-18 8:54 ` Chanwoo Choi
2014-06-18 2:21 ` [PATCHv4 4/4] ARM: dts: Fix wrong compatible string " Chanwoo Choi
2014-06-18 8:37 ` Tomasz Figa
2014-06-18 8:51 ` Chanwoo Choi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).