Re: [PATCH 2/6 v2] crypto:s5p-sss: Add device tree support

From: Tomasz Figa
Date: Fri Jan 10 2014 - 08:44:42 EST


Hi Naveen,

On 10.01.2014 07:07, Naveen Krishna Ch wrote:
Hello Tomasz,

Thanks for time and review comments they are really helping me a lot
in getting the patches merged.

Secondly, accept my apologies for not giving proper writeup of why i
din't address
few of your comments on v1 of this patchset.

It's okay.


On 9 January 2014 19:44, Tomasz Figa <t.figa@xxxxxxxxxxx> wrote:
Hi Naveen,


On 09.01.2014 05:59, Naveen Krishna Chatradhi wrote:

This patch adds device tree support to the s5p-sss.c crypto driver.
Implements a varient struct to address the changes in SSS hardware
on various SoCs from Samsung.

Also, Documentation under devicetree/bindings added.

Signed-off-by: Naveen Krishna Ch <ch.naveen@xxxxxxxxxxx>
CC: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
CC: David S. Miller <davem@xxxxxxxxxxxxx>
CC: Vladimir Zapolskiy <vzapolskiy@xxxxxxxxx>
TO: <linux-crypto@xxxxxxxxxxxxxxx>
CC: <linux-samsung-soc@xxxxxxxxxxxxxxx>
---
Changes since v1:
1. Added description of the SSS module in Documentation
2. Corrected the interrupts description
3. Added varient struct instead of the version variable

.../devicetree/bindings/crypto/samsung-sss.txt | 20 +++++
drivers/crypto/s5p-sss.c | 81
++++++++++++++++++--
2 files changed, 95 insertions(+), 6 deletions(-)
create mode 100644
Documentation/devicetree/bindings/crypto/samsung-sss.txt

diff --git a/Documentation/devicetree/bindings/crypto/samsung-sss.txt
b/Documentation/devicetree/bindings/crypto/samsung-sss.txt
new file mode 100644
index 0000000..0e45b0d
--- /dev/null
+++ b/Documentation/devicetree/bindings/crypto/samsung-sss.txt
@@ -0,0 +1,20 @@
+Samsung SoC SSS (Security SubSystem) module
+
+The SSS module in S5PV210 SoC supports the following:
+-- Feeder (FeedCtrl)
+-- Advanced Encryption Standard (AES)
+-- Data Encryption Standard (DES)/3DES
+-- Public Key Accelerator (PKA)
+-- SHA-1/SHA-256/MD5/HMAC (SHA-1/SHA-256/MD5)/PRNG
+-- PRNG: Pseudo Random Number Generator
+
+Required properties:
+
+- compatible : Should contain entries for this and backward compatible
+ SSS versions:
+ - "samsung,s5p-secss" for S5PV210 SoC.


As I wrote in my previous reply, please use specific compatible string
containing name of SoC on which the block described by it appeared first.
Let me say it again, for security block that showed up first on S5PV210 the
string will be "samsung,s5pv210-secss".
1. .name = "s5p-secss", is the name used in the present driver.
So, i dint modify that.

Please note that compatible strings and platform driver names are completely different things. There is no relation between them. Moreover, there are different rules (or recommendation) involved for creating both, so it's completely fine to add a compatible string of "samsung,s5pv210-secss", while keeping platform driver named "s5p-secss".

2. I'm not sure which one is the first soc to have the new varient.
May be Exynos4210. Will modify in the next version.

Let's see.

From what I can find in various user's manuals, S5PV210 is the first to have the first variant, while Exynos4210 already has the new one.


+ },
+ { },
+};
+MODULE_DEVICE_TABLE(of, s5p_sss_dt_match);
+#else
+static struct of_device_id s5p_sss_dt_match[] = {
+ { },
+};


Hmm, I don't think there is any need for this.
I think all Exynos4 and Exynos5 now supports DT
But, i'm not sure of the S5PV210 series.

S5PV210 does not support DT yet. Work is already in progress, but board file support will have to be retained for some time, so this driver should support non-DT probing too.

It doesn't affect my comment, though, as you can use of_match_ptr() macro.

@@ -674,9 +741,11 @@ static int s5p_aes_remove(struct platform_device
*pdev)
static struct platform_driver s5p_aes_crypto = {
.probe = s5p_aes_probe,
.remove = s5p_aes_remove,
+ .id_table = s5p_sss_ids,
.driver = {
.owner = THIS_MODULE,
.name = "s5p-secss",
+ .of_match_table = s5p_sss_dt_match,


.of_match_table = of_match_ptr(s5p_sss_dt_match),
I dint use it, Some time back there was a patchset from Sachin
https://lkml.org/lkml/2013/9/28/61
Please suggest me on this.

I believe Sachin already explained this in his reply to your patch. Generally the driver from your link is supposed to always support DT, while this one can be built without DT support.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/