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@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. 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 -- 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