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:broo...@kernel.org] Sent: Tuesday, July 09, 2013 5:44 PM To: Gong Yibin-B38343 Cc: linux-kernel@vger.kernel.org; grant.lik...@linaro.org; rob.herr...@calxeda.com; lgirdw...@gmail.com 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
