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