diff mbox

[PATCHv6,3/4] iio: devicetree: Add DT binding documentation for Exynos3250 ADC

Message ID 1405663186-26464-4-git-send-email-cw00.choi@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Chanwoo Choi July 18, 2014, 5:59 a.m. UTC
This patch add DT binding documentation for Exynos3250 ADC IP. Exynos3250 has
special clock ('sclk_adc') for ADC which provide clock to internal ADC.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
Reviewed-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
Reviewed-by: Tomasz Figa <t.figa@samsung.com>
Acked-by: Kukjin Kim <kgene.kim@samsung.com>
---
 .../devicetree/bindings/arm/samsung/exynos-adc.txt | 25 ++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

Comments

Arnd Bergmann July 18, 2014, 9:50 a.m. UTC | #1
On Friday 18 July 2014 14:59:45 Chanwoo Choi wrote:
>                         Must be "samsung,exynos-adc-v2" for
>                                 future controllers.

It would be good to change 'future controllers' to something else now.
Presumably that word was used before the actual products were announced,
but now they are publically known.

> +                       Must be "samsung,exynos3250-adc-v2" for
> +                               controllers compatible with ADC of Exynos3250.

Doesn't this version have a specific name as well? The ADC block
seems to use version numbers, so better put those in here to avoid
confusion when another Exynos7890 comes out that uses the same
ADC as exynos3250.

	Arnd
Chanwoo Choi July 18, 2014, 4:23 p.m. UTC | #2
On Fri, Jul 18, 2014 at 6:50 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 18 July 2014 14:59:45 Chanwoo Choi wrote:
>>                         Must be "samsung,exynos-adc-v2" for
>>                                 future controllers.
>
> It would be good to change 'future controllers' to something else now.
> Presumably that word was used before the actual products were announced,
> but now they are publically known.
>
>> +                       Must be "samsung,exynos3250-adc-v2" for
>> +                               controllers compatible with ADC of Exynos3250.
>
> Doesn't this version have a specific name as well? The ADC block
> seems to use version numbers, so better put those in here to avoid
> confusion when another Exynos7890 comes out that uses the same
> ADC as exynos3250.

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.


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@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;
                }

Thanks,
Chanwoo Choi
Arnd Bergmann July 18, 2014, 4:33 p.m. UTC | #3
On Saturday 19 July 2014 01:23:15 Chanwoo Choi wrote:
> 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.
> 
> 
> 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@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.

	Arnd
Chanwoo Choi July 18, 2014, 5:02 p.m. UTC | #4
On Sat, Jul 19, 2014 at 1:33 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Saturday 19 July 2014 01:23:15 Chanwoo Choi wrote:
>> 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.
>>
>>
>> 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@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

Thanks,
Chanwoo Choi
Arnd Bergmann July 18, 2014, 6:48 p.m. UTC | #5
On Saturday 19 July 2014 02:02:09 Chanwoo Choi wrote:
> On Sat, Jul 19, 2014 at 1:33 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Saturday 19 July 2014 01:23:15 Chanwoo Choi wrote:
> >> 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.

> >> 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@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));
		}
	}

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.

	Arnd
Chanwoo Choi July 21, 2014, 1:52 a.m. UTC | #6
On 07/19/2014 03:48 AM, Arnd Bergmann wrote:
> On Saturday 19 July 2014 02:02:09 Chanwoo Choi wrote:
>> On Sat, Jul 19, 2014 at 1:33 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>>> On Saturday 19 July 2014 01:23:15 Chanwoo Choi wrote:
>>>> 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".

> 
>>>> 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@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@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@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
Arnd Bergmann July 21, 2014, 8 a.m. UTC | #7
On Monday 21 July 2014 10:52:28 Chanwoo Choi wrote:
> > 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@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@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;
>                 };

But neither of those cases is actually a correct DT representation for
exynos3250: The first case describes an ADC that doesn't need a second
clock, so if the hardware actually needs it to function, it is clearly
unsufficiently described. The second case is even worse because it refers
to a clock that isn't there. Actually that should probably return a different
error code, but that's a different matter.

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

I don't mind you adding a field to the data, especially since you already
need to have separate structures to encode the different number of channels.
However, I don't see it as necessary either. What you do here is just
checking the DT representation for correctness and consistency. It's not
harmful but we don't normally do that because passing incorrect DT blobs
can cause an infinite number of other problems that we don't check for.

	Arnd
Tomasz Figa July 21, 2014, 10:38 a.m. UTC | #8
On 21.07.2014 10:00, Arnd Bergmann wrote:
> On Monday 21 July 2014 10:52:28 Chanwoo Choi wrote:
>>> 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@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@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;
>>                 };
> 
> But neither of those cases is actually a correct DT representation for
> exynos3250: The first case describes an ADC that doesn't need a second
> clock, so if the hardware actually needs it to function, it is clearly
> unsufficiently described. The second case is even worse because it refers
> to a clock that isn't there. Actually that should probably return a different
> error code, but that's a different matter.
> 
>> So, I think exynos-adc needs to use 'needs_sclk' field suggested by Tomasz Figa.
> 
> I don't mind you adding a field to the data, especially since you already
> need to have separate structures to encode the different number of channels.
> However, I don't see it as necessary either. What you do here is just
> checking the DT representation for correctness and consistency. It's not
> harmful but we don't normally do that because passing incorrect DT blobs
> can cause an infinite number of other problems that we don't check for.

I believe we should be enforcing as much correctness on DT data as
possible without too much burden in the code. Otherwise how we are
supposed to know if an error is caused by wrong/missing data in DT, bug
in the driver or who knows else?

In this case making this clock required depending on compatible string
doesn't seem too bothersome to me.

Best regards,
Tomasz
Arnd Bergmann July 21, 2014, 10:47 a.m. UTC | #9
On Monday 21 July 2014 12:38:55 Tomasz Figa wrote:
> 
> I believe we should be enforcing as much correctness on DT data as
> possible without too much burden in the code. Otherwise how we are
> supposed to know if an error is caused by wrong/missing data in DT, bug
> in the driver or who knows else?
> 
> In this case making this clock required depending on compatible string
> doesn't seem too bothersome to me.

It makes some sense given that we already need the separate compatible
string (for the number of channels). Without that part, I'd probably
prefer not having the new string just for the extra clock being required,
that could be handled more easily by just making it an optional clock.

	Arnd
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
index 832fe8c..26232f9 100644
--- a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
+++ b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
@@ -14,14 +14,21 @@  Required properties:
 				for exynos4412/5250 controllers.
 			Must be "samsung,exynos-adc-v2" for
 				future controllers.
+			Must be "samsung,exynos3250-adc-v2" for
+				controllers compatible with ADC of Exynos3250.
 - reg:			Contains ADC register address range (base address and
 			length) and the address of the phy enable register.
 - interrupts: 		Contains the interrupt information for the timer. The
 			format is being dependent on which interrupt controller
 			the Samsung device uses.
 - #io-channel-cells = <1>; As ADC has multiple outputs
-- clocks		From common clock binding: handle to adc clock.
-- clock-names		From common clock binding: Shall be "adc".
+- clocks		From common clock bindings: handles to clocks specified
+			in "clock-names" property, in the same order.
+- clock-names		From common clock bindings: list of clock input names
+			used by ADC block:
+			- "adc" : ADC bus clock
+			- "sclk_adc" : ADC special clock (only for Exynos3250
+				       and compatible ADC block)
 - vdd-supply		VDD input supply.
 
 Note: child nodes can be added for auto probing from device tree.
@@ -41,6 +48,20 @@  adc: adc@12D10000 {
 	vdd-supply = <&buck5_reg>;
 };
 
+Example: adding device info in dtsi file for Exynos3250 with additional sclk
+
+adc: adc@126C0000 {
+	compatible = "samsung,exynos3250-adc-v2";
+	reg = <0x126C0000 0x100>, <0x10020718 0x4>;
+	interrupts = <0 137 0>;
+	#io-channel-cells = <1>;
+	io-channel-ranges;
+
+	clocks = <&cmu CLK_TSADC>, <&cmu CLK_SCLK_TSADC>;
+	clock-names = "adc", "sclk_adc";
+
+	vdd-supply = <&buck5_reg>;
+};
 
 Example: Adding child nodes in dts file