From: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> To: Russell King - ARM Linux <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, devicetree <devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, linux-ide-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Oliver Schinagl <oliver-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>, Richard Zhu <Hong-Xing.Zhu-KZfg59tc24xl57MIdRCFDg@public.gmane.org>, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org, Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org Subject: Re: [RFC v3 06/13] ahci-platform: Add support for devices with more then 1 clock Date: Sun, 19 Jan 2014 20:20:19 +0100 [thread overview] Message-ID: <52DC2573.4020308@redhat.com> (raw) In-Reply-To: <20140119123854.GP15937-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org> Hi, On 01/19/2014 01:38 PM, Russell King - ARM Linux wrote: > On Sun, Jan 19, 2014 at 12:48:48AM +0100, Hans de Goede wrote: >> +static int ahci_enable_clks(struct device *dev, struct ahci_host_priv *hpriv) >> +{ >> + int c, rc; >> + >> + for (c = 0; c < AHCI_MAX_CLKS && hpriv->clks[c]; c++) { > > for (c = 0; c < AHCI_MAX_CLKS && !IS_ERR(hpriv->clks[c]); c++) { That won't work, hpriv->clks == NULL for clks entries which are not used, and before we get into a discussion about leaving any PTR_ERR returns from clk_get in-place. I've had similar discussions when doing similar changes to ohci-platform.c and ehci-platform.c and there the conclusion was that "if (clk)" is just much more nice to read then "if (!IS_ERR(clk))", I would like to avoid having the same discussion again. More-over all clk_foo() methods check for and will safely handle clk == NULL, and will crash and burn with clk == IS_ERR(clk). I've chosen to still explicitly check for clk == NULL as that makes it much more clear when reading the code that clk maybe NULL. >> + rc = clk_prepare_enable(hpriv->clks[c]); >> + if (rc) { >> + dev_err(dev, "clock prepare enable failed"); >> + goto disable_unprepare_clk; >> + } >> + } >> + return 0; >> + >> +disable_unprepare_clk: >> + while (--c >= 0) >> + clk_disable_unprepare(hpriv->clks[c]); >> + return rc; >> +} >> + >> +static void ahci_disable_clks(struct ahci_host_priv *hpriv) >> +{ >> + int c; >> + >> + for (c = AHCI_MAX_CLKS - 1; c >= 0; c--) >> + if (hpriv->clks[c]) > > if (!IS_ERR(hpriv->clks[c])) > Idem. >> + clk_disable_unprepare(hpriv->clks[c]); >> +} >> + >> +static void ahci_put_clks(struct ahci_host_priv *hpriv) >> +{ >> + int c; >> + >> + for (c = 0; c < AHCI_MAX_CLKS && hpriv->clks[c]; c++) > > for (c = 0; c < AHCI_MAX_CLKS && !IS_ERR(hpriv->clks[c]); c++) > Idem. >> + clk_put(hpriv->clks[c]); >> +} > > Better still for this one, consider using devm_clk_get() - in which case > the above is even more important to get right. The above depends on how errors are handled when calling clk_get (or variants), which in the case of this patch is such that hpriv->clks[i] == NULL when not present. > We really should have a devm_of_clk_get() too. Agreed, but that seems something for another patch-set, this one is big enough as is. Regards, Hans
WARNING: multiple messages have this Message-ID (diff)
From: hdegoede@redhat.com (Hans de Goede) To: linux-arm-kernel@lists.infradead.org Subject: [RFC v3 06/13] ahci-platform: Add support for devices with more then 1 clock Date: Sun, 19 Jan 2014 20:20:19 +0100 [thread overview] Message-ID: <52DC2573.4020308@redhat.com> (raw) In-Reply-To: <20140119123854.GP15937@n2100.arm.linux.org.uk> Hi, On 01/19/2014 01:38 PM, Russell King - ARM Linux wrote: > On Sun, Jan 19, 2014 at 12:48:48AM +0100, Hans de Goede wrote: >> +static int ahci_enable_clks(struct device *dev, struct ahci_host_priv *hpriv) >> +{ >> + int c, rc; >> + >> + for (c = 0; c < AHCI_MAX_CLKS && hpriv->clks[c]; c++) { > > for (c = 0; c < AHCI_MAX_CLKS && !IS_ERR(hpriv->clks[c]); c++) { That won't work, hpriv->clks == NULL for clks entries which are not used, and before we get into a discussion about leaving any PTR_ERR returns from clk_get in-place. I've had similar discussions when doing similar changes to ohci-platform.c and ehci-platform.c and there the conclusion was that "if (clk)" is just much more nice to read then "if (!IS_ERR(clk))", I would like to avoid having the same discussion again. More-over all clk_foo() methods check for and will safely handle clk == NULL, and will crash and burn with clk == IS_ERR(clk). I've chosen to still explicitly check for clk == NULL as that makes it much more clear when reading the code that clk maybe NULL. >> + rc = clk_prepare_enable(hpriv->clks[c]); >> + if (rc) { >> + dev_err(dev, "clock prepare enable failed"); >> + goto disable_unprepare_clk; >> + } >> + } >> + return 0; >> + >> +disable_unprepare_clk: >> + while (--c >= 0) >> + clk_disable_unprepare(hpriv->clks[c]); >> + return rc; >> +} >> + >> +static void ahci_disable_clks(struct ahci_host_priv *hpriv) >> +{ >> + int c; >> + >> + for (c = AHCI_MAX_CLKS - 1; c >= 0; c--) >> + if (hpriv->clks[c]) > > if (!IS_ERR(hpriv->clks[c])) > Idem. >> + clk_disable_unprepare(hpriv->clks[c]); >> +} >> + >> +static void ahci_put_clks(struct ahci_host_priv *hpriv) >> +{ >> + int c; >> + >> + for (c = 0; c < AHCI_MAX_CLKS && hpriv->clks[c]; c++) > > for (c = 0; c < AHCI_MAX_CLKS && !IS_ERR(hpriv->clks[c]); c++) > Idem. >> + clk_put(hpriv->clks[c]); >> +} > > Better still for this one, consider using devm_clk_get() - in which case > the above is even more important to get right. The above depends on how errors are handled when calling clk_get (or variants), which in the case of this patch is such that hpriv->clks[i] == NULL when not present. > We really should have a devm_of_clk_get() too. Agreed, but that seems something for another patch-set, this one is big enough as is. Regards, Hans
next prev parent reply other threads:[~2014-01-19 19:20 UTC|newest] Thread overview: 112+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-01-18 23:48 [RFC v3 00/13] ahci: add sunxi driver and cleanup imx driver Hans de Goede 2014-01-18 23:48 ` Hans de Goede [not found] ` <1390088935-7193-1-git-send-email-hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2014-01-18 23:48 ` [RFC v3 01/13] libahci: Allow drivers to override start_engine Hans de Goede 2014-01-18 23:48 ` Hans de Goede [not found] ` <1390088935-7193-2-git-send-email-hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2014-01-19 4:46 ` Tejun Heo 2014-01-19 4:46 ` Tejun Heo [not found] ` <20140119044643.GH3640-Gd/HAXX7CRxy/B6EtB590w@public.gmane.org> 2014-01-19 18:48 ` Hans de Goede 2014-01-19 18:48 ` Hans de Goede 2014-01-18 23:48 ` [RFC v3 02/13] sata-highbank: Remove unnecessary ahci_platform.h include Hans de Goede 2014-01-18 23:48 ` Hans de Goede [not found] ` <1390088935-7193-3-git-send-email-hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2014-01-19 11:15 ` Tejun Heo 2014-01-19 11:15 ` Tejun Heo 2014-01-18 23:48 ` [RFC v3 03/13] ahci-platform: Fix clk enable/disable unbalance on suspend/resume Hans de Goede 2014-01-18 23:48 ` Hans de Goede [not found] ` <1390088935-7193-4-git-send-email-hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2014-01-19 11:14 ` Tejun Heo 2014-01-19 11:14 ` Tejun Heo [not found] ` <20140119111412.GA11123-Gd/HAXX7CRxy/B6EtB590w@public.gmane.org> 2014-01-19 18:47 ` Hans de Goede 2014-01-19 18:47 ` Hans de Goede [not found] ` <52DC1DAA.3010403-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2014-01-19 19:15 ` Tejun Heo 2014-01-19 19:15 ` Tejun Heo [not found] ` <20140119191554.GB32165-9pTldWuhBndy/B6EtB590w@public.gmane.org> 2014-01-19 19:52 ` Hans de Goede 2014-01-19 19:52 ` Hans de Goede [not found] ` <52DC2CF5.5080109-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2014-01-20 12:26 ` Tejun Heo 2014-01-20 12:26 ` Tejun Heo 2014-01-18 23:48 ` [RFC v3 04/13] ahci-platform: Undo pdata->resume on resume failure Hans de Goede 2014-01-18 23:48 ` Hans de Goede [not found] ` <1390088935-7193-5-git-send-email-hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2014-01-19 11:27 ` Tejun Heo 2014-01-19 11:27 ` Tejun Heo [not found] ` <20140119112737.GC11123-Gd/HAXX7CRxy/B6EtB590w@public.gmane.org> 2014-01-19 18:40 ` Hans de Goede 2014-01-19 18:40 ` Hans de Goede [not found] ` <52DC1C15.1030107-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2014-01-19 19:13 ` Tejun Heo 2014-01-19 19:13 ` Tejun Heo [not found] ` <20140119191349.GA32165-9pTldWuhBndy/B6EtB590w@public.gmane.org> 2014-01-19 19:34 ` Hans de Goede 2014-01-19 19:34 ` Hans de Goede [not found] ` <52DC28DB.7070804-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2014-01-19 19:42 ` Tejun Heo 2014-01-19 19:42 ` Tejun Heo [not found] ` <20140119194226.GE32165-9pTldWuhBndy/B6EtB590w@public.gmane.org> 2014-01-19 19:53 ` Hans de Goede 2014-01-19 19:53 ` Hans de Goede 2014-01-18 23:48 ` [RFC v3 05/13] ahci-platform: Pass ahci_host_priv ptr to ahci_platform_data init method Hans de Goede 2014-01-18 23:48 ` Hans de Goede [not found] ` <1390088935-7193-6-git-send-email-hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2014-01-19 11:30 ` Tejun Heo 2014-01-19 11:30 ` Tejun Heo [not found] ` <20140119113051.GD11123-Gd/HAXX7CRxy/B6EtB590w@public.gmane.org> 2014-01-19 18:51 ` Hans de Goede 2014-01-19 18:51 ` Hans de Goede [not found] ` <52DC1EC4.3090807-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2014-01-19 19:17 ` Tejun Heo 2014-01-19 19:17 ` Tejun Heo [not found] ` <20140119191706.GC32165-9pTldWuhBndy/B6EtB590w@public.gmane.org> 2014-01-19 19:56 ` Hans de Goede 2014-01-19 19:56 ` Hans de Goede [not found] ` <52DC2DD9.7070403-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2014-01-20 12:28 ` Tejun Heo 2014-01-20 12:28 ` Tejun Heo 2014-01-18 23:48 ` [RFC v3 06/13] ahci-platform: Add support for devices with more then 1 clock Hans de Goede 2014-01-18 23:48 ` Hans de Goede [not found] ` <1390088935-7193-7-git-send-email-hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2014-01-19 12:38 ` Russell King - ARM Linux 2014-01-19 12:38 ` Russell King - ARM Linux [not found] ` <20140119123854.GP15937-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org> 2014-01-19 19:20 ` Hans de Goede [this message] 2014-01-19 19:20 ` Hans de Goede 2014-01-18 23:48 ` [RFC v3 07/13] ahci-platform: Add support for an optional regulator for sata-target power Hans de Goede 2014-01-18 23:48 ` Hans de Goede 2014-01-18 23:48 ` [RFC v3 08/13] ahci-platform: Allow specifying platform_data through of_device_id Hans de Goede 2014-01-18 23:48 ` Hans de Goede [not found] ` <1390088935-7193-9-git-send-email-hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2014-01-19 11:38 ` Tejun Heo 2014-01-19 11:38 ` Tejun Heo 2014-01-19 12:30 ` Russell King - ARM Linux 2014-01-19 12:30 ` Russell King - ARM Linux [not found] ` <20140119123055.GO15937-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org> 2014-01-19 13:19 ` Tejun Heo 2014-01-19 13:19 ` Tejun Heo [not found] ` <20140119113838.GE11123-Gd/HAXX7CRxy/B6EtB590w@public.gmane.org> 2014-01-19 18:56 ` Hans de Goede 2014-01-19 18:56 ` Hans de Goede [not found] ` <52DC1FF5.80101-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2014-01-19 19:22 ` Tejun Heo 2014-01-19 19:22 ` Tejun Heo 2014-01-20 8:24 ` Sascha Hauer 2014-01-20 8:24 ` Sascha Hauer [not found] ` <20140120082438.GH16215-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> 2014-01-20 8:35 ` Hans de Goede 2014-01-20 8:35 ` Hans de Goede 2014-01-20 9:09 ` Sascha Hauer 2014-01-20 9:09 ` Sascha Hauer [not found] ` <20140120090950.GI16215-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> 2014-01-20 9:17 ` Hans de Goede 2014-01-20 9:17 ` Hans de Goede 2014-01-20 9:57 ` Sascha Hauer 2014-01-20 9:57 ` Sascha Hauer 2014-01-18 23:48 ` [RFC v3 09/13] ARM: sunxi: Add support for Allwinner SUNXi SoCs sata to ahci_platform Hans de Goede 2014-01-18 23:48 ` Hans de Goede 2014-01-19 12:22 ` Russell King - ARM Linux 2014-01-19 12:22 ` Russell King - ARM Linux [not found] ` <20140119122216.GM15937-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org> 2014-01-19 19:07 ` Hans de Goede 2014-01-19 19:07 ` Hans de Goede 2014-01-19 19:56 ` Russell King - ARM Linux 2014-01-19 19:56 ` Russell King - ARM Linux [not found] ` <20140119195642.GU15937-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org> 2014-01-19 20:01 ` Hans de Goede 2014-01-19 20:01 ` Hans de Goede 2014-01-18 23:48 ` [RFC v3 10/13] ahci_imx: Adjust for ahci_platform managing the clocks Hans de Goede 2014-01-18 23:48 ` Hans de Goede [not found] ` <1390088935-7193-11-git-send-email-hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2014-01-19 12:41 ` Russell King - ARM Linux 2014-01-19 12:41 ` Russell King - ARM Linux [not found] ` <20140119124134.GQ15937-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org> 2014-01-19 19:30 ` Hans de Goede 2014-01-19 19:30 ` Hans de Goede [not found] ` <52DC27DD.5030309-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2014-01-19 19:32 ` Russell King - ARM Linux 2014-01-19 19:32 ` Russell King - ARM Linux [not found] ` <20140119193258.GT15937-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org> 2014-01-19 19:38 ` Hans de Goede 2014-01-19 19:38 ` Hans de Goede 2014-01-18 23:48 ` [RFC v3 11/13] ahci-imx: Don't create a nested platform device from probe Hans de Goede 2014-01-18 23:48 ` Hans de Goede [not found] ` <1390088935-7193-12-git-send-email-hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2014-01-19 12:25 ` Russell King - ARM Linux 2014-01-19 12:25 ` Russell King - ARM Linux [not found] ` <20140119122540.GN15937-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org> 2014-01-19 19:08 ` Hans de Goede 2014-01-19 19:08 ` Hans de Goede 2014-01-18 23:48 ` [RFC v3 12/13] ARM: sun4i: dts: Add ahci / sata support Hans de Goede 2014-01-18 23:48 ` Hans de Goede 2014-01-18 23:48 ` [RFC v3 13/13] ARM: sun7i: " Hans de Goede 2014-01-18 23:48 ` Hans de Goede 2014-01-19 9:52 ` [RFC v3 00/13] ahci: add sunxi driver and cleanup imx driver Russell King - ARM Linux 2014-01-19 9:52 ` Russell King - ARM Linux
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=52DC2573.4020308@redhat.com \ --to=hdegoede-h+wxahxf7alqt0dzr+alfa@public.gmane.org \ --cc=Hong-Xing.Zhu-KZfg59tc24xl57MIdRCFDg@public.gmane.org \ --cc=devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org \ --cc=linux-ide-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org \ --cc=linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org \ --cc=maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org \ --cc=oliver-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org \ --cc=tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org \ /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.