From: "H. Nikolaus Schaller" <hns@goldelico.com> To: Paul Cercueil <paul@crapouillou.net> Cc: Rob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Thomas Bogendoerfer <tsbogend@alpha.franken.de>, Geert Uytterhoeven <geert+renesas@glider.be>, Kees Cook <keescook@chromium.org>, "Eric W. Biederman" <ebiederm@xmission.com>, Miquel Raynal <miquel.raynal@bootlin.com>, David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>, Neil Armstrong <narmstrong@baylibre.com>, Robert Foss <robert.foss@linaro.org>, Laurent Pinchart <Laurent.pinchart@ideasonboard.com>, Jernej Skrabec <jernej.skrabec@gmail.com>, Ezequiel Garcia <ezequiel@collabora.com>, Harry Wentland <harry.wentland@amd.com>, Sam Ravnborg <sam@ravnborg.org>, Maxime Ripard <maxime@cerno.tech>, Hans Verkuil <hverkuil-cisco@xs4all.nl>, Liam Girdwood <lgirdwood@gmail.com>, Mark Brown <broonie@kernel.org>, Paul Boddie <paul@boddie.org.uk>, devicetree@vger.kernel.org, linux-mips@vger.kernel.org, linux-kernel@vger.kernel.org, letux-kernel@openphoenux.org, Jonas Karlman <jonas@kwiboo.se>, dri-devel@lists.freedesktop.org Subject: Re: [PATCH v8 4/8] drm/ingenic: Add dw-hdmi driver for jz4780 Date: Wed, 24 Nov 2021 22:28:43 +0100 [thread overview] Message-ID: <D5272A7B-8943-4AB9-8705-1EC4E9891C1D@goldelico.com> (raw) In-Reply-To: <DIA33R.QE29K7RKLI2C1@crapouillou.net> Hi Paul, >>> You probably should disable the regulator (if not NULL) here. >> Indeed. Would it be ok to make struct regulator *regulator static >> or do we need dynamically allocated memory? > > static non-const is almost always a bad idea, so avoid it. Well some years ago it was a perfectly simple solution that still works... But I asked because I had a lot of doubt. > > You can either: > > - create a "ingenic_dw_hdmi" struct that will contain a pointer to dw_hdmi and a pointer to the regulator. Instanciate it in the probe with devm_kzalloc() and set the pointers, then set it as the driver data (platform_set_drvdata). In the remove function you can then obtain the pointer to your ingenic_dw_hdmi struct with platform_get_drvdata(), and you can remove the dw_hdmi and unregister the regulator. > > - register cleanup functions, using devm_add_action_or_reset(dev, f, priv). When it's time to cleanup, the kernel core will call f(priv) automatically. So you can add a small wrapper around dw_hdmi_remove() and another one around regulator_disable(), and those will be called automatically if your probe function fails, or when the driver is removed. Then you can completely remove the ".remove" callback. There are a few examples of these in the ingenic-drm-drv.c if you want to take a look. The second one turned out to be cleaner to handle special cases like if there is no regulator. We just register the disabler only if there is a regulator and enable succeeds. So v9 is coming now. BR and thanks, Nikolaus
WARNING: multiple messages have this Message-ID (diff)
From: "H. Nikolaus Schaller" <hns@goldelico.com> To: Paul Cercueil <paul@crapouillou.net> Cc: Mark Rutland <mark.rutland@arm.com>, Paul Boddie <paul@boddie.org.uk>, Geert Uytterhoeven <geert+renesas@glider.be>, Neil Armstrong <narmstrong@baylibre.com>, David Airlie <airlied@linux.ie>, dri-devel@lists.freedesktop.org, linux-mips@vger.kernel.org, Laurent Pinchart <Laurent.pinchart@ideasonboard.com>, Miquel Raynal <miquel.raynal@bootlin.com>, Sam Ravnborg <sam@ravnborg.org>, Jernej Skrabec <jernej.skrabec@gmail.com>, devicetree@vger.kernel.org, Kees Cook <keescook@chromium.org>, Jonas Karlman <jonas@kwiboo.se>, Mark Brown <broonie@kernel.org>, Maxime Ripard <maxime@cerno.tech>, letux-kernel@openphoenux.org, Ezequiel Garcia <ezequiel@collabora.com>, Thomas Bogendoerfer <tsbogend@alpha.franken.de>, Liam Girdwood <lgirdwood@gmail.com>, Robert Foss <robert.foss@linaro.org>, linux-kernel@vger.kernel.org, Rob Herring <robh+dt@kernel.org>, "Eric W. Biederman" <ebiederm@xmission.com>, Hans Verkuil <hverkuil-cisco@xs4all.nl> Subject: Re: [PATCH v8 4/8] drm/ingenic: Add dw-hdmi driver for jz4780 Date: Wed, 24 Nov 2021 22:28:43 +0100 [thread overview] Message-ID: <D5272A7B-8943-4AB9-8705-1EC4E9891C1D@goldelico.com> (raw) In-Reply-To: <DIA33R.QE29K7RKLI2C1@crapouillou.net> Hi Paul, >>> You probably should disable the regulator (if not NULL) here. >> Indeed. Would it be ok to make struct regulator *regulator static >> or do we need dynamically allocated memory? > > static non-const is almost always a bad idea, so avoid it. Well some years ago it was a perfectly simple solution that still works... But I asked because I had a lot of doubt. > > You can either: > > - create a "ingenic_dw_hdmi" struct that will contain a pointer to dw_hdmi and a pointer to the regulator. Instanciate it in the probe with devm_kzalloc() and set the pointers, then set it as the driver data (platform_set_drvdata). In the remove function you can then obtain the pointer to your ingenic_dw_hdmi struct with platform_get_drvdata(), and you can remove the dw_hdmi and unregister the regulator. > > - register cleanup functions, using devm_add_action_or_reset(dev, f, priv). When it's time to cleanup, the kernel core will call f(priv) automatically. So you can add a small wrapper around dw_hdmi_remove() and another one around regulator_disable(), and those will be called automatically if your probe function fails, or when the driver is removed. Then you can completely remove the ".remove" callback. There are a few examples of these in the ingenic-drm-drv.c if you want to take a look. The second one turned out to be cleaner to handle special cases like if there is no regulator. We just register the disabler only if there is a regulator and enable succeeds. So v9 is coming now. BR and thanks, Nikolaus
next prev parent reply other threads:[~2021-11-24 21:29 UTC|newest] Thread overview: 55+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-11-23 18:13 [PATCH v8 0/8] MIPS: JZ4780 and CI20 HDMI H. Nikolaus Schaller 2021-11-23 18:13 ` H. Nikolaus Schaller 2021-11-23 18:13 ` [PATCH v8 1/8] drm/ingenic: prepare ingenic drm for later addition of JZ4780 H. Nikolaus Schaller 2021-11-23 18:13 ` H. Nikolaus Schaller 2021-11-23 18:13 ` [PATCH v8 2/8] drm/ingenic: Add support for JZ4780 and HDMI output H. Nikolaus Schaller 2021-11-23 18:13 ` H. Nikolaus Schaller 2021-11-23 18:13 ` [PATCH v8 3/8] dt-bindings: display: Add ingenic,jz4780-dw-hdmi DT Schema H. Nikolaus Schaller 2021-11-23 18:13 ` [PATCH v8 3/8] dt-bindings: display: Add ingenic, jz4780-dw-hdmi " H. Nikolaus Schaller 2021-11-24 2:59 ` [PATCH v8 3/8] dt-bindings: display: Add ingenic,jz4780-dw-hdmi " Rob Herring 2021-11-24 2:59 ` [PATCH v8 3/8] dt-bindings: display: Add ingenic, jz4780-dw-hdmi " Rob Herring 2021-11-24 16:23 ` [PATCH v8 3/8] dt-bindings: display: Add ingenic,jz4780-dw-hdmi " H. Nikolaus Schaller 2021-11-24 16:23 ` H. Nikolaus Schaller 2021-11-24 9:17 ` Paul Cercueil 2021-11-24 9:17 ` Paul Cercueil 2021-11-24 16:21 ` H. Nikolaus Schaller 2021-11-24 16:21 ` H. Nikolaus Schaller 2021-11-23 18:13 ` [PATCH v8 4/8] drm/ingenic: Add dw-hdmi driver for jz4780 H. Nikolaus Schaller 2021-11-23 18:13 ` H. Nikolaus Schaller 2021-11-23 20:05 ` Paul Cercueil 2021-11-23 20:05 ` Paul Cercueil 2021-11-24 16:13 ` H. Nikolaus Schaller 2021-11-24 16:13 ` H. Nikolaus Schaller 2021-11-24 18:39 ` Paul Cercueil 2021-11-24 18:39 ` Paul Cercueil 2021-11-24 21:28 ` H. Nikolaus Schaller [this message] 2021-11-24 21:28 ` H. Nikolaus Schaller 2021-11-23 18:13 ` [PATCH v8 5/8] MIPS: DTS: jz4780: Account for Synopsys HDMI driver and LCD controllers H. Nikolaus Schaller 2021-11-23 18:13 ` H. Nikolaus Schaller 2021-11-23 18:13 ` [PATCH v8 6/8] MIPS: DTS: CI20: Add DT nodes for HDMI setup H. Nikolaus Schaller 2021-11-23 18:13 ` H. Nikolaus Schaller 2021-11-23 20:10 ` Paul Cercueil 2021-11-23 20:10 ` Paul Cercueil 2021-11-24 16:19 ` H. Nikolaus Schaller 2021-11-24 16:19 ` H. Nikolaus Schaller 2021-11-24 16:21 ` Geert Uytterhoeven 2021-11-24 16:21 ` Geert Uytterhoeven 2021-11-24 16:30 ` H. Nikolaus Schaller 2021-11-24 16:30 ` H. Nikolaus Schaller 2021-11-25 7:58 ` Geert Uytterhoeven 2021-11-25 7:58 ` Geert Uytterhoeven 2021-11-25 8:29 ` H. Nikolaus Schaller 2021-11-25 8:29 ` H. Nikolaus Schaller 2021-11-25 9:02 ` Paul Cercueil 2021-11-25 9:13 ` Geert Uytterhoeven 2021-11-25 9:22 ` H. Nikolaus Schaller 2021-11-23 18:14 ` [PATCH v8 7/8] MIPS: defconfig: CI20: configure for DRM_DW_HDMI_JZ4780 H. Nikolaus Schaller 2021-11-23 18:14 ` H. Nikolaus Schaller 2021-11-23 18:14 ` [PATCH v8 8/8] [RFC] MIPS: DTS: Ingenic: adjust register size to available registers H. Nikolaus Schaller 2021-11-23 18:14 ` H. Nikolaus Schaller 2021-11-23 20:12 ` [PATCH v8 0/8] MIPS: JZ4780 and CI20 HDMI Paul Cercueil 2021-11-23 20:12 ` Paul Cercueil 2021-11-23 20:44 ` H. Nikolaus Schaller 2021-11-23 20:44 ` H. Nikolaus Schaller 2021-11-24 16:48 ` H. Nikolaus Schaller 2021-11-24 16:48 ` H. Nikolaus Schaller
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=D5272A7B-8943-4AB9-8705-1EC4E9891C1D@goldelico.com \ --to=hns@goldelico.com \ --cc=Laurent.pinchart@ideasonboard.com \ --cc=airlied@linux.ie \ --cc=broonie@kernel.org \ --cc=daniel@ffwll.ch \ --cc=devicetree@vger.kernel.org \ --cc=dri-devel@lists.freedesktop.org \ --cc=ebiederm@xmission.com \ --cc=ezequiel@collabora.com \ --cc=geert+renesas@glider.be \ --cc=harry.wentland@amd.com \ --cc=hverkuil-cisco@xs4all.nl \ --cc=jernej.skrabec@gmail.com \ --cc=jonas@kwiboo.se \ --cc=keescook@chromium.org \ --cc=letux-kernel@openphoenux.org \ --cc=lgirdwood@gmail.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mips@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=maxime@cerno.tech \ --cc=miquel.raynal@bootlin.com \ --cc=narmstrong@baylibre.com \ --cc=paul@boddie.org.uk \ --cc=paul@crapouillou.net \ --cc=robert.foss@linaro.org \ --cc=robh+dt@kernel.org \ --cc=sam@ravnborg.org \ --cc=tsbogend@alpha.franken.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.