[{"id":2912277,"web_url":"http://patchwork.ozlabs.org/comment/2912277/","msgid":"<3bf20887-6e2f-41f4-e4ec-5c2278f6cb18@opensource.wdc.com>","list_archive_url":null,"date":"2022-06-14T08:22:02","subject":"Re: [PATCH v4 07/23] ata: libahci_platform: Convert to using devm\n bulk clocks API","submitter":{"id":82259,"url":"http://patchwork.ozlabs.org/api/people/82259/","name":"Damien Le Moal","email":"damien.lemoal@opensource.wdc.com"},"content":"On 6/10/22 17:17, Serge Semin wrote:\n> In order to simplify the clock-related code there is a way to convert the\n> current fixed clocks array into using the common bulk clocks kernel API\n> with dynamic set of the clock handlers and device-managed clock-resource\n> tracking. It's a bit tricky due to the complication coming from the\n> requirement to support the platforms (da850, spear13xx) with the\n> non-OF-based clock source, but still doable.\n> \n> Before this modification there are two methods have been used to get the\n> clocks connected to an AHCI device: clk_get() - to get the very first\n> clock in the list and of_clk_get() - to get the rest of them. Basically\n> the platforms with non-OF-based clocks definition could specify only a\n> single reference clock source. The platforms with OF-hw clocks have been\n> luckier and could setup up to AHCI_MAX_CLKS clocks. Such semantic can be\n> retained with using devm_clk_bulk_get_all() to retrieve the clocks defined\n> via the DT firmware and devm_clk_get_optional() otherwise. In both cases\n> using the device-managed version of the methods will cause the automatic\n> resources deallocation on the AHCI device removal event. The only\n> complicated part in the suggested approach is the explicit allocation and\n> initialization of the clk_bulk_data structure instance for the non-OF\n> reference clocks. It's required in order to use the Bulk Clocks API for\n> the both denoted cases of the clocks definition.\n> \n> Note aside with the clock-related code reduction and natural\n> simplification, there are several bonuses the suggested modification\n> provides. First of all the limitation of having no greater than\n> AHCI_MAX_CLKS clocks is now removed, since the devm_clk_bulk_get_all()\n> method will allocate as many reference clocks data descriptors as there\n> are clocks specified for the device. Secondly the clock names are\n> auto-detected. So the LLDD (glue) drivers can make sure that the required\n> clocks are specified just by checking the clock IDs in the clk_bulk_data\n> array. Thirdly using the handy Bulk Clocks kernel API improves the\n> clocks-handling code readability. And the last but not least this\n> modification implements a true optional clocks support to the\n> ahci_platform_get_resources() method. Indeed the previous clocks getting\n> procedure just stopped getting the clocks on any errors (aside from\n> non-critical -EPROBE_DEFER) in a way so the callee wasn't even informed\n> about abnormal loop termination. The new implementation lacks of such\n> problem. The ahci_platform_get_resources() will return an error code if\n> the corresponding clocks getting method ends execution abnormally.\n> \n> Signed-off-by: Serge Semin \n> Reviewed-by: Hannes Reinecke \n> \n> ---\n> \n> Changelog v2:\n> - Convert to checking the error-case first in the devm_clk_bulk_get_all()\n> method invocation. (@Damien)\n> - Fix some grammar mistakes in the comments.\n> ---\n> drivers/ata/ahci.h | 4 +-\n> drivers/ata/libahci_platform.c | 84 ++++++++++++++++------------------\n> 2 files changed, 41 insertions(+), 47 deletions(-)\n> \n> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h\n> index ad11a4c52fbe..c3770a19781b 100644\n> --- a/drivers/ata/ahci.h\n> +++ b/drivers/ata/ahci.h\n> @@ -38,7 +38,6 @@\n> \n> enum {\n> \tAHCI_MAX_PORTS\t\t= 32,\n> -\tAHCI_MAX_CLKS\t\t= 5,\n> \tAHCI_MAX_SG\t\t= 168, /* hardware max is 64K */\n> \tAHCI_DMA_BOUNDARY\t= 0xffffffff,\n> \tAHCI_MAX_CMDS\t\t= 32,\n> @@ -339,7 +338,8 @@ struct ahci_host_priv {\n> \tu32\t\t\tem_msg_type;\t/* EM message type */\n> \tu32\t\t\tremapped_nvme;\t/* NVMe remapped device count */\n> \tbool\t\t\tgot_runtime_pm; /* Did we do pm_runtime_get? */\n> -\tstruct clk\t\t*clks[AHCI_MAX_CLKS]; /* Optional */\n> +\tunsigned int\t\tn_clks;\n> +\tstruct clk_bulk_data\t*clks;\t\t/* Optional */\n> \tstruct reset_control\t*rsts;\t\t/* Optional */\n> \tstruct regulator\t**target_pwrs;\t/* Optional */\n> \tstruct regulator\t*ahci_regulator;/* Optional */\n> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c\n> index 1e9e825d6cc5..814804582d1d 100644\n> --- a/drivers/ata/libahci_platform.c\n> +++ b/drivers/ata/libahci_platform.c\n> @@ -8,6 +8,7 @@\n> * Anton Vorontsov \n> */\n> \n> +#include \n> #include \n> #include \n> #include \n> @@ -97,28 +98,14 @@ EXPORT_SYMBOL_GPL(ahci_platform_disable_phys);\n> * ahci_platform_enable_clks - Enable platform clocks\n> * @hpriv: host private area to store config values\n> *\n> - * This function enables all the clks found in hpriv->clks, starting at\n> - * index 0. If any clk fails to enable it disables all the clks already\n> - * enabled in reverse order, and then returns an error.\n> + * This function enables all the clks found for the AHCI device.\n> *\n> * RETURNS:\n> * 0 on success otherwise a negative error code\n> */\n> int ahci_platform_enable_clks(struct ahci_host_priv *hpriv)\n> {\n> -\tint c, rc;\n> -\n> -\tfor (c = 0; c < AHCI_MAX_CLKS && hpriv->clks[c]; c++) {\n> -\t\trc = clk_prepare_enable(hpriv->clks[c]);\n> -\t\tif (rc)\n> -\t\t\tgoto disable_unprepare_clk;\n> -\t}\n> -\treturn 0;\n> -\n> -disable_unprepare_clk:\n> -\twhile (--c >= 0)\n> -\t\tclk_disable_unprepare(hpriv->clks[c]);\n> -\treturn rc;\n> +\treturn clk_bulk_prepare_enable(hpriv->n_clks, hpriv->clks);\n> }\n> EXPORT_SYMBOL_GPL(ahci_platform_enable_clks);\n> \n> @@ -126,16 +113,13 @@ EXPORT_SYMBOL_GPL(ahci_platform_enable_clks);\n> * ahci_platform_disable_clks - Disable platform clocks\n> * @hpriv: host private area to store config values\n> *\n> - * This function disables all the clks found in hpriv->clks, in reverse\n> - * order of ahci_platform_enable_clks (starting at the end of the array).\n> + * This function disables all the clocks enabled before\n> + * (bulk-clocks-disable function is supposed to do that in reverse\n> + * from the enabling procedure order).\n> */\n> void ahci_platform_disable_clks(struct ahci_host_priv *hpriv)\n> {\n> -\tint c;\n> -\n> -\tfor (c = AHCI_MAX_CLKS - 1; c >= 0; c--)\n> -\t\tif (hpriv->clks[c])\n> -\t\t\tclk_disable_unprepare(hpriv->clks[c]);\n> +\tclk_bulk_disable_unprepare(hpriv->n_clks, hpriv->clks);\n> }\n> EXPORT_SYMBOL_GPL(ahci_platform_disable_clks);\n> \n> @@ -292,8 +276,6 @@ static void ahci_platform_put_resources(struct device *dev, void *res)\n> \t\tpm_runtime_disable(dev);\n> \t}\n> \n> -\tfor (c = 0; c < AHCI_MAX_CLKS && hpriv->clks[c]; c++)\n> -\t\tclk_put(hpriv->clks[c]);\n> \t/*\n> \t * The regulators are tied to child node device and not to the\n> \t * SATA device itself. So we can't use devm for automatically\n> @@ -374,8 +356,8 @@ static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port,\n> * 1) mmio registers (IORESOURCE_MEM 0, mandatory)\n> * 2) regulator for controlling the targets power (optional)\n> * regulator for controlling the AHCI controller (optional)\n> - * 3) 0 - AHCI_MAX_CLKS clocks, as specified in the devs devicetree node,\n> - * or for non devicetree enabled platforms a single clock\n> + * 3) all clocks specified in the devicetree node, or a single\n> + * clock for non-OF platforms (optional)\n> * 4) resets, if flags has AHCI_PLATFORM_GET_RESETS (optional)\n> * 5) phys (optional)\n> *\n> @@ -385,11 +367,10 @@ static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port,\n> struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,\n> \t\t\t\t\t\t unsigned int flags)\n> {\n> +\tint child_nodes, rc = -ENOMEM, enabled_ports = 0;\n> \tstruct device *dev = &pdev->dev;\n> \tstruct ahci_host_priv *hpriv;\n> -\tstruct clk *clk;\n> \tstruct device_node *child;\n> -\tint i, enabled_ports = 0, rc = -ENOMEM, child_nodes;\n> \tu32 mask_port_map = 0;\n> \n> \tif (!devres_open_group(dev, NULL, GFP_KERNEL))\n> @@ -415,25 +396,38 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,\n> \t\tgoto err_out;\n> \t}\n> \n> -\tfor (i = 0; i < AHCI_MAX_CLKS; i++) {\n> +\t/*\n> +\t * Bulk clocks getting procedure can fail to find any clock due to\n> +\t * running on a non-OF platform or due to the clocks being defined in\n> +\t * bypass of the DT firmware (like da850, spear13xx). In that case we\n> +\t * fallback to getting a single clock source right from the dev clocks\n> +\t * list.\n> +\t */\n> +\trc = devm_clk_bulk_get_all(dev, &hpriv->clks);\n> +\tif (rc < 0)\n> +\t\tgoto err_out;\n> +\n> +\tif (rc > 0) {\n> +\t\t/* Got clocks in bulk */\n> +\t\thpriv->n_clks = rc;\n> +\t} else {\n> \t\t/*\n> -\t\t * For now we must use clk_get(dev, NULL) for the first clock,\n> -\t\t * because some platforms (da850, spear13xx) are not yet\n> -\t\t * converted to use devicetree for clocks. For new platforms\n> -\t\t * this is equivalent to of_clk_get(dev->of_node, 0).\n> +\t\t * No clock bulk found: fallback to manually getting\n> +\t\t * the optional clock.\n> \t\t */\n> -\t\tif (i == 0)\n> -\t\t\tclk = clk_get(dev, NULL);\n> -\t\telse\n> -\t\t\tclk = of_clk_get(dev->of_node, i);\n> -\n> -\t\tif (IS_ERR(clk)) {\n> -\t\t\trc = PTR_ERR(clk);\n> -\t\t\tif (rc == -EPROBE_DEFER)\n> -\t\t\t\tgoto err_out;\n> -\t\t\tbreak;\n> +\t\thpriv->clks = devm_kzalloc(dev, sizeof(*hpriv->clks), GFP_KERNEL);\n> +\t\tif (!hpriv->clks) {\n> +\t\t\trc = -ENOMEM;\n> +\t\t\tgoto err_out;\n> +\t\t}\n> +\t\thpriv->clks->clk = devm_clk_get_optional(dev, NULL);\n> +\t\tif (IS_ERR(hpriv->clks->clk)) {\n> +\t\t\trc = PTR_ERR(hpriv->clks->clk);\n> +\t\t\tgoto err_out;\n> +\t\t} else if (hpriv->clks->clk) {\n\nNit: the else is not needed here.\n\n> +\t\t\thpriv->clks->id = __clk_get_name(hpriv->clks->clk);\n> +\t\t\thpriv->n_clks = 1;\n> \t\t}\n> -\t\thpriv->clks[i] = clk;\n> \t}\n> \n> \thpriv->ahci_regulator = devm_regulator_get(dev, \"ahci\");","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["bilbo.ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n unprotected) header.d=wdc.com header.i=@wdc.com header.a=rsa-sha256\n header.s=dkim.wdc.com header.b=oCLrjA+G;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=opensource.wdc.com header.i=@opensource.wdc.com\n header.a=rsa-sha256 header.s=dkim header.b=LoMhA/ga;\n\tdkim-atps=neutral","ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=2620:137:e000::1:20; helo=out1.vger.email;\n envelope-from=linux-ide-owner@vger.kernel.org; receiver=)","usg-ed-osssrv.wdc.com (amavisd-new); dkim=pass\n reason=\"pass (just generated, assumed good)\"\n header.d=opensource.wdc.com"],"Received":["from out1.vger.email (out1.vger.email [IPv6:2620:137:e000::1:20])\n\tby bilbo.ozlabs.org (Postfix) with ESMTP id 4LMhHs5MWxz9sGH\n\tfor ; Tue, 14 Jun 2022 18:22:17 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n id S240348AbiFNIWP (ORCPT );\n Tue, 14 Jun 2022 04:22:15 -0400","from lindbergh.monkeyblade.net ([23.128.96.19]:44692 \"EHLO\n lindbergh.monkeyblade.net\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n with ESMTP id S240255AbiFNIWN (ORCPT\n ); Tue, 14 Jun 2022 04:22:13 -0400","from esa4.hgst.iphmx.com (esa4.hgst.iphmx.com [216.71.154.42])\n by lindbergh.monkeyblade.net (Postfix) with ESMTPS id CB0B9419A9\n for ; Tue, 14 Jun 2022 01:22:11 -0700 (PDT)","from h199-255-45-15.hgst.com (HELO uls-op-cesaep02.wdc.com)\n ([199.255.45.15])\n by ob1.hgst.iphmx.com with ESMTP; 14 Jun 2022 16:22:09 +0800","from uls-op-cesaip01.wdc.com ([10.248.3.36])\n by uls-op-cesaep02.wdc.com with ESMTP/TLS/ECDHE-RSA-AES128-GCM-SHA256;\n 14 Jun 2022 00:40:44 -0700","from usg-ed-osssrv.wdc.com ([10.3.10.180])\n by uls-op-cesaip01.wdc.com with ESMTP/TLS/ECDHE-RSA-AES128-GCM-SHA256;\n 14 Jun 2022 01:22:09 -0700","from usg-ed-osssrv.wdc.com (usg-ed-osssrv.wdc.com [127.0.0.1])\n by usg-ed-osssrv.wdc.com (Postfix) with ESMTP id 4LMhHh3vyFz1SVnx\n for ; Tue, 14 Jun 2022 01:22:08 -0700 (PDT)","from usg-ed-osssrv.wdc.com ([127.0.0.1])\n by usg-ed-osssrv.wdc.com (usg-ed-osssrv.wdc.com [127.0.0.1])\n (amavisd-new, port 10026)\n with ESMTP id diz94e7RCtTY for ;\n Tue, 14 Jun 2022 01:22:06 -0700 (PDT)","from [10.225.163.77] (unknown [10.225.163.77])\n by usg-ed-osssrv.wdc.com (Postfix) with ESMTPSA id 4LMhHc126Gz1Rvlc;\n Tue, 14 Jun 2022 01:22:03 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=simple/simple;\n d=wdc.com; i=@wdc.com; q=dns/txt; s=dkim.wdc.com;\n t=1655194931; x=1686730931;\n h=message-id:date:mime-version:subject:to:cc:references:\n from:in-reply-to:content-transfer-encoding;\n bh=AaMT0IGd7k5gzjqCn0qpz01EgHi4ZkJG3tpDWeFzWJk=;\n b=oCLrjA+GUpt9MwCirbeDtMaj/9UDTiUpJgIoOIJDJM6x9tDXyXyP4NR3\n sq+N2mwU/sw3aDes3vToXJHsgFs6nVAcxFrh5Uh6sfsbc6/i/f1aSQO2V\n ovBjue/kll+eA6YLkcHlw8NBrpNATwQSfzpGR3hnFlRbewQxRFQRcWN60\n Y47oT9NhJ244Rm41E5BoY8AtyeT/paeHQ8mHK9HSheAEVG1KxUWm9YNuc\n jGBqrTXYY2KgagjOawuhca/T5STARlWlA8XbtpwdGKiZl6CHLS1GG9FTF\n wlQQds0LakRDQBwPr7QIIqorDNquX9tblusONhHWkYiDYuXyUM24Az2yK\n A==;","v=1; a=rsa-sha256; c=relaxed/simple; d=\n opensource.wdc.com; h=content-transfer-encoding:content-type\n :in-reply-to:organization:from:references:to:content-language\n :subject:user-agent:mime-version:date:message-id; s=dkim; t=\n 1655194926; x=1657786927; bh=AaMT0IGd7k5gzjqCn0qpz01EgHi4ZkJG3tp\n DWeFzWJk=; b=LoMhA/ga+XExP07R0kpt+GcAYVZb/LT0oQXer45OGt/tMEdOkLU\n sYwHxf3gcu81RNxlmvgK2EQ40f9iocie2R0eSR3xyrmjg0FJpchadV9jHos8nkuj\n 8rMksvwOsWQqQmdcFvEUNQ2oJUQTOSSSeNoXOnNSvu2YWyczMnFYtJ075aOVd/L+\n C1GinbGuMqN8Fdfl/zgieNP6r1d9MINaTwLxEdMGruVIzg7lCItHGzD61uvUcCRe\n bEvxegGhi1ghXaKmN7jIhx2TkSpJxXNvO6rPlXrs/q1EPASPWQLlF1UMyfITyYqm\n GfVBwCOiP+TRe4J5n+3vbsQfvaTQ6zStocw=="],"X-IronPort-AV":"E=Sophos;i=\"5.91,299,1647273600\";\n d=\"scan'208\";a=\"201810707\"","IronPort-SDR":["\n d2XAZhTUsNB8Y8ejzoEemLdDgccR8wGrBLSICbjUpF48rV/+XL+EFoPVPT/BFvAexE/AIl+j2k\n J2k32xonCBOTRyEWje97j+IOWB9Qs4GnoNsFliiN0Qx+6e+xS5q7oA1mMGfE98xKtU3JhFh2+5\n aBIvwQ8DsoL+0ajHF89jrPRVTamwG1lipsIAkJ3cQZf5rtZPSwxjygCXIXdtV4OGYXMlbQW4wB\n RTlsw4k0WrBdRQZ3GmJOsXanGNU6O9LN3bFt/XOHuHt8WSkBvl7aP7uP8aadLvRW4auD9HuCLL\n kpU00+X+nlNtwC8pOotDUsHd","\n luroB9XD2YfBZUPiH21N4aakm3zgP+Eu1KtiB2ZIshB6Sipd8EuOPdptYqdts4VV3zekPwpIKa\n slHC0TO9fve7a+pebnNoQsq3fYmYjPi+/ZgTqvaE2ru1ryQT+cI1LDs8MgOLEYG3LMDbT9fKb/\n jAJOEbeLjvJhCT+onuCejTWE21XW7v+fEA11rzMiKqmlee9z2FUDGKsl18PJcaMNSXrmrn1pLf\n 4yGVedv6+pUrKMjUmlDNb8dk99oJ3KIxzXNIsGHl7hdwa9KU7niH4uvwayd4F0AOmzzgggH/sq\n Ors="],"WDCIronportException":"Internal","X-Virus-Scanned":"amavisd-new at usg-ed-osssrv.wdc.com","Message-ID":"<3bf20887-6e2f-41f4-e4ec-5c2278f6cb18@opensource.wdc.com>","Date":"Tue, 14 Jun 2022 17:22:02 +0900","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n Thunderbird/91.10.0","Subject":"Re: [PATCH v4 07/23] ata: libahci_platform: Convert to using devm\n bulk clocks API","Content-Language":"en-US","To":"Serge Semin ,\n Hans de Goede ,\n Jens Axboe , Hannes Reinecke ","Cc":"Serge Semin ,\n Alexey Malahov ,\n Pavel Parkhomenko ,\n Rob Herring , linux-ide@vger.kernel.org,\n linux-kernel@vger.kernel.org, devicetree@vger.kernel.org","References":"<20220610081801.11854-1-Sergey.Semin@baikalelectronics.ru>\n <20220610081801.11854-8-Sergey.Semin@baikalelectronics.ru>","From":"Damien Le Moal ","Organization":"Western Digital Research","In-Reply-To":"<20220610081801.11854-8-Sergey.Semin@baikalelectronics.ru>","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"7bit","X-Spam-Status":"No, score=-5.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,\n DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_MED,\n SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable\n autolearn_force=no version=3.4.6","X-Spam-Checker-Version":"SpamAssassin 3.4.6 (2021-04-09) on\n lindbergh.monkeyblade.net","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-ide@vger.kernel.org"}},{"id":2913711,"web_url":"http://patchwork.ozlabs.org/comment/2913711/","msgid":"<20220615204509.siz54h4vbgvb3zkm@mobilestation>","list_archive_url":null,"date":"2022-06-15T20:45:09","subject":"Re: [PATCH v4 07/23] ata: libahci_platform: Convert to using devm\n bulk clocks API","submitter":{"id":70038,"url":"http://patchwork.ozlabs.org/api/people/70038/","name":"Serge Semin","email":"fancer.lancer@gmail.com"},"content":"On Tue, Jun 14, 2022 at 05:22:02PM +0900, Damien Le Moal wrote:\n> On 6/10/22 17:17, Serge Semin wrote:\n> > In order to simplify the clock-related code there is a way to convert the\n> > current fixed clocks array into using the common bulk clocks kernel API\n> > with dynamic set of the clock handlers and device-managed clock-resource\n> > tracking. It's a bit tricky due to the complication coming from the\n> > requirement to support the platforms (da850, spear13xx) with the\n> > non-OF-based clock source, but still doable.\n> > \n> > Before this modification there are two methods have been used to get the\n> > clocks connected to an AHCI device: clk_get() - to get the very first\n> > clock in the list and of_clk_get() - to get the rest of them. Basically\n> > the platforms with non-OF-based clocks definition could specify only a\n> > single reference clock source. The platforms with OF-hw clocks have been\n> > luckier and could setup up to AHCI_MAX_CLKS clocks. Such semantic can be\n> > retained with using devm_clk_bulk_get_all() to retrieve the clocks defined\n> > via the DT firmware and devm_clk_get_optional() otherwise. In both cases\n> > using the device-managed version of the methods will cause the automatic\n> > resources deallocation on the AHCI device removal event. The only\n> > complicated part in the suggested approach is the explicit allocation and\n> > initialization of the clk_bulk_data structure instance for the non-OF\n> > reference clocks. It's required in order to use the Bulk Clocks API for\n> > the both denoted cases of the clocks definition.\n> > \n> > Note aside with the clock-related code reduction and natural\n> > simplification, there are several bonuses the suggested modification\n> > provides. First of all the limitation of having no greater than\n> > AHCI_MAX_CLKS clocks is now removed, since the devm_clk_bulk_get_all()\n> > method will allocate as many reference clocks data descriptors as there\n> > are clocks specified for the device. Secondly the clock names are\n> > auto-detected. So the LLDD (glue) drivers can make sure that the required\n> > clocks are specified just by checking the clock IDs in the clk_bulk_data\n> > array. Thirdly using the handy Bulk Clocks kernel API improves the\n> > clocks-handling code readability. And the last but not least this\n> > modification implements a true optional clocks support to the\n> > ahci_platform_get_resources() method. Indeed the previous clocks getting\n> > procedure just stopped getting the clocks on any errors (aside from\n> > non-critical -EPROBE_DEFER) in a way so the callee wasn't even informed\n> > about abnormal loop termination. The new implementation lacks of such\n> > problem. The ahci_platform_get_resources() will return an error code if\n> > the corresponding clocks getting method ends execution abnormally.\n> > \n> > Signed-off-by: Serge Semin \n> > Reviewed-by: Hannes Reinecke \n> > \n> > ---\n> > \n> > Changelog v2:\n> > - Convert to checking the error-case first in the devm_clk_bulk_get_all()\n> > method invocation. (@Damien)\n> > - Fix some grammar mistakes in the comments.\n> > ---\n> > drivers/ata/ahci.h | 4 +-\n> > drivers/ata/libahci_platform.c | 84 ++++++++++++++++------------------\n> > 2 files changed, 41 insertions(+), 47 deletions(-)\n> > \n> > diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h\n> > index ad11a4c52fbe..c3770a19781b 100644\n> > --- a/drivers/ata/ahci.h\n> > +++ b/drivers/ata/ahci.h\n> > @@ -38,7 +38,6 @@\n> > \n> > enum {\n> > \tAHCI_MAX_PORTS\t\t= 32,\n> > -\tAHCI_MAX_CLKS\t\t= 5,\n> > \tAHCI_MAX_SG\t\t= 168, /* hardware max is 64K */\n> > \tAHCI_DMA_BOUNDARY\t= 0xffffffff,\n> > \tAHCI_MAX_CMDS\t\t= 32,\n> > @@ -339,7 +338,8 @@ struct ahci_host_priv {\n> > \tu32\t\t\tem_msg_type;\t/* EM message type */\n> > \tu32\t\t\tremapped_nvme;\t/* NVMe remapped device count */\n> > \tbool\t\t\tgot_runtime_pm; /* Did we do pm_runtime_get? */\n> > -\tstruct clk\t\t*clks[AHCI_MAX_CLKS]; /* Optional */\n> > +\tunsigned int\t\tn_clks;\n> > +\tstruct clk_bulk_data\t*clks;\t\t/* Optional */\n> > \tstruct reset_control\t*rsts;\t\t/* Optional */\n> > \tstruct regulator\t**target_pwrs;\t/* Optional */\n> > \tstruct regulator\t*ahci_regulator;/* Optional */\n> > diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c\n> > index 1e9e825d6cc5..814804582d1d 100644\n> > --- a/drivers/ata/libahci_platform.c\n> > +++ b/drivers/ata/libahci_platform.c\n> > @@ -8,6 +8,7 @@\n> > * Anton Vorontsov \n> > */\n> > \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -97,28 +98,14 @@ EXPORT_SYMBOL_GPL(ahci_platform_disable_phys);\n> > * ahci_platform_enable_clks - Enable platform clocks\n> > * @hpriv: host private area to store config values\n> > *\n> > - * This function enables all the clks found in hpriv->clks, starting at\n> > - * index 0. If any clk fails to enable it disables all the clks already\n> > - * enabled in reverse order, and then returns an error.\n> > + * This function enables all the clks found for the AHCI device.\n> > *\n> > * RETURNS:\n> > * 0 on success otherwise a negative error code\n> > */\n> > int ahci_platform_enable_clks(struct ahci_host_priv *hpriv)\n> > {\n> > -\tint c, rc;\n> > -\n> > -\tfor (c = 0; c < AHCI_MAX_CLKS && hpriv->clks[c]; c++) {\n> > -\t\trc = clk_prepare_enable(hpriv->clks[c]);\n> > -\t\tif (rc)\n> > -\t\t\tgoto disable_unprepare_clk;\n> > -\t}\n> > -\treturn 0;\n> > -\n> > -disable_unprepare_clk:\n> > -\twhile (--c >= 0)\n> > -\t\tclk_disable_unprepare(hpriv->clks[c]);\n> > -\treturn rc;\n> > +\treturn clk_bulk_prepare_enable(hpriv->n_clks, hpriv->clks);\n> > }\n> > EXPORT_SYMBOL_GPL(ahci_platform_enable_clks);\n> > \n> > @@ -126,16 +113,13 @@ EXPORT_SYMBOL_GPL(ahci_platform_enable_clks);\n> > * ahci_platform_disable_clks - Disable platform clocks\n> > * @hpriv: host private area to store config values\n> > *\n> > - * This function disables all the clks found in hpriv->clks, in reverse\n> > - * order of ahci_platform_enable_clks (starting at the end of the array).\n> > + * This function disables all the clocks enabled before\n> > + * (bulk-clocks-disable function is supposed to do that in reverse\n> > + * from the enabling procedure order).\n> > */\n> > void ahci_platform_disable_clks(struct ahci_host_priv *hpriv)\n> > {\n> > -\tint c;\n> > -\n> > -\tfor (c = AHCI_MAX_CLKS - 1; c >= 0; c--)\n> > -\t\tif (hpriv->clks[c])\n> > -\t\t\tclk_disable_unprepare(hpriv->clks[c]);\n> > +\tclk_bulk_disable_unprepare(hpriv->n_clks, hpriv->clks);\n> > }\n> > EXPORT_SYMBOL_GPL(ahci_platform_disable_clks);\n> > \n> > @@ -292,8 +276,6 @@ static void ahci_platform_put_resources(struct device *dev, void *res)\n> > \t\tpm_runtime_disable(dev);\n> > \t}\n> > \n> > -\tfor (c = 0; c < AHCI_MAX_CLKS && hpriv->clks[c]; c++)\n> > -\t\tclk_put(hpriv->clks[c]);\n> > \t/*\n> > \t * The regulators are tied to child node device and not to the\n> > \t * SATA device itself. So we can't use devm for automatically\n> > @@ -374,8 +356,8 @@ static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port,\n> > * 1) mmio registers (IORESOURCE_MEM 0, mandatory)\n> > * 2) regulator for controlling the targets power (optional)\n> > * regulator for controlling the AHCI controller (optional)\n> > - * 3) 0 - AHCI_MAX_CLKS clocks, as specified in the devs devicetree node,\n> > - * or for non devicetree enabled platforms a single clock\n> > + * 3) all clocks specified in the devicetree node, or a single\n> > + * clock for non-OF platforms (optional)\n> > * 4) resets, if flags has AHCI_PLATFORM_GET_RESETS (optional)\n> > * 5) phys (optional)\n> > *\n> > @@ -385,11 +367,10 @@ static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port,\n> > struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,\n> > \t\t\t\t\t\t unsigned int flags)\n> > {\n> > +\tint child_nodes, rc = -ENOMEM, enabled_ports = 0;\n> > \tstruct device *dev = &pdev->dev;\n> > \tstruct ahci_host_priv *hpriv;\n> > -\tstruct clk *clk;\n> > \tstruct device_node *child;\n> > -\tint i, enabled_ports = 0, rc = -ENOMEM, child_nodes;\n> > \tu32 mask_port_map = 0;\n> > \n> > \tif (!devres_open_group(dev, NULL, GFP_KERNEL))\n> > @@ -415,25 +396,38 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,\n> > \t\tgoto err_out;\n> > \t}\n> > \n> > -\tfor (i = 0; i < AHCI_MAX_CLKS; i++) {\n> > +\t/*\n> > +\t * Bulk clocks getting procedure can fail to find any clock due to\n> > +\t * running on a non-OF platform or due to the clocks being defined in\n> > +\t * bypass of the DT firmware (like da850, spear13xx). In that case we\n> > +\t * fallback to getting a single clock source right from the dev clocks\n> > +\t * list.\n> > +\t */\n> > +\trc = devm_clk_bulk_get_all(dev, &hpriv->clks);\n> > +\tif (rc < 0)\n> > +\t\tgoto err_out;\n> > +\n> > +\tif (rc > 0) {\n> > +\t\t/* Got clocks in bulk */\n> > +\t\thpriv->n_clks = rc;\n> > +\t} else {\n> > \t\t/*\n> > -\t\t * For now we must use clk_get(dev, NULL) for the first clock,\n> > -\t\t * because some platforms (da850, spear13xx) are not yet\n> > -\t\t * converted to use devicetree for clocks. For new platforms\n> > -\t\t * this is equivalent to of_clk_get(dev->of_node, 0).\n> > +\t\t * No clock bulk found: fallback to manually getting\n> > +\t\t * the optional clock.\n> > \t\t */\n> > -\t\tif (i == 0)\n> > -\t\t\tclk = clk_get(dev, NULL);\n> > -\t\telse\n> > -\t\t\tclk = of_clk_get(dev->of_node, i);\n> > -\n> > -\t\tif (IS_ERR(clk)) {\n> > -\t\t\trc = PTR_ERR(clk);\n> > -\t\t\tif (rc == -EPROBE_DEFER)\n> > -\t\t\t\tgoto err_out;\n> > -\t\t\tbreak;\n> > +\t\thpriv->clks = devm_kzalloc(dev, sizeof(*hpriv->clks), GFP_KERNEL);\n> > +\t\tif (!hpriv->clks) {\n> > +\t\t\trc = -ENOMEM;\n> > +\t\t\tgoto err_out;\n> > +\t\t}\n> > +\t\thpriv->clks->clk = devm_clk_get_optional(dev, NULL);\n\n> > +\t\tif (IS_ERR(hpriv->clks->clk)) {\n> > +\t\t\trc = PTR_ERR(hpriv->clks->clk);\n> > +\t\t\tgoto err_out;\n> > +\t\t} else if (hpriv->clks->clk) {\n> \n> Nit: the else is not needed here.\n\nWell, it depends on what you see behind it. I see many reasons to keep\nit and only one tiny reason to drop it. Keeping it will improve the\ncode readability and maintainability like having a more natural\nexecution flow representation, thus clearer read-flow (else part as\nexception to the if part), less modifications should the goto part is\nchanged/removed, a more exact program flow representation can be used\nby the compiler for some internal optimizations, it's one line shorter\nthan the case we no 'else' here. On the other hand indeed we can drop\nit since if the conditional statement is true, the code afterwards\nwon't be executed due to the goto operator. But as I see it dropping\nthe else operator won't improve anything, but vise-versa will worsen\nthe code instead. So if I get to miss something please justify why you\nwant it being dropped, otherwise I would rather preserve it.\n\n-Sergey\n\n> \n> > +\t\t\thpriv->clks->id = __clk_get_name(hpriv->clks->clk);\n> > +\t\t\thpriv->n_clks = 1;\n> > \t\t}\n> > -\t\thpriv->clks[i] = clk;\n> > \t}\n> > \n> > \thpriv->ahci_regulator = devm_regulator_get(dev, \"ahci\");\n> \n> \n> -- \n> Damien Le Moal\n> Western Digital Research","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["bilbo.ozlabs.org;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256\n header.s=20210112 header.b=EGSzFePX;\n\tdkim-atps=neutral","ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=2620:137:e000::1:20; helo=out1.vger.email;\n envelope-from=linux-ide-owner@vger.kernel.org; receiver=)"],"Received":["from out1.vger.email (out1.vger.email [IPv6:2620:137:e000::1:20])\n\tby bilbo.ozlabs.org (Postfix) with ESMTP id 4LNckl6DMYz9sGF\n\tfor ; Thu, 16 Jun 2022 06:45:19 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n id S1347398AbiFOUpQ (ORCPT );\n Wed, 15 Jun 2022 16:45:16 -0400","from lindbergh.monkeyblade.net ([23.128.96.19]:39394 \"EHLO\n lindbergh.monkeyblade.net\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n with ESMTP id S238164AbiFOUpP (ORCPT\n ); Wed, 15 Jun 2022 16:45:15 -0400","from mail-lj1-x22f.google.com (mail-lj1-x22f.google.com\n [IPv6:2a00:1450:4864:20::22f])\n by lindbergh.monkeyblade.net (Postfix) with ESMTPS id DC1D12CE2D;\n Wed, 15 Jun 2022 13:45:13 -0700 (PDT)","by mail-lj1-x22f.google.com with SMTP id h23so14615074ljl.3;\n Wed, 15 Jun 2022 13:45:13 -0700 (PDT)","from mobilestation ([95.79.189.214])\n by smtp.gmail.com with ESMTPSA id\n br21-20020a056512401500b0047255d2114asm1906795lfb.121.2022.06.15.13.45.10\n (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n Wed, 15 Jun 2022 13:45:11 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n d=gmail.com; s=20210112;\n h=date:from:to:cc:subject:message-id:references:mime-version\n :content-disposition:in-reply-to;\n bh=5h3h0ojea/V/Vb6J1m80YjDBkY9SwNaJhzgK7nDN9jc=;\n b=EGSzFePXgCLDQMMmYU+A/8xzThfId/dsct6PuUYk4QIJwVUpkJdcGAUbq5XhsDaXVR\n EZ5sh557OCbcMF00RxvZ9fr+l2uas+UJP7uKacB3XVuu65QcI4nipVPcT7TLfK+ng6Md\n SjR3j/fXXY/AKKTPVASUqoFlZa9UH4FCRR/lzi2SLrG4ct4X0sLSTvMSkf3mqb5ak+6a\n o820jU8Z+77oxmPxC1dtt1qhjMN4mbutJBDX+B4kFiwNsI7d2qLj4CKICBwGYBPKnTG0\n +VGxFTpUecBmPWgLYO7EbKO7qyUAJLQrPx6WKg+2rwjaNevjFIny0+Erjas67Xz8H/gr\n ofGw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n d=1e100.net; s=20210112;\n h=x-gm-message-state:date:from:to:cc:subject:message-id:references\n :mime-version:content-disposition:in-reply-to;\n bh=5h3h0ojea/V/Vb6J1m80YjDBkY9SwNaJhzgK7nDN9jc=;\n b=H2kF7vP1yoD/KjM3BXEZt7z7n4t5vVTqc4bb22JnT7CgI20yET1jw5UsJC89XPty99\n AanGfr8RXaYihYhbO/MKNV8vxoHNnuHg9Te6lrxq1nUfic6kJ+Ls/QneOdQIzpXtgl/d\n jnNzeBuWHVuux3g46cUvJ0bkes82nCIdl8ZcIZ54pdV7HHPbn3MILeNS/pIP5y7QCnlG\n fDvFyefCLe78bvLCio0V5+DF55mCH8ShynrEjzh8JqloShWkIHa/PTBeSEHbdi5cpZq2\n WSkWPcceraSkmkfPrrvF8URZBuikVvlXBBcp0pb2iam4ZEjAz7Wxe7JNMvKiiZuWKqGP\n IBmw==","X-Gm-Message-State":"AJIora/pzek4VS6+aRlZlu+uRxzi11ZN4E99JtMIR+sLlqk9/5Eo8EpG\n FZDiLkihHIVDY5a8zbhsRIUcSKCKkzPjgg==","X-Google-Smtp-Source":"\n AGRyM1v1T86xfgb4p/zngTUo4+PBpPqyKtcso2mDUygEhBID8S035JaN09cLtJMtzt6xwHAMBFjkAA==","X-Received":"by 2002:a2e:98d9:0:b0:255:85f4:14b5 with SMTP id\n s25-20020a2e98d9000000b0025585f414b5mr798284ljj.399.1655325911961;\n Wed, 15 Jun 2022 13:45:11 -0700 (PDT)","Date":"Wed, 15 Jun 2022 23:45:09 +0300","From":"Serge Semin ","To":"Damien Le Moal ","Cc":"Serge Semin ,\n Hans de Goede ,\n Jens Axboe , Hannes Reinecke ,\n Alexey Malahov ,\n Pavel Parkhomenko ,\n Rob Herring , linux-ide@vger.kernel.org,\n linux-kernel@vger.kernel.org, devicetree@vger.kernel.org","Subject":"Re: [PATCH v4 07/23] ata: libahci_platform: Convert to using devm\n bulk clocks API","Message-ID":"<20220615204509.siz54h4vbgvb3zkm@mobilestation>","References":"<20220610081801.11854-1-Sergey.Semin@baikalelectronics.ru>\n <20220610081801.11854-8-Sergey.Semin@baikalelectronics.ru>\n <3bf20887-6e2f-41f4-e4ec-5c2278f6cb18@opensource.wdc.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<3bf20887-6e2f-41f4-e4ec-5c2278f6cb18@opensource.wdc.com>","X-Spam-Status":"No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,\n DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,\n RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE\n autolearn=ham autolearn_force=no version=3.4.6","X-Spam-Checker-Version":"SpamAssassin 3.4.6 (2021-04-09) on\n lindbergh.monkeyblade.net","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-ide@vger.kernel.org"}},{"id":2913849,"web_url":"http://patchwork.ozlabs.org/comment/2913849/","msgid":"<0dcebae2-5e4e-a0d3-181d-37bb9b40d564@opensource.wdc.com>","list_archive_url":null,"date":"2022-06-16T00:23:28","subject":"Re: [PATCH v4 07/23] ata: libahci_platform: Convert to using devm\n bulk clocks API","submitter":{"id":82259,"url":"http://patchwork.ozlabs.org/api/people/82259/","name":"Damien Le Moal","email":"damien.lemoal@opensource.wdc.com"},"content":"On 2022/06/16 5:45, Serge Semin wrote:\n[...]\n>>> +\t\thpriv->clks = devm_kzalloc(dev, sizeof(*hpriv->clks), GFP_KERNEL);\n>>> +\t\tif (!hpriv->clks) {\n>>> +\t\t\trc = -ENOMEM;\n>>> +\t\t\tgoto err_out;\n>>> +\t\t}\n>>> +\t\thpriv->clks->clk = devm_clk_get_optional(dev, NULL);\n> \n>>> +\t\tif (IS_ERR(hpriv->clks->clk)) {\n>>> +\t\t\trc = PTR_ERR(hpriv->clks->clk);\n>>> +\t\t\tgoto err_out;\n>>> +\t\t} else if (hpriv->clks->clk) {\n>>\n>> Nit: the else is not needed here.\n> \n> Well, it depends on what you see behind it. I see many reasons to keep\n> it and only one tiny reason to drop it. Keeping it will improve the\n> code readability and maintainability like having a more natural\n> execution flow representation, thus clearer read-flow (else part as\n> exception to the if part), less modifications should the goto part is\n> changed/removed, a more exact program flow representation can be used\n> by the compiler for some internal optimizations, it's one line shorter\n> than the case we no 'else' here. On the other hand indeed we can drop\n> it since if the conditional statement is true, the code afterwards\n> won't be executed due to the goto operator. But as I see it dropping\n> the else operator won't improve anything, but vise-versa will worsen\n> the code instead. So if I get to miss something please justify why you\n> want it being dropped, otherwise I would rather preserve it.\n\nAn else after a goto or return is never necessary and in my opinion makes the\ncode harder to read. I am not interested in debating this in general anyway. For\nthis particular case, the code would be:\n\n\t\thpriv->clks->clk = devm_clk_get_optional(dev, NULL);\n\t\tif (IS_ERR(hpriv->clks->clk)) {\n\t\t\t/* Error path */\n\t\t\trc = PTR_ERR(hpriv->clks->clk);\n\t\t\tgoto err_out;\n\t\t}\n\n\t\t/* Normal path */\n\t\tif (hpriv->clks->clk) {\n\t\t\t...\n\t\t}\n\nWhich in my opinion is a lot easier to understand compared to having to parse\nthe if/else if and figure out which case in that sequence is normal vs error.\n\nAs noted, this is a nit. If you really insist, keep that else if.\n\n> \n> -Sergey\n> \n>>\n>>> +\t\t\thpriv->clks->id = __clk_get_name(hpriv->clks->clk);\n>>> +\t\t\thpriv->n_clks = 1;\n>>> \t\t}\n>>> -\t\thpriv->clks[i] = clk;\n>>> \t}\n>>> \n>>> \thpriv->ahci_regulator = devm_regulator_get(dev, \"ahci\");\n>>\n>>\n>> -- \n>> Damien Le Moal\n>> Western Digital Research","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["bilbo.ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n unprotected) header.d=wdc.com header.i=@wdc.com header.a=rsa-sha256\n header.s=dkim.wdc.com header.b=grHp2xL9;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=opensource.wdc.com header.i=@opensource.wdc.com\n header.a=rsa-sha256 header.s=dkim header.b=TEqAgS/h;\n\tdkim-atps=neutral","ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=2620:137:e000::1:20; helo=out1.vger.email;\n envelope-from=linux-ide-owner@vger.kernel.org; receiver=)","usg-ed-osssrv.wdc.com (amavisd-new); dkim=pass\n reason=\"pass (just generated, assumed good)\"\n header.d=opensource.wdc.com"],"Received":["from out1.vger.email (out1.vger.email [IPv6:2620:137:e000::1:20])\n\tby bilbo.ozlabs.org (Postfix) with ESMTP id 4LNjbT4cgmz9s0r\n\tfor ; Thu, 16 Jun 2022 10:24:21 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n id S234849AbiFPAYQ (ORCPT );\n Wed, 15 Jun 2022 20:24:16 -0400","from lindbergh.monkeyblade.net ([23.128.96.19]:32964 \"EHLO\n lindbergh.monkeyblade.net\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n with ESMTP id S1347032AbiFPAXf (ORCPT\n ); Wed, 15 Jun 2022 20:23:35 -0400","from esa6.hgst.iphmx.com (esa6.hgst.iphmx.com [216.71.154.45])\n by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 8110A1EED2\n for ; Wed, 15 Jun 2022 17:23:34 -0700 (PDT)","from uls-op-cesaip02.wdc.com (HELO uls-op-cesaep02.wdc.com)\n ([199.255.45.15])\n by ob1.hgst.iphmx.com with ESMTP; 16 Jun 2022 08:23:33 +0800","from uls-op-cesaip01.wdc.com ([10.248.3.36])\n by uls-op-cesaep02.wdc.com with ESMTP/TLS/ECDHE-RSA-AES128-GCM-SHA256;\n 15 Jun 2022 16:41:56 -0700","from usg-ed-osssrv.wdc.com ([10.3.10.180])\n by uls-op-cesaip01.wdc.com with ESMTP/TLS/ECDHE-RSA-AES128-GCM-SHA256;\n 15 Jun 2022 17:23:32 -0700","from usg-ed-osssrv.wdc.com (usg-ed-osssrv.wdc.com [127.0.0.1])\n by usg-ed-osssrv.wdc.com (Postfix) with ESMTP id 4LNjZW65z5z1SVp3\n for ; Wed, 15 Jun 2022 17:23:31 -0700 (PDT)","from usg-ed-osssrv.wdc.com ([127.0.0.1])\n by usg-ed-osssrv.wdc.com (usg-ed-osssrv.wdc.com [127.0.0.1])\n (amavisd-new, port 10026)\n with ESMTP id WXd1eyLX1K7n for ;\n Wed, 15 Jun 2022 17:23:31 -0700 (PDT)","from [10.89.84.185] (c02drav6md6t.dhcp.fujisawa.hgst.com\n [10.89.84.185])\n by usg-ed-osssrv.wdc.com (Postfix) with ESMTPSA id 4LNjZT1QXqz1Rvlc;\n Wed, 15 Jun 2022 17:23:29 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=simple/simple;\n d=wdc.com; i=@wdc.com; q=dns/txt; s=dkim.wdc.com;\n t=1655339015; x=1686875015;\n h=message-id:date:mime-version:subject:to:cc:references:\n from:in-reply-to:content-transfer-encoding;\n bh=zZ9XlC7/d8yGFVGGImcAjZu6pedLsVXV7Ze6hqnSWA8=;\n b=grHp2xL9hoSZMPy7TRqMFnB5MdkHXA4/zS0IIkE58ucrx7k/9vJaQQ9o\n 74sp/KojgIuLNtVoeG73TAN/riT1f6GU2svrjrCdG3VNA/sl/UElIpHZn\n jGCv1wyYQFvpg2OKiUw/3jDw6F6I66gSfiwgZKLyYoufKWxaZUnC0/8OL\n Nu+m+K0Qv/X3f5lWulo6E2/DV6fGgriLetd1mZZKyNY1Sy/TrpJX8mlYR\n L239HQKMG54HTAv6W+n709NSQJyrMcry+pGzWY/5ECr0bAZzoSchWR4K5\n oCVVTFnHaeYbid3kk2bsrx2L+VP2PXW/gl4wrmAaQRO0ZzEIYRolKL1dX\n A==;","v=1; a=rsa-sha256; c=relaxed/simple; d=\n opensource.wdc.com; h=content-transfer-encoding:content-type\n :in-reply-to:organization:from:references:to:content-language\n :subject:user-agent:mime-version:date:message-id; s=dkim; t=\n 1655339011; x=1657931012; bh=zZ9XlC7/d8yGFVGGImcAjZu6pedLsVXV7Ze\n 6hqnSWA8=; b=TEqAgS/hsApCNwl+CHO+6SoIkPpK4yadlxMSKbvnsWr5u+1VXOr\n ymFIk6PgOMjbnMFJ3+L91utl1WBhUhJ626wWWPVzMBequEDL22TvFlB3fQUEBVYd\n gsjbE6gE3kUNNAF9560R4uvZ9N/VKc17841muTB6cF+jJ11N3iokeL2egcbzFur6\n Kx3Blsu89K3oR4EX99B7TuOASebdKyVWyachP9ZeMyX669Dw2qh7yUWudFGYubKt\n 02LA9LglJLXCDRh84iyTD2oYZ5V5t+5YOrjL91fHypjuSnwmGWbuGvoOuvds8xBb\n 0+jN4VI+kTjLgQD+G1YoPQX/2p8i9nTeP2Q=="],"X-IronPort-AV":"E=Sophos;i=\"5.91,302,1647273600\";\n d=\"scan'208\";a=\"204038061\"","IronPort-SDR":["\n 9QvH7HdRiPRiYYCAsSFv8oMrTEDPFTrKLpe9fyHGsWALCH62OaRn8zTAmD5mKfCIGroAyfX6dI\n vr7r09K4SWXhb3K2AAH3qZFfA+LDKsNz3p363hZHv43zkHxteRZQK9di0pgo/oyU3yN6U64BYk\n ckx2jbo/Oz5F/46Vqf29J9zNO063s/63q2wkU/NkK7/3E8ZqGbUOxtt9TEzdhYyeg3JX76VpLS\n hxtSniYtr76Ed2BQJEijhVw9gBaalauO2/xIsO64s1N4F8hCOyj6KvE40ZDHHbhwyGR8b3XGkd\n XsutYMqPvcVwYJujpBKbij6a","\n ctIpqTGWjsMt1ykHxrzlUGzU+kuxoc6zvsLy/56NdZdeEE2AgUKJFAaKNv7Cvb8JGVsqxjRU05\n bVvZQzrfvyRfGonHaYwdQeRR5nmAMllvj1woRzG7IUDsDH4E0M024tM6asBvHUhTETuIC/dtFc\n 0c1hvUAvcQ87e1woC2FfQHdOSnwt0LkSMkVQqMcYy5x6zDDwHOfaQfyjBbVy54BUM1RD7MK7E9\n EsMjfMuvivqw+xgcmq6TKXz9utiX0zPkppPcTPfjJRZrWr0621/YZB/zZ5STNcdZTH463UxjSI\n RwY="],"WDCIronportException":"Internal","X-Virus-Scanned":"amavisd-new at usg-ed-osssrv.wdc.com","Message-ID":"<0dcebae2-5e4e-a0d3-181d-37bb9b40d564@opensource.wdc.com>","Date":"Thu, 16 Jun 2022 09:23:28 +0900","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)\n Gecko/20100101 Thunderbird/91.10.0","Subject":"Re: [PATCH v4 07/23] ata: libahci_platform: Convert to using devm\n bulk clocks API","Content-Language":"en-US","To":"Serge Semin ","Cc":"Serge Semin ,\n Hans de Goede ,\n Jens Axboe , Hannes Reinecke ,\n Alexey Malahov ,\n Pavel Parkhomenko ,\n Rob Herring , linux-ide@vger.kernel.org,\n linux-kernel@vger.kernel.org, devicetree@vger.kernel.org","References":"<20220610081801.11854-1-Sergey.Semin@baikalelectronics.ru>\n <20220610081801.11854-8-Sergey.Semin@baikalelectronics.ru>\n <3bf20887-6e2f-41f4-e4ec-5c2278f6cb18@opensource.wdc.com>\n <20220615204509.siz54h4vbgvb3zkm@mobilestation>","From":"Damien Le Moal ","Organization":"Western Digital Research","In-Reply-To":"<20220615204509.siz54h4vbgvb3zkm@mobilestation>","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"7bit","X-Spam-Status":"No, score=-5.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,\n DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_MED,\n SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable\n autolearn_force=no version=3.4.6","X-Spam-Checker-Version":"SpamAssassin 3.4.6 (2021-04-09) on\n lindbergh.monkeyblade.net","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-ide@vger.kernel.org"}},{"id":2915206,"web_url":"http://patchwork.ozlabs.org/comment/2915206/","msgid":"<20220617195403.wbqy5ozm6x7tq3dh@mobilestation>","list_archive_url":null,"date":"2022-06-17T19:54:03","subject":"Re: [PATCH v4 07/23] ata: libahci_platform: Convert to using devm\n bulk clocks API","submitter":{"id":70038,"url":"http://patchwork.ozlabs.org/api/people/70038/","name":"Serge Semin","email":"fancer.lancer@gmail.com"},"content":"On Thu, Jun 16, 2022 at 09:23:28AM +0900, Damien Le Moal wrote:\n> On 2022/06/16 5:45, Serge Semin wrote:\n> [...]\n> >>> +\t\thpriv->clks = devm_kzalloc(dev, sizeof(*hpriv->clks), GFP_KERNEL);\n> >>> +\t\tif (!hpriv->clks) {\n> >>> +\t\t\trc = -ENOMEM;\n> >>> +\t\t\tgoto err_out;\n> >>> +\t\t}\n> >>> +\t\thpriv->clks->clk = devm_clk_get_optional(dev, NULL);\n> > \n> >>> +\t\tif (IS_ERR(hpriv->clks->clk)) {\n> >>> +\t\t\trc = PTR_ERR(hpriv->clks->clk);\n> >>> +\t\t\tgoto err_out;\n> >>> +\t\t} else if (hpriv->clks->clk) {\n> >>\n> >> Nit: the else is not needed here.\n> > \n> > Well, it depends on what you see behind it. I see many reasons to keep\n> > it and only one tiny reason to drop it. Keeping it will improve the\n> > code readability and maintainability like having a more natural\n> > execution flow representation, thus clearer read-flow (else part as\n> > exception to the if part), less modifications should the goto part is\n> > changed/removed, a more exact program flow representation can be used\n> > by the compiler for some internal optimizations, it's one line shorter\n> > than the case we no 'else' here. On the other hand indeed we can drop\n> > it since if the conditional statement is true, the code afterwards\n> > won't be executed due to the goto operator. But as I see it dropping\n> > the else operator won't improve anything, but vise-versa will worsen\n> > the code instead. So if I get to miss something please justify why you\n> > want it being dropped, otherwise I would rather preserve it.\n> \n> An else after a goto or return is never necessary and in my opinion makes the\n> code harder to read. I am not interested in debating this in general anyway. For\n> this particular case, the code would be:\n> \n> \t\thpriv->clks->clk = devm_clk_get_optional(dev, NULL);\n> \t\tif (IS_ERR(hpriv->clks->clk)) {\n> \t\t\t/* Error path */\n> \t\t\trc = PTR_ERR(hpriv->clks->clk);\n> \t\t\tgoto err_out;\n> \t\t}\n> \n> \t\t/* Normal path */\n> \t\tif (hpriv->clks->clk) {\n> \t\t\t...\n> \t\t}\n> \n> Which in my opinion is a lot easier to understand compared to having to parse\n> the if/else if and figure out which case in that sequence is normal vs error.\n> \n\n> As noted, this is a nit. If you really insist, keep that else if.\n\nOk. I'll leave it as is then.\n\nThanks\n-Sergey\n\n> \n> > \n> > -Sergey\n> > \n> >>\n> >>> +\t\t\thpriv->clks->id = __clk_get_name(hpriv->clks->clk);\n> >>> +\t\t\thpriv->n_clks = 1;\n> >>> \t\t}\n> >>> -\t\thpriv->clks[i] = clk;\n> >>> \t}\n> >>> \n> >>> \thpriv->ahci_regulator = devm_regulator_get(dev, \"ahci\");\n> >>\n> >>\n> >> -- \n> >> Damien Le Moal\n> >> Western Digital Research\n> \n> \n> -- \n> Damien Le Moal\n> Western Digital Research","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["bilbo.ozlabs.org;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256\n header.s=20210112 header.b=qKuKUQO/;\n\tdkim-atps=neutral","ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=2620:137:e000::1:20; helo=out1.vger.email;\n envelope-from=linux-ide-owner@vger.kernel.org; receiver=)"],"Received":["from out1.vger.email (out1.vger.email [IPv6:2620:137:e000::1:20])\n\tby bilbo.ozlabs.org (Postfix) with ESMTP id 4LPqVp4DQfz9sFx\n\tfor ; Sat, 18 Jun 2022 05:54:10 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n id S234252AbiFQTyJ (ORCPT );\n Fri, 17 Jun 2022 15:54:09 -0400","from lindbergh.monkeyblade.net ([23.128.96.19]:35212 \"EHLO\n lindbergh.monkeyblade.net\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n with ESMTP id S229794AbiFQTyI (ORCPT\n ); Fri, 17 Jun 2022 15:54:08 -0400","from mail-lf1-x133.google.com (mail-lf1-x133.google.com\n [IPv6:2a00:1450:4864:20::133])\n by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 58E022E9DA;\n Fri, 17 Jun 2022 12:54:07 -0700 (PDT)","by mail-lf1-x133.google.com with SMTP id y32so8415160lfa.6;\n Fri, 17 Jun 2022 12:54:07 -0700 (PDT)","from mobilestation ([95.79.189.214])\n by smtp.gmail.com with ESMTPSA id\n g18-20020ac24d92000000b0047f523ae57csm381524lfe.17.2022.06.17.12.54.04\n (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n Fri, 17 Jun 2022 12:54:05 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n d=gmail.com; s=20210112;\n h=date:from:to:cc:subject:message-id:references:mime-version\n :content-disposition:in-reply-to;\n bh=02rUpC1JaF7EaBeJHvEU6ZBWvHsGGlNTq1EAsa26d+U=;\n b=qKuKUQO/dcX5z9teCBIMgIMk3uOFQJLA3kHLgJnf3nxxZ7xzS/DXSwQhi0dsCujAcf\n 3BXckUda/elXHotAMIpNINg170iI42nMz4NCxKxIaqXZ5C7V1x3pV2rKW2kIyEXBbQdS\n l46xTw46DtID6yKeA7MEPDIFrok8wY0EntHqq/Sy/eP2oT5dfG8MsjG2vVz0UbF5jq6o\n /XmaSdi3IZnBNmzrEivxuZqSOeMTxakgYM4UL8r+FwVQiAzkP8jH7KO3cYeT/dU9DH2o\n ITCzXEqkEBMx0f1eo5mpo4XCURBWxCA1XCQSxHspUeV7C1G5Rf15NmQe1KvSjmcA7wyV\n G7CA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n d=1e100.net; s=20210112;\n h=x-gm-message-state:date:from:to:cc:subject:message-id:references\n :mime-version:content-disposition:in-reply-to;\n bh=02rUpC1JaF7EaBeJHvEU6ZBWvHsGGlNTq1EAsa26d+U=;\n b=JlLjKdOPfaO6M0JcRiwhdnToMgWYmGQGlUylxVj0EOV4EqDh2bnRDXVx19AyCAdaEY\n 7kZuOCugaA4aaxFI0Ij8LxKW/MFbgtOap71IJAYkT+XVQ8ofFV4o1OCGz9AVDiQorBGS\n 26OtwcgfCmgKk4WbJ8V3MXYCOzcqyMQ3jH7lwhrMD7sSG5AhAXjdEQl3MxruZL7ZE0kv\n 8En4YPOhkGaJH2D+Ulfb+PakWajwkKO6vapkFJjs5ZCLFtd/i+w2WtW3SFoOvsQoGQW9\n mogFKbB+cf9RJeDV6OGxDhKfpH+Kqcxikh26VqxZZM9tFSHGHDzev+LJv5ZTzHHsxVCE\n AKUQ==","X-Gm-Message-State":"AJIora9cvLleZRWlPql3f0QsSmcS+2T7lKwZ0pwa5l2bsEtkELC3POZf\n gjMr3wjQAjSTuF9WkeuwwhE=","X-Google-Smtp-Source":"\n AGRyM1scl8XJBZO2QfldF/lmnmDPuZMn9aZDccuJGjXCRN2CeOWsSuAYogHMxuezZr/3ib1KJwEHmw==","X-Received":"by 2002:ac2:4314:0:b0:47e:53ff:7db with SMTP id\n l20-20020ac24314000000b0047e53ff07dbmr6548634lfh.118.1655495645696;\n Fri, 17 Jun 2022 12:54:05 -0700 (PDT)","Date":"Fri, 17 Jun 2022 22:54:03 +0300","From":"Serge Semin ","To":"Damien Le Moal ","Cc":"Serge Semin ,\n Hans de Goede ,\n Jens Axboe , Hannes Reinecke ,\n Alexey Malahov ,\n Pavel Parkhomenko ,\n Rob Herring , linux-ide@vger.kernel.org,\n linux-kernel@vger.kernel.org, devicetree@vger.kernel.org","Subject":"Re: [PATCH v4 07/23] ata: libahci_platform: Convert to using devm\n bulk clocks API","Message-ID":"<20220617195403.wbqy5ozm6x7tq3dh@mobilestation>","References":"<20220610081801.11854-1-Sergey.Semin@baikalelectronics.ru>\n <20220610081801.11854-8-Sergey.Semin@baikalelectronics.ru>\n <3bf20887-6e2f-41f4-e4ec-5c2278f6cb18@opensource.wdc.com>\n <20220615204509.siz54h4vbgvb3zkm@mobilestation>\n <0dcebae2-5e4e-a0d3-181d-37bb9b40d564@opensource.wdc.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<0dcebae2-5e4e-a0d3-181d-37bb9b40d564@opensource.wdc.com>","X-Spam-Status":"No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,\n DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,\n RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE\n autolearn=ham autolearn_force=no version=3.4.6","X-Spam-Checker-Version":"SpamAssassin 3.4.6 (2021-04-09) on\n lindbergh.monkeyblade.net","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-ide@vger.kernel.org"}}]