Messages in this thread | | | Subject | Re: [PATCH 1/2] AB3100 regulator support v2 | From | Mark Brown <> | Date | Mon, 31 Aug 2009 13:44:01 +0100 |
| |
On Sun, 2009-08-30 at 23:29 +0200, Linus Walleij wrote:
> Signed-off-by: Linus Walleij <linus.walleij@stericsson.com> > Reviewed-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
It's probably better to only use this if someone's reviewed the driver and said it's OK.
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?
> + 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.
> +/* > + * 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.
> + /* 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?
| |