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

[Chanwoo Choi <cw00.choi@samsung.com>]

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