On Fri, Jul 12, 2013 at 12:54:15PM +0800, Robin Gong wrote: > Add pfuze100 regulator driver.
This looks mostly good. A few small issues below but nothing major.
> +enum pfuze_id {
> + PFUZE_ID_PFUZE100,
> + PFUZE_ID_INVALID,
> +};
> +struct pfuze_chip {
Missing blank line here - there are a few other small coding style
things, checkpatch should help.
> +static struct regulator_ops pfuze100_ldo_regulator_ops;
> +static struct regulator_ops pfuze100_fixed_regulator_ops;
> +static struct regulator_ops pfuze100_sw_regulator_ops;
> +static struct regulator_ops pfuze100_swb_regulator_ops;
Better to just reorder things so that no forward declaration is needed.
> +static const int pfuze100_swbst[] = {
> + 5000000, 5050000, 5100000, 5150000,
> +};
This looks like a linear map, the steps are all 50mV?
> + num_regulators = pfuze_get_num_regulators_dt(&client->dev);
> + if (num_regulators <= 0 && pdata)
> + num_regulators = pdata->num_regulators;
> + if (num_regulators <= 0) {
> + dev_err(&client->dev, "no platform data,please add it!\n");
> + ret = -EINVAL;
> + return ret;
You should just register all the regulators rather than only registering
those that the user explicitly selects. This allows users to inpect the
current configuration and simplifies the code - for example you don't
need to count the DT nodes and you can just have a simple array in the
platform data (see how wm831x does this for an example).
> + /* SW2~SW4 high bit check and modify the voltage value table */
> + if (i > PFUZE100_SW1C && i < PFUZE100_SWBST) {
> + regmap_read(pfuze_chip->regmap, PFUZE100_SW2VOL +
> + (i - PFUZE100_SW2) * 7, &val);
> + if (val & 0x40) {
> + pfuze100_regulators[id].desc.min_uV = 800000;
> + pfuze100_regulators[id].desc.uV_step = 50000;
> + }
> + }
You should really be doing this on a copy of the regulators table rather
than on the table itself.
> + for (i = 0; i < pfuze_chip->num_regulators; i++)
> + regulator_unregister(pfuze_chip->regulators[i]);
> + kfree(pfuze_chip);
Use devm_kzalloc().
signature.asc
Description: Digital signature
_______________________________________________ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
