Message-ID: <1433852051.16887.94.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: Tue, 09 Jun 2015 14:14:11 +0200
In-Reply-To: <1433768106-8212-2-git-send-email-yizhang@marvell.com>
References: <1433768106-8212-1-git-send-email-yizhang@marvell.com>
	 <1433768106-8212-2-git-send-email-yizhang@marvell.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3584
Lines: 147

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>.

> +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.

> +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?

> --- /dev/null
> +++ b/drivers/mfd/88pm88x-core.c

> +#include <linux/module.h>

See above.

> +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?

> --- /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?

> +{
> +	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?

> +{
> +	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.

> +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?

> --- 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?

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/