From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from michel.telenet-ops.be (michel.telenet-ops.be [195.130.137.88]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 5223418C32B for ; Tue, 20 Aug 2024 09:38:42 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=195.130.137.88 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1724146726; cv=none; b=m4biLTXnu4h4wALjuDasdHNh6X7Vfc4Lgkm8xTrI3wHlu0jbOZ47YHgrOwOhr+aPhAhGOuhKmSFWYkcMnMcEAgsFqcv4igT2t2k1nsa7cT6VWAF+jlO5Dd7x9EncK2YHvDpV8fNecNX0/udEeeHK2bt2ybuhxfD5PozRhNezQW8= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1724146726; c=relaxed/simple; bh=paWZtgBvhp7Vcd9o1gKj5AoPY5sacgrHwJDRltdRJuo=; h=Date:From:To:cc:Subject:In-Reply-To:Message-ID:References: MIME-Version:Content-Type; b=R3C81XkDwkh4K8VcckhGetk+c5NSDFG4b/sh8eSjvkfF+H2Ps/ruHn1Eb+4ABdSh6H0g3eAa0XlmPu0bo9/uJiHtwem/7hMDW6H8aA+j9WxuU8iDp8xQbTbb7pZ9PqPXhkLY+Vzz2GKxMt3QKI+1NUJKee2NGAHjQt+xS5+m0wA= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=linux-m68k.org; spf=none smtp.mailfrom=linux-m68k.org; arc=none smtp.client-ip=195.130.137.88 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=linux-m68k.org Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=linux-m68k.org Received: from ramsan.of.borg ([IPv6:2a02:1810:ac12:ed80:a2e4:464c:5828:2da3]) by michel.telenet-ops.be with bizsmtp id 29eg2D0062WQTnu069egcj; Tue, 20 Aug 2024 11:38:41 +0200 Received: from geert (helo=localhost) by ramsan.of.borg with local-esmtp (Exim 4.95) (envelope-from ) id 1sgLK4-000Mdw-0L; Tue, 20 Aug 2024 11:38:40 +0200 Date: Tue, 20 Aug 2024 11:38:39 +0200 (CEST) From: Geert Uytterhoeven To: Pavitrakumar M cc: herbert@gondor.apana.org.au, linux-crypto@vger.kernel.org, Ruud.Derwig@synopsys.com, manjunath.hadli@vayavyalabs.com, bhoomikak@vayavyalabs.com, shwetar , Rob Herring , Krzysztof Kozlowski , Conor Dooley , devicetree@vger.kernel.org Subject: Re: [PATCH v4 1/7] Add SPAcc Skcipher support In-Reply-To: <20240618042750.485720-2-pavitrakumarm@vayavyalabs.com> Message-ID: <41ed1cd-1c1a-c38-5032-a997cd13179@linux-m68k.org> References: <20240618042750.485720-1-pavitrakumarm@vayavyalabs.com> <20240618042750.485720-2-pavitrakumarm@vayavyalabs.com> Precedence: bulk X-Mailing-List: devicetree@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Hi Pavitrakumar, CC devicetree On Tue, 18 Jun 2024, Pavitrakumar M wrote: > Signed-off-by: shwetar > Signed-off-by: Pavitrakumar M > Acked-by: Ruud Derwig 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