Messages in this thread | | | Date | Thu, 11 Jun 2015 18:05:17 +0800 | From | Yi Zhang <> | Subject | Re: [PATCH 2/2] mfd: 88pm88x: initial 88pm886/88pm880 base support |
| |
On Tue, Jun 09, 2015 at 02:14:11PM +0200, Paul Bolle wrote:
Hi, Paul:
Thanks very much for your review; > On Mon, 2015-06-08 at 20:55 +0800, Yi Zhang wrote: > > --- /dev/null > > +++ b/drivers/mfd/88pm880-table.c > > > +#include <linux/module.h> > > I'm _guessing_ this could as well be <linux/export.h>.
Yes, you are right; > > > +EXPORT_SYMBOL_GPL(pm880_base_i2c_regmap); > > > +EXPORT_SYMBOL_GPL(pm880_power_i2c_regmap); > > > +EXPORT_SYMBOL_GPL(pm880_gpadc_i2c_regmap); > > > +EXPORT_SYMBOL_GPL(pm880_battery_i2c_regmap); > > > +EXPORT_SYMBOL_GPL(pm880_test_i2c_regmap); > > > +EXPORT_SYMBOL_GPL(pm880_cell_devs); > > > +EXPORT_SYMBOL_GPL(pm880_cell_info); > > > +EXPORT_SYMBOL_GPL(pm880_apply_patch); > > > --- /dev/null > > +++ b/drivers/mfd/88pm886-table.c > > > +#include <linux/module.h> > > See above.
OK, got it; > > > +EXPORT_SYMBOL_GPL(pm886_base_i2c_regmap); > > > +EXPORT_SYMBOL_GPL(pm886_power_i2c_regmap); > > > +EXPORT_SYMBOL_GPL(pm886_gpadc_i2c_regmap); > > > +EXPORT_SYMBOL_GPL(pm886_battery_i2c_regmap); > > > +EXPORT_SYMBOL_GPL(pm886_test_i2c_regmap); > > > +EXPORT_SYMBOL_GPL(pm886_cell_devs); > > > +EXPORT_SYMBOL_GPL(pm886_cell_info); > > > +EXPORT_SYMBOL_GPL(pm886_apply_patch); > > All of the exports until now are all made public via > drivers/mfd/88pm88x.h. So they're not available for code outside of the > files this patch adds. I think the EXPORT_SYMBOL_GPL() lines are not > needed. Is that right?
I think so, yes, thanks for pointing this out; > > > --- /dev/null > > +++ b/drivers/mfd/88pm88x-core.c > > > +#include <linux/module.h> > > See above.
OK, got it; > > > +EXPORT_SYMBOL_GPL(pm88x_of_match); > > This export is made available through include/linux/mfd/88pm88x.h. Do > you expect pm88x_of_match to be used by some other module?
my fault, I am not expecting it's used by other modules, I'll change it; thanks; > > > --- /dev/null > > +++ b/drivers/mfd/88pm88x-i2c.c > > > +#include <linux/module.h> > > > +MODULE_DEVICE_TABLE(i2c, pm88x_i2c_id); > > > +static int pm88x_i2c_init(void) > > No __init here?
Thanks for pointing this out; > > > +{ > > + int ret; > > + > > + ret = i2c_add_driver(&pm88x_i2c_driver); > > + if (ret != 0) { > > + pr_err("88pm88x I2C registration failed %d\n", ret); > > + return ret; > > + } > > + > > + return 0; > > +} > > +subsys_initcall(pm88x_i2c_init); > > + > > +static void pm88x_i2c_exit(void) > > No __exit here?
Thanks for pointing this out, I'll add; > > > +{ > > + i2c_del_driver(&pm88x_i2c_driver); > > +} > > +module_exit(pm88x_i2c_exit); > > But if this code stays built-in only than pm88x_i2c_exit() would never > be called. (I think it will be thrown away during the build, actually.) > So then the entire function should be dropped.
I agree with you; but actually, my intention is supporting this chip to work as module, so it is still needed, am I right? thanks;
> > > +MODULE_DESCRIPTION("88pm88x I2C bus interface"); > > +MODULE_AUTHOR("Yi Zhang<yizhang@marvell.com>"); > > +MODULE_LICENSE("GPL v2"); > > > --- /dev/null > > +++ b/drivers/mfd/88pm88x-irq.c > > > +#include <linux/module.h> > > Needed?
I will remove it; > > > --- a/drivers/mfd/Kconfig > > +++ b/drivers/mfd/Kconfig > > > +config MFD_88PM88X > > + bool "Marvell 88PM886/880 PMIC" > > + depends on I2C=y > > + select REGMAP_I2C > > + select MFD_CORE > > + help > > + This supports for Marvell 88PM88X Series Power Management IC: > > + 88pm886 and 88pm880; > > + This includes the I2C driver, the interrupt resource distribution > > + and the core APIs, for individual sub-device as voltage regulators, > > + RTC, charger, fuelgauge, etc please select under the corresponding menus. > > > --- a/drivers/mfd/Makefile > > +++ b/drivers/mfd/Makefile > > > +88pm88x-objs := 88pm88x-core.o 88pm88x-i2c.o 88pm88x-irq.o 88pm886-table.o 88pm880-table.o > > +obj-$(CONFIG_MFD_88PM88X) += 88pm88x.o > > MFD_88PM88X is bool, but the patch adds module specific code too (ie, > code that serves no purpose when built-in). Did you perhaps intend for > MFD_88PM88X to be tristate? > Yes, I should use tristate; thanks for pointing this out; > Thanks, > > > Paul Bolle >
| |