Re: [PATCH 2/2] mfd: 88pm88x: initial 88pm886/88pm880 base support

[Yi Zhang <yizhang@marvell.com>]

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
>