From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Message-id: <53A37EDA.2030403@samsung.com> Date: Fri, 20 Jun 2014 09:22:50 +0900 From: Chanwoo Choi MIME-version: 1.0 To: Tomasz Figa Cc: jic23@kernel.org, ch.naveen@samsung.com, t.figa@samsung.com, kgene.kim@samsung.com, robh+dt@kernel.org, pawel.moll@arm.com, mark.rutland@arm.com, ijc+devicetree@hellion.org.uk, galak@codeaurora.org, rdunlap@infradead.org, sachin.kamat@linaro.org, linux-iio@vger.kernel.org, linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-doc@vger.kernel.org Subject: Re: [PATCHv4 2/4] iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC References: <1403058061-24271-1-git-send-email-cw00.choi@samsung.com> <1403058061-24271-3-git-send-email-cw00.choi@samsung.com> <53A146A8.7050501@gmail.com> In-reply-to: <53A146A8.7050501@gmail.com> Content-type: text/plain; charset=ISO-8859-1 List-ID: 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 >> Acked-by: Kyungmin Park >> --- >> 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chanwoo Choi Subject: Re: [PATCHv4 2/4] iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC Date: Fri, 20 Jun 2014 09:22:50 +0900 Message-ID: <53A37EDA.2030403@samsung.com> References: <1403058061-24271-1-git-send-email-cw00.choi@samsung.com> <1403058061-24271-3-git-send-email-cw00.choi@samsung.com> <53A146A8.7050501@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: In-reply-to: <53A146A8.7050501-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> Sender: linux-iio-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Tomasz Figa Cc: jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, ch.naveen-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org, t.figa-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org, kgene.kim-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org, robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, pawel.moll-5wv7dgnIgG8@public.gmane.org, mark.rutland-5wv7dgnIgG8@public.gmane.org, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org, galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org, rdunlap-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org, sachin.kamat-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org, linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: linux-samsung-soc@vger.kernel.org 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 >> Acked-by: Kyungmin Park >> --- >> 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