Message-ID: <1434030728.2271.114.camel@x220>
Subject: Re: [PATCH 2/2] mfd: 88pm88x: initial 88pm886/88pm880 base support
From: Paul Bolle <pebolle@tiscali.nl>
To: Yi Zhang <yizhang@marvell.com>
Cc: sameo@linux.intel.com, lee.jones@linaro.org, zhouqiao@marvell.com,
        zhenzh@marvell.com, linux-kernel@vger.kernel.org
Date: Thu, 11 Jun 2015 15:52:08 +0200
In-Reply-To: <20150611100517.GA20596@yizhang>
References: <1433768106-8212-1-git-send-email-yizhang@marvell.com>
	 <1433768106-8212-2-git-send-email-yizhang@marvell.com>
	 <1433852051.16887.94.camel@x220> <20150611100517.GA20596@yizhang>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3196
Lines: 114

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

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