by Chanwoo Choi

[permalink] [raw]

Subject: Re: [PATCHv4 2/4] iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC

Hi Tomasz,

On 06/20/2014 12:21 PM, Naveen Krishna Ch wrote:
> Hello Tomasz,
>
> On 20 June 2014 06:00, Tomasz Figa <[email protected]> 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 <[email protected]>
>>>>>>> Acked-by: Kyungmin Park <[email protected]>
>>>>>>> ---
>>>>>>> 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