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.c...@samsung.com>
>> Acked-by: Kyungmin Park <kyungmin.p...@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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Reply via email to