[{"id":1770984,"web_url":"http://patchwork.ozlabs.org/comment/1770984/","msgid":"<20170919130944.ldoskrm54tx2oemd@sirena.co.uk>","list_archive_url":null,"date":"2017-09-19T13:09:44","subject":"Re: [PATCH 1/2] regulator: core: Add coupled regulators mechanism","submitter":{"id":24878,"url":"http://patchwork.ozlabs.org/api/people/24878/","name":"Mark Brown","email":"broonie@kernel.org"},"content":"On Mon, Sep 18, 2017 at 10:39:51AM +0200, Maciej Purski wrote:\n> On Odroid XU3/4 and other Exynos5422 based boards there is a case, that\n> different devices on the board are supplied by different regulators\n> with non-fixed voltages. If one of these devices temporarily requires\n> higher voltage, there might occur a situation that the spread between\n> two devices' voltages is so high, that there is a risk of changing\n> 'high' and 'low' states on the interconnection between devices powered\n> by those two regulators.\n> \n> Keeping spread between those voltages below defined max_spread should\n> be handled by the framework. Information required to do so is obtained\n> from the device tree. On each voltage change the core should find the\n> best voltages which suit all consumers' demands and max_spread.\n> Then set them for a coupled regulator also.\n\nThis leads me none the wiser as to how this will be implemented which\nmakes this rather hard to review, especially since this appears to have\na lot of random refactoring mixed in with it.\n\n> +static int regulator_set_voltage_safe(struct regulator_dev *rdev,\n> +\t\t\t\t      int min_uV, int max_uV);\n\nWhy would we want a way to set voltages that isn't safe?\n\n> @@ -2181,6 +2183,8 @@ static int _regulator_enable(struct regulator_dev *rdev)\n>  \t\t/* Fallthrough on positive return values - already enabled */\n>  \t}\n>  \n> +\tif (rdev->coupled_desc)\n> +\t\trdev->coupled_desc->enable_count++;\n>  \trdev->use_count++;\n>  \n>  \treturn 0;\n\nThere's no locking here, and we appear to take no action when these\ncounts change - do we need to bother with this at all?\n\n> +\t/* check if changing voltage won't interfere with other\n> +\t * consumers' demands\n> +\t */\n>  \tret = regulator_check_consumers(rdev, &min_uV, &max_uV);\n>  \tif (ret < 0)\n>  \t\tgoto out2;\n\nThese extra comments that are being added are pretty much all readouts\nof the name of the function that's being called (and many of them are\nmisformatted)...\n\n> +static int regulator_set_voltage_safe(struct regulator_dev *rdev, int min_uV,\n> +\t\t\t\t\t   int max_uV)\n> +{\n\n...which is a bit ironic given that this isn't documented at all :/\n\n> +static int regulator_set_coupled_voltage(struct coupled_reg_desc *c_desc)\n> +{\n> +\tstruct regulator_dev **c_rdevs = c_desc->coupled_rdevs;\n> +\tint max_spread = c_desc->max_spread;\n> +\tint best_volt[2] = { };\n> +\tint actual_volt[2];\n> +\tint min_volt, max_volt;\n> +\tint ret = 0, i, max;\n> +\tint ready = 0;\n> +\n> +\t/* Get voltages desired by all consumers of the coupled regulator */\n> +\tfor (i = 0; i < 2; i++) {\n\nIt appears we can't couple more than two regulators?\n\n> +\tmax_volt = max(best_volt[0], best_volt[1]);\n> +\tmin_volt = min(best_volt[0], best_volt[1]);\n\nSo the maximum and minimum are the *least* constrained?\n\n> +\t\t\tmax_possible = actual_volt[(i + 1) % 2] + max_spread;\n> +\t\t\tmin_possible = actual_volt[(i + 1) % 2] - max_spread;\n\nI'm lost, this magic array indexing is just illegible.","headers":{"Return-Path":"<devicetree-owner@vger.kernel.org>","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=<UNKNOWN>)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=sirena.org.uk header.i=@sirena.org.uk\n\theader.b=\"qNO+jLtg\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xxNWf2zVYz9sBZ\n\tfor <incoming-dt@patchwork.ozlabs.org>;\n\tTue, 19 Sep 2017 23:09:50 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751284AbdISNJs (ORCPT\n\t<rfc822;incoming-dt@patchwork.ozlabs.org>);\n\tTue, 19 Sep 2017 09:09:48 -0400","from heliosphere.sirena.org.uk ([172.104.155.198]:37248 \"EHLO\n\theliosphere.sirena.org.uk\" rhost-flags-OK-OK-OK-OK) by\n\tvger.kernel.org with ESMTP id S1751053AbdISNJr (ORCPT\n\t<rfc822; devicetree@vger.kernel.org>); Tue, 19 Sep 2017 09:09:47 -0400","from debutante.sirena.org.uk ([2001:470:1f1d:6b5::3]\n\thelo=debutante) by heliosphere.sirena.org.uk with esmtpsa\n\t(TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.89)\n\t(envelope-from <broonie@sirena.co.uk>)\n\tid 1duIHZ-0001JE-CU; Tue, 19 Sep 2017 13:09:45 +0000","from broonie by debutante with local (Exim 4.89)\n\t(envelope-from <broonie@sirena.co.uk>)\n\tid 1duIHY-0006aG-EW; Tue, 19 Sep 2017 14:09:44 +0100"],"DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed;\n\td=sirena.org.uk; s=20170815-heliosphere; h=In-Reply-To:Content-Type:\n\tMIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To:\n\tContent-Transfer-Encoding:Content-ID:Content-Description:Resent-Date:\n\tResent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id:\n\tList-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive;\n\tbh=0PSu7Ojzoapm5BiZM8s6FDreAFmxMOYpUkPHtsH1RUQ=;\n\tb=qNO+jLtg/FZH7hyLfLh9UvL6o\n\t0FeV5toLDMzWDSNUZWNgSlOEG321HVDYwcsvqL+KGgWOsiSPxq0L3wQdoe5+3rSTdb7MnhMVchOXX\n\thFpT2vMgDt7AJyZIkv+/cW7BUm/dGIffktJa113WoVqnpSc36KUgZVUYJSOzBM6QkSwpo=;","Date":"Tue, 19 Sep 2017 14:09:44 +0100","From":"Mark Brown <broonie@kernel.org>","To":"Maciej Purski <m.purski@samsung.com>","Cc":"linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,\n\tLiam Girdwood <lgirdwood@gmail.com>, Rob Herring <robh+dt@kernel.org>,\n\tMark Rutland <mark.rutland@arm.com>,\n\tMarek Szyprowski <m.szyprowski@samsung.com>,\n\tBartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>","Subject":"Re: [PATCH 1/2] regulator: core: Add coupled regulators mechanism","Message-ID":"<20170919130944.ldoskrm54tx2oemd@sirena.co.uk>","References":"<1505723992-11772-1-git-send-email-m.purski@samsung.com>\n\t<CGME20170918084022eucas1p1398f18c5c90535ce484e3952fae80882@eucas1p1.samsung.com>\n\t<1505723992-11772-2-git-send-email-m.purski@samsung.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"vhzcojvceyk3bhky\"","Content-Disposition":"inline","In-Reply-To":"<1505723992-11772-2-git-send-email-m.purski@samsung.com>","X-Cookie":"I'm also against BODY-SURFING!!","User-Agent":"NeoMutt/20170609 (1.8.3)","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"<devicetree.vger.kernel.org>","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1771087,"web_url":"http://patchwork.ozlabs.org/comment/1771087/","msgid":"<8ac4fa17-43ed-7a4d-d086-6ad81d97900e@samsung.com>","list_archive_url":null,"date":"2017-09-19T14:35:54","subject":"Re: [PATCH 1/2] regulator: core: Add coupled regulators mechanism","submitter":{"id":72100,"url":"http://patchwork.ozlabs.org/api/people/72100/","name":"Maciej Purski","email":"m.purski@samsung.com"},"content":"On 09/19/2017 03:09 PM, Mark Brown wrote:\n\n> On Mon, Sep 18, 2017 at 10:39:51AM +0200, Maciej Purski wrote:\n>> On Odroid XU3/4 and other Exynos5422 based boards there is a case, that\n>> different devices on the board are supplied by different regulators\n>> with non-fixed voltages. If one of these devices temporarily requires\n>> higher voltage, there might occur a situation that the spread between\n>> two devices' voltages is so high, that there is a risk of changing\n>> 'high' and 'low' states on the interconnection between devices powered\n>> by those two regulators.\n>>\n>> Keeping spread between those voltages below defined max_spread should\n>> be handled by the framework. Information required to do so is obtained\n>> from the device tree. On each voltage change the core should find the\n>> best voltages which suit all consumers' demands and max_spread.\n>> Then set them for a coupled regulator also.\n> This leads me none the wiser as to how this will be implemented which\n> makes this rather hard to review, especially since this appears to have\n> a lot of random refactoring mixed in with it.\n\nSorry. I'll document it better in the next version.\n\n>\n>> +static int regulator_set_voltage_safe(struct regulator_dev *rdev,\n>> +\t\t\t\t      int min_uV, int max_uV);\n> Why would we want a way to set voltages that isn't safe?\n\nThis function is extracted from original regulator_set_voltage_unlocked(). It contains\ncode responsible for calling do_set_voltage on the given regulator and its ancestors (if needed).\nThis function is called both from regulator_set_voltage_unlocked(), from where it was extracted\nand from my new function regulator_set_coupled_voltage(). I added this in order to avoid\ncode duplication. I agree that the name might not be adequate. What name would you find more suitable?\n\n>\n>> @@ -2181,6 +2183,8 @@ static int _regulator_enable(struct regulator_dev *rdev)\n>>   \t\t/* Fallthrough on positive return values - already enabled */\n>>   \t}\n>>   \n>> +\tif (rdev->coupled_desc)\n>> +\t\trdev->coupled_desc->enable_count++;\n>>   \trdev->use_count++;\n>>   \n>>   \treturn 0;\n> There's no locking here, and we appear to take no action when these\n> counts change - do we need to bother with this at all?\n\nVariable enable_count is used for checking if both regulators are enabled and there's a need for\nusing the coupling mechanism. It is checked in regulator_set_coupled_voltage_unlocked(), where the\nmutex is already locked. I think that locking it here would be useful. Thanks.\n\n>\n>> +\t/* check if changing voltage won't interfere with other\n>> +\t * consumers' demands\n>> +\t */\n>>   \tret = regulator_check_consumers(rdev, &min_uV, &max_uV);\n>>   \tif (ret < 0)\n>>   \t\tgoto out2;\n> These extra comments that are being added are pretty much all readouts\n> of the name of the function that's being called (and many of them are\n> misformatted)...\n>\n>> +static int regulator_set_voltage_safe(struct regulator_dev *rdev, int min_uV,\n>> +\t\t\t\t\t   int max_uV)\n>> +{\n> ...which is a bit ironic given that this isn't documented at all :/\n\nEverything will be documented better in the next version.\n\n>\n>> +static int regulator_set_coupled_voltage(struct coupled_reg_desc *c_desc)\n>> +{\n>> +\tstruct regulator_dev **c_rdevs = c_desc->coupled_rdevs;\n>> +\tint max_spread = c_desc->max_spread;\n>> +\tint best_volt[2] = { };\n>> +\tint actual_volt[2];\n>> +\tint min_volt, max_volt;\n>> +\tint ret = 0, i, max;\n>> +\tint ready = 0;\n>> +\n>> +\t/* Get voltages desired by all consumers of the coupled regulator */\n>> +\tfor (i = 0; i < 2; i++) {\n> It appears we can't couple more than two regulators?\n\nWe can couple just two regulators. We have never found any case for coupling\nmore than two regulators. Limiting the mechanism to just two regulators simplifies\nalgorithm a little bit. Would you prefer it working for more than two\nregulators also even if there isn't any use case?\n\n>\n>> +\tmax_volt = max(best_volt[0], best_volt[1]);\n>> +\tmin_volt = min(best_volt[0], best_volt[1]);\n> So the maximum and minimum are the *least* constrained?\n>\n>> +\t\t\tmax_possible = actual_volt[(i + 1) % 2] + max_spread;\n>> +\t\t\tmin_possible = actual_volt[(i + 1) % 2] - max_spread;\n> I'm lost, this magic array indexing is just illegible.\n\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at  http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"<devicetree-owner@vger.kernel.org>","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=<UNKNOWN>)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xxQRR5TVMz9s7h\n\tfor <incoming-dt@patchwork.ozlabs.org>;\n\tWed, 20 Sep 2017 00:36:19 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751690AbdISOgB (ORCPT\n\t<rfc822;incoming-dt@patchwork.ozlabs.org>);\n\tTue, 19 Sep 2017 10:36:01 -0400","from mailout1.w1.samsung.com ([210.118.77.11]:55967 \"EHLO\n\tmailout1.w1.samsung.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751165AbdISOf7 (ORCPT\n\t<rfc822; devicetree@vger.kernel.org>); Tue, 19 Sep 2017 10:35:59 -0400","from eucas1p1.samsung.com (unknown [182.198.249.206])\n\tby mailout1.w1.samsung.com (KnoxPortal) with ESMTP id\n\t20170919143557euoutp018cb6c74139ac3df2b3e464108912f0a7~lygFQ_1TO2187221872euoutp01o;\n\tTue, 19 Sep 2017 14:35:57 +0000 (GMT)","from eusmges4.samsung.com (unknown [203.254.199.244]) by\n\teucas1p2.samsung.com (KnoxPortal) with ESMTP id\n\t20170919143556eucas1p21bae1029257e04fa175ed19f6154fc93~lygEZVN2W2037620376eucas1p2h;\n\tTue, 19 Sep 2017 14:35:56 +0000 (GMT)","from eucas1p1.samsung.com ( [182.198.249.206]) by\n\teusmges4.samsung.com (EUCPMTA) with SMTP id B9.2F.12944.C4B21C95;\n\tTue, 19 Sep 2017 15:35:56 +0100 (BST)","from eusmgms1.samsung.com (unknown [182.198.249.179]) by\n\teucas1p2.samsung.com (KnoxPortal) with ESMTP id\n\t20170919143555eucas1p2b4d241e4240cd46d1deef6dabcedc9fd~lygDjZwxj1688416884eucas1p2t;\n\tTue, 19 Sep 2017 14:35:55 +0000 (GMT)","from eusync2.samsung.com ( [203.254.199.212]) by\n\teusmgms1.samsung.com (EUCPMTA) with SMTP id 3F.AC.18832.B4B21C95;\n\tTue, 19 Sep 2017 15:35:55 +0100 (BST)","from [106.120.51.25] by eusync2.samsung.com (Oracle Communications\n\tMessaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTPA\n\tid <0OWJ00IKL77V2Y00@eusync2.samsung.com>;\n\tTue, 19 Sep 2017 15:35:55 +0100 (BST)"],"X-AuditID":"cbfec7f4-f79ab6d000003290-96-59c12b4c240d","Subject":"Re: [PATCH 1/2] regulator: core: Add coupled regulators mechanism","To":"Mark Brown <broonie@kernel.org>","Cc":"linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,\n\tLiam Girdwood <lgirdwood@gmail.com>, Rob Herring <robh+dt@kernel.org>,\n\tMark Rutland <mark.rutland@arm.com>,\n\tMarek Szyprowski <m.szyprowski@samsung.com>,\n\tBartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>","From":"Maciej Purski <m.purski@samsung.com>","Message-id":"<8ac4fa17-43ed-7a4d-d086-6ad81d97900e@samsung.com>","Date":"Tue, 19 Sep 2017 16:35:54 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-version":"1.0","In-reply-to":"<20170919130944.ldoskrm54tx2oemd@sirena.co.uk>","Content-type":"text/plain; charset=\"windows-1252\"; format=\"flowed\"","Content-transfer-encoding":"7bit","Content-language":"en-US","X-Brightmail-Tracker":["H4sIAAAAAAAAA+NgFlrEKsWRmVeSWpSXmKPExsWy7djPc7o+2gcjDZb+57XYOGM9q8XUh0/Y\n\tLOYfOcdq8e1KB5PF5V1z2CzWHrnLbrH0+kUmi9a9R9gdODzWzFvD6LFz1l12j02rOtk8+ras\n\tYvT4vEkugDWKyyYlNSezLLVI3y6BK2NS42mmgjOKFT+f3mZtYPwq1cXIwSEhYCLx4W1sFyMn\n\tkCkmceHeerYuRi4OIYGljBJ715xihHA+M0qcnzOdCaLKROLFzLvMEIlljBKvHu6BannGKLFq\n\t4hZ2kCphAS+JXW97WEBsEQFliavf97KAFDELTGSSuPBnMRPIbjYBLYk17fEgNbwCdhLf1n9i\n\tBrFZBFQlXjw+yghiiwpESGz7PoMNokZQ4sfke2AzOQWsJRa97wPbxSzgKPFg0U5WCFteYvOa\n\tt8wQtrhEc+tNsL0SArfZJDZ9/c4G8YKLxN3N3SwQtrDEq+MQR0sIyEh0dhyEerNa4uLXXVD1\n\tNRKNtzdA1VhLfJ60BWoBn8SkbdOZIeHIK9HRJgRR4iFxbMlcqDGOEpeeX2GHBFALk0T/vaPM\n\tExjlZyH5ZxaSH2Yh+WEWkh8WMLKsYhRJLS3OTU8tNtErTswtLs1L10vOz93ECEw5p/8d/7KD\n\tcfExq0OMAhyMSjy8Enf3RwqxJpYVV+YeYpTgYFYS4V2udjBSiDclsbIqtSg/vqg0J7X4EKM0\n\tB4uSOK9tVFukkEB6YklqdmpqQWoRTJaJg1OqgXGO0Yojz+yKpwQuXnC7bfvmxjYr+9+X2HO8\n\twkwsRa+UxDOrfAtbeCHydkni7NbGsIem3hsnzGnreea3YnOm0wszJaPGUz/tyx6cvyjWWbLp\n\tWuDhK5e8OC9f2NEUzXfATed66x3tJJsjMQvvHm178cRj80Lbl7orlmmqt4jumcBxXOyjys28\n\tM9FKLMUZiYZazEXFiQC52JY3NQMAAA==","H4sIAAAAAAAAA+NgFvrJLMWRmVeSWpSXmKPExsVy+t/xK7re2gcjDWY2KllsnLGe1WLqwyds\n\tFvOPnGO1+Halg8ni8q45bBZrj9xlt1h6/SKTReveI+wOHB5r5q1h9Ng56y67x6ZVnWwefVtW\n\tMXp83iQXwBrFZZOSmpNZllqkb5fAlTGp8TRTwRnFip9Pb7M2MH6V6mLk5JAQMJF4MfMuM4Qt\n\tJnHh3nq2LkYuDiGBJYwSe2bOYoFwnjFKXPv9B6xKWMBLYtfbHhYQW0RAWeLq971gRcwCk5kk\n\tvv1sh+poYZI4fnE70CwODjYBLYk17fEgDbwCdhLf1n8CG8QioCrx4vFRRhBbVCBCou/tZXaI\n\tGkGJH5PvgS3gFLCWWPS+DyzOLGArseD9OhYIW15i85q3zBC2uERz602WCYyCs5C0z0LSMgtJ\n\tyywkLQsYWVYxiqSWFuem5xYb6hUn5haX5qXrJefnbmIERsi2Yz8372C8tDH4EKMAB6MSD6/A\n\ttf2RQqyJZcWVuYcYJTiYlUR4l6sdjBTiTUmsrEotyo8vKs1JLT7EKM3BoiTO27tndaSQQHpi\n\tSWp2ampBahFMlomDU6qBMaxeZP3DyY+7e52u3F/JeHgi21HO45ZVsr1s3tE5v/o23OZb91zJ\n\t+g77Tt+w+i0WrSrztjPJzD+71m/acbVzbodYah5xmfAdeir1MvWIVPPVSfqPD3b9Mrn8cIcG\n\tR49P6YS+aYkuYam1uRZbA2o+3v+7Sepxs+W2tXai8scvM+ksktg6o8WxXomlOCPRUIu5qDgR\n\tAJ9LFJiMAgAA"],"X-CMS-MailID":"20170919143555eucas1p2b4d241e4240cd46d1deef6dabcedc9fd","X-Msg-Generator":"CA","X-Sender-IP":"182.198.249.179","X-Local-Sender":"=?utf-8?q?Maciej_Purski=1BSecurity_=28TP=29=1BSamsung_Ele?=\n\t=?utf-8?q?ctronics=1BTrainee_=28=29?=","X-Global-Sender":"=?utf-8?q?Maciej_Purski=1BSecurity_=28TP=29=1BSamsung_El?=\n\t=?utf-8?q?ectronics=1BTrainee_=28=29?=","X-Sender-Code":"=?utf-8?q?C10=1BEHQ=1BC10CD02CD027395?=","CMS-TYPE":"201P","X-CMS-RootMailID":"20170918084022eucas1p1398f18c5c90535ce484e3952fae80882","X-RootMTR":"20170918084022eucas1p1398f18c5c90535ce484e3952fae80882","References":"<1505723992-11772-1-git-send-email-m.purski@samsung.com>\n\t<CGME20170918084022eucas1p1398f18c5c90535ce484e3952fae80882@eucas1p1.samsung.com>\n\t<1505723992-11772-2-git-send-email-m.purski@samsung.com>\n\t<20170919130944.ldoskrm54tx2oemd@sirena.co.uk>","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"<devicetree.vger.kernel.org>","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1771108,"web_url":"http://patchwork.ozlabs.org/comment/1771108/","msgid":"<20170919144738.g2xwjaph6bjvwt35@sirena.co.uk>","list_archive_url":null,"date":"2017-09-19T14:47:38","subject":"Re: [PATCH 1/2] regulator: core: Add coupled regulators mechanism","submitter":{"id":24878,"url":"http://patchwork.ozlabs.org/api/people/24878/","name":"Mark Brown","email":"broonie@kernel.org"},"content":"On Tue, Sep 19, 2017 at 04:35:54PM +0200, Maciej Purski wrote:\n\nPlease fix your mail client to word wrap within paragraphs at something\nsubstantially less than 80 columns.  Doing this makes your messages much\neasier to read and reply to.\n\n> On 09/19/2017 03:09 PM, Mark Brown wrote:\n\n> and from my new function regulator_set_coupled_voltage(). I added this in order to avoid\n> code duplication. I agree that the name might not be adequate. What name would you find more suitable?\n\nI think if the single regulator case isn't just a special case of the\nmulti regulator case then we're doing this wrong and there will be\nmaintainability problems so I'm not sure if this split makes sense at\nall.\n\n> > There's no locking here, and we appear to take no action when these\n> > counts change - do we need to bother with this at all?\n\n> Variable enable_count is used for checking if both regulators are enabled and there's a need for\n> using the coupling mechanism. It is checked in regulator_set_coupled_voltage_unlocked(), where the\n> mutex is already locked. I think that locking it here would be useful. Thanks.\n\nSo what happens if one regulator is enabled after the other and the\nconstraints become unsatisified?\n\n> > > +\t/* Get voltages desired by all consumers of the coupled regulator */\n> > > +\tfor (i = 0; i < 2; i++) {\n\n> > It appears we can't couple more than two regulators?\n\n> We can couple just two regulators. We have never found any case for coupling\n> more than two regulators. Limiting the mechanism to just two regulators simplifies\n> algorithm a little bit. Would you prefer it working for more than two\n> regulators also even if there isn't any use case?\n\nIt seems cleaner.","headers":{"Return-Path":"<devicetree-owner@vger.kernel.org>","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=<UNKNOWN>)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=sirena.org.uk header.i=@sirena.org.uk\n\theader.b=\"HphTQsnK\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xxQhw1Kb4z9sBZ\n\tfor <incoming-dt@patchwork.ozlabs.org>;\n\tWed, 20 Sep 2017 00:48:00 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751016AbdISOrp (ORCPT\n\t<rfc822;incoming-dt@patchwork.ozlabs.org>);\n\tTue, 19 Sep 2017 10:47:45 -0400","from heliosphere.sirena.org.uk ([172.104.155.198]:60874 \"EHLO\n\theliosphere.sirena.org.uk\" rhost-flags-OK-OK-OK-OK) by\n\tvger.kernel.org with ESMTP id S1751519AbdISOro (ORCPT\n\t<rfc822; devicetree@vger.kernel.org>); Tue, 19 Sep 2017 10:47:44 -0400","from debutante.sirena.org.uk ([2001:470:1f1d:6b5::3]\n\thelo=debutante) by heliosphere.sirena.org.uk with esmtpsa\n\t(TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.89)\n\t(envelope-from <broonie@sirena.co.uk>)\n\tid 1duJoJ-0001Vu-My; Tue, 19 Sep 2017 14:47:39 +0000","from broonie by debutante with local (Exim 4.89)\n\t(envelope-from <broonie@sirena.co.uk>)\n\tid 1duJoI-0001bJ-RS; Tue, 19 Sep 2017 15:47:38 +0100"],"DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed;\n\td=sirena.org.uk; s=20170815-heliosphere; h=In-Reply-To:Content-Type:\n\tMIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To:\n\tContent-Transfer-Encoding:Content-ID:Content-Description:Resent-Date:\n\tResent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id:\n\tList-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive;\n\tbh=VNYKW8Wx0627poTZFFZnvvQIWHlgtijvQZq4vsn7+q4=;\n\tb=HphTQsnKeaHUNmmVSJT4bJ96K\n\t7yWmMvSzGu8W+04n1sd/G/nwRxQvoVSS3t4KmctgRK3WoKxp8CPOoknapt4X+WoYy+Tmdwqw2kyBf\n\tr6E/QlvjGRROOTMvhDoh6FKkhT7X9keeAIHshKzkyV1WJoYijivx0u/NEn6xcDVy19y5Y=;","Date":"Tue, 19 Sep 2017 15:47:38 +0100","From":"Mark Brown <broonie@kernel.org>","To":"Maciej Purski <m.purski@samsung.com>","Cc":"linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,\n\tLiam Girdwood <lgirdwood@gmail.com>, Rob Herring <robh+dt@kernel.org>,\n\tMark Rutland <mark.rutland@arm.com>,\n\tMarek Szyprowski <m.szyprowski@samsung.com>,\n\tBartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>","Subject":"Re: [PATCH 1/2] regulator: core: Add coupled regulators mechanism","Message-ID":"<20170919144738.g2xwjaph6bjvwt35@sirena.co.uk>","References":"<1505723992-11772-1-git-send-email-m.purski@samsung.com>\n\t<CGME20170918084022eucas1p1398f18c5c90535ce484e3952fae80882@eucas1p1.samsung.com>\n\t<1505723992-11772-2-git-send-email-m.purski@samsung.com>\n\t<20170919130944.ldoskrm54tx2oemd@sirena.co.uk>\n\t<8ac4fa17-43ed-7a4d-d086-6ad81d97900e@samsung.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"mz7u2bgfyjphaezv\"","Content-Disposition":"inline","In-Reply-To":"<8ac4fa17-43ed-7a4d-d086-6ad81d97900e@samsung.com>","X-Cookie":"I'm also against BODY-SURFING!!","User-Agent":"NeoMutt/20170609 (1.8.3)","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"<devicetree.vger.kernel.org>","X-Mailing-List":"devicetree@vger.kernel.org"}}]