On 07/19/2014 03:48 AM, Arnd Bergmann wrote:

On Saturday 19 July 2014 02:02:09 Chanwoo Choi wrote:

quoted

On Sat, Jul 19, 2014 at 1:33 AM, Arnd Bergmann [off-list ref] wrote:

quoted

On Saturday 19 July 2014 01:23:15 Chanwoo Choi wrote:

quoted

If don't add new compatible including specific exynos version,
I would add new 'adc-needs-sclk' property with existing 'exynos-adc-v2'
compatible name.

What I actually meant is using compatible="exynos-adc-v2.1" or similar
rather than "exynos3250-adc". However, as you already explained, the
version numbers are apparently just made up, so using "exynos3250-adc"
is actually better here. If a future exynos7890 uses the same clocks
as exynos3250, it can simply use the same "exynos3250-adc" string here.

OK, I'll use "exynos3250-adc" compatible string instead of "exynos3250-adc-v2".

quoted

quoted

quoted

Dear Naveen, Tomasz,

If existing exynos-adc driver add just one property for 'sclk_adc'
as following, exynos-adc could not include the exynos version
in compatible name.

I need your opinion about it.

                adc: adc at 126C0000 {
                        compatible = "samsung,exynos-adc-v2";
                        reg = <0x126C0000 0x100>, <0x10020718 0x4>;
                        interrupts = <0 137 0>;
                        clock-names = "adc", "sclk_adc";
                        clocks = <&cmu CLK_TSADC>, <&cmu CLK_SCLK_TSADC>;
+                        adc-needs-sclk;
                        #io-channel-cells = <1>;
                        io-channel-ranges;
                }

How about just making it an optional clock? That would be much
easier because then you can simply see if the clock itself is
there and use it, or otherwise ignore it.

The v1 of this patchset[1] got the clock of 'sclk_adc'  but if the dt node
of ADC in dtsi file didn't include 'sclk_adc', print just warning message
without stopping probe as following:

 [1] https://lkml.org/lkml/2014/4/10/710

+       info->sclk = devm_clk_get(&pdev->dev, "sclk_adc");
+       if (IS_ERR(info->sclk)) {
+               dev_warn(&pdev->dev, "failed getting sclk clock, err = %ld\n",
+                                                       PTR_ERR(info->sclk));
+               info->sclk = NULL;
+       }

But, Tomasz Figa suggested the method[2] of this patchset(v6).
 [2] https://lkml.org/lkml/2014/4/11/189

Yes, your current version is certainly better than this, but another way
to address Tomasz' comment would be to change the binding to list the "sclk"
as optional for any device and make the code silently ignore missing sclk
entries, like:


	info->sclk = devm_clk_get(&pdev->dev, "sclk");
	if (IS_ERR(info->sclk)) {
		switch (PTR_ERR(info->sclk)) {
		case -EPROBE_DEFER:
			/* silently return error so we can retry */
			return -EPROBE_DEFER:
		case -ENOENT:
			/* silently ignore missing optional clk */
			info->sclk = NULL;
			break;
		default:
			/* any other error: clk is defined by doesn't work  */
			dev_err(&pdev->dev, "failed getting sclk clock, err = %ld\n",
				PTR_ERR(info->sclk));
			return PTR_ERR(info->sclk));
		}
	}

I tested this patch suggested by you.
But, devm_clk_get returns always '-ENOENT' on follwong two cases:

Case 1.
ADC dt node in exynos3250.dtsi don't include 'sclk' clock as following:

                adc: adc at 126C0000 {
                        compatible = "samsung,exynos3250-adc";
                        reg = <0x126C0000 0x100>, <0x10020718 0x4>;
                        interrupts = <0 137 0>;
                        clock-names = "adc";
                        clocks = <&cmu CLK_TSADC>;
                        #io-channel-cells = <1>;
                        io-channel-ranges;
		};

Case 2.
ADC dt node in exynos3250.dtsi don't include 'sclk' clock
but, exynos3250 clock controller don't support CLK_SCLK_TSADC clock as following:

                adc: adc at 126C0000 {
                        compatible = "samsung,exynos3250-adc";
                        reg = <0x126C0000 0x100>, <0x10020718 0x4>;
                        interrupts = <0 137 0>;
                        clock-names = "adc", "sclk";
                        clocks = <&cmu CLK_TSADC>, <&cmu CLK_SCLK_TSADC>;
                        #io-channel-cells = <1>;
                        io-channel-ranges;
		};


So, I think exynos-adc needs to use 'needs_sclk' field suggested by Tomasz Figa.

One more comment about the name: Both in the code you use "sclk" as the
name, so presumably that is the actual name of the clk as known to this
driver, and it makes sense to use clock-names="sclk" as well, if you want
to have any name.

OK, I'll use 'sclk' clock name for special clock of ADC.

Thanks,
Chanwoo Choi