This patchset support Exynos3250 ADC (Analog Digital Converter) because
Exynos3250 has additional special clock for ADC IP.
Changes from v1:
- Add new "samsung,exynos-adc-v3" compatible to support Exynos3250 ADC
- Add a patch about DT binding documentation
Chanwoo Choi (2):
iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC
iio: devicetree: Add DT binding documentation for Exynos3250 ADC
.../devicetree/bindings/arm/samsung/exynos-adc.txt | 20 ++++++++
drivers/iio/adc/exynos_adc.c | 54 ++++++++++++++++------
2 files changed, 61 insertions(+), 13 deletions(-)
--
1.8.0
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_tsadc' clock as following:
- 'sclk_tsadc' clock: special clock for ADC which provide clock to internal ADC
Exynos 4210/4212/4412 and Exynos5250/5420 has not included 'sclk_tsadc' clock
in FSYS_BLK. But, Exynos3250 based on Cortex-A7 has only included 'sclk_tsadc'
clock in FSYS_BLK.
Cc: Jonathan Cameron <[email protected]>
Cc: Kukjin Kim <[email protected]>
Cc: Naveen Krishna Chatradhi
Cc: [email protected]
Signed-off-by: Chanwoo Choi <[email protected]>
Acked-by: Kyungmin Park <[email protected]>
---
drivers/iio/adc/exynos_adc.c | 54 +++++++++++++++++++++++++++++++++-----------
1 file changed, 41 insertions(+), 13 deletions(-)
diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
index d25b262..3c99243 100644
--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -40,8 +40,9 @@
#include <linux/iio/driver.h>
enum adc_version {
- ADC_V1,
- ADC_V2
+ ADC_V1 = 0x1,
+ ADC_V2 = 0x2,
+ ADC_V3 = (ADC_V1 | ADC_V2),
};
/* EXYNOS4412/5250 ADC_V1 registers definitions */
@@ -88,6 +89,7 @@ struct exynos_adc {
void __iomem *regs;
void __iomem *enable_reg;
struct clk *clk;
+ struct clk *sclk;
unsigned int irq;
struct regulator *vdd;
@@ -100,6 +102,7 @@ struct exynos_adc {
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-v3", .data = (void *)ADC_V3 },
{},
};
MODULE_DEVICE_TABLE(of, exynos_adc_match);
@@ -128,7 +131,7 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
mutex_lock(&indio_dev->mlock);
/* Select the channel to be used and Trigger conversion */
- if (info->version == ADC_V2) {
+ 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);
@@ -165,7 +168,7 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
info->value = readl(ADC_V1_DATX(info->regs)) &
ADC_DATX_MASK;
/* clear irq */
- if (info->version == ADC_V2)
+ if (info->version & ADC_V2)
writel(1, ADC_V2_INT_ST(info->regs));
else
writel(1, ADC_V1_INTCLR(info->regs));
@@ -226,11 +229,25 @@ static int exynos_adc_remove_devices(struct device *dev, void *c)
return 0;
}
+static void exynos_adc_enable_clock(struct exynos_adc *info, bool enable)
+{
+ if (enable) {
+ clk_prepare_enable(info->clk);
+ if (info->version == ADC_V3)
+ clk_prepare_enable(info->sclk);
+
+ } else {
+ if (info->version == ADC_V3)
+ clk_disable_unprepare(info->sclk);
+ clk_disable_unprepare(info->clk);
+ }
+}
+
static void exynos_adc_hw_init(struct exynos_adc *info)
{
u32 con1, con2;
- if (info->version == ADC_V2) {
+ if (info->version & ADC_V2) {
con1 = ADC_V2_CON1_SOFT_RESET;
writel(con1, ADC_V2_CON1(info->regs));
@@ -300,6 +317,8 @@ static int exynos_adc_probe(struct platform_device *pdev)
writel(1, info->enable_reg);
+ info->version = exynos_adc_get_version(pdev);
+
info->clk = devm_clk_get(&pdev->dev, "adc");
if (IS_ERR(info->clk)) {
dev_err(&pdev->dev, "failed getting clock, err = %ld\n",
@@ -308,6 +327,17 @@ static int exynos_adc_probe(struct platform_device *pdev)
goto err_irq;
}
+ if (info->version == ADC_V3) {
+ info->sclk = devm_clk_get(&pdev->dev, "sclk_tsadc");
+ if (IS_ERR(info->sclk)) {
+ dev_warn(&pdev->dev,
+ "failed getting sclk clock, err = %ld\n",
+ PTR_ERR(info->sclk));
+ ret = PTR_ERR(info->sclk);
+ goto err_irq;
+ }
+ }
+
info->vdd = devm_regulator_get(&pdev->dev, "vdd");
if (IS_ERR(info->vdd)) {
dev_err(&pdev->dev, "failed getting regulator, err = %ld\n",
@@ -316,8 +346,6 @@ static int exynos_adc_probe(struct platform_device *pdev)
goto err_irq;
}
- info->version = exynos_adc_get_version(pdev);
-
platform_set_drvdata(pdev, indio_dev);
indio_dev->name = dev_name(&pdev->dev);
@@ -340,7 +368,7 @@ static int exynos_adc_probe(struct platform_device *pdev)
if (ret)
goto err_iio_dev;
- clk_prepare_enable(info->clk);
+ exynos_adc_enable_clock(info, true);
exynos_adc_hw_init(info);
@@ -356,7 +384,7 @@ err_of_populate:
device_for_each_child(&pdev->dev, NULL,
exynos_adc_remove_devices);
regulator_disable(info->vdd);
- clk_disable_unprepare(info->clk);
+ exynos_adc_enable_clock(info, false);
err_iio_dev:
iio_device_unregister(indio_dev);
err_irq:
@@ -372,7 +400,7 @@ static int exynos_adc_remove(struct platform_device *pdev)
device_for_each_child(&pdev->dev, NULL,
exynos_adc_remove_devices);
regulator_disable(info->vdd);
- clk_disable_unprepare(info->clk);
+ exynos_adc_enable_clock(info, false);
writel(0, info->enable_reg);
iio_device_unregister(indio_dev);
free_irq(info->irq, info);
@@ -387,7 +415,7 @@ static int exynos_adc_suspend(struct device *dev)
struct exynos_adc *info = iio_priv(indio_dev);
u32 con;
- if (info->version == ADC_V2) {
+ if (info->version & ADC_V2) {
con = readl(ADC_V2_CON1(info->regs));
con &= ~ADC_CON_EN_START;
writel(con, ADC_V2_CON1(info->regs));
@@ -397,7 +425,7 @@ static int exynos_adc_suspend(struct device *dev)
writel(con, ADC_V1_CON(info->regs));
}
- clk_disable_unprepare(info->clk);
+ exynos_adc_enable_clock(info, false);
writel(0, info->enable_reg);
regulator_disable(info->vdd);
@@ -415,7 +443,7 @@ static int exynos_adc_resume(struct device *dev)
return ret;
writel(1, info->enable_reg);
- clk_prepare_enable(info->clk);
+ exynos_adc_enable_clock(info, true);
exynos_adc_hw_init(info);
--
1.8.0
This patch add DT binding documentation for Exynos3250 ADC IP. Exynos3250 has
special clock ('sclk_tsadc') for ADC which provide clock to internal ADC.
Cc: Rob Herring <[email protected]>
Cc: Pawel Moll <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Ian Campbell <[email protected]>
Cc: Kumar Gala <[email protected]>
Cc: Randy Dunlap <[email protected]>
Cc: Kukjin Kim <[email protected]>
Cc: Naveen Krishna Chatradhi <[email protected]>
Cc: Tomasz Figa <[email protected]>
Signed-off-by: Chanwoo Choi <[email protected]>
Acked-by: Kyungmin Park <[email protected]>
---
.../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..7532ec3 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,exynos-adc-v3" 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>;
};
+If Exynos3250 uses ADC,
+adc: adc@126C0000 {
+ compatible = "samsung,exynos-adc-v3";
+ reg = <0x126C0000 0x100>, <0x10020718 0x4>;
+ interrupts = <0 137 0>;
+ #io-channel-cells = <1>;
+ io-channel-ranges;
+
+ clock-names = "adc", "sclk_tsadc";
+ clocks = <&cmu CLK_TSADC>, <&cmu CLK_SCLK_TSADC>;
+
+ vdd-supply = <&buck5_reg>;
+};
+
Example: Adding child nodes in dts file
--
1.8.0
Hi Jonathan,
Any comment of this patchset?
Best Regards,
Chanwoo Choi
On 04/14/2014 06:07 PM, 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_tsadc' clock as following:
> - 'sclk_tsadc' clock: special clock for ADC which provide clock to internal ADC
>
> Exynos 4210/4212/4412 and Exynos5250/5420 has not included 'sclk_tsadc' clock
> in FSYS_BLK. But, Exynos3250 based on Cortex-A7 has only included 'sclk_tsadc'
> clock in FSYS_BLK.
>
> Cc: Jonathan Cameron <[email protected]>
> Cc: Kukjin Kim <[email protected]>
> Cc: Naveen Krishna Chatradhi
> Cc: [email protected]
> Signed-off-by: Chanwoo Choi <[email protected]>
> Acked-by: Kyungmin Park <[email protected]>
> ---
> drivers/iio/adc/exynos_adc.c | 54 +++++++++++++++++++++++++++++++++-----------
> 1 file changed, 41 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
> index d25b262..3c99243 100644
> --- a/drivers/iio/adc/exynos_adc.c
> +++ b/drivers/iio/adc/exynos_adc.c
> @@ -40,8 +40,9 @@
> #include <linux/iio/driver.h>
>
> enum adc_version {
> - ADC_V1,
> - ADC_V2
> + ADC_V1 = 0x1,
> + ADC_V2 = 0x2,
> + ADC_V3 = (ADC_V1 | ADC_V2),
> };
>
> /* EXYNOS4412/5250 ADC_V1 registers definitions */
> @@ -88,6 +89,7 @@ struct exynos_adc {
> void __iomem *regs;
> void __iomem *enable_reg;
> struct clk *clk;
> + struct clk *sclk;
> unsigned int irq;
> struct regulator *vdd;
>
> @@ -100,6 +102,7 @@ struct exynos_adc {
> 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-v3", .data = (void *)ADC_V3 },
> {},
> };
> MODULE_DEVICE_TABLE(of, exynos_adc_match);
> @@ -128,7 +131,7 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
> mutex_lock(&indio_dev->mlock);
>
> /* Select the channel to be used and Trigger conversion */
> - if (info->version == ADC_V2) {
> + 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);
> @@ -165,7 +168,7 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
> info->value = readl(ADC_V1_DATX(info->regs)) &
> ADC_DATX_MASK;
> /* clear irq */
> - if (info->version == ADC_V2)
> + if (info->version & ADC_V2)
> writel(1, ADC_V2_INT_ST(info->regs));
> else
> writel(1, ADC_V1_INTCLR(info->regs));
> @@ -226,11 +229,25 @@ static int exynos_adc_remove_devices(struct device *dev, void *c)
> return 0;
> }
>
> +static void exynos_adc_enable_clock(struct exynos_adc *info, bool enable)
> +{
> + if (enable) {
> + clk_prepare_enable(info->clk);
> + if (info->version == ADC_V3)
> + clk_prepare_enable(info->sclk);
> +
> + } else {
> + if (info->version == ADC_V3)
> + clk_disable_unprepare(info->sclk);
> + clk_disable_unprepare(info->clk);
> + }
> +}
> +
> static void exynos_adc_hw_init(struct exynos_adc *info)
> {
> u32 con1, con2;
>
> - if (info->version == ADC_V2) {
> + if (info->version & ADC_V2) {
> con1 = ADC_V2_CON1_SOFT_RESET;
> writel(con1, ADC_V2_CON1(info->regs));
>
> @@ -300,6 +317,8 @@ static int exynos_adc_probe(struct platform_device *pdev)
>
> writel(1, info->enable_reg);
>
> + info->version = exynos_adc_get_version(pdev);
> +
> info->clk = devm_clk_get(&pdev->dev, "adc");
> if (IS_ERR(info->clk)) {
> dev_err(&pdev->dev, "failed getting clock, err = %ld\n",
> @@ -308,6 +327,17 @@ static int exynos_adc_probe(struct platform_device *pdev)
> goto err_irq;
> }
>
> + if (info->version == ADC_V3) {
> + info->sclk = devm_clk_get(&pdev->dev, "sclk_tsadc");
> + if (IS_ERR(info->sclk)) {
> + dev_warn(&pdev->dev,
> + "failed getting sclk clock, err = %ld\n",
> + PTR_ERR(info->sclk));
> + ret = PTR_ERR(info->sclk);
> + goto err_irq;
> + }
> + }
> +
> info->vdd = devm_regulator_get(&pdev->dev, "vdd");
> if (IS_ERR(info->vdd)) {
> dev_err(&pdev->dev, "failed getting regulator, err = %ld\n",
> @@ -316,8 +346,6 @@ static int exynos_adc_probe(struct platform_device *pdev)
> goto err_irq;
> }
>
> - info->version = exynos_adc_get_version(pdev);
> -
> platform_set_drvdata(pdev, indio_dev);
>
> indio_dev->name = dev_name(&pdev->dev);
> @@ -340,7 +368,7 @@ static int exynos_adc_probe(struct platform_device *pdev)
> if (ret)
> goto err_iio_dev;
>
> - clk_prepare_enable(info->clk);
> + exynos_adc_enable_clock(info, true);
>
> exynos_adc_hw_init(info);
>
> @@ -356,7 +384,7 @@ err_of_populate:
> device_for_each_child(&pdev->dev, NULL,
> exynos_adc_remove_devices);
> regulator_disable(info->vdd);
> - clk_disable_unprepare(info->clk);
> + exynos_adc_enable_clock(info, false);
> err_iio_dev:
> iio_device_unregister(indio_dev);
> err_irq:
> @@ -372,7 +400,7 @@ static int exynos_adc_remove(struct platform_device *pdev)
> device_for_each_child(&pdev->dev, NULL,
> exynos_adc_remove_devices);
> regulator_disable(info->vdd);
> - clk_disable_unprepare(info->clk);
> + exynos_adc_enable_clock(info, false);
> writel(0, info->enable_reg);
> iio_device_unregister(indio_dev);
> free_irq(info->irq, info);
> @@ -387,7 +415,7 @@ static int exynos_adc_suspend(struct device *dev)
> struct exynos_adc *info = iio_priv(indio_dev);
> u32 con;
>
> - if (info->version == ADC_V2) {
> + if (info->version & ADC_V2) {
> con = readl(ADC_V2_CON1(info->regs));
> con &= ~ADC_CON_EN_START;
> writel(con, ADC_V2_CON1(info->regs));
> @@ -397,7 +425,7 @@ static int exynos_adc_suspend(struct device *dev)
> writel(con, ADC_V1_CON(info->regs));
> }
>
> - clk_disable_unprepare(info->clk);
> + exynos_adc_enable_clock(info, false);
> writel(0, info->enable_reg);
> regulator_disable(info->vdd);
>
> @@ -415,7 +443,7 @@ static int exynos_adc_resume(struct device *dev)
> return ret;
>
> writel(1, info->enable_reg);
> - clk_prepare_enable(info->clk);
> + exynos_adc_enable_clock(info, true);
>
> exynos_adc_hw_init(info);
>
>
Hi Chanwoo,
On 14 April 2014 14:37, Chanwoo Choi <[email protected]> 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_tsadc' clock as following:
> - 'sclk_tsadc' clock: special clock for ADC which provide clock to internal ADC
>
> Exynos 4210/4212/4412 and Exynos5250/5420 has not included 'sclk_tsadc' clock
> in FSYS_BLK. But, Exynos3250 based on Cortex-A7 has only included 'sclk_tsadc'
> clock in FSYS_BLK.
>
> Cc: Jonathan Cameron <[email protected]>
> Cc: Kukjin Kim <[email protected]>
> Cc: Naveen Krishna Chatradhi
> Cc: [email protected]
> Signed-off-by: Chanwoo Choi <[email protected]>
> Acked-by: Kyungmin Park <[email protected]>
> ---
> drivers/iio/adc/exynos_adc.c | 54 +++++++++++++++++++++++++++++++++-----------
> 1 file changed, 41 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
> index d25b262..3c99243 100644
> --- a/drivers/iio/adc/exynos_adc.c
> +++ b/drivers/iio/adc/exynos_adc.c
> @@ -40,8 +40,9 @@
> #include <linux/iio/driver.h>
>
> enum adc_version {
> - ADC_V1,
> - ADC_V2
> + ADC_V1 = 0x1,
> + ADC_V2 = 0x2,
> + ADC_V3 = (ADC_V1 | ADC_V2),
Can't this be simply 0x3? Or is this not really a h/w version?
> };
>
> /* EXYNOS4412/5250 ADC_V1 registers definitions */
> @@ -88,6 +89,7 @@ struct exynos_adc {
> void __iomem *regs;
> void __iomem *enable_reg;
> struct clk *clk;
> + struct clk *sclk;
> unsigned int irq;
> struct regulator *vdd;
>
> @@ -100,6 +102,7 @@ struct exynos_adc {
> 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-v3", .data = (void *)ADC_V3 },
> {},
> };
> MODULE_DEVICE_TABLE(of, exynos_adc_match);
> @@ -128,7 +131,7 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
> mutex_lock(&indio_dev->mlock);
>
> /* Select the channel to be used and Trigger conversion */
> - if (info->version == ADC_V2) {
> + if (info->version & ADC_V2) {
So, now this would be applicable for ADC_V3 too, right?
> con2 = readl(ADC_V2_CON2(info->regs));
> con2 &= ~ADC_V2_CON2_ACH_MASK;
> con2 |= ADC_V2_CON2_ACH_SEL(chan->address);
> @@ -165,7 +168,7 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
> info->value = readl(ADC_V1_DATX(info->regs)) &
> ADC_DATX_MASK;
> /* clear irq */
> - if (info->version == ADC_V2)
> + if (info->version & ADC_V2)
> writel(1, ADC_V2_INT_ST(info->regs));
> else
> writel(1, ADC_V1_INTCLR(info->regs));
> @@ -226,11 +229,25 @@ static int exynos_adc_remove_devices(struct device *dev, void *c)
> return 0;
> }
>
> +static void exynos_adc_enable_clock(struct exynos_adc *info, bool enable)
> +{
> + if (enable) {
> + clk_prepare_enable(info->clk);
This could fail. Is it OK without any checks?
> + if (info->version == ADC_V3)
> + clk_prepare_enable(info->sclk);
ditto.
> +
> + } else {
> + if (info->version == ADC_V3)
> + clk_disable_unprepare(info->sclk);
> + clk_disable_unprepare(info->clk);
> + }
> +}
> +
> static void exynos_adc_hw_init(struct exynos_adc *info)
> {
> u32 con1, con2;
>
> - if (info->version == ADC_V2) {
> + if (info->version & ADC_V2) {
> con1 = ADC_V2_CON1_SOFT_RESET;
> writel(con1, ADC_V2_CON1(info->regs));
>
> @@ -300,6 +317,8 @@ static int exynos_adc_probe(struct platform_device *pdev)
>
> writel(1, info->enable_reg);
>
> + info->version = exynos_adc_get_version(pdev);
> +
> info->clk = devm_clk_get(&pdev->dev, "adc");
> if (IS_ERR(info->clk)) {
> dev_err(&pdev->dev, "failed getting clock, err = %ld\n",
> @@ -308,6 +327,17 @@ static int exynos_adc_probe(struct platform_device *pdev)
> goto err_irq;
> }
>
> + if (info->version == ADC_V3) {
> + info->sclk = devm_clk_get(&pdev->dev, "sclk_tsadc");
> + if (IS_ERR(info->sclk)) {
> + dev_warn(&pdev->dev,
> + "failed getting sclk clock, err = %ld\n",
> + PTR_ERR(info->sclk));
> + ret = PTR_ERR(info->sclk);
nit: you could move this line above dev_warn and use 'ret' in the print
statement.
--
With warm regards,
Sachin
Hi Sachin,
On 04/16/2014 12:48 PM, Sachin Kamat wrote:
> Hi Chanwoo,
>
> On 14 April 2014 14:37, Chanwoo Choi <[email protected]> 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_tsadc' clock as following:
>> - 'sclk_tsadc' clock: special clock for ADC which provide clock to internal ADC
>>
>> Exynos 4210/4212/4412 and Exynos5250/5420 has not included 'sclk_tsadc' clock
>> in FSYS_BLK. But, Exynos3250 based on Cortex-A7 has only included 'sclk_tsadc'
>> clock in FSYS_BLK.
>>
>> Cc: Jonathan Cameron <[email protected]>
>> Cc: Kukjin Kim <[email protected]>
>> Cc: Naveen Krishna Chatradhi
>> Cc: [email protected]
>> Signed-off-by: Chanwoo Choi <[email protected]>
>> Acked-by: Kyungmin Park <[email protected]>
>> ---
>> drivers/iio/adc/exynos_adc.c | 54 +++++++++++++++++++++++++++++++++-----------
>> 1 file changed, 41 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
>> index d25b262..3c99243 100644
>> --- a/drivers/iio/adc/exynos_adc.c
>> +++ b/drivers/iio/adc/exynos_adc.c
>> @@ -40,8 +40,9 @@
>> #include <linux/iio/driver.h>
>>
>> enum adc_version {
>> - ADC_V1,
>> - ADC_V2
>> + ADC_V1 = 0x1,
>> + ADC_V2 = 0x2,
>> + ADC_V3 = (ADC_V1 | ADC_V2),
>
> Can't this be simply 0x3? Or is this not really a h/w version?
Even thought ADC_V3 isn't h/w revision, ADC_V3 include all featues of ADC_V2
and only one difference of clock(sclk_tsadc) from ADC_V2.
I want to describethat ADC_V3 include ADC_V2 feature So, I add as following:
>> + ADC_V3 = (ADC_V1 | ADC_V2),
>
>> };
>>
>> /* EXYNOS4412/5250 ADC_V1 registers definitions */
>> @@ -88,6 +89,7 @@ struct exynos_adc {
>> void __iomem *regs;
>> void __iomem *enable_reg;
>> struct clk *clk;
>> + struct clk *sclk;
>> unsigned int irq;
>> struct regulator *vdd;
>>
>> @@ -100,6 +102,7 @@ struct exynos_adc {
>> 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-v3", .data = (void *)ADC_V3 },
>> {},
>> };
>> MODULE_DEVICE_TABLE(of, exynos_adc_match);
>> @@ -128,7 +131,7 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
>> mutex_lock(&indio_dev->mlock);
>>
>> /* Select the channel to be used and Trigger conversion */
>> - if (info->version == ADC_V2) {
>> + if (info->version & ADC_V2) {
>
> So, now this would be applicable for ADC_V3 too, right?
>
>
>> con2 = readl(ADC_V2_CON2(info->regs));
>> con2 &= ~ADC_V2_CON2_ACH_MASK;
>> con2 |= ADC_V2_CON2_ACH_SEL(chan->address);
>> @@ -165,7 +168,7 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
>> info->value = readl(ADC_V1_DATX(info->regs)) &
>> ADC_DATX_MASK;
>> /* clear irq */
>> - if (info->version == ADC_V2)
>> + if (info->version & ADC_V2)
>> writel(1, ADC_V2_INT_ST(info->regs));
>> else
>> writel(1, ADC_V1_INTCLR(info->regs));
>> @@ -226,11 +229,25 @@ static int exynos_adc_remove_devices(struct device *dev, void *c)
>> return 0;
>> }
>>
>> +static void exynos_adc_enable_clock(struct exynos_adc *info, bool enable)
>> +{
>> + if (enable) {
>> + clk_prepare_enable(info->clk);
>
> This could fail. Is it OK without any checks?
OK, I'll check return value.
>
>> + if (info->version == ADC_V3)
>> + clk_prepare_enable(info->sclk);
>
> ditto.
ditto.
>
>> +
>> + } else {
>> + if (info->version == ADC_V3)
>> + clk_disable_unprepare(info->sclk);
>> + clk_disable_unprepare(info->clk);
>> + }
>> +}
>> +
>> static void exynos_adc_hw_init(struct exynos_adc *info)
>> {
>> u32 con1, con2;
>>
>> - if (info->version == ADC_V2) {
>> + if (info->version & ADC_V2) {
>> con1 = ADC_V2_CON1_SOFT_RESET;
>> writel(con1, ADC_V2_CON1(info->regs));
>>
>> @@ -300,6 +317,8 @@ static int exynos_adc_probe(struct platform_device *pdev)
>>
>> writel(1, info->enable_reg);
>>
>> + info->version = exynos_adc_get_version(pdev);
>> +
>> info->clk = devm_clk_get(&pdev->dev, "adc");
>> if (IS_ERR(info->clk)) {
>> dev_err(&pdev->dev, "failed getting clock, err = %ld\n",
>> @@ -308,6 +327,17 @@ static int exynos_adc_probe(struct platform_device *pdev)
>> goto err_irq;
>> }
>>
>> + if (info->version == ADC_V3) {
>> + info->sclk = devm_clk_get(&pdev->dev, "sclk_tsadc");
>> + if (IS_ERR(info->sclk)) {
>> + dev_warn(&pdev->dev,
>> + "failed getting sclk clock, err = %ld\n",
>> + PTR_ERR(info->sclk));
>> + ret = PTR_ERR(info->sclk);
>
> nit: you could move this line above dev_warn and use 'ret' in the print
> statement.
As I knew, usually show log meesage and then initialize return value.
But If you find this code ugly, I can fix it.
Thanks for your review.
Best Regards,
Chanwoo Choi
Hi Sachin,
On 04/16/2014 01:44 PM, Chanwoo Choi wrote:
> Hi Sachin,
>
> On 04/16/2014 12:48 PM, Sachin Kamat wrote:
>> Hi Chanwoo,
>>
>> On 14 April 2014 14:37, Chanwoo Choi <[email protected]> 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_tsadc' clock as following:
>>> - 'sclk_tsadc' clock: special clock for ADC which provide clock to internal ADC
>>>
>>> Exynos 4210/4212/4412 and Exynos5250/5420 has not included 'sclk_tsadc' clock
>>> in FSYS_BLK. But, Exynos3250 based on Cortex-A7 has only included 'sclk_tsadc'
>>> clock in FSYS_BLK.
>>>
>>> Cc: Jonathan Cameron <[email protected]>
>>> Cc: Kukjin Kim <[email protected]>
>>> Cc: Naveen Krishna Chatradhi
>>> Cc: [email protected]
>>> Signed-off-by: Chanwoo Choi <[email protected]>
>>> Acked-by: Kyungmin Park <[email protected]>
>>> ---
>>> drivers/iio/adc/exynos_adc.c | 54 +++++++++++++++++++++++++++++++++-----------
>>> 1 file changed, 41 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
>>> index d25b262..3c99243 100644
>>> --- a/drivers/iio/adc/exynos_adc.c
>>> +++ b/drivers/iio/adc/exynos_adc.c
>>> @@ -40,8 +40,9 @@
>>> #include <linux/iio/driver.h>
>>>
>>> enum adc_version {
>>> - ADC_V1,
>>> - ADC_V2
>>> + ADC_V1 = 0x1,
>>> + ADC_V2 = 0x2,
>>> + ADC_V3 = (ADC_V1 | ADC_V2),
>>
>> Can't this be simply 0x3? Or is this not really a h/w version?
>
> Even thought ADC_V3 isn't h/w revision, ADC_V3 include all featues of ADC_V2
> and only one difference of clock(sclk_tsadc) from ADC_V2.
> I want to describethat ADC_V3 include ADC_V2 feature So, I add as following:
> >> + ADC_V3 = (ADC_V1 | ADC_V2),
>
>>
>>> };
>>>
>>> /* EXYNOS4412/5250 ADC_V1 registers definitions */
>>> @@ -88,6 +89,7 @@ struct exynos_adc {
>>> void __iomem *regs;
>>> void __iomem *enable_reg;
>>> struct clk *clk;
>>> + struct clk *sclk;
>>> unsigned int irq;
>>> struct regulator *vdd;
>>>
>>> @@ -100,6 +102,7 @@ struct exynos_adc {
>>> 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-v3", .data = (void *)ADC_V3 },
>>> {},
>>> };
>>> MODULE_DEVICE_TABLE(of, exynos_adc_match);
>>> @@ -128,7 +131,7 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
>>> mutex_lock(&indio_dev->mlock);
>>>
>>> /* Select the channel to be used and Trigger conversion */
>>> - if (info->version == ADC_V2) {
>>> + if (info->version & ADC_V2) {
>>
>> So, now this would be applicable for ADC_V3 too, right?
ADC_V3 isn't h/w version. So, I think this code is proper instead of using ADC_V3 direclty.
I want to use ADC_V3 version on checking clock(sclk_tsadc).
>>
>>
>>> con2 = readl(ADC_V2_CON2(info->regs));
>>> con2 &= ~ADC_V2_CON2_ACH_MASK;
>>> con2 |= ADC_V2_CON2_ACH_SEL(chan->address);
>>> @@ -165,7 +168,7 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
>>> info->value = readl(ADC_V1_DATX(info->regs)) &
>>> ADC_DATX_MASK;
>>> /* clear irq */
>>> - if (info->version == ADC_V2)
>>> + if (info->version & ADC_V2)
>>> writel(1, ADC_V2_INT_ST(info->regs));
>>> else
>>> writel(1, ADC_V1_INTCLR(info->regs));
>>> @@ -226,11 +229,25 @@ static int exynos_adc_remove_devices(struct device *dev, void *c)
>>> return 0;
>>> }
>>>
>>> +static void exynos_adc_enable_clock(struct exynos_adc *info, bool enable)
>>> +{
>>> + if (enable) {
>>> + clk_prepare_enable(info->clk);
>>
>> This could fail. Is it OK without any checks?
>
> OK, I'll check return value.
Do you want to check return value always?
I think again, Some device drivers in mainline would not check
return value of clock function. If maintainer confirm this modification,
I'll fix it as your comment.
Best Regards,
Chanwoo Choi
Hi Chanwoo,
On 16 April 2014 10:25, Chanwoo Choi <[email protected]> wrote:
> Hi Sachin,
>
> On 04/16/2014 01:44 PM, Chanwoo Choi wrote:
>> Hi Sachin,
>>
>> On 04/16/2014 12:48 PM, Sachin Kamat wrote:
>>> Hi Chanwoo,
>>>
>>> On 14 April 2014 14:37, Chanwoo Choi <[email protected]> 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_tsadc' clock as following:
>>>> - 'sclk_tsadc' clock: special clock for ADC which provide clock to internal ADC
>>>>
>>>> Exynos 4210/4212/4412 and Exynos5250/5420 has not included 'sclk_tsadc' clock
>>>> in FSYS_BLK. But, Exynos3250 based on Cortex-A7 has only included 'sclk_tsadc'
>>>> clock in FSYS_BLK.
>>>>
>>>> Cc: Jonathan Cameron <[email protected]>
>>>> Cc: Kukjin Kim <[email protected]>
>>>> Cc: Naveen Krishna Chatradhi
>>>> Cc: [email protected]
>>>> Signed-off-by: Chanwoo Choi <[email protected]>
>>>> Acked-by: Kyungmin Park <[email protected]>
>>>> ---
>>>> drivers/iio/adc/exynos_adc.c | 54 +++++++++++++++++++++++++++++++++-----------
>>>> 1 file changed, 41 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
>>>> index d25b262..3c99243 100644
>>>> --- a/drivers/iio/adc/exynos_adc.c
>>>> +++ b/drivers/iio/adc/exynos_adc.c
>>>> @@ -40,8 +40,9 @@
>>>> #include <linux/iio/driver.h>
>>>>
>>>> enum adc_version {
>>>> - ADC_V1,
>>>> - ADC_V2
>>>> + ADC_V1 = 0x1,
>>>> + ADC_V2 = 0x2,
>>>> + ADC_V3 = (ADC_V1 | ADC_V2),
>>>
>>> Can't this be simply 0x3? Or is this not really a h/w version?
>>
>> Even thought ADC_V3 isn't h/w revision, ADC_V3 include all featues of ADC_V2
>> and only one difference of clock(sclk_tsadc) from ADC_V2.
>> I want to describethat ADC_V3 include ADC_V2 feature So, I add as following:
>> >> + ADC_V3 = (ADC_V1 | ADC_V2),
>>
>>>
>>>> };
>>>>
>>>> /* EXYNOS4412/5250 ADC_V1 registers definitions */
>>>> @@ -88,6 +89,7 @@ struct exynos_adc {
>>>> void __iomem *regs;
>>>> void __iomem *enable_reg;
>>>> struct clk *clk;
>>>> + struct clk *sclk;
>>>> unsigned int irq;
>>>> struct regulator *vdd;
>>>>
>>>> @@ -100,6 +102,7 @@ struct exynos_adc {
>>>> 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-v3", .data = (void *)ADC_V3 },
>>>> {},
>>>> };
>>>> MODULE_DEVICE_TABLE(of, exynos_adc_match);
>>>> @@ -128,7 +131,7 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
>>>> mutex_lock(&indio_dev->mlock);
>>>>
>>>> /* Select the channel to be used and Trigger conversion */
>>>> - if (info->version == ADC_V2) {
>>>> + if (info->version & ADC_V2) {
>>>
>>> So, now this would be applicable for ADC_V3 too, right?
>
> ADC_V3 isn't h/w version. So, I think this code is proper instead of using ADC_V3 direclty.
> I want to use ADC_V3 version on checking clock(sclk_tsadc).
OK. Just a readability concern. Probably a check something like
(version >= ADC_V2) would
have made it more explicit.
>
>>>
>>>
>>>> con2 = readl(ADC_V2_CON2(info->regs));
>>>> con2 &= ~ADC_V2_CON2_ACH_MASK;
>>>> con2 |= ADC_V2_CON2_ACH_SEL(chan->address);
>>>> @@ -165,7 +168,7 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
>>>> info->value = readl(ADC_V1_DATX(info->regs)) &
>>>> ADC_DATX_MASK;
>>>> /* clear irq */
>>>> - if (info->version == ADC_V2)
>>>> + if (info->version & ADC_V2)
>>>> writel(1, ADC_V2_INT_ST(info->regs));
>>>> else
>>>> writel(1, ADC_V1_INTCLR(info->regs));
>>>> @@ -226,11 +229,25 @@ static int exynos_adc_remove_devices(struct device *dev, void *c)
>>>> return 0;
>>>> }
>>>>
>>>> +static void exynos_adc_enable_clock(struct exynos_adc *info, bool enable)
>>>> +{
>>>> + if (enable) {
>>>> + clk_prepare_enable(info->clk);
>>>
>>> This could fail. Is it OK without any checks?
>>
>> OK, I'll check return value.
>
> Do you want to check return value always?
It is a good practice to check the return values for errors. Having
said that it depends on
your s/w design and the h/w requirements. If proceeding with the error
does not cause any
functional issues, then it is OK to ignore them. However I would
atleast prefer to print
a warning/info about such failures.
--
With warm regards,
Sachin
Hi Sachin,
On 04/16/2014 02:04 PM, Sachin Kamat wrote:
> Hi Chanwoo,
>
> On 16 April 2014 10:25, Chanwoo Choi <[email protected]> wrote:
>> Hi Sachin,
>>
>> On 04/16/2014 01:44 PM, Chanwoo Choi wrote:
>>> Hi Sachin,
>>>
>>> On 04/16/2014 12:48 PM, Sachin Kamat wrote:
>>>> Hi Chanwoo,
>>>>
>>>> On 14 April 2014 14:37, Chanwoo Choi <[email protected]> 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_tsadc' clock as following:
>>>>> - 'sclk_tsadc' clock: special clock for ADC which provide clock to internal ADC
>>>>>
>>>>> Exynos 4210/4212/4412 and Exynos5250/5420 has not included 'sclk_tsadc' clock
>>>>> in FSYS_BLK. But, Exynos3250 based on Cortex-A7 has only included 'sclk_tsadc'
>>>>> clock in FSYS_BLK.
>>>>>
>>>>> Cc: Jonathan Cameron <[email protected]>
>>>>> Cc: Kukjin Kim <[email protected]>
>>>>> Cc: Naveen Krishna Chatradhi
>>>>> Cc: [email protected]
>>>>> Signed-off-by: Chanwoo Choi <[email protected]>
>>>>> Acked-by: Kyungmin Park <[email protected]>
>>>>> ---
>>>>> drivers/iio/adc/exynos_adc.c | 54 +++++++++++++++++++++++++++++++++-----------
>>>>> 1 file changed, 41 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
>>>>> index d25b262..3c99243 100644
>>>>> --- a/drivers/iio/adc/exynos_adc.c
>>>>> +++ b/drivers/iio/adc/exynos_adc.c
>>>>> @@ -40,8 +40,9 @@
>>>>> #include <linux/iio/driver.h>
>>>>>
>>>>> enum adc_version {
>>>>> - ADC_V1,
>>>>> - ADC_V2
>>>>> + ADC_V1 = 0x1,
>>>>> + ADC_V2 = 0x2,
>>>>> + ADC_V3 = (ADC_V1 | ADC_V2),
>>>>
>>>> Can't this be simply 0x3? Or is this not really a h/w version?
>>>
>>> Even thought ADC_V3 isn't h/w revision, ADC_V3 include all featues of ADC_V2
>>> and only one difference of clock(sclk_tsadc) from ADC_V2.
>>> I want to describethat ADC_V3 include ADC_V2 feature So, I add as following:
>>> >> + ADC_V3 = (ADC_V1 | ADC_V2),
>>>
>>>>
>>>>> };
>>>>>
>>>>> /* EXYNOS4412/5250 ADC_V1 registers definitions */
>>>>> @@ -88,6 +89,7 @@ struct exynos_adc {
>>>>> void __iomem *regs;
>>>>> void __iomem *enable_reg;
>>>>> struct clk *clk;
>>>>> + struct clk *sclk;
>>>>> unsigned int irq;
>>>>> struct regulator *vdd;
>>>>>
>>>>> @@ -100,6 +102,7 @@ struct exynos_adc {
>>>>> 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-v3", .data = (void *)ADC_V3 },
>>>>> {},
>>>>> };
>>>>> MODULE_DEVICE_TABLE(of, exynos_adc_match);
>>>>> @@ -128,7 +131,7 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
>>>>> mutex_lock(&indio_dev->mlock);
>>>>>
>>>>> /* Select the channel to be used and Trigger conversion */
>>>>> - if (info->version == ADC_V2) {
>>>>> + if (info->version & ADC_V2) {
>>>>
>>>> So, now this would be applicable for ADC_V3 too, right?
>>
>> ADC_V3 isn't h/w version. So, I think this code is proper instead of using ADC_V3 direclty.
>> I want to use ADC_V3 version on checking clock(sclk_tsadc).
>
> OK. Just a readability concern. Probably a check something like
> (version >= ADC_V2) would
> have made it more explicit.
OK I'll modify it as your comment.
(version >= ADC_V2)
>
>>
>>>>
>>>>
>>>>> con2 = readl(ADC_V2_CON2(info->regs));
>>>>> con2 &= ~ADC_V2_CON2_ACH_MASK;
>>>>> con2 |= ADC_V2_CON2_ACH_SEL(chan->address);
>>>>> @@ -165,7 +168,7 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
>>>>> info->value = readl(ADC_V1_DATX(info->regs)) &
>>>>> ADC_DATX_MASK;
>>>>> /* clear irq */
>>>>> - if (info->version == ADC_V2)
>>>>> + if (info->version & ADC_V2)
>>>>> writel(1, ADC_V2_INT_ST(info->regs));
>>>>> else
>>>>> writel(1, ADC_V1_INTCLR(info->regs));
>>>>> @@ -226,11 +229,25 @@ static int exynos_adc_remove_devices(struct device *dev, void *c)
>>>>> return 0;
>>>>> }
>>>>>
>>>>> +static void exynos_adc_enable_clock(struct exynos_adc *info, bool enable)
>>>>> +{
>>>>> + if (enable) {
>>>>> + clk_prepare_enable(info->clk);
>>>>
>>>> This could fail. Is it OK without any checks?
>>>
>>> OK, I'll check return value.
>>
>> Do you want to check return value always?
>
> It is a good practice to check the return values for errors. Having
> said that it depends on
> your s/w design and the h/w requirements. If proceeding with the error
> does not cause any
> functional issues, then it is OK to ignore them. However I would
> atleast prefer to print
> a warning/info about such failures.
OK, I'll fix it.
Thanks,
Chanwoo Choi
On April 16, 2014 2:13:39 AM GMT+01:00, Chanwoo Choi <[email protected]> wrote:
>Hi Jonathan,
>
>Any comment of this patchset?
>
>Best Regards,
>Chanwoo Choi
Hi Chanwoo
Not got to it yet I'm afraid. May be sometime next week before I do.
Jonathan
>
>On 04/14/2014 06:07 PM, 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_tsadc' clock as following:
>> - 'sclk_tsadc' clock: special clock for ADC which provide clock to
>internal ADC
>>
>> Exynos 4210/4212/4412 and Exynos5250/5420 has not included
>'sclk_tsadc' clock
>> in FSYS_BLK. But, Exynos3250 based on Cortex-A7 has only included
>'sclk_tsadc'
>> clock in FSYS_BLK.
>>
>> Cc: Jonathan Cameron <[email protected]>
>> Cc: Kukjin Kim <[email protected]>
>> Cc: Naveen Krishna Chatradhi
>> Cc: [email protected]
>> Signed-off-by: Chanwoo Choi <[email protected]>
>> Acked-by: Kyungmin Park <[email protected]>
>> ---
>> drivers/iio/adc/exynos_adc.c | 54
>+++++++++++++++++++++++++++++++++-----------
>> 1 file changed, 41 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/iio/adc/exynos_adc.c
>b/drivers/iio/adc/exynos_adc.c
>> index d25b262..3c99243 100644
>> --- a/drivers/iio/adc/exynos_adc.c
>> +++ b/drivers/iio/adc/exynos_adc.c
>> @@ -40,8 +40,9 @@
>> #include <linux/iio/driver.h>
>>
>> enum adc_version {
>> - ADC_V1,
>> - ADC_V2
>> + ADC_V1 = 0x1,
>> + ADC_V2 = 0x2,
>> + ADC_V3 = (ADC_V1 | ADC_V2),
>> };
>>
>> /* EXYNOS4412/5250 ADC_V1 registers definitions */
>> @@ -88,6 +89,7 @@ struct exynos_adc {
>> void __iomem *regs;
>> void __iomem *enable_reg;
>> struct clk *clk;
>> + struct clk *sclk;
>> unsigned int irq;
>> struct regulator *vdd;
>>
>> @@ -100,6 +102,7 @@ struct exynos_adc {
>> 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-v3", .data = (void *)ADC_V3 },
>> {},
>> };
>> MODULE_DEVICE_TABLE(of, exynos_adc_match);
>> @@ -128,7 +131,7 @@ static int exynos_read_raw(struct iio_dev
>*indio_dev,
>> mutex_lock(&indio_dev->mlock);
>>
>> /* Select the channel to be used and Trigger conversion */
>> - if (info->version == ADC_V2) {
>> + 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);
>> @@ -165,7 +168,7 @@ static irqreturn_t exynos_adc_isr(int irq, void
>*dev_id)
>> info->value = readl(ADC_V1_DATX(info->regs)) &
>> ADC_DATX_MASK;
>> /* clear irq */
>> - if (info->version == ADC_V2)
>> + if (info->version & ADC_V2)
>> writel(1, ADC_V2_INT_ST(info->regs));
>> else
>> writel(1, ADC_V1_INTCLR(info->regs));
>> @@ -226,11 +229,25 @@ static int exynos_adc_remove_devices(struct
>device *dev, void *c)
>> return 0;
>> }
>>
>> +static void exynos_adc_enable_clock(struct exynos_adc *info, bool
>enable)
>> +{
>> + if (enable) {
>> + clk_prepare_enable(info->clk);
>> + if (info->version == ADC_V3)
>> + clk_prepare_enable(info->sclk);
>> +
>> + } else {
>> + if (info->version == ADC_V3)
>> + clk_disable_unprepare(info->sclk);
>> + clk_disable_unprepare(info->clk);
>> + }
>> +}
>> +
>> static void exynos_adc_hw_init(struct exynos_adc *info)
>> {
>> u32 con1, con2;
>>
>> - if (info->version == ADC_V2) {
>> + if (info->version & ADC_V2) {
>> con1 = ADC_V2_CON1_SOFT_RESET;
>> writel(con1, ADC_V2_CON1(info->regs));
>>
>> @@ -300,6 +317,8 @@ static int exynos_adc_probe(struct
>platform_device *pdev)
>>
>> writel(1, info->enable_reg);
>>
>> + info->version = exynos_adc_get_version(pdev);
>> +
>> info->clk = devm_clk_get(&pdev->dev, "adc");
>> if (IS_ERR(info->clk)) {
>> dev_err(&pdev->dev, "failed getting clock, err = %ld\n",
>> @@ -308,6 +327,17 @@ static int exynos_adc_probe(struct
>platform_device *pdev)
>> goto err_irq;
>> }
>>
>> + if (info->version == ADC_V3) {
>> + info->sclk = devm_clk_get(&pdev->dev, "sclk_tsadc");
>> + if (IS_ERR(info->sclk)) {
>> + dev_warn(&pdev->dev,
>> + "failed getting sclk clock, err = %ld\n",
>> + PTR_ERR(info->sclk));
>> + ret = PTR_ERR(info->sclk);
>> + goto err_irq;
>> + }
>> + }
>> +
>> info->vdd = devm_regulator_get(&pdev->dev, "vdd");
>> if (IS_ERR(info->vdd)) {
>> dev_err(&pdev->dev, "failed getting regulator, err = %ld\n",
>> @@ -316,8 +346,6 @@ static int exynos_adc_probe(struct
>platform_device *pdev)
>> goto err_irq;
>> }
>>
>> - info->version = exynos_adc_get_version(pdev);
>> -
>> platform_set_drvdata(pdev, indio_dev);
>>
>> indio_dev->name = dev_name(&pdev->dev);
>> @@ -340,7 +368,7 @@ static int exynos_adc_probe(struct
>platform_device *pdev)
>> if (ret)
>> goto err_iio_dev;
>>
>> - clk_prepare_enable(info->clk);
>> + exynos_adc_enable_clock(info, true);
>>
>> exynos_adc_hw_init(info);
>>
>> @@ -356,7 +384,7 @@ err_of_populate:
>> device_for_each_child(&pdev->dev, NULL,
>> exynos_adc_remove_devices);
>> regulator_disable(info->vdd);
>> - clk_disable_unprepare(info->clk);
>> + exynos_adc_enable_clock(info, false);
>> err_iio_dev:
>> iio_device_unregister(indio_dev);
>> err_irq:
>> @@ -372,7 +400,7 @@ static int exynos_adc_remove(struct
>platform_device *pdev)
>> device_for_each_child(&pdev->dev, NULL,
>> exynos_adc_remove_devices);
>> regulator_disable(info->vdd);
>> - clk_disable_unprepare(info->clk);
>> + exynos_adc_enable_clock(info, false);
>> writel(0, info->enable_reg);
>> iio_device_unregister(indio_dev);
>> free_irq(info->irq, info);
>> @@ -387,7 +415,7 @@ static int exynos_adc_suspend(struct device *dev)
>> struct exynos_adc *info = iio_priv(indio_dev);
>> u32 con;
>>
>> - if (info->version == ADC_V2) {
>> + if (info->version & ADC_V2) {
>> con = readl(ADC_V2_CON1(info->regs));
>> con &= ~ADC_CON_EN_START;
>> writel(con, ADC_V2_CON1(info->regs));
>> @@ -397,7 +425,7 @@ static int exynos_adc_suspend(struct device *dev)
>> writel(con, ADC_V1_CON(info->regs));
>> }
>>
>> - clk_disable_unprepare(info->clk);
>> + exynos_adc_enable_clock(info, false);
>> writel(0, info->enable_reg);
>> regulator_disable(info->vdd);
>>
>> @@ -415,7 +443,7 @@ static int exynos_adc_resume(struct device *dev)
>> return ret;
>>
>> writel(1, info->enable_reg);
>> - clk_prepare_enable(info->clk);
>> + exynos_adc_enable_clock(info, true);
>>
>> exynos_adc_hw_init(info);
>>
>>
--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
On April 16, 2014 5:55:17 AM GMT+01:00, Chanwoo Choi <[email protected]> wrote:
>Hi Sachin,
>
>On 04/16/2014 01:44 PM, Chanwoo Choi wrote:
>> Hi Sachin,
>>
>> On 04/16/2014 12:48 PM, Sachin Kamat wrote:
>>> Hi Chanwoo,
>>>
>>> On 14 April 2014 14:37, Chanwoo Choi <[email protected]> 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_tsadc' clock as following:
>>>> - 'sclk_tsadc' clock: special clock for ADC which provide clock to
>internal ADC
>>>>
>>>> Exynos 4210/4212/4412 and Exynos5250/5420 has not included
>'sclk_tsadc' clock
>>>> in FSYS_BLK. But, Exynos3250 based on Cortex-A7 has only included
>'sclk_tsadc'
>>>> clock in FSYS_BLK.
>>>>
>>>> Cc: Jonathan Cameron <[email protected]>
>>>> Cc: Kukjin Kim <[email protected]>
>>>> Cc: Naveen Krishna Chatradhi
>>>> Cc: [email protected]
>>>> Signed-off-by: Chanwoo Choi <[email protected]>
>>>> Acked-by: Kyungmin Park <[email protected]>
>>>> ---
>>>> drivers/iio/adc/exynos_adc.c | 54
>+++++++++++++++++++++++++++++++++-----------
>>>> 1 file changed, 41 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/adc/exynos_adc.c
>b/drivers/iio/adc/exynos_adc.c
>>>> index d25b262..3c99243 100644
>>>> --- a/drivers/iio/adc/exynos_adc.c
>>>> +++ b/drivers/iio/adc/exynos_adc.c
>>>> @@ -40,8 +40,9 @@
>>>> #include <linux/iio/driver.h>
>>>>
>>>> enum adc_version {
>>>> - ADC_V1,
>>>> - ADC_V2
>>>> + ADC_V1 = 0x1,
>>>> + ADC_V2 = 0x2,
>>>> + ADC_V3 = (ADC_V1 | ADC_V2),
>>>
>>> Can't this be simply 0x3? Or is this not really a h/w version?
>>
>> Even thought ADC_V3 isn't h/w revision, ADC_V3 include all featues of
>ADC_V2
>> and only one difference of clock(sclk_tsadc) from ADC_V2.
>> I want to describethat ADC_V3 include ADC_V2 feature So, I add as
>following:
>> >> + ADC_V3 = (ADC_V1 | ADC_V2),
>>
>>>
>>>> };
>>>>
>>>> /* EXYNOS4412/5250 ADC_V1 registers definitions */
>>>> @@ -88,6 +89,7 @@ struct exynos_adc {
>>>> void __iomem *regs;
>>>> void __iomem *enable_reg;
>>>> struct clk *clk;
>>>> + struct clk *sclk;
>>>> unsigned int irq;
>>>> struct regulator *vdd;
>>>>
>>>> @@ -100,6 +102,7 @@ struct exynos_adc {
>>>> 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-v3", .data = (void
>*)ADC_V3 },
>>>> {},
>>>> };
>>>> MODULE_DEVICE_TABLE(of, exynos_adc_match);
>>>> @@ -128,7 +131,7 @@ static int exynos_read_raw(struct iio_dev
>*indio_dev,
>>>> mutex_lock(&indio_dev->mlock);
>>>>
>>>> /* Select the channel to be used and Trigger conversion */
>>>> - if (info->version == ADC_V2) {
>>>> + if (info->version & ADC_V2) {
>>>
>>> So, now this would be applicable for ADC_V3 too, right?
>
>ADC_V3 isn't h/w version. So, I think this code is proper instead of
>using ADC_V3 direclty.
>I want to use ADC_V3 version on checking clock(sclk_tsadc).
>
>>>
>>>
>>>> con2 = readl(ADC_V2_CON2(info->regs));
>>>> con2 &= ~ADC_V2_CON2_ACH_MASK;
>>>> con2 |= ADC_V2_CON2_ACH_SEL(chan->address);
>>>> @@ -165,7 +168,7 @@ static irqreturn_t exynos_adc_isr(int irq, void
>*dev_id)
>>>> info->value = readl(ADC_V1_DATX(info->regs)) &
>>>> ADC_DATX_MASK;
>>>> /* clear irq */
>>>> - if (info->version == ADC_V2)
>>>> + if (info->version & ADC_V2)
>>>> writel(1, ADC_V2_INT_ST(info->regs));
>>>> else
>>>> writel(1, ADC_V1_INTCLR(info->regs));
>>>> @@ -226,11 +229,25 @@ static int exynos_adc_remove_devices(struct
>device *dev, void *c)
>>>> return 0;
>>>> }
>>>>
>>>> +static void exynos_adc_enable_clock(struct exynos_adc *info, bool
>enable)
>>>> +{
>>>> + if (enable) {
>>>> + clk_prepare_enable(info->clk);
>>>
>>> This could fail. Is it OK without any checks?
>>
>> OK, I'll check return value.
>
>Do you want to check return value always?
>I think again, Some device drivers in mainline would not check
>return value of clock function. If maintainer confirm this
>modification,
>I'll fix it as your comment.
Its general good practice to check all return values. Even if a function doesn't return an
error now, it might in future. There is lots of old or lazy code out there doing many much
stranger things than this!
So yes, please check return values and pass on up the call stack if an error.
>
>Best Regards,
>Chanwoo Choi
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to [email protected]
>More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
Hi Jonathan,
On 04/16/2014 04:05 PM, Jonathan Cameron wrote:
>
>
> On April 16, 2014 5:55:17 AM GMT+01:00, Chanwoo Choi <[email protected]> wrote:
>> Hi Sachin,
>>
>> On 04/16/2014 01:44 PM, Chanwoo Choi wrote:
>>> Hi Sachin,
>>>
>>> On 04/16/2014 12:48 PM, Sachin Kamat wrote:
>>>> Hi Chanwoo,
>>>>
>>>> On 14 April 2014 14:37, Chanwoo Choi <[email protected]> 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_tsadc' clock as following:
>>>>> - 'sclk_tsadc' clock: special clock for ADC which provide clock to
>> internal ADC
>>>>>
>>>>> Exynos 4210/4212/4412 and Exynos5250/5420 has not included
>> 'sclk_tsadc' clock
>>>>> in FSYS_BLK. But, Exynos3250 based on Cortex-A7 has only included
>> 'sclk_tsadc'
>>>>> clock in FSYS_BLK.
>>>>>
>>>>> Cc: Jonathan Cameron <[email protected]>
>>>>> Cc: Kukjin Kim <[email protected]>
>>>>> Cc: Naveen Krishna Chatradhi
>>>>> Cc: [email protected]
>>>>> Signed-off-by: Chanwoo Choi <[email protected]>
>>>>> Acked-by: Kyungmin Park <[email protected]>
>>>>> ---
>>>>> drivers/iio/adc/exynos_adc.c | 54
>> +++++++++++++++++++++++++++++++++-----------
>>>>> 1 file changed, 41 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iio/adc/exynos_adc.c
>> b/drivers/iio/adc/exynos_adc.c
>>>>> index d25b262..3c99243 100644
>>>>> --- a/drivers/iio/adc/exynos_adc.c
>>>>> +++ b/drivers/iio/adc/exynos_adc.c
>>>>> @@ -40,8 +40,9 @@
>>>>> #include <linux/iio/driver.h>
>>>>>
>>>>> enum adc_version {
>>>>> - ADC_V1,
>>>>> - ADC_V2
>>>>> + ADC_V1 = 0x1,
>>>>> + ADC_V2 = 0x2,
>>>>> + ADC_V3 = (ADC_V1 | ADC_V2),
>>>>
>>>> Can't this be simply 0x3? Or is this not really a h/w version?
>>>
>>> Even thought ADC_V3 isn't h/w revision, ADC_V3 include all featues of
>> ADC_V2
>>> and only one difference of clock(sclk_tsadc) from ADC_V2.
>>> I want to describethat ADC_V3 include ADC_V2 feature So, I add as
>> following:
>>> >> + ADC_V3 = (ADC_V1 | ADC_V2),
>>>
>>>>
>>>>> };
>>>>>
>>>>> /* EXYNOS4412/5250 ADC_V1 registers definitions */
>>>>> @@ -88,6 +89,7 @@ struct exynos_adc {
>>>>> void __iomem *regs;
>>>>> void __iomem *enable_reg;
>>>>> struct clk *clk;
>>>>> + struct clk *sclk;
>>>>> unsigned int irq;
>>>>> struct regulator *vdd;
>>>>>
>>>>> @@ -100,6 +102,7 @@ struct exynos_adc {
>>>>> 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-v3", .data = (void
>> *)ADC_V3 },
>>>>> {},
>>>>> };
>>>>> MODULE_DEVICE_TABLE(of, exynos_adc_match);
>>>>> @@ -128,7 +131,7 @@ static int exynos_read_raw(struct iio_dev
>> *indio_dev,
>>>>> mutex_lock(&indio_dev->mlock);
>>>>>
>>>>> /* Select the channel to be used and Trigger conversion */
>>>>> - if (info->version == ADC_V2) {
>>>>> + if (info->version & ADC_V2) {
>>>>
>>>> So, now this would be applicable for ADC_V3 too, right?
>>
>> ADC_V3 isn't h/w version. So, I think this code is proper instead of
>> using ADC_V3 direclty.
>> I want to use ADC_V3 version on checking clock(sclk_tsadc).
>>
>>>>
>>>>
>>>>> con2 = readl(ADC_V2_CON2(info->regs));
>>>>> con2 &= ~ADC_V2_CON2_ACH_MASK;
>>>>> con2 |= ADC_V2_CON2_ACH_SEL(chan->address);
>>>>> @@ -165,7 +168,7 @@ static irqreturn_t exynos_adc_isr(int irq, void
>> *dev_id)
>>>>> info->value = readl(ADC_V1_DATX(info->regs)) &
>>>>> ADC_DATX_MASK;
>>>>> /* clear irq */
>>>>> - if (info->version == ADC_V2)
>>>>> + if (info->version & ADC_V2)
>>>>> writel(1, ADC_V2_INT_ST(info->regs));
>>>>> else
>>>>> writel(1, ADC_V1_INTCLR(info->regs));
>>>>> @@ -226,11 +229,25 @@ static int exynos_adc_remove_devices(struct
>> device *dev, void *c)
>>>>> return 0;
>>>>> }
>>>>>
>>>>> +static void exynos_adc_enable_clock(struct exynos_adc *info, bool
>> enable)
>>>>> +{
>>>>> + if (enable) {
>>>>> + clk_prepare_enable(info->clk);
>>>>
>>>> This could fail. Is it OK without any checks?
>>>
>>> OK, I'll check return value.
>>
>> Do you want to check return value always?
>> I think again, Some device drivers in mainline would not check
>> return value of clock function. If maintainer confirm this
>> modification,
>> I'll fix it as your comment.
> Its general good practice to check all return values. Even if a function doesn't return an
> error now, it might in future. There is lots of old or lazy code out there doing many much
> stranger things than this!
>
> So yes, please check return values and pass on up the call stack if an error.
OK, I'll check return value of clock function. Thanks.
Best Regards,
Chanwoo Choi