Re: [PATCH v2] regulator: pfuze100: add pfuze100 regulator driver

[Robin Gong]

Mark, 
	Thanks for your kindly review, please see below comments.

On Fri, Jul 12, 2013 at 03:40:37PM +0100, Mark Brown wrote:
> 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.
>
Thanks Mark, I will check carefully, although I have run the script...
> > +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.
> 
Accept.
> > +static const int pfuze100_swbst[] = {
> > +	5000000, 5050000, 5100000, 5150000,
> > +};
> 
> This looks like a linear map, the steps are all 50mV?
> 
Yes, but the swbst regulator share the same define type with the vsnvs regulator, and the later voltage table is not linear, so I use volt_table in swbst regulator . I don't want to  add another regulator type for this.
> > +	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).
> 
Yes, it will simplifies the code, but sometimes we will not use all the regulators on boards, in this case, Is it better that only register the available regulators?
> > +		/* 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.
> 
everyone of the four regulators(SW2~SW4) has two different linear voltage table which decided by the specific bit(one regulator ,one different bit) . So  will modify the voltage table dynamically before regulator register. I think this way is more simple , although looks little weird and uncomfortable.
> > +	for (i = 0; i < pfuze_chip->num_regulators; i++)
> > +		regulator_unregister(pfuze_chip->regulators[i]);
> > +	kfree(pfuze_chip);
> 
> Use devm_kzalloc().
Accept



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/