Re: [PATCH v3 4/5] regulators: tps65912: Add regulator driver for the TPS65912 PMIC
From: Andrew F. Davis
Date: Thu Oct 01 2015 - 12:08:59 EST
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/