RE: [PATCH v1] regulator: pfuze100: add pfuze100 regulator driver

[Gong Yibin-B38343]

Thanks Mark. The pfuze driver refer to the mc13xxx regulator driver, so I didn't notice regulator core make some changes and some work have been done by regulator core. Sorry.. I will update the patch later.

-----Original Message-----
From: Mark Brown [mailto:broonie@xxxxxxxxxx] 
Sent: Tuesday, July 09, 2013 5:44 PM
To: Gong Yibin-B38343
Cc: linux-kernel@xxxxxxxxxxxxxxx; grant.likely@xxxxxxxxxx; rob.herring@xxxxxxxxxxx; lgirdwood@xxxxxxxxx
Subject: Re: [PATCH v1] regulator: pfuze100: add pfuze100 regulator driver

On Tue, Jul 09, 2013 at 02:38:00PM +0800, Robin Gong wrote:

I've not fully reviewed this, the major thing here is that the driver is not making use of the regmap helpers for anything - most of the code in this driver looks like it duplicates standard code available in the core.

> @@ -45,6 +45,7 @@ obj-$(CONFIG_REGULATOR_MAX77693) += max77693.o
>  obj-$(CONFIG_REGULATOR_MC13783) += mc13783-regulator.o
>  obj-$(CONFIG_REGULATOR_MC13892) += mc13892-regulator.o
>  obj-$(CONFIG_REGULATOR_MC13XXX_CORE) +=  mc13xxx-regulator-core.o
> +obj-$(CONFIG_REGULATOR_PFUZE100) += pfuze100-regulator.o
>  obj-$(CONFIG_REGULATOR_PALMAS) += palmas-regulator.o
>  obj-$(CONFIG_REGULATOR_TPS51632) += tps51632-regulator.o
>  obj-$(CONFIG_REGULATOR_PCAP) += pcap-regulator.o @@ -72,6 +73,7 @@ 
> obj-$(CONFIG_REGULATOR_WM831X) += wm831x-ldo.o
>  obj-$(CONFIG_REGULATOR_WM8350) += wm8350-regulator.o
>  obj-$(CONFIG_REGULATOR_WM8400) += wm8400-regulator.o
>  obj-$(CONFIG_REGULATOR_WM8994) += wm8994-regulator.o
> +obj-$(CONFIG_REGULATOR_PFUZE100) += pfuze100-regulator.o

This looks broken...

> +++ b/drivers/regulator/pfuze-regulator.h
> @@ -0,0 +1,110 @@

Why is this a separate file?

> +struct pfuze_regulator {
> +	struct regulator_desc desc;
> +	unsigned int reg;
> +	unsigned int stby_reg;
> +	unsigned char enable_bit;
> +	unsigned char stby_bit;
> +	unsigned char vsel_shift;
> +	unsigned char vsel_mask;
> +	unsigned char stby_vsel_shift;
> +	unsigned char stby_vsel_mask;
> +	int const *voltages;

This pretty much all looks like you are duplicating standard framework features - things like the vsels for example.

> +static const int pfuze100_sw1[] = {
> +	300000, 325000, 350000, 375000, 400000, 425000, 450000, 475000,
> +	500000, 525000, 550000, 575000, 600000, 625000, 650000, 675000,
> +	700000, 725000, 750000, 775000, 800000, 825000, 850000, 875000,
> +	900000, 925000, 950000, 975000, 1000000, 1025000, 1050000, 1075000,
> +	1100000, 1125000, 1150000, 1175000, 1200000, 1225000, 1250000, 1275000,
> +	1300000, 1325000, 1350000, 1375000, 1400000, 1425000, 1450000, 1475000,
> +	1500000, 1525000, 1550000, 1575000, 1600000, 1625000, 1650000, 1675000,
> +	1700000, 1725000, 1750000, 1775000, 1800000, 1825000, 1850000, 
> +1875000, };

This looks like a linear map, use one if you can.  Similarly for a bunch of the other tables (possibly all of them).

> +#if defined(CONFIG_OF)
> +static const struct of_device_id pfuze_dt_ids[] = {
> +	{ .compatible = "fsl,pfuze100", .data = (void *)PFUZE_ID_PFUZE100},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, pfuze_dt_ids); #endif

No need for ifdefs.

> +static int pfuze100_regulator_enable(struct regulator_dev *rdev)

You should be using the standard regmap helpers for this.

> +int pfuze100_regulator_list_voltage(struct regulator_dev *rdev,
> +				    unsigned selector)

This too (even if you need to use the tables there are helpers for that too).

> +static int pfuze100_regulator_sw_standby_enable(struct regulator_dev 
> +*rdev) {
> +	return 0;
> +}
> +static int pfuze100_regulator_sw_standby_disable(struct regulator_dev 
> +*rdev) {
> +	return 0;
> +}
> +static struct regulator_ops pfuze100_sw_regulator_ops = {

Coding style and you shouldn't have any empty functions.

> +#ifdef CONFIG_OF
> +
> +static int  pfuze_get_num_regulators_dt(struct device *dev) {

You need to document any new device tree bindings.

> +static struct regmap_config pfuze_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +
> +	.max_register = PFUZE_NUMREGS,
> +
> +	.cache_type = REGCACHE_NONE,

This is the defualt, though you should consider caching for performance reasons.

> +subsys_initcall(pfuze100_regulator_init);

Use module_i2c_driver() (and watch out for coding style).

--
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/