[PATCHv4 0/4] iio: adc: exynos_adc: Support Exynos3250 ADC and code clean

[PATCHv4 0/4] iio: adc: exynos_adc: Support Exynos3250 ADC and code clean

[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

[PATCHv4 1/4] iio: adc: exynos_adc: Add exynos_adc_ops structure to improve readability

[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

Re: [PATCHv4 1/4] iio: adc: exynos_adc: Add exynos_adc_ops structure to improve readability

[Naveen Krishna Ch]

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 :)

Re: [PATCHv4 1/4] iio: adc: exynos_adc: Add exynos_adc_ops structure to improve readability

[Chanwoo Choi]

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

Re: [PATCHv4 1/4] iio: adc: exynos_adc: Add exynos_adc_ops structure to improve readability

[Tomasz Figa]

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

Re: [PATCHv4 1/4] iio: adc: exynos_adc: Add exynos_adc_ops structure to improve readability

[Chanwoo Choi]

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

Re: [PATCHv4 1/4] iio: adc: exynos_adc: Add exynos_adc_ops structure to improve readability

[Tomasz Figa]

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

[PATCHv4 2/4] iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC

[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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Acked-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 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

Re: [PATCHv4 2/4] iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC

[Tomasz Figa]

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

Re: [PATCHv4 2/4] iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC

[Chanwoo Choi]

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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> Acked-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> ---
>>  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

Re: [PATCHv4 2/4] iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC

[Tomasz Figa]

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

Re: [PATCHv4 2/4] iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC

[Chanwoo Choi]

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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>>>> Acked-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>>>> ---
>>>>  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

Re: [PATCHv4 2/4] iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC

[Tomasz Figa]

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

Re: [PATCHv4 2/4] iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC

[Naveen Krishna Ch]

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 :)

Re: [PATCHv4 2/4] iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC

[Chanwoo Choi]

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

[PATCHv4 3/4] iio: devicetree: Add DT binding documentation for Exynos3250 ADC

[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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Acked-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 .../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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv4 3/4] iio: devicetree: Add DT binding documentation for Exynos3250 ADC

[Naveen Krishna Ch]

Hello Chanwoo,

On 18 June 2014 07:51, Chanwoo Choi <cw00.choi-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> 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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Acked-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

Changes look good to me.
Reviewed-by: Naveen Krishna Chatradhi <ch.naveen-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

> ---
>  .../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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Shine bright,
(: Nav :)

Re: [PATCHv4 3/4] iio: devicetree: Add DT binding documentation for Exynos3250 ADC

[Chanwoo Choi]

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

Re: [PATCHv4 3/4] iio: devicetree: Add DT binding documentation for Exynos3250 ADC

[Tomasz Figa]

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

Re: [PATCHv4 3/4] iio: devicetree: Add DT binding documentation for Exynos3250 ADC

[Chanwoo Choi]

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

[PATCHv4 4/4] ARM: dts: Fix wrong compatible string for Exynos3250 ADC

[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

Re: [PATCHv4 4/4] ARM: dts: Fix wrong compatible string for Exynos3250 ADC

[Tomasz Figa]

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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Acked-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>  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

Re: [PATCHv4 4/4] ARM: dts: Fix wrong compatible string for Exynos3250 ADC

[Chanwoo Choi]

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