[PATCH 3/4] mfd: imanager2: Add Core supports for IT8516/18/28

[PATCH 3/4] mfd: imanager2: Add Core supports for IT8516/18/28

[Wei-Chun Pan]

Signed-off-by: Wei-Chun Pan <weichun.pan@advantech.com.tw>
---
 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
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.dorsch@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>
+
+#define DRV_NAME	"imanager2"
+#define DRV_VERSION	"4.0.1"
+
+static struct mfd_cell imanager2_cells[] = {
+	{
+		.name = "imanager2_hwm",
+	},
+	{
+		.name = "imanager2_i2c",
+	},
+};
+
+enum chips {
+	it8516 = 0x8516,
+	it8518 = 0x8518,
+	it8528 = 0x8528,
+};
+
+#define EC_CMD_AUTHENTICATION	0x30
+
+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 */
+
+	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;
+
+	if (tmp != 0x95)
+		return -ENODEV;
+
+	return 0;
+}
+
+#define EC_ITE_CHIPID_H8	0x20
+#define EC_ITE_CHIPID_L8	0x21
+
+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:
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+/*
+ * EC provides IO channel and ITE mailbox ways to access mailbox. IO channel is
+ * a common way to access mailbox, but IET mailbox way is much faster than IO
+ * channel. We prefer ITE mailbox if firmware supports. Source kernel code
+ * X11_05 the first firmware version that supports ITE mailbox.
+ */
+#define EC_CHIPFW_SUPP_ITEMAILBOX	0x1105
+
+static int imanager2_get_info(struct imanager2 *ec)
+{
+	int ret;
+	u8 *tmp = (u8 *)&ec->version.kernel_ver;
+
+	mutex_lock(&ec->lock);
+
+	ret = imanager2_mbox_io_read(EC_CMD_ACPIRAM_READ,
+				     EC_ACPIRAM_ADDR_KERNEL_MAJOR_VERSION,
+				     &tmp[0], 2);
+
+	if (ret)
+		goto unlock;
+
+	if (ec->version.kernel_ver >= EC_CHIPFW_SUPP_ITEMAILBOX) {
+		ec->flag |= EC_FLAG_MAILBOX;
+		ret = imanager2_mbox_get_project_information(
+			ec->flag, ec->prj_name,
+			&ec->version.kernel_ver, &ec->version.chip_code,
+			&ec->version.prj_id, &ec->version.prj_ver);
+	} else {
+		ec->flag &= ~EC_FLAG_MAILBOX;
+		ret = imanager2_mbox_io_read(
+			EC_CMD_ACPIRAM_READ,
+			EC_ACPIRAM_ADDR_KERNEL_MAJOR_VERSION,
+			&tmp[0], sizeof(struct ec_version));
+	}
+unlock:
+	mutex_unlock(&ec->lock);
+
+	return ret;
+}
+
+static int imanager2_device_initial_by_ite(struct imanager2 *ec)
+{
+	int i, ret;
+
+	mutex_lock(&ec->lock);
+
+	ret = imanager2_mbox_get_dynamic_table(ec->flag, EC_DYNAMIC_DEVICE_ID,
+					       ec->table.devid);
+	if (ret)
+		goto unlock;
+
+	ret = imanager2_mbox_get_dynamic_table(ec->flag, EC_DYNAMIC_HW_PIN,
+					       ec->table.pinnum);
+unlock:
+	mutex_unlock(&ec->lock);
+
+	if (ret)
+		return ret;
+
+	for (i = 0; i < EC_MAX_ITEM_NUM; i++) {
+		if (ec->table.devid[i] == EC_TABLE_DID_NODEV)
+			break;
+
+		ec->table.devid2itemnum[ec->table.devid[i]] = i;
+	}
+
+	return 0;
+}
+
+static int imanager2_device_initial_by_io(struct imanager2 *ec)
+{
+	int i, ret;
+	u8 tmp;
+
+	mutex_lock(&ec->lock);
+
+	for (i = 0; i < EC_MAX_ITEM_NUM; i++) {
+		ret = imanager2_mbox_io_read
+		      (EC_CMD_HWCTRLTABLE_INDEX, i, &tmp, 1);
+		if (ret)
+			break;
+		if (tmp == EC_TABLE_NOITEM)
+			break;
+
+		ret = imanager2_mbox_io_simple_read(
+			EC_CMD_HWCTRLTABLE_GET_PIN_NUM, &ec->table.pinnum[i]);
+		if (ret)
+			break;
+
+		ret = imanager2_mbox_io_simple_read(
+			EC_CMD_HWCTRLTABLE_GET_DEVICE_ID, &ec->table.devid[i]);
+		if (ret)
+			break;
+
+		if (ec->table.devid[i] == EC_TABLE_DID_NODEV)
+			continue;
+
+		ec->table.devid2itemnum[ec->table.devid[i]] = i;
+	}
+
+	mutex_unlock(&ec->lock);
+
+	if (ret)
+		return ret;
+
+	if (i < EC_MAX_ITEM_NUM) {
+		memset(&ec->table.devid[i], EC_TABLE_DID_NODEV,
+		       EC_MAX_ITEM_NUM - i);
+		memset(&ec->table.pinnum[i], EC_TABLE_HWP_NODEV,
+		       EC_MAX_ITEM_NUM - i);
+	}
+
+	return ret;
+}
+
+static int imanager2_build_device_table(struct imanager2 *ec)
+{
+	memset(&ec->table.devid2itemnum[0], EC_TABLE_ITEM_UNUSED,
+	       ARRAY_SIZE(ec->table.devid2itemnum));
+
+	if (ec->flag & EC_FLAG_MAILBOX)
+		return imanager2_device_initial_by_ite(ec);
+	else
+		return imanager2_device_initial_by_io(ec);
+}
+
+static struct platform_device *ec_pdev;
+
+static int __init imanager2_mfd_device_init(void)
+{
+	struct imanager2 ec = { 0 };
+	int ret;
+
+	mutex_init(&ec.lock);
+
+	ret = imanager2_authentication(&ec);
+	if (ret)
+		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;
+
+	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);
+
+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);
-- 
1.9.1

