On Sat, Sep 27, 2014 at 12:59:48AM +0000, Stefan Wahren wrote:
> + pr_debug("%s: min_uV %d, max_uV %d, min %d, max %d\n", __func__,
> + min_uV, max_uV, con->min_uV, con->max_uV);
> +
> + if (max_uV < con->min_uV || max_uV > con->max_uV)
> + return -EINVAL;
This is replicating checks done by the core.
> + val = (max_uV - con->min_uV) * sreg->rdesc.n_voltages /
> + (con->max_uV - con->min_uV);
Drivers should never look at their constraints, they should let the core
do that and just do what they're asked. In this case it is probably
best to implement a get_voltage_sel() operation and have the conversion
to voltage done by regulator_map_voltage_linear(), this will both make
the code look better and mean the driver gets the benefit of all the
error checking done by the core.
> + writel(val | regs, sreg->base_addr);
> + for (i = 20; i; i--) {
> + /* delay for fast mode */
> + if (readl(power_sts) & BM_POWER_STS_DC_OK)
> + return 0;
> +
> + udelay(1);
> + }
> +
> + writel(val | regs, sreg->base_addr);
> + start = jiffies;
> + while (1) {
> + /* delay for normal mode */
> + if (readl(power_sts) & BM_POWER_STS_DC_OK)
> + return 0;
This really needs a comment to explain what on earth is going on here -
the whole thing with writing the same thing twice with two delays is
more than a little odd. It looks like the driver is trying to busy wait
in cases where the change happens quickly but the comments about "fast"
and "normal" mode make this unclear.
> + pr_debug("%s: %s register val %d\n", __func__, sreg->name, val);
> +
> + switch (sreg->rdesc.id) {
> + case MXS_VDDA:
> + val >>= 16;
> + break;
> + case MXS_VDDD:
> + val >>= 20;
> + break;
> + }
> +
> + return val ? 1 : 0;
> +}
This seems buggy - it'll always return true for MXS_VDDD if MXS_VDDA
enabled won't it?
> +static unsigned int mxs_get_mode(struct regulator_dev *reg)
> +{
> + struct mxs_regulator *sreg = rdev_get_drvdata(reg);
> + u32 val = readl(sreg->base_addr) & sreg->mode_mask;
> +
> + return val ? REGULATOR_MODE_FAST : REGULATOR_MODE_NORMAL;
> +}
Please try to avoid the ternery operator, it does nothing for
legibility.
> + if (of_property_read_string(np, "regulator-name", &name)) {
> + dev_err(dev, "missing property regulator-name\n");
> + return -EINVAL;
> + }
Use different compatibles to identify the regulators, regulator-name
should never be mandatory.
> + switch (sreg->rdesc.id) {
> + case MXS_VDDIO:
> + sreg->mode_mask = BIT(17);
> + break;
> + case MXS_VDDA:
> + sreg->mode_mask = BIT(18);
> + break;
> + case MXS_VDDD:
> + sreg->mode_mask = BIT(22);
> + break;
> + }
Why is this not looked up from the data structure like the rest of the
data?
> + dev_info(dev, "%s found\n", name);
Don't add noise like this to the boot log, it provides no useful
information.
> + regulator_unregister(rdev);
> + iounmap(power_addr);
> + iounmap(base_addr);
Use devm_ versions of functions.
> +static int __init mxs_regulator_init(void)
> +{
> + return platform_driver_register(&mxs_regulator_driver);
> +}
> +postcore_initcall(mxs_regulator_init);
module_platform_driver().
Attachment:
signature.asc
Description: Digital signature
