linux-kernel - Re: [PATCHv6 3/4] iio: devicetree: Add DT binding documentation for Exynos3250 ADC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 21 Jul 2014 12:38:55 +0200
From:	Tomasz Figa <tomasz.figa@...il.com>
To:	Arnd Bergmann <arnd@...db.de>, Chanwoo Choi <cw00.choi@...sung.com>
CC:	Chanwoo Choi <cwchoi00@...il.com>, jic23@...nel.org,
	naveen krishna <ch.naveen@...sung.com>,
	Kukjin Kim <kgene.kim@...sung.com>,
	Rob Herring <robh+dt@...nel.org>, pawel.moll@....com,
	Mark Rutland <mark.rutland@....com>,
	ijc+devicetree@...lion.org.uk, Kumar Gala <galak@...eaurora.org>,
	rdunlap@...radead.org, Kyungmin Park <kyungmin.park@...sung.com>,
	Tomasz Figa <t.figa@...sung.com>, linux-iio@...r.kernel.org,
	linux-samsung-soc <linux-samsung-soc@...r.kernel.org>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
	devicetree <devicetree@...r.kernel.org>,
	linux-doc@...r.kernel.org
Subject: Re: [PATCHv6 3/4] iio: devicetree: Add DT binding documentation for
 Exynos3250 ADC

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@...C0000 {
>>                         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@...C0000 {
>>                         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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/