On Wed, Apr 29, 2015 at 10:32:26PM +0000, Stefan Wahren wrote:
> +static struct regulator_ops mxs_dcdc_ops = {
> + .is_enabled = regulator_is_enabled_regmap,
> +};
Why do we have an is_enabled() operation but no enable or disable
operation? I'm also not 100% clear what code the DCDCs and LDOs are
sharing here...
> + initdata = of_get_regulator_init_data(dev, dev->of_node, &info->desc);
> + if (!initdata) {
> + dev_err(dev, "missing regulator init data\n");
> + return -EINVAL;
> + }
This is not an error, init data is totally optional.
> + rdev = devm_regulator_register(dev, &info->desc, &config);
> + if (IS_ERR(rdev)) {
> + int ret = PTR_ERR(rdev);
> +
> + dev_err(dev, "%s: failed to register regulator(%d)\n",
> + __func__, ret);
> + return ret;
> + }
> +
> + if (!of_property_read_u32(dev->of_node, "switching-frequency",
> + &switch_freq))
> + mxs_set_dcdc_freq(rdev, switch_freq);
why are we registering the regulator before we've finished parsing the
DT - I'd expect us to do this first rather than have things potentially
start using the regulator with the confguration not completed.
> + platform_set_drvdata(pdev, rdev);
This looks unsafe - either we don't need to set this at all or one of
the ops could be invoked when the regulator is registered and try to use
the driver data before it is set.
Attachment:
signature.asc
Description: Digital signature
