From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Andrew F. Davis" Subject: Re: [PATCH v3 4/5] regulators: tps65912: Add regulator driver for the TPS65912 PMIC Date: Tue, 29 Sep 2015 13:58:41 -0500 Message-ID: <560ADF61.9030105@ti.com> References: <1443106374-4126-1-git-send-email-afd@ti.com> <1443106374-4126-5-git-send-email-afd@ti.com> <20150925180533.GN30445@sirena.org.uk> <5605AA1C.3090905@ti.com> <20150929151320.GT30445@sirena.org.uk> <560AD3B2.1070102@ti.com> <20150929183818.GA15635@sirena.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from comal.ext.ti.com ([198.47.26.152]:38422 "EHLO comal.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965241AbbI2S7A (ORCPT ); Tue, 29 Sep 2015 14:59:00 -0400 In-Reply-To: <20150929183818.GA15635@sirena.org.uk> Sender: linux-gpio-owner@vger.kernel.org List-Id: linux-gpio@vger.kernel.org To: Mark Brown Cc: Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Lee Jones , Linus Walleij , Alexandre Courbot , Samuel Ortiz , Liam Girdwood , linux-gpio@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org On 09/29/2015 01:38 PM, Mark Brown wrote: > On Tue, Sep 29, 2015 at 01:08:50PM -0500, Andrew F. Davis wrote: >> On 09/29/2015 10:13 AM, Mark Brown wrote: > >>>> sure that will save me anything as my probe function is called with a DT >>>> match already, so no searching is needed. > >>> You've not understood what that change is replacing, the code I'm >>> quoting above is exactly that code. Check out some of the existing >>> drivers using this API. > >> Looking at other drivers that use this API they all call regulator_register >> in a loop in their probe, once for each possible regulator, in this case >> letting the API do the DT node search makes sense. My probe on the other-hand >> is only called when we already have a DT match, therefor searching is not >> necessary and all I have to do is call of_get_regulator_init_data myself on >> the already found DT node. No need to add node names to my regulator_desc >> and make the API re-search for the node. > > Oh, ick. The binding has a compatible string in the individual > regulator bindings which is broken unless there really are lots of > variants being configured via DT (which is just not the case here). > It's not only more typing in the DT, I don't see this, the alternative is matching to this "regulator-compatible", why not just use the existing compatible. > it also means that we can't read > back the configuration of the device unless the user goes and creates a > DT which explicitly lists each regulator on the device which is > unhelpful. We should be able to read back the configurations of all the > regulators by simply listing the device in DT. > Could you expand this? I'm not sure I understand why we still cant do this using this new way. Bindings should have compatible strings when they describe hardware like this, we can then do stuff like put the LDO and DCDC drivers in separate modules for instance, letting DT only load what we need. There are other benefits like not having to search our own DT binding for data, and we only get probed for devices in the DT. This also eliminates the need for MFD_CORE, we just call of_platform_populate on ourself and DT helpers do the rest. Why hard code mfd_cell's and do matching when DT does the same thing. > The fact that this is different to the bindings for other regulator > drivers and requires more code ought to have been a big warning sign > here :( > The binding is the same as the new tps65218 driver, different isn't always a warning sign. And what do you mean "requires more code"? This regulator driver is smaller than almost any other. DT takes care of everything for us relating to hardware instantiation like it should. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965349AbbI2S7J (ORCPT ); Tue, 29 Sep 2015 14:59:09 -0400 Received: from comal.ext.ti.com ([198.47.26.152]:38422 "EHLO comal.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965241AbbI2S7A (ORCPT ); Tue, 29 Sep 2015 14:59:00 -0400 Subject: Re: [PATCH v3 4/5] regulators: tps65912: Add regulator driver for the TPS65912 PMIC To: Mark Brown References: <1443106374-4126-1-git-send-email-afd@ti.com> <1443106374-4126-5-git-send-email-afd@ti.com> <20150925180533.GN30445@sirena.org.uk> <5605AA1C.3090905@ti.com> <20150929151320.GT30445@sirena.org.uk> <560AD3B2.1070102@ti.com> <20150929183818.GA15635@sirena.org.uk> CC: Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Lee Jones , Linus Walleij , Alexandre Courbot , Samuel Ortiz , Liam Girdwood , , , From: "Andrew F. Davis" Message-ID: <560ADF61.9030105@ti.com> Date: Tue, 29 Sep 2015 13:58:41 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <20150929183818.GA15635@sirena.org.uk> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 09/29/2015 01:38 PM, Mark Brown wrote: > On Tue, Sep 29, 2015 at 01:08:50PM -0500, Andrew F. Davis wrote: >> On 09/29/2015 10:13 AM, Mark Brown wrote: > >>>> sure that will save me anything as my probe function is called with a DT >>>> match already, so no searching is needed. > >>> You've not understood what that change is replacing, the code I'm >>> quoting above is exactly that code. Check out some of the existing >>> drivers using this API. > >> Looking at other drivers that use this API they all call regulator_register >> in a loop in their probe, once for each possible regulator, in this case >> letting the API do the DT node search makes sense. My probe on the other-hand >> is only called when we already have a DT match, therefor searching is not >> necessary and all I have to do is call of_get_regulator_init_data myself on >> the already found DT node. No need to add node names to my regulator_desc >> and make the API re-search for the node. > > Oh, ick. The binding has a compatible string in the individual > regulator bindings which is broken unless there really are lots of > variants being configured via DT (which is just not the case here). > It's not only more typing in the DT, I don't see this, the alternative is matching to this "regulator-compatible", why not just use the existing compatible. > it also means that we can't read > back the configuration of the device unless the user goes and creates a > DT which explicitly lists each regulator on the device which is > unhelpful. We should be able to read back the configurations of all the > regulators by simply listing the device in DT. > Could you expand this? I'm not sure I understand why we still cant do this using this new way. Bindings should have compatible strings when they describe hardware like this, we can then do stuff like put the LDO and DCDC drivers in separate modules for instance, letting DT only load what we need. There are other benefits like not having to search our own DT binding for data, and we only get probed for devices in the DT. This also eliminates the need for MFD_CORE, we just call of_platform_populate on ourself and DT helpers do the rest. Why hard code mfd_cell's and do matching when DT does the same thing. > The fact that this is different to the bindings for other regulator > drivers and requires more code ought to have been a big warning sign > here :( > The binding is the same as the new tps65218 driver, different isn't always a warning sign. And what do you mean "requires more code"? This regulator driver is smaller than almost any other. DT takes care of everything for us relating to hardware instantiation like it should.