Messages in this thread | | | Subject | Re: [PATCH v3 4/5] regulators: tps65912: Add regulator driver for the TPS65912 PMIC | From | "Andrew F. Davis" <> | Date | Thu, 1 Oct 2015 11:08:38 -0500 |
| |
On 10/01/2015 10:33 AM, Grygorii Strashko wrote: > Hi Andrew, > > On 09/30/2015 03:29 PM, Andrew F. Davis wrote: >> On 09/30/2015 12:28 PM, Mark Brown wrote: >>> On Tue, Sep 29, 2015 at 01:58:41PM -0500, Andrew F. Davis wrote: >>>> On 09/29/2015 01:38 PM, Mark Brown wrote: >>> >>>>> 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. >>> >>> No, you don't need to use regulator-compatible - that's deprecated. >>> Just use the node names. >>> >> >> Are we sure matching on node names is a good idea? Most are just arbitrary >> names meant to be human readable and reference-able, giving them function >> may lead to confusion. This seems to be why we have "compatible", for >> specific >> identification of node function. But I'm new so maybe I'm wrong? >> >>>>> 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. >>> >>> I'm not sure what there is to add... if the regulator is only >>> instantiated when it features in the device tree then obviously it must >>> be included in the device to be instantiated. >>> >> >> This is already the case then, missing regulator nodes in old drivers >> will not >> get instantiated ether. And old drivers don't always store any more info >> about >> available regulators than mine does. >> >>>> 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. >>> >>> Only getting probed for device is in DT is exactly the problem here, and >>> nothing prevents us having separate modules for things without >>> enumerating everything in DT. >>> >> >> Sure, but then we have to do some fiddling with MFD_CORE to do that work, >> why not remove the dependency and let DT do that for DT only drivers? >> >>>> 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. >>> >>> Putting everything in DT means more work for people integrating the >>> device and means that we have to have a full and complete understanding >>> of the device at the time we write the DT, including decions about how >>> we split the functionality of the device between subsystems. >>> >> >> We are not adding anything extra to the DT node, we just use the >> "compatible" >> string to identify and match the node vs. "regulator-name", or the nodes >> name, >> or whatever else has been used. The node is then just filled with the >> standard >> optional properties just like every other driver's node. >> >>>>> 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. >>> >>> That's not a new driver, it's from more than a year ago (before or about >>> the same time the helpers got added IIRC). >>> >> >> Newer than a lot, I chose to base my driver off of that not just because >> it is a similar TI part, but because it was the cleanest, simplest looking >> one IMHO. The helpers would require more code (you need to know how many >> regulators you have and call the helpers in a loop). >> >> I have another PMIC I'm about to push a driver for when this gets >> figured out >> that does the same thing, and it's more important I think to do it this >> way for >> this new part. Some of the new regulators are designed without a dedicated >> SOC or board to power in mind, so they will have a whole bunch for >> different >> regulator types on one chip and it will be up to the designer to pick >> which ones >> to turn on and use. With this DT approach you can just list the ones you >> want, >> and we may even be able to split different types into different modules, >> then >> we can use the same regulator driver in different spins of the PMIC with >> more >> or less of that type of regulator, we just add that same node under a >> different >> parent PMIC driver. >> > > That's all make sense only if you have all information required for driver probing in DT. > But you don't, because a lot of information are still hard-coded in driver: > - regulator IDs, > - registers required to control regulator > - ranges information and etc. > > So, you will not be be able to easily separate some regulator and reuse it with another PMIC. >
Unless we encode the needed registers/info into the DT, that info could be considered a device description if you consider each regulator to be a separate device burned onto the same silicon, much like we do with the different IPs on SOCs. It would be nice to have 100% generic regulator drivers, but the DT maintainers might not go for that :)
> Actually, both approaches have right to live and which one to select depends on > functionality which has to be implemented by regulator driver. > For example, with this (your) approach the separate platform device will be created > for each regulator, so, if needed, corresponding driver's callback (.remove/shutdown and > dev_pm_ops) can be implemented individually for each regulator. Do you need this? > As for me, such approach is reasonable if you have devices which represents one/two regulators max. > > The disadvantage of this approach is that you need separate compatible string fore each regulator > - and this, actually, is usually banned by DT maintainers ;) > > For devices, like this one (or twl, or plamas) which has dozens of regulators it simply make no sense :) > Just imaging that tomorrow tps65912-1 will be released and it will be fully compatible with tps65912, but > with one exception - it will have dcdc5 and all followed registers offsets will be shifted. > What will you do? All this code, based on compatible strings, will not work any more. > And you will need to introduce another bunch of compatible strings.. >
Agree, unless we go with my above idea! :)
> By the way, this implementation is not optimal any way :) > - you are using compatible string to get tps65912_pmic_regs structure > - then tps65912_pmic_regs is used to get regulator ID > - and then, finally, regulator ID is used to get regulator_desc structure >
Yeah, I saw that after I posted this, my new version just puts a pointer to the right regulator_desc struct in the of_device_id.data, then we can use it directly with no middle man steps, much cleaner.
> And, finally, pay attention pls, that regulator_of_get_init_data() is called from > from regulator_register(). >
Right, but only if you supply .of_match in your regulator_desc, and even then it only matches on node name, not "compatible" string, if it did I could use it to eliminate my call to of_get_regulator_init_data, the whole probe function would become about 10 lines of code :)
> So, in my opinion, the tps65217-regulator.c driver is really good example of how it could be done. > >
Thanks, I was looking for good examples, that and rt5033 seem to be the requested style for new regulator drivers.
-- Andrew F. Davis
| |