There is no way that you are introducing a 300 line driver and have nothing to say about it. Please put a nice descriptive commit log here when you resubmit.
> Signed-off-by: Wei-Chun Pan <weichun....@advantech.com.tw> > --- I also expect to see a change log here and version information in the $SUBJECT line of the email. I think the next one is v3 (or is it v4?), so the start of the subject should read [PATCH v4 x/y]. > drivers/mfd/Kconfig | 6 + > drivers/mfd/Makefile | 2 + > drivers/mfd/imanager2_core.c | 303 > +++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 311 insertions(+) > create mode 100644 drivers/mfd/imanager2_core.c > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index 3383412..48b063f 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -10,6 +10,12 @@ config MFD_CORE > select IRQ_DOMAIN > default n > > +config MFD_IMANAGER2 > + tristate "Support for Advantech iManager2 EC ICs" > + select MFD_CORE > + help > + Support for Advantech iManager2 EC ICs > + > config MFD_CS5535 > tristate "AMD CS5535 and CS5536 southbridge core functions" > select MFD_CORE > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index 2851275..10c64ae 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -166,3 +166,5 @@ obj-$(CONFIG_MFD_RETU) += retu-mfd.o > obj-$(CONFIG_MFD_AS3711) += as3711.o > obj-$(CONFIG_MFD_AS3722) += as3722.o > obj-$(CONFIG_MFD_STW481X) += stw481x.o > +imanager2-objs := imanager2_core.o imanager2_ec.o > +obj-$(CONFIG_MFD_IMANAGER2) += imanager2.o No need to do this. Just do: obj-$(CONFIG_MFD_IMANAGER2) += imanager2_core.o imanager2_ec.o > diff --git a/drivers/mfd/imanager2_core.c b/drivers/mfd/imanager2_core.c > new file mode 100644 > index 0000000..2264d29 > --- /dev/null > +++ b/drivers/mfd/imanager2_core.c > @@ -0,0 +1,303 @@ > +/* > + * imanager2_core.c - MFD core driver of Advantech EC IT8516/18/28 > + * Copyright (C) 2014 Richard Vidal-Dorsch <richard.dor...@advantech.com> > + * > + * This program is free software: you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation, either version 3 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program. If not, see <http://www.gnu.org/licenses/>. > + */ > + > +#include <linux/module.h> > +#include <linux/mfd/core.h> > +#include <linux/mfd/imanager2_ec.h> You need more headers than this. > +#define DRV_NAME "imanager2" No need to #define the driver name. Just use the string, where you need to use it. > +#define DRV_VERSION "4.0.1" Remove this. > +static struct mfd_cell imanager2_cells[] = { > + { > + .name = "imanager2_hwm", > + }, > + { > + .name = "imanager2_i2c", > + }, Put these on a single line. > +}; > + > +enum chips { > + it8516 = 0x8516, > + it8518 = 0x8518, > + it8528 = 0x8528, s/it/CHIP_IT > +}; Move this to the top. > +#define EC_CMD_AUTHENTICATION 0x30 Why is this seperate from the rest of the #defines? > +static int imanager2_authentication(struct imanager2 *ec) > +{ > + u8 tmp; > + int ret; > + > + mutex_lock(&ec->lock); > + > + if (inb(EC_IO_PORT_CMD) == 0xFF && inb(EC_IO_PORT_DATA) == 0xFF) { > + ret = -ENODEV; > + goto unlock; > + } > + > + if (inb(EC_IO_PORT_CMD) & IO_FLAG_OBF) > + inb(EC_IO_PORT_DATA); /* initial OBF */ What's OBF? > + if (ec_outb_after_ibc0(EC_IO_PORT_CMD, EC_CMD_AUTHENTICATION)) { > + ret = -ENODEV; > + goto unlock; > + } > + > + ret = ec_inb_after_obf1(&tmp); > + > +unlock: > + mutex_unlock(&ec->lock); > + > + if (ret) > + return ret; Remove this. > + if (tmp != 0x95) ... and change this to: if (!ret && tmp != 0x95) But 0x95 should be #defined somewhere. > + return -ENODEV; > + > + return 0; > +} > + > +#define EC_ITE_CHIPID_H8 0x20 > +#define EC_ITE_CHIPID_L8 0x21 Group all of the #defines somewhere. > +static int imanager2_get_chip_type(struct imanager2 *ec) > +{ > + mutex_lock(&ec->lock); > + > + outb(EC_ITE_CHIPID_H8, EC_SIO_CMD); > + ec->id = inb(EC_SIO_DATA) << 8; > + outb(EC_ITE_CHIPID_L8, EC_SIO_CMD); > + ec->id |= inb(EC_SIO_DATA); > + > + mutex_unlock(&ec->lock); > + > + switch (ec->id) { > + case it8516: > + case it8518: > + ec->flag = EC_FLAG_IO; > + break; > + case it8528: > + ec->flag |= EC_FLAG_IO_MAILBOX; > + break; > + default: No error message to the user? > + return -ENODEV; > + } > + > + return 0; > +} [...] > +static int __init imanager2_mfd_device_init(void) Drop the '_mfd_device' part of this. > +{ All of the code below should be in .probe(), not init(). > + struct imanager2 ec = { 0 }; Allocate memory instead. > + int ret; > + > + mutex_init(&ec.lock); > + > + ret = imanager2_authentication(&ec); > + if (ret) How does the user know what broke? I don't see any err/warn prints? > + return ret; > + > + ret = imanager2_get_chip_type(&ec); > + if (ret) > + return ret; > + > + ret = imanager2_get_info(&ec); > + if (ret) > + return ret; > + > + ret = imanager2_build_device_table(&ec); > + if (ret) > + return ret; > + > + ec_pdev = platform_device_alloc(DRV_NAME, PLATFORM_DEVID_AUTO); > + if (!ec_pdev) > + return -ENOMEM; > + > + if (!devm_request_region(&ec_pdev->dev, EC_IO_PORT_DATA, 2, DRV_NAME)) > + return -EIO; > + > + if (!devm_request_region(&ec_pdev->dev, EC_ITE_PORT_OFS, 2, DRV_NAME)) > + return -EIO; > + > + ret = platform_device_add_data(ec_pdev, &ec, sizeof(struct imanager2)); > + if (ret) > + goto exit_device_put; > + > + ret = platform_device_add(ec_pdev); > + if (ret) > + goto exit_device_put; If you convert this driver to be a prober platform device, you can get rid of all of this above. > + ret = mfd_add_devices(&ec_pdev->dev, -1, imanager2_cells, > + ARRAY_SIZE(imanager2_cells), NULL, -1, NULL); > + if (ret) > + goto exit_device_unregister; > + > + return 0; > + > +exit_device_unregister: > + platform_device_unregister(ec_pdev); > +exit_device_put: > + platform_device_put(ec_pdev); > + > + return ret; > +} > + > +static void __exit imanager2_mfd_device_exit(void) > +{ > + mfd_remove_devices(&ec_pdev->dev); > + platform_device_unregister(ec_pdev); > +} > + > +module_init(imanager2_mfd_device_init); > +module_exit(imanager2_mfd_device_exit); This is a mess. Use module_platform_driver() instead. > +MODULE_AUTHOR("Richard Vidal-Dorsch <richard.dorsch at advantech.com>"); > +MODULE_DESCRIPTION("iManager2 platform device definitions v" DRV_VERSION); > +MODULE_LICENSE("GPL"); > +MODULE_VERSION(DRV_VERSION); -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