Re: [PATCH 3/4] mfd: imanager2: Add Core supports for IT8516/18/28

[Lee Jones]

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.pan@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.dorsch@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

RE: [PATCH 3/4] mfd: imanager2: Add Core supports for IT8516/18/28

[Wei-Chun Pan]

I have a question for your comment. As your mail,

> > +
> > +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);
> 

How can I use module_platform_driver() and my driver exit function also can do
mfd_remove_devices().
Or any other better function install of mfd_remove_devices()?

Signed-off-by: Wei-Chun Pan <weichun.pan@advantech.com.tw>

Re: [PATCH 3/4] mfd: imanager2: Add Core supports for IT8516/18/28

[Lee Jones]

You've replied to this email as an independent message, so now there
is little context.  When you reply, please ensure your mailer replies
to the thread.

On Thu, 07 Aug 2014, Wei-Chun Pan wrote:
> I have a question for your comment. As your mail,
> 
> > > +
> > > +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);
> > 
> 
> How can I use module_platform_driver() and my driver exit function also can do
> mfd_remove_devices().
> Or any other better function install of mfd_remove_devices()?

That's the point - you don't do mfd_remove_devices() in exit(), you do
mfd_add_devices() in .probe() and mfd_remove_devices() in .remove().

The only thing you want to do in init() is platform_driver_register(),
or even better - don't have init() and exit() calls and use use
module_platform_driver_probe() instead.

> Signed-off-by: Wei-Chun Pan <weichun.pan@advantech.com.tw>

Not sure why you've signed-off on an email?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 3/4] mfd: imanager2: Add Core supports for IT8516/18/28

[Wei-Chun Pan]

> You've replied to this email as an independent message, so now there is little
> context.  When you reply, please ensure your mailer replies to the thread.
> 

Could you tell me more detail about this?
I use Git to reply mails but don't know how to set them to be dependent on.

> On Thu, 07 Aug 2014, Wei-Chun Pan wrote:
> > I have a question for your comment. As your mail,
> >
> > > > +
> > > > +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);
> > >
> >
> > How can I use module_platform_driver() and my driver exit function
> > also can do mfd_remove_devices().
> > Or any other better function install of mfd_remove_devices()?
> 
> That's the point - you don't do mfd_remove_devices() in exit(), you do
> mfd_add_devices() in .probe() and mfd_remove_devices() in .remove().
> 
> The only thing you want to do in init() is platform_driver_register(), or even better
> - don't have init() and exit() calls and use use
> module_platform_driver_probe() instead.
> 

It seems that module_platform_driver() only registers/unregisters driver but not device.
Is there a method to register/unregister my device?

--
Wei-Chun Pan

Re: [PATCH 3/4] mfd: imanager2: Add Core supports for IT8516/18/28

[Lee Jones]

On Tue, 19 Aug 2014, Wei-Chun Pan wrote:

> > You've replied to this email as an independent message, so now there is little
> > context.  When you reply, please ensure your mailer replies to the thread.
> > 
> 
> Could you tell me more detail about this?
> I use Git to reply mails but don't know how to set them to be dependent on.

How do you use Git to reply to emails?

> > On Thu, 07 Aug 2014, Wei-Chun Pan wrote:
> > > I have a question for your comment. As your mail,
> > >
> > > > > +
> > > > > +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);
> > > >
> > >
> > > How can I use module_platform_driver() and my driver exit function
> > > also can do mfd_remove_devices().
> > > Or any other better function install of mfd_remove_devices()?
> > 
> > That's the point - you don't do mfd_remove_devices() in exit(), you do
> > mfd_add_devices() in .probe() and mfd_remove_devices() in .remove().
> > 
> > The only thing you want to do in init() is platform_driver_register(), or even better
> > - don't have init() and exit() calls and use use
> > module_platform_driver_probe() instead.
> > 
> 
> It seems that module_platform_driver() only registers/unregisters driver but not device.
> Is there a method to register/unregister my device?

You need to do that from Device Tree or from within arch/<arch>/{mach,plat}-*

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog