Messages in this thread | | | Subject | Re: [PATCH 2/2] mfd: 88pm88x: initial 88pm886/88pm880 base support | From | Paul Bolle <> | Date | Thu, 11 Jun 2015 15:52:08 +0200 |
| |
On Thu, 2015-06-11 at 18:05 +0800, Yi Zhang wrote: > On Tue, Jun 09, 2015 at 02:14:11PM +0200, Paul Bolle wrote: > > 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;
In that case the include of export.h is probably not needed. Please check, I didn't check myself.
> > > --- /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;
Ditto for export.h here.
> > > +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;
Yes, since you also have a module_init() function then this is still needed. That is if you want your module to be easily unloadable (and want to do a little cleanup when unloading).
Thanks,
Paul Bolle
| |