From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933028AbbFIMOZ (ORCPT ); Tue, 9 Jun 2015 08:14:25 -0400 Received: from lb2-smtp-cloud2.xs4all.net ([194.109.24.25]:37736 "EHLO lb2-smtp-cloud2.xs4all.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751077AbbFIMOQ (ORCPT ); Tue, 9 Jun 2015 08:14:16 -0400 Message-ID: <1433852051.16887.94.camel@x220> Subject: Re: [PATCH 2/2] mfd: 88pm88x: initial 88pm886/88pm880 base support From: Paul Bolle To: Yi Zhang 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" X-Mailer: Evolution 3.10.4 (3.10.4-4.fc20) Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 2015-06-08 at 20:55 +0800, Yi Zhang wrote: > --- /dev/null > +++ b/drivers/mfd/88pm880-table.c > +#include I'm _guessing_ this could as well be . > +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 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 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 > +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"); > +MODULE_LICENSE("GPL v2"); > --- /dev/null > +++ b/drivers/mfd/88pm88x-irq.c > +#include 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