| Message ID | 1405663186-26464-4-git-send-email-cw00.choi@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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
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
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
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 --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
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(-)