* Re: [PATCH v4 1/7] Add SPAcc Skcipher support
[not found] ` <20240618042750.485720-2-pavitrakumarm@vayavyalabs.com>
@ 2024-08-20 9:38 ` Geert Uytterhoeven
0 siblings, 0 replies; 3+ messages in thread
From: Geert Uytterhoeven @ 2024-08-20 9:38 UTC (permalink / raw)
To: Pavitrakumar M
Cc: herbert, linux-crypto, Ruud.Derwig, manjunath.hadli, bhoomikak,
shwetar, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
devicetree
Hi Pavitrakumar,
CC devicetree
On Tue, 18 Jun 2024, Pavitrakumar M wrote:
> Signed-off-by: shwetar <shwetar@vayavyalabs.com>
> Signed-off-by: Pavitrakumar M <pavitrakumarm@vayavyalabs.com>
> Acked-by: Ruud Derwig <Ruud.Derwig@synopsys.com>
Thanks for your patch, which is now commit fc61c658c94cb740 ("crypto:
spacc - Enable Driver compilation in crypto Kconfig and Makefile")
in crypto/master.
> --- /dev/null
> +++ b/drivers/crypto/dwc-spacc/spacc_core.c
> +int spacc_probe(struct platform_device *pdev,
> + const struct of_device_id snps_spacc_id[])
There should not be a need to pass snps_spacc_id[] around.
> +{
> + int spacc_idx = -1;
> + struct resource *mem;
> + int spacc_endian = 0;
> + void __iomem *baseaddr;
> + struct pdu_info info;
> + int spacc_priority = -1;
> + struct spacc_priv *priv;
> + int x = 0, err, oldmode, irq_num;
> + const struct of_device_id *match, *id;
> + u64 oldtimer = 100000, timer = 100000;
> +
> + if (pdev->dev.of_node) {
> + id = of_match_node(snps_spacc_id, pdev->dev.of_node);
> + if (!id) {
> + dev_err(&pdev->dev, "DT node did not match\n");
> + return -EINVAL;
> + }
> + }
This check is not needed.
> +
> + /* Initialize DDT DMA pools based on this device's resources */
> + if (pdu_mem_init(&pdev->dev)) {
> + dev_err(&pdev->dev, "Could not initialize DMA pools\n");
> + return -ENOMEM;
> + }
> +
> + match = of_match_device(of_match_ptr(snps_spacc_id), &pdev->dev);
> + if (!match) {
> + dev_err(&pdev->dev, "SPAcc dtb missing");
> + return -ENODEV;
> + }
This check is also not needed.
Besides, in case of an error, you leak the ddt mem pool.
> +
> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!mem) {
> + dev_err(&pdev->dev, "no memory resource for spacc\n");
> + err = -ENXIO;
> + goto free_ddt_mem_pool;
> + }
> +
> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv) {
> + err = -ENOMEM;
> + goto free_ddt_mem_pool;
> + }
> +
> + /* Read spacc priority and index and save inside priv.spacc.config */
> + if (of_property_read_u32(pdev->dev.of_node, "spacc_priority",
Please no underscores in DT properties.
> + &spacc_priority)) {
> + dev_err(&pdev->dev, "No vspacc priority specified\n");
> + err = -EINVAL;
> + goto free_ddt_mem_pool;
> + }
> +
> + if (spacc_priority < 0 && spacc_priority > VSPACC_PRIORITY_MAX) {
> + dev_err(&pdev->dev, "Invalid vspacc priority\n");
> + err = -EINVAL;
> + goto free_ddt_mem_pool;
> + }
> + priv->spacc.config.priority = spacc_priority;
> +
> + if (of_property_read_u32(pdev->dev.of_node, "spacc_index",
> + &spacc_idx)) {
> + dev_err(&pdev->dev, "No vspacc index specified\n");
> + err = -EINVAL;
> + goto free_ddt_mem_pool;
> + }
> + priv->spacc.config.idx = spacc_idx;
> +
> + if (of_property_read_u32(pdev->dev.of_node, "spacc_endian",
Please use the standard big-endian / little-endian properties.
> + &spacc_endian)) {
> + dev_dbg(&pdev->dev, "No spacc_endian specified\n");
> + dev_dbg(&pdev->dev, "Default spacc Endianness (0==little)\n");
> + spacc_endian = 0;
> + }
> + priv->spacc.config.spacc_endian = spacc_endian;
> +
> + if (of_property_read_u64(pdev->dev.of_node, "oldtimer",
> + &oldtimer)) {
> + dev_dbg(&pdev->dev, "No oldtimer specified\n");
> + dev_dbg(&pdev->dev, "Default oldtimer (100000)\n");
> + oldtimer = 100000;
> + }
> + priv->spacc.config.oldtimer = oldtimer;
> +
> + if (of_property_read_u64(pdev->dev.of_node, "timer", &timer)) {
> + dev_dbg(&pdev->dev, "No timer specified\n");
> + dev_dbg(&pdev->dev, "Default timer (100000)\n");
> + timer = 100000;
> + }
> + priv->spacc.config.timer = timer;
This device lacks DT binding documentation.
> +static struct platform_driver spacc_driver = {
> + .probe = spacc_crypto_probe,
> + .remove = spacc_crypto_remove,
> + .driver = {
> + .name = "spacc",
> + .of_match_table = of_match_ptr(snps_spacc_id),
Please drop the of_match_ptr(), as your driver requires DT to function.
> + .owner = THIS_MODULE,
> + },
> +};
I didn't review the rest.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 3+ messages in thread