Subject: Re: [RESEND PATCH 1/4] Crypto: Crypto driver support aes/des/des3 for
 rk3288
To: Heiko Stuebner <heiko@sntech.de>
References: <1446193369-4453-1-git-send-email-zain.wang@rock-chips.com>
 <1446193369-4453-2-git-send-email-zain.wang@rock-chips.com>
 <2230290.pTq1BehVXW@phil>
Cc: zhengsq@rock-chips.com, hl@rock-chips.com, herbert@gondor.apana.org.au,
        davem@davemloft.net, mturquette@baylibre.com, pawel.moll@arm.com,
        ijc+devicetree@hellion.org.uk, robh+dt@kernel.org,
        galak@codeaurora.org, linux@arm.linux.org.uk, mark.rutland@arm.com,
        linux-kernel@vger.kernel.org, linux-crypto@vger.kernel.org,
        linux-rockchip@lists.infradead.org, devicetree@vger.kernel.org,
        eddie.cai@rock-chips.com
From: Zain <zain.wang@rock-chips.com>
Message-ID: <563808E3.5090005@rock-chips.com>
Date: Tue, 3 Nov 2015 09:07:47 +0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.3.0
MIME-Version: 1.0
In-Reply-To: <2230290.pTq1BehVXW@phil>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3586
Lines: 93

Hi Heiko,

On 2015年10月30日 22:26, Heiko Stuebner wrote:
> Hi,
>
> first of all, thanks for working on this, it will be really cool to see the
> crypto accelerator supported in the kernel :-)
>
> Am Freitag, 30. Oktober 2015, 16:22:46 schrieb Zain Wang:
>> Crypto driver support cbc/ecb two chainmode, and aes/des/des3 three cipher mode.
>> The names registered are:
>>     ecb(aes) cbc(aes) ecb(des) cbc(des) ecb(des3_ede) cbc(des3_ede)
>> You can alloc tags above in your case.
>>
>> And other algorithms and platforms will be added later on.
>>
>> Signed-off-by: Zain Wang <zain.wang@rock-chips.com>
>> ---
>>  drivers/crypto/Makefile                            |   1 +
>>  drivers/crypto/rk_crypto/Makefile                  |   3 +
>>  drivers/crypto/rk_crypto/rk3288_crypto.c           | 393 ++++++++++++++++
>>  drivers/crypto/rk_crypto/rk3288_crypto.h           | 291 ++++++++++++
>>  .../crypto/rk_crypto/rk3288_crypto_ablkcipher.c    | 502 +++++++++++++++++++++
>>  5 files changed, 1190 insertions(+)
>>  create mode 100644 drivers/crypto/rk_crypto/Makefile
>>  create mode 100644 drivers/crypto/rk_crypto/rk3288_crypto.c
>>  create mode 100644 drivers/crypto/rk_crypto/rk3288_crypto.h
>>  create mode 100644 drivers/crypto/rk_crypto/rk3288_crypto_ablkcipher.c
>>
>> diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile
>> index c3ced6f..00d103c 100644
>> --- a/drivers/crypto/Makefile
>> +++ b/drivers/crypto/Makefile
>> @@ -29,3 +29,4 @@ obj-$(CONFIG_CRYPTO_DEV_QAT) += qat/
>>  obj-$(CONFIG_CRYPTO_DEV_QCE) += qce/
>>  obj-$(CONFIG_CRYPTO_DEV_VMX) += vmx/
>>  obj-$(CONFIG_CRYPTO_DEV_SUN4I_SS) += sunxi-ss/
>> +obj-$(CONFIG_CRYPTO_DEV_RK3288) += rk_crypto/
> as Mark said in the other mail, there may very well follow other Rockchip
> crypto-related drivers, so maybe it might be better to name the core
> option CONFIG_CRYPTO_DEV_ROCKCHIP. And then maybe define
> CONFIG_CRYPTO_DEV_RK3288 in a Kconfig in the subdirectory (like vmx does).
ok! done!
>
>
>> diff --git a/drivers/crypto/rk_crypto/Makefile b/drivers/crypto/rk_crypto/Makefile
>> new file mode 100644
>> index 0000000..0f62d87
>> --- /dev/null
>> +++ b/drivers/crypto/rk_crypto/Makefile
> generally, a lot of those subdirectories reference only the manufacturer
> name, so my suggestion would be to name yours just "rockchip" instead of
> "rk_crypto", to also ease readability.
ok! done!
>
>
>> @@ -0,0 +1,3 @@
>> +obj-$(CONFIG_CRYPTO_DEV_RK3288) += rk_crypto_driver.o
>> +rk_crypto_driver-objs := rk3288_crypto.o \
>> +			 rk3288_crypto_ablkcipher.o \
> that looks looks wrong.
>
> (1) the config option does not get defined in any Kconfig file in your set
i am sorry about it, i will commit it next version.
> (2) naming the driver generically while you compile a variant will hinder
>     multiplatform kernels, because you might want to build both the rk3288
>     code and some additional stuff?
i want to commit hash base on this patch later.
> (3) the _driver suffix is unnecessary
ok! done.
>
> So I guess just name it rk3288_crypto.o ?
>
> [...]
>
> While I have read through the code implementing the device, I haven't
> seen anything spring out but do not know enough about the crypto subsystem
> itself. But it looks like you got some feedback already anyway from others :-)
thanks for your comments.
>
> Thanks
> Heiko
>
>
>
Thanks
Zain

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