Re: [PATCH v4 1/7] Add SPAcc Skcipher support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



	Hi Pavitrakumar,

CC devicetree

On Tue, 18 Jun 2024, Pavitrakumar M wrote:
Signed-off-by: shwetar <shwetar@xxxxxxxxxxxxxxx>
Signed-off-by: Pavitrakumar M <pavitrakumarm@xxxxxxxxxxxxxxx>
Acked-by: Ruud Derwig <Ruud.Derwig@xxxxxxxxxxxx>

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@xxxxxxxxxxxxxx

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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux