From: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
To: Maxime Ripard <maxime@cerno.tech>
Cc: linux-media@vger.kernel.org, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org,
linux-phy@lists.infradead.org, linux-clk@vger.kernel.org,
linux-staging@lists.linux.dev, Yong Deng <yong.deng@magewell.com>,
Mauro Carvalho Chehab <mchehab@kernel.org>,
Rob Herring <robh+dt@kernel.org>,
Sakari Ailus <sakari.ailus@linux.intel.com>,
Hans Verkuil <hans.verkuil@cisco.com>,
Chen-Yu Tsai <wens@csie.org>,
Jernej Skrabec <jernej.skrabec@gmail.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Helen Koike <helen.koike@collabora.com>,
Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Subject: Re: [PATCH v2 21/66] media: sun6i-csi: Always set exclusive module clock rate
Date: Fri, 11 Feb 2022 17:29:37 +0100 [thread overview]
Message-ID: <YgaO8bfP4gKW8BM0@aptenodytes> (raw)
In-Reply-To: <20220207091443.cr5udv7fxx65ptty@houat>
[-- Attachment #1: Type: text/plain, Size: 4868 bytes --]
Hi,
Thanks for the review,
On Mon 07 Feb 22, 10:14, Maxime Ripard wrote:
> On Sat, Feb 05, 2022 at 07:53:44PM +0100, Paul Kocialkowski wrote:
> > In some situations the default rate of the module clock is not the
> > required one for operation (for example when reconfiguring the clock
> > tree to use a different parent). As a result, always set the correct
> > rate for the clock (and take care of cleanup).
> >
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > ---
> > .../platform/sunxi/sun6i-csi/sun6i_csi.c | 54 ++++++++++++++-----
> > 1 file changed, 41 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > index 8155e9560164..2355088fdc37 100644
> > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > @@ -154,9 +154,6 @@ int sun6i_csi_set_power(struct sun6i_csi_device *csi_dev, bool enable)
> > regmap_update_bits(regmap, CSI_EN_REG, CSI_EN_CSI_EN, 0);
> >
> > clk_disable_unprepare(csi_dev->clk_ram);
> > - if (of_device_is_compatible(dev->of_node,
> > - "allwinner,sun50i-a64-csi"))
> > - clk_rate_exclusive_put(csi_dev->clk_mod);
> > clk_disable_unprepare(csi_dev->clk_mod);
> > reset_control_assert(csi_dev->reset);
> > return 0;
> > @@ -168,9 +165,6 @@ int sun6i_csi_set_power(struct sun6i_csi_device *csi_dev, bool enable)
> > return ret;
> > }
> >
> > - if (of_device_is_compatible(dev->of_node, "allwinner,sun50i-a64-csi"))
> > - clk_set_rate_exclusive(csi_dev->clk_mod, 300000000);
> > -
> > ret = clk_prepare_enable(csi_dev->clk_ram);
> > if (ret) {
> > dev_err(csi_dev->dev, "Enable clk_dram_csi clk err %d\n", ret);
> > @@ -190,8 +184,6 @@ int sun6i_csi_set_power(struct sun6i_csi_device *csi_dev, bool enable)
> > clk_ram_disable:
> > clk_disable_unprepare(csi_dev->clk_ram);
> > clk_mod_disable:
> > - if (of_device_is_compatible(dev->of_node, "allwinner,sun50i-a64-csi"))
> > - clk_rate_exclusive_put(csi_dev->clk_mod);
> > clk_disable_unprepare(csi_dev->clk_mod);
> > return ret;
> > }
> > @@ -819,6 +811,7 @@ static int sun6i_csi_resources_setup(struct sun6i_csi_device *csi_dev,
> > struct platform_device *platform_dev)
> > {
> > struct device *dev = csi_dev->dev;
> > + unsigned long clk_mod_rate;
> > void __iomem *io_base;
> > int ret;
> > int irq;
> > @@ -856,28 +849,53 @@ static int sun6i_csi_resources_setup(struct sun6i_csi_device *csi_dev,
> > return PTR_ERR(csi_dev->clk_ram);
> > }
> >
> > + if (of_device_is_compatible(dev->of_node, "allwinner,sun50i-a64-csi"))
> > + clk_mod_rate = 300000000;
> > + else
> > + clk_mod_rate = 297000000;
> > +
> > + ret = clk_set_rate_exclusive(csi_dev->clk_mod, clk_mod_rate);
> > + if (ret) {
> > + dev_err(dev, "failed to set mod clock rate\n");
> > + return ret;
> > + }
> > +
> > /* Reset */
> >
> > csi_dev->reset = devm_reset_control_get_shared(dev, NULL);
> > if (IS_ERR(csi_dev->reset)) {
> > dev_err(dev, "failed to acquire reset\n");
> > - return PTR_ERR(csi_dev->reset);
> > + ret = PTR_ERR(csi_dev->reset);
> > + goto error_clk_rate_exclusive;
> > }
> >
> > /* Interrupt */
> >
> > irq = platform_get_irq(platform_dev, 0);
> > - if (irq < 0)
> > - return -ENXIO;
> > + if (irq < 0) {
> > + dev_err(dev, "failed to get interrupt\n");
> > + ret = -ENXIO;
> > + goto error_clk_rate_exclusive;
> > + }
> >
> > ret = devm_request_irq(dev, irq, sun6i_csi_isr, 0, SUN6I_CSI_NAME,
> > csi_dev);
> > if (ret) {
> > dev_err(dev, "failed to request interrupt\n");
> > - return ret;
> > + goto error_clk_rate_exclusive;
> > }
> >
> > return 0;
> > +
> > +error_clk_rate_exclusive:
> > + clk_rate_exclusive_put(csi_dev->clk_mod);
> > +
> > + return ret;
> > +}
> > +
> > +static void sun6i_csi_resources_cleanup(struct sun6i_csi_device *csi_dev)
> > +{
> > + clk_rate_exclusive_put(csi_dev->clk_mod);
> > }
>
> If you're going to have that function anyway, let's use
> devm_add_action_or_reset, it'll simplify the rest of the patch.
Well, this will cause issues later on when adding runtime pm support to
sun6i_csi_resources_cleanup: then it will no longer be equivalent
to the error case label.
Also I feel like making the resources_cleanup call a devm action would not
help clarify the general flow of the driver, where the matching setup/cleanup
calls are ordered in probe/remove. The driver is quite big with various parts
and I'd rather have them behave in a coherent and similar way.
What do you think?
Paul
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
next prev parent reply other threads:[~2022-02-11 16:29 UTC|newest]
Thread overview: 141+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-05 18:53 [PATCH v2 00/66] Allwinner A31/A83T MIPI CSI-2 Support and A31 ISP Support Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 01/66] ARM: dts: sun8i: v3s: Move the csi1 block to follow address order Paul Kocialkowski
2022-02-07 9:24 ` (subset) " Maxime Ripard
2022-02-05 18:53 ` [PATCH v2 02/66] dt-bindings: interconnect: sunxi: Add V3s mbus compatible Paul Kocialkowski
2022-02-05 20:14 ` Samuel Holland
2022-02-07 8:43 ` Paul Kocialkowski
2022-02-07 8:50 ` Jernej Škrabec
2022-02-07 9:44 ` Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 03/66] clk: sunxi-ng: v3s: Export the MBUS clock to the public header Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 04/66] ARM: dts: sun8i: v3s: Add mbus node to represent the interconnect Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 05/66] dt-bindings: sun6i-a31-mipi-dphy: Add optional direction property Paul Kocialkowski
2022-02-11 15:03 ` Rob Herring
2022-02-11 15:12 ` Paul Kocialkowski
2022-02-11 20:47 ` Laurent Pinchart
2022-02-05 18:53 ` [PATCH v2 06/66] phy: allwinner: phy-sun6i-mipi-dphy: Support D-PHY Rx mode for MIPI CSI-2 Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 07/66] dt-bindings: media: sun6i-a31-csi: Add MIPI CSI-2 input port Paul Kocialkowski
2022-02-07 16:03 ` Laurent Pinchart
2022-02-11 16:10 ` Paul Kocialkowski
2022-02-11 20:56 ` Laurent Pinchart
2022-02-14 16:10 ` Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 08/66] dt-bindings: media: Add Allwinner A31 MIPI CSI-2 bindings documentation Paul Kocialkowski
2022-02-07 16:09 ` Laurent Pinchart
2022-02-11 16:03 ` Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 09/66] media: sunxi: Add support for the A31 MIPI CSI-2 controller Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 10/66] MAINTAINERS: Add entry for the Allwinner A31 MIPI CSI-2 bridge driver Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 11/66] ARM: dts: sun8i: v3s: Add nodes for MIPI CSI-2 support Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 12/66] dt-bindings: media: Add Allwinner A83T MIPI CSI-2 bindings documentation Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 13/66] media: sunxi: Add support for the A83T MIPI CSI-2 controller Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 14/66] MAINTAINERS: Add entry for the Allwinner A83T MIPI CSI-2 bridge Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 15/66] ARM: dts: sun8i: a83t: Add MIPI CSI-2 controller node Paul Kocialkowski
2022-02-05 18:53 ` [PATCH NOT FOR MERGE v2 16/66] ARM: dts: sun8i: a83t: bananapi-m3: Enable MIPI CSI-2 with OV8865 Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 17/66] media: sun6i-csi: Define and use driver name and (reworked) description Paul Kocialkowski
2022-02-07 9:10 ` Maxime Ripard
2022-02-05 18:53 ` [PATCH v2 18/66] media: sun6i-csi: Refactor main driver data structures Paul Kocialkowski
2022-02-07 9:11 ` Maxime Ripard
2022-02-05 18:53 ` [PATCH v2 19/66] media: sun6i-csi: Grab bus clock instead of passing it to regmap Paul Kocialkowski
2022-02-09 9:20 ` Maxime Ripard
2022-02-05 18:53 ` [PATCH v2 20/66] media: sun6i-csi: Tidy up platform code Paul Kocialkowski
2022-02-07 9:13 ` Maxime Ripard
2022-02-05 18:53 ` [PATCH v2 21/66] media: sun6i-csi: Always set exclusive module clock rate Paul Kocialkowski
2022-02-07 9:14 ` Maxime Ripard
2022-02-11 16:29 ` Paul Kocialkowski [this message]
2022-02-14 16:31 ` Sakari Ailus
2022-03-01 15:39 ` Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 22/66] media: sun6i-csi: Use runtime pm for clocks and reset Paul Kocialkowski
2022-02-09 9:22 ` Maxime Ripard
2022-02-11 16:01 ` Paul Kocialkowski
2022-02-11 16:41 ` Maxime Ripard
2022-02-05 18:53 ` [PATCH v2 23/66] media: sun6i-csi: Tidy up v4l2 code Paul Kocialkowski
2022-02-07 9:55 ` Maxime Ripard
2022-02-05 18:53 ` [PATCH v2 24/66] media: sun6i-csi: Tidy up video code Paul Kocialkowski
2022-02-07 9:56 ` Maxime Ripard
2022-02-05 18:53 ` [PATCH v2 25/66] media: sun6i-csi: Pass and store csi device directly in " Paul Kocialkowski
2022-02-07 9:58 ` Maxime Ripard
2022-02-05 18:53 ` [PATCH v2 26/66] media: sun6i-csi: Register the media device after creation Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 27/66] media: sun6i-csi: Add media ops with link notify callback Paul Kocialkowski
2022-02-14 16:40 ` Sakari Ailus
2022-02-15 10:01 ` Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 28/66] media: sun6i-csi: Introduce and use video helper functions Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 29/66] media: sun6i-csi: Move csi buffer definition to main header file Paul Kocialkowski
2022-02-09 9:23 ` Maxime Ripard
2022-02-05 18:53 ` [PATCH v2 30/66] media: sun6i-csi: Add bridge v4l2 subdev with port management Paul Kocialkowski
2022-02-09 9:24 ` Maxime Ripard
2022-02-11 15:43 ` Paul Kocialkowski
2022-02-11 16:44 ` Maxime Ripard
2022-02-14 18:12 ` Sakari Ailus
2022-03-02 14:59 ` Paul Kocialkowski
2022-03-03 22:43 ` Sakari Ailus
2022-03-04 9:00 ` Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 31/66] media: sun6i-csi: Rename sun6i_video to sun6i_csi_capture Paul Kocialkowski
2022-02-09 9:25 ` Maxime Ripard
2022-02-05 18:53 ` [PATCH v2 32/66] media: sun6i-csi: Add capture state using vsync for page flip Paul Kocialkowski
2022-02-09 9:26 ` Maxime Ripard
2022-02-11 15:40 ` Paul Kocialkowski
2022-02-11 16:45 ` Maxime Ripard
2022-02-05 18:53 ` [PATCH v2 33/66] media: sun6i-csi: Rework register definitions, invert misleading fields Paul Kocialkowski
2022-02-09 9:39 ` Maxime Ripard
2022-02-11 15:17 ` Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 34/66] media: sun6i-csi: Add dimensions and format helpers to capture Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 35/66] media: sun6i-csi: Implement address configuration without indirection Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 36/66] media: sun6i-csi: Split stream sequences and irq code in capture Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 37/66] media: sun6i-csi: Move power management to runtime pm " Paul Kocialkowski
2022-02-14 18:30 ` Sakari Ailus
2022-02-15 9:56 ` Paul Kocialkowski
2022-02-15 10:04 ` Laurent Pinchart
2022-02-15 10:21 ` Paul Kocialkowski
2022-02-15 21:21 ` Sakari Ailus
2022-02-05 18:54 ` [PATCH v2 38/66] media: sun6i-csi: Move register configuration to capture Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 39/66] media: sun6i-csi: Rework capture format management with helper Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 40/66] media: sun6i-csi: Remove custom format helper and rework configure Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 41/66] media: sun6i-csi: Add bridge dimensions and format helpers Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 42/66] media: sun6i-csi: Get mbus code from bridge instead of storing it Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 43/66] media: sun6i-csi: Tidy capture configure code Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 44/66] media: sun6i-csi: Introduce bridge format structure, list and helper Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 45/66] media: sun6i-csi: Introduce capture " Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 46/66] media: sun6i-csi: Configure registers from format tables Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 47/66] media: sun6i-csi: Introduce format match structure, list and helper Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 48/66] media: sun6i-csi: Implement capture link validation with logic Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 49/66] media: sun6i-csi: Get bridge subdev directly in capture stream ops Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 50/66] media: sun6i-csi: Move hardware control to the bridge Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 51/66] media: sun6i-csi: Unset bridge source on capture streamon fail Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 52/66] media: sun6i-csi: Rename the capture video device to sun6i-csi-capture Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 53/66] media: sun6i-csi: Cleanup headers and includes, update copyright lines Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 54/66] media: sun6i-csi: Add support for MIPI CSI-2 to the bridge code Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 55/66] media: sun6i-csi: Only configure capture when streaming Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 56/66] media: sun6i-csi: Add extra checks to the interrupt routine Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 57/66] media: sun6i-csi: Request a shared interrupt Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 58/66] media: sun6i-csi: Detect the availability of the ISP Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 59/66] media: sun6i-csi: Add support for hooking to the isp devices Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 60/66] MAINTAINERS: Add myself as sun6i-csi maintainer and rename/move entry Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 61/66] dt-bindings: media: Add Allwinner A31 ISP bindings documentation Paul Kocialkowski
2022-02-07 15:51 ` Laurent Pinchart
2022-02-11 15:13 ` Rob Herring
2022-02-11 20:52 ` Laurent Pinchart
2022-02-14 16:28 ` Paul Kocialkowski
2022-02-14 17:10 ` Laurent Pinchart
2022-02-14 16:18 ` Paul Kocialkowski
2022-02-14 17:09 ` Laurent Pinchart
2022-02-15 10:10 ` Paul Kocialkowski
2022-02-15 10:16 ` Laurent Pinchart
2022-03-01 15:38 ` Paul Kocialkowski
2022-03-04 12:01 ` Laurent Pinchart
2022-03-04 13:57 ` Paul Kocialkowski
2022-03-04 14:09 ` Laurent Pinchart
2022-03-11 14:17 ` Paul Kocialkowski
2022-03-13 16:15 ` Laurent Pinchart
2022-03-15 9:49 ` Paul Kocialkowski
2022-03-15 10:13 ` Laurent Pinchart
2022-02-05 18:54 ` [PATCH v2 62/66] dt-bindings: media: sun6i-a31-csi: Add ISP output port Paul Kocialkowski
2022-02-07 16:04 ` Laurent Pinchart
2022-02-11 15:16 ` Rob Herring
2022-02-05 18:54 ` [PATCH v2 63/66] staging: media: Add support for the Allwinner A31 ISP Paul Kocialkowski
2022-02-07 16:16 ` Laurent Pinchart
2022-03-01 15:58 ` Paul Kocialkowski
2022-03-02 8:51 ` Laurent Pinchart
2022-03-02 13:23 ` Paul Kocialkowski
2022-03-02 13:33 ` Laurent Pinchart
2022-03-02 15:10 ` Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 64/66] MAINTAINERS: Add entry for the Allwinner A31 ISP driver Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 65/66] ARM: dts: sun8i: v3s: Add support for the ISP Paul Kocialkowski
2022-02-05 18:54 ` [PATCH NOT FOR MERGE v2 66/66] of: Mark interconnects property supplier as optional Paul Kocialkowski
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=YgaO8bfP4gKW8BM0@aptenodytes \
--to=paul.kocialkowski@bootlin.com \
--cc=devicetree@vger.kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=hans.verkuil@cisco.com \
--cc=helen.koike@collabora.com \
--cc=jernej.skrabec@gmail.com \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-phy@lists.infradead.org \
--cc=linux-staging@lists.linux.dev \
--cc=linux-sunxi@lists.linux.dev \
--cc=maxime@cerno.tech \
--cc=mchehab@kernel.org \
--cc=robh+dt@kernel.org \
--cc=sakari.ailus@linux.intel.com \
--cc=thomas.petazzoni@bootlin.com \
--cc=wens@csie.org \
--cc=yong.deng@magewell.com \
/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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).