| Message ID | 1403058061-24271-3-git-send-email-cw00.choi@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- 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
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() Best Regards, Chanwoo Choi -- 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
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 -- 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
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. Best Regards, Chanwoo Choi -- 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
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 -- 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
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
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 -- 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
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);