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

Lee Jones
2014-07-22T08:56:49Z

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