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.
Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
