From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS,USER_AGENT_NEOMUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D9B18C3279B for ; Tue, 10 Jul 2018 08:42:41 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 9017D208E1 for ; Tue, 10 Jul 2018 08:42:41 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 9017D208E1 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=bootlin.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751329AbeGJImi (ORCPT ); Tue, 10 Jul 2018 04:42:38 -0400 Received: from mail.bootlin.com ([62.4.15.54]:56994 "EHLO mail.bootlin.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751099AbeGJImf (ORCPT ); Tue, 10 Jul 2018 04:42:35 -0400 Received: by mail.bootlin.com (Postfix, from userid 110) id 2EE612079F; Tue, 10 Jul 2018 10:42:34 +0200 (CEST) Received: from localhost (AAubervilliers-681-1-12-56.w90-88.abo.wanadoo.fr [90.88.133.56]) by mail.bootlin.com (Postfix) with ESMTPSA id 032F820012; Tue, 10 Jul 2018 10:42:24 +0200 (CEST) Date: Tue, 10 Jul 2018 10:42:23 +0200 From: Maxime Ripard To: Paul Kocialkowski Cc: linux-media@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Mauro Carvalho Chehab , Rob Herring , Mark Rutland , Chen-Yu Tsai , Marco Franchi , Icenowy Zheng , Hans Verkuil , Keiichi Watanabe , Jonathan Corbet , Smitha T Murthy , Tom Saeger , Andrzej Hajda , "David S . Miller" , Greg Kroah-Hartman , Andrew Morton , Randy Dunlap , Arnd Bergmann , Geert Uytterhoeven , Laurent Pinchart , Jacob Chen , Neil Armstrong , Benoit Parrot , Todor Tomov , Alexandre Courbot , Sakari Ailus , Andy Shevchenko , Pawel Osciak , Ricardo Ribalda Delgado , Hans de Goede , Sami Tolvanen , Niklas =?utf-8?Q?S=C3=B6derlund?= , linux-sunxi@googlegroups.com, Thomas Petazzoni , Hugues Fruchet , Randy Li Subject: Re: [PATCH v5 18/22] media: platform: Add Sunxi-Cedrus VPU decoder driver Message-ID: <20180710084223.jguogmvlwloi2utf@flea> References: <20180710080114.31469-1-paul.kocialkowski@bootlin.com> <20180710080114.31469-19-paul.kocialkowski@bootlin.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="okv6qmpowge4nc7u" Content-Disposition: inline In-Reply-To: <20180710080114.31469-19-paul.kocialkowski@bootlin.com> User-Agent: NeoMutt/20180622 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --okv6qmpowge4nc7u Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Jul 10, 2018 at 10:01:10AM +0200, Paul Kocialkowski wrote: > +static int cedrus_remove(struct platform_device *pdev) > +{ > + struct cedrus_dev *dev =3D platform_get_drvdata(pdev); > + > + v4l2_info(&dev->v4l2_dev, "Removing " CEDRUS_NAME); That log is kind of pointless. > +static void cedrus_hw_set_capabilities(struct cedrus_dev *dev) > +{ > + unsigned int engine_version; > + > + engine_version =3D cedrus_read(dev, VE_VERSION) >> VE_VERSION_SHIFT; > + > + if (engine_version >=3D 0x1667) > + dev->capabilities |=3D CEDRUS_CAPABILITY_UNTILED; The version used here would need a define, but I'm wondering if this is the right solution here. You are using at the same time the version ID returned by the register and the compatible in various places, and they are both redundant. If you want to base the capabilities on the compatible, then you can do it for all of those properties and capabilities, and if you want to use the version register, then you don't need all those compatibles but just one. I think that basing all our capabilities on the compatible makes more sense, since you need to have access to the registers in order to read the version register, and this changes from one SoC generation to the other (for example, keeping the reset line asserted would prevent you =66rom reading it, and the fact that there is a reset line depends on the SoC). > +int cedrus_hw_probe(struct cedrus_dev *dev) > +{ > + struct resource *res; > + int irq_dec; > + int ret; > + > + irq_dec =3D platform_get_irq(dev->pdev, 0); > + if (irq_dec <=3D 0) { > + v4l2_err(&dev->v4l2_dev, "Failed to get IRQ\n"); > + return -ENXIO; > + } You already have an error code returned by platform_get_irq, there's no point in masking it and returning -ENXIO. This can even lead to some bugs, for example when the error code is EPROBE_DEFER. (and please add a new line there). > + ret =3D devm_request_threaded_irq(dev->dev, irq_dec, cedrus_irq, > + cedrus_bh, 0, dev_name(dev->dev), > + dev); > + if (ret) { > + v4l2_err(&dev->v4l2_dev, "Failed to request IRQ\n"); > + return -ENXIO; > + } Same thing here, you're masking the actual error code. > + res =3D platform_get_resource(dev->pdev, IORESOURCE_MEM, 0); > + dev->base =3D devm_ioremap_resource(dev->dev, res); > + if (!dev->base) { > + v4l2_err(&dev->v4l2_dev, "Failed to map registers\n"); > + > + ret =3D -EFAULT; ENOMEM is usually returned in such a case. > + goto err_sram; > + } > + > + ret =3D clk_set_rate(dev->mod_clk, CEDRUS_CLOCK_RATE_DEFAULT); > + if (ret) { > + v4l2_err(&dev->v4l2_dev, "Failed to set clock rate\n"); > + goto err_sram; > + } > + > + ret =3D clk_prepare_enable(dev->ahb_clk); > + if (ret) { > + v4l2_err(&dev->v4l2_dev, "Failed to enable AHB clock\n"); > + > + ret =3D -EFAULT; > + goto err_sram; Same thing for the error code. > + } > + > + ret =3D clk_prepare_enable(dev->mod_clk); > + if (ret) { > + v4l2_err(&dev->v4l2_dev, "Failed to enable MOD clock\n"); > + > + ret =3D -EFAULT; > + goto err_ahb_clk; Ditto. > + } > + > + ret =3D clk_prepare_enable(dev->ram_clk); > + if (ret) { > + v4l2_err(&dev->v4l2_dev, "Failed to enable RAM clock\n"); > + > + ret =3D -EFAULT; > + goto err_mod_clk; Ditto. > + } > + > + ret =3D reset_control_reset(dev->rstc); > + if (ret) { > + v4l2_err(&dev->v4l2_dev, "Failed to apply reset\n"); > + > + ret =3D -EFAULT; Ditto. There's also a bunch of warnings/checks reported by checkpatch that should be fixed in the next iteration: the spaces after a cast, the NULL comparison, macros arguments precedence, parenthesis alignments issues, etc.) Thanks! Maxime --=20 Maxime Ripard, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com --okv6qmpowge4nc7u Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEE0VqZU19dR2zEVaqr0rTAlCFNr3QFAltEcW4ACgkQ0rTAlCFN r3TqrQ//XlmctO0y2jW0/UHLJ/Ercl7EzUQBNk7890aXUuJHEpLwN9CTyWZUWyJ/ JTQux7Vrf2x7RxBjlzThdeyZ6LFfQotPI7+bal1H30QQxuClVHeZcIPxng5bzSiE BmyKirQLr/A1iR0mG+G9ZbzSl3JzGu3UdvxW44CfVuOHiWIFHHteUIKpgJ6o/HZl j8aIlBqqp3buurPyr2jN34fRcoOCFvItfYgIHhO8a09+Vye1KWzrhkA/hitsHISN IxE6WzAf6KKxeSUyRFU/O2hE3tTFigts8P0InfzZ9LfMdFs3pFsCY8ViJTUM1Kga Tb5thhFY6CkMX3duHUnK2LeEvPtvrRpeVTqY4sX3b31wmgc0u+Iw15tD2zQkiylT rVJVnqUBHUg3bLfzmdpgAKS+fMpgn0ufoYne7wkfAiXF49du1PUd5KRaB5/MYwXZ o9l3YREqbUYLY+6FL2Fe+sZ1MTzOIw7h4jZMP9PJuE1ILVPLjWIyDrHTSUs2rfp+ 7oXBZDjpiyv1II8cnaODT+9sD0FEvMYOOh2936GaRv7kObWJFqG/XghFeieKVFjL nFoVuytSsvUNqxF42HLS6FrDYp3yYrYYGlYCiGGoqUlNUScEU0b1a6kkRfmYvx0T uIB56yZSXZ8DJtWmLQIg27XULzzMj0Qc9s5qMVi+mZuoXc44Sek= =7Hqq -----END PGP SIGNATURE----- --okv6qmpowge4nc7u--