On Tue, Dec 17, 2013 at 01:36:35PM +0800, Wenyou Yang wrote:
> Signed-off-by: Wenyou Yang <wenyou.yang@xxxxxxxxx>
Much better. Still a few small issues though.
> +/* ACt8865 voltage table */
> +static const u32 act8865_voltages_table[] = {
This is the wrong type for a voltage table but it looks like it
shouldn't be a voltage table at all - this looks like it should be
mapped as linear ranges.
> +static struct regulator_ops act8865_ops = {
> + .list_voltage = regulator_list_voltage_table,
> + .get_voltage_sel = regulator_get_voltage_sel_regmap,
> + .set_voltage_sel = regulator_set_voltage_sel_regmap,
> + .enable = regulator_enable_regmap,
> + .disable = regulator_disable_regmap,
> + .is_enabled = regulator_is_enabled_regmap,
> + .set_suspend_voltage = act8865_set_suspend_voltage,
> + .set_suspend_enable = regulator_enable_regmap,
> + .set_suspend_disable = regulator_disable_regmap,
> +};
This is missing a map_voltage() operation.
> +#ifdef CONFIG_OF
> +static const struct of_device_id act8865_dt_ids[] = {
> + { .compatible = "active-semi,act8865" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, act8865_dt_ids);
active-semi needs adding to vendor-prefixes.txt.
> + np = of_find_node_by_name(dev->of_node, "regulators");
> + if (!np) {
> + dev_err(dev, "missing 'regulators' subnode in DT\n");
> + return -EINVAL;
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(rmatch); i++)
> + rmatch[i].name = act8865_reg[i].name;
> +
> + matched = of_regulator_match(dev, np, rmatch, ARRAY_SIZE(rmatch));
> + if (matched <= 0)
> + return matched;
You should register all regulators the device has regardless of how many
have parameters configured in the DT so that the settings can be read
back for diagnostic purposes.
> + if (dev->of_node && !pdata) {
> + const struct of_device_id *id;
> + struct act8865_platform_data pdata_of;
> +
> + id = of_match_device(of_match_ptr(act8865_dt_ids), dev);
> + if (!id)
> + return -ENODEV;
> +
> + ret = act8865_pdata_from_dt(dev, of_node, &pdata_of);
> + if (ret < 0)
> + return ret;
> +
> + pdata = &pdata_of;
> + } else {
> + memset(of_node, 0, sizeof(of_node));
> + }
What's this memset() for?
> + rdev[i] = devm_regulator_register(&client->dev,
> + &act8865_reg[i], &config);
> +err_unregister:
> + while (--i >= 0)
> + regulator_unregister(rdev[i]);
The whole purpose of devm_ APIs is to avoid the need to explicitly
unregister, you shouldn't need to call regulator_unregister() at all.
Attachment:
signature.asc
Description: Digital signature
