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@xxxxxxxxxxx> >>>> Acked-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx> >>>> --- >>>> 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html