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

[Naveen Krishna Ch]

Hello Tomasz,

On 20 June 2014 06:00, Tomasz Figa <tomasz.figa@xxxxxxxxx> 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@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.

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@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Shine bright,
(: Nav :)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/