Re: [PATCH 1/2] AB3100 regulator support v2
From: Linus Walleij
Date: Mon Aug 31 2009 - 10:16:24 EST
2009/8/31 Mark Brown <broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx>:
> On Sun, 2009-08-30 at 23:29 +0200, Linus Walleij wrote:
>
>> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxxxxxx>
>> Reviewed-by: Mark Brown <broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx>
>
> It's probably better to only use this if someone's reviewed the driver
> and said it's OK.
Yeah OK sorry man, I wouldn't ever try to merge it until you say it's OK though.
> Overall this looks pretty good, it's addressed almost all of the issues
> I had last time. There's a few sticky bits below, though.
>
>> + err = ab3100_get_register_interruptible(abreg->ab3100, abreg->regreg,
>> + ®val);
>> + if (err) {
>> + if (err != -ERESTARTSYS)
>> + dev_err(®->dev, "unable to get register 0x%x\n",
>> + abreg->regreg);
>> + else
>> + dev_info(®->dev,
>> + "interrupted while getting register 0x%x\n",
>> + abreg->regreg);
>> + return err;
>> + }
>
> I did query last time if having these operations be interruptible is a
> good idea - I can't see it helping robustness, it's not something that
> other drivers are doing and it'd complicate things for all API users to
> add handling for the error. I don't recall any discussion of the
> thinking here?
OK sorry for missing this, I was planning to follow up on the previous
mail but forgot.
I recently renamed all the ab3100 accessor functions to *_interruptible
to reflect the fact that the accessor mutex on ab3100 uses
mutex_lock_interruptible() so this suffix should propagate so it is
clear that stuff like -ERESTARTSYS can be returned.
So the above errorcheck is probably bogus.
That said, I think the regulator paths are entirely in-kernel and
under such circumstances that signals from userspace are blocked
anyway. The problem is that the ab3100 is accessed by complex
userspace programs and I2C is sometimes slow so there is a need
for being able to interrupt it, but I *could* go in and use an
uniterruptable mutex if you prefer that, I'll ask around here if
we should do this. Can the function name stand as it is for the time being?
>> + bestmatch = INT_MAX;
>> + bestindex = -1;
>> + for (i = 0; i < abreg->voltages_len; i++) {
>> + if (abreg->typ_voltages[i] <= max_uV &&
>> + abreg->typ_voltages[i] >= min_uV &&
>> + abreg->typ_voltages[i] < bestmatch) {
>> + bestmatch = abreg->typ_voltages[i];
>> + bestindex = i;
>> + }
>> + }
>> +
>> + if (i < 0) {
>> + dev_warn(®->dev, "requested %d<=x<=%d uV, out of range!\n",
>> + min_uV, max_uV);
>
> That should be a check for bestindex, not i - i will always be
> abreg->voltages_len.
How embarrassing. Will fix in v3. Thanks Mark.
>> +/*
>> + * The external regulator just calls back into the platform
>> + * board setup to get/set the regulator.
>> + */
>> +static int ab3100_get_voltage_regulator_external(struct regulator_dev *reg)
>> +{
>> + struct ab3100_regulator *abreg = reg->reg_data;
>> +
>> + if (abreg->plfdata->get_ext_voltage)
>> + return abreg->plfdata->get_ext_voltage();
>> + return -ENXIO;
>> +}
>
> Hrm. I suspect that you either want to add some platform data to
> specify the voltage as a plain number or just have boards use the
> regulator supply mechanism with a fixed voltage regulator supplied by
> this one if they need to specify the voltage of the supply.
I was designing for it to be controllable but not controllable by the
AB3100 driver, perhaps it is a regulator somewhere else here,
defined in the board data. But I went for a fixed int member
voltage setting for the time being, we can discuss that stuff later
when I have some practical use for it.
>> + /* Set up regulators */
>> + for (i = 0; i < ARRAY_SIZE(ab3100_regulators); i++) {
>> + /* This regulator is special and has to be set last */
>> + if (ab3100_regulators[i].regreg == AB3100_LDO_D) {
>> + ldo_d_val = plfdata->reg_initvals[i];
>> + continue;
>> + }
>> +
>> + err = ab3100_set_register_interruptible(ab3100,
>> + ab3100_regulators[i].regreg,
>> + plfdata->reg_initvals[i]);
>> + if (err) {
>> + dev_err(&pdev->dev, "regulator initialization failed with error %d\n",
>> + err);
>> + return err;
>> + }
>> + }
>
> It is a big improvement to have the configuration be specified as
> platform data but I'm still a bit concerned about the idea of providing
> this power sequencing as a driver-local feature.
>
> The regulator API already has mechanisms for setting the default state
> for regulators and the general problem of sequencing the initial setup
> isn't specific to this chip. There's currently no sequencing support in
> the API, largely because most systems have some explicit power
> sequencing in the hardware and a specific way of signalling the PMIC to
> kick off the powerdown or suspend sequences so it's not a big deal. I
> suspect that with the implementation you've got here you might run into
> trouble with systems which need some sequencing beyond just ordering
> everything else with respect to LDO D, which I suspect is more likely to
> occur if you need this level of soft implementation. I'd also be worried
> that any core sequencing that is introduced (I expect we will need it at
> some point - possibly soon, Mike Rappaport has some issues that look
> like they might need to be fixed sequencing support) might break your
> systems, though I think that may just be an excess of caution.
>
> I've been having a bit of a think about the best way to handle this;
> it'd mean that we'd need to have some way for a machine driver to
> provide sequences for the power on and off transitions that we might
> need to do. Working out how to actually run the sequences would be
> slightly tricky - we could wait for all the mentioned regulators to be
> registered but that'd create issues if some of the later regulators in
> the sequence depend on the earlier parts of the sequence for
> registration.
>
> Have you looked at the possibility of integrating this into the core?
I wish I could... I'm a bit newcomer on the regulator API, I'm learning
as we speak.
I can promise to go in and use such a framework once it's available.
If this sequence is a dependency graph of regulators that need to
have deps in all strange directions you get a directed graph
http://en.wikipedia.org/wiki/Directed_graph
Or you could limit yourself to a directed acyclic graph
http://en.wikipedia.org/wiki/Directed_acyclic_graph
in either case it's rather a delicate computational problem
but I guess you're after a simple linear sequence here, like
switch on A, B, C, D ... N in a special order?
In my case it's actually not the switching-on or of that is
the problem, it's more of putting some magic numbers
into some registers in a special order (well, any order
actually except for one register that is special).
I'll see if I can think of something more elegant to
make this more appealing, like tagging each default
register value with a sequence number or so.
Yours,
Linus Walleij
--
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/