[PATCH v2] I2C: add driver for SMBus Control Method Interface

[PATCH v2] I2C: add driver for SMBus Control Method Interface

[Crane Cai]

This driver supports the SMBus Control Method Interface. It needs BIOS declare
ACPI control methods which described in SMBus Control Method Interface Spec.
http://smbus.org/specs/smbus_cmi10.pdf

Hi Jean,
This driver is modified as you commented last time and it is verified OK. Could
you put some time to review and apply it?
Thanks.

Signed-off-by: Crane Cai <crane.cai-5C7GfCeVMHo@public.gmane.org>
---
 drivers/i2c/busses/Kconfig   |   11 +
 drivers/i2c/busses/Makefile  |    3 +
 drivers/i2c/busses/i2c-cmi.c |  442 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 456 insertions(+), 0 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-cmi.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 8206442..afbc468 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -761,4 +761,15 @@ config SCx200_ACB
 	  This support is also available as a module.  If so, the module
 	  will be called scx200_acb.
 
+config I2C_CMI
+	tristate "SMBus Control Method Interface"
+	depends on ACPI
+	help
+	  This driver supports the SMBus Control Method Interface. It needs the
+	  BIOS to declare ACPI control methods as described in the SMBus Control
+	  Method Interface specification.
+
+	  To compile this driver as a module, choose M here:
+	  the module will be called i2c-cmi.
+
 endmenu
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index e654263..4e83162 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -2,6 +2,9 @@
 # Makefile for the i2c bus drivers.
 #
 
+# SMBus CMI driver
+obj-$(CONFIG_I2C_CMI)		+= i2c-cmi.o
+
 # PC SMBus host controller drivers
 obj-$(CONFIG_I2C_ALI1535)	+= i2c-ali1535.o
 obj-$(CONFIG_I2C_ALI1563)	+= i2c-ali1563.o
diff --git a/drivers/i2c/busses/i2c-cmi.c b/drivers/i2c/busses/i2c-cmi.c
new file mode 100644
index 0000000..555b482
--- /dev/null
+++ b/drivers/i2c/busses/i2c-cmi.c
@@ -0,0 +1,442 @@
+/*
+ * SMBus driver for ACPI SMBus CMI
+ *
+ * Copyright (C) 2009 Crane Cai <crane.cai-5C7GfCeVMHo@public.gmane.org>
+ *
+ * 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 version 2.
+ */
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/kernel.h>
+#include <linux/stddef.h>
+#include <linux/init.h>
+#include <linux/i2c.h>
+#include <linux/acpi.h>
+
+#define ACPI_SMBUS_HC_COMPONENT		0x00080000
+#define ACPI_SMBUS_HC_CLASS		"smbus"
+#define ACPI_SMBUS_HC_DEVICE_NAME	"cmi"
+
+ACPI_MODULE_NAME("smbus_cmi");
+
+struct smbus_methods_t {
+	const char *mt_info;
+	const char *mt_sbr;
+	const char *mt_sbw;
+};
+
+struct acpi_smbus_cmi {
+	acpi_handle handle;
+	struct i2c_adapter adapter;
+	u8 cap_info:1;
+	u8 cap_read:1;
+	u8 cap_write:1;
+};
+
+static const struct smbus_methods_t smbus_methods = {
+	.mt_info = "_SBI",
+	.mt_sbr  = "_SBR",
+	.mt_sbw  = "_SBW",
+};
+
+static const struct acpi_device_id acpi_smbus_cmi_ids[] = {
+	{"SMBUS01", 0},
+	{"", 0}
+};
+
+#define ACPI_SMBUS_STATUS_OK			0x00
+#define ACPI_SMBUS_STATUS_FAIL			0x07
+#define ACPI_SMBUS_STATUS_DNAK			0x10
+#define ACPI_SMBUS_STATUS_DERR			0x11
+#define ACPI_SMBUS_STATUS_CMD_DENY		0x12
+#define ACPI_SMBUS_STATUS_UNKNOWN		0x13
+#define ACPI_SMBUS_STATUS_ACC_DENY		0x17
+#define ACPI_SMBUS_STATUS_TIMEOUT		0x18
+#define ACPI_SMBUS_STATUS_NOTSUP		0x19
+#define ACPI_SMBUS_STATUS_BUSY			0x1A
+#define ACPI_SMBUS_STATUS_PEC			0x1F
+
+#define ACPI_SMBUS_PRTCL_WRITE			0x00
+#define ACPI_SMBUS_PRTCL_READ			0x01
+#define ACPI_SMBUS_PRTCL_QUICK			0x02
+#define ACPI_SMBUS_PRTCL_BYTE			0x04
+#define ACPI_SMBUS_PRTCL_BYTE_DATA		0x06
+#define ACPI_SMBUS_PRTCL_WORD_DATA		0x08
+#define ACPI_SMBUS_PRTCL_BLOCK_DATA		0x0a
+
+
+static int
+acpi_smbus_cmi_access(struct i2c_adapter *adap, u16 addr, unsigned short flags,
+		   char read_write, u8 command, int size,
+		   union i2c_smbus_data *data)
+{
+	int result = 0;
+	struct acpi_smbus_cmi *smbus_cmi = adap->algo_data;
+	unsigned char protocol;
+	acpi_status status = 0;
+	struct acpi_object_list input;
+	union acpi_object mt_params[5];
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *obj;
+	union acpi_object *pkg;
+	char *method;
+	int len = 0;
+
+	dev_dbg(&adap->dev, "access size: %d %s\n", size,
+		(read_write) ? "READ" : "WRITE");
+	switch (size) {
+	case I2C_SMBUS_QUICK:
+		protocol = ACPI_SMBUS_PRTCL_QUICK;
+		command = 0;
+		if (read_write == I2C_SMBUS_WRITE) {
+			mt_params[3].type = ACPI_TYPE_INTEGER;
+			mt_params[3].integer.value = 0;
+			mt_params[4].type = ACPI_TYPE_INTEGER;
+			mt_params[4].integer.value = 0;
+		}
+		break;
+
+	case I2C_SMBUS_BYTE:
+		protocol = ACPI_SMBUS_PRTCL_BYTE;
+		if (read_write == I2C_SMBUS_WRITE) {
+			mt_params[3].type = ACPI_TYPE_INTEGER;
+			mt_params[3].integer.value = 0;
+			mt_params[4].type = ACPI_TYPE_INTEGER;
+			mt_params[4].integer.value = 0;
+		} else {
+			command = 0;
+		}
+		break;
+
+	case I2C_SMBUS_BYTE_DATA:
+		protocol = ACPI_SMBUS_PRTCL_BYTE_DATA;
+		if (read_write == I2C_SMBUS_WRITE) {
+			mt_params[3].type = ACPI_TYPE_INTEGER;
+			mt_params[3].integer.value = 1;
+			mt_params[4].type = ACPI_TYPE_INTEGER;
+			mt_params[4].integer.value = data->byte;
+		}
+		break;
+
+	case I2C_SMBUS_WORD_DATA:
+		protocol = ACPI_SMBUS_PRTCL_WORD_DATA;
+		if (read_write == I2C_SMBUS_WRITE) {
+			mt_params[3].type = ACPI_TYPE_INTEGER;
+			mt_params[3].integer.value = 2;
+			mt_params[4].type = ACPI_TYPE_INTEGER;
+			mt_params[4].integer.value = data->word;
+		}
+		break;
+
+	case I2C_SMBUS_BLOCK_DATA:
+		protocol = ACPI_SMBUS_PRTCL_BLOCK_DATA;
+		if (read_write == I2C_SMBUS_WRITE) {
+			len = data->block[0];
+			if (len == 0 || len > I2C_SMBUS_BLOCK_MAX)
+				return -EINVAL;
+			mt_params[3].type = ACPI_TYPE_INTEGER;
+			mt_params[3].integer.value = len;
+			mt_params[4].type = ACPI_TYPE_BUFFER;
+			mt_params[4].buffer.pointer = data->block + 1;
+		}
+		break;
+
+	default:
+		dev_warn(&adap->dev, "Unsupported transaction %d\n", size);
+		return -EOPNOTSUPP;
+	}
+
+	if (read_write == I2C_SMBUS_READ) {
+		protocol |= ACPI_SMBUS_PRTCL_READ;
+		method = (char *)smbus_methods.mt_sbr;
+		input.count = 3;
+	} else {
+		protocol |= ACPI_SMBUS_PRTCL_WRITE;
+		method = (char *)smbus_methods.mt_sbw;
+		input.count = 5;
+	}
+
+	input.pointer = mt_params;
+	mt_params[0].type = ACPI_TYPE_INTEGER;
+	mt_params[0].integer.value = protocol;
+	mt_params[1].type = ACPI_TYPE_INTEGER;
+	mt_params[1].integer.value = addr;
+	mt_params[2].type = ACPI_TYPE_INTEGER;
+	mt_params[2].integer.value = command;
+
+	status = acpi_evaluate_object(smbus_cmi->handle, method, &input,
+				      &buffer);
+	if (ACPI_FAILURE(status)) {
+		ACPI_ERROR((AE_INFO, "Evaluating %s: %i", method, status));
+		return -EIO;
+	}
+
+	pkg = buffer.pointer;
+	if (pkg && pkg->type == ACPI_TYPE_PACKAGE)
+		obj = pkg->package.elements;
+	else {
+		ACPI_ERROR((AE_INFO, "Invalid argument type"));
+		result = -EIO;
+		goto out;
+	}
+	if (obj == NULL || obj->type != ACPI_TYPE_INTEGER) {
+		ACPI_ERROR((AE_INFO, "Invalid argument type"));
+		result = -EIO;
+		goto out;
+	}
+
+	result = obj->integer.value;
+	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "%s return status: %i\n",
+			  method, result));
+
+	switch (result) {
+	case ACPI_SMBUS_STATUS_OK:
+		break;
+	case ACPI_SMBUS_STATUS_BUSY:
+		result = -EBUSY;
+		goto out;
+	case ACPI_SMBUS_STATUS_TIMEOUT:
+		result = -ETIMEDOUT;
+		goto out;
+	case ACPI_SMBUS_STATUS_DNAK:
+		result = -ENXIO;
+		goto out;
+	default:
+		result = -EIO;
+		goto out;
+	}
+
+	if (read_write == I2C_SMBUS_WRITE || size == I2C_SMBUS_QUICK)
+		goto out;
+
+	obj = pkg->package.elements + 1;
+	if (obj == NULL || obj->type != ACPI_TYPE_INTEGER) {
+		ACPI_ERROR((AE_INFO, "Invalid argument type"));
+		result = -EIO;
+		goto out;
+	}
+
+	len = obj->integer.value;
+	obj = pkg->package.elements + 2;
+	switch (size) {
+	case I2C_SMBUS_BYTE:
+	case I2C_SMBUS_BYTE_DATA:
+	case I2C_SMBUS_WORD_DATA:
+		if (obj == NULL || obj->type != ACPI_TYPE_INTEGER) {
+			ACPI_ERROR((AE_INFO, "Invalid argument type"));
+			result = -EIO;
+			goto out;
+		}
+		if (len == 2)
+			data->word = obj->integer.value;
+		else
+			data->byte = obj->integer.value;
+		break;
+	case I2C_SMBUS_BLOCK_DATA:
+		if (obj == NULL || obj->type != ACPI_TYPE_BUFFER) {
+			ACPI_ERROR((AE_INFO, "Invalid argument type"));
+			result = -EIO;
+			goto out;
+		}
+		if (len == 0 || len > I2C_SMBUS_BLOCK_MAX)
+			return -EPROTO;
+		data->block[0] = len;
+		memcpy(data->block + 1, obj->buffer.pointer, len);
+		break;
+	}
+
+out:
+	kfree(buffer.pointer);
+	dev_dbg(&adap->dev, "Transaction status: %i\n", result);
+	return result;
+}
+
+static u32 acpi_smbus_cmi_func(struct i2c_adapter *adapter)
+{
+	struct acpi_smbus_cmi *smbus_cmi = adapter->algo_data;
+	u32 ret;
+
+	ret = smbus_cmi->cap_read | smbus_cmi->cap_write ?
+		I2C_FUNC_SMBUS_QUICK : 0;
+
+	ret |= smbus_cmi->cap_read ?
+		(I2C_FUNC_SMBUS_READ_BYTE |
+		I2C_FUNC_SMBUS_READ_BYTE_DATA |
+		I2C_FUNC_SMBUS_READ_WORD_DATA |
+		I2C_FUNC_SMBUS_READ_BLOCK_DATA) : 0;
+
+	ret |= smbus_cmi->cap_write ?
+		(I2C_FUNC_SMBUS_WRITE_BYTE |
+		I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
+		I2C_FUNC_SMBUS_WRITE_WORD_DATA |
+		I2C_FUNC_SMBUS_WRITE_BLOCK_DATA) : 0;
+
+	return ret;
+}
+
+static const struct i2c_algorithm acpi_smbus_cmi_algorithm = {
+	.smbus_xfer = acpi_smbus_cmi_access,
+	.functionality = acpi_smbus_cmi_func,
+};
+
+static int acpi_smbus_cmi_add_cap(struct acpi_smbus_cmi *smbus_cmi,
+				  const char *name)
+{
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *obj;
+	int i;
+	acpi_status status;
+
+	if (!strcmp(name, smbus_methods.mt_info)) {
+		status = acpi_evaluate_object(smbus_cmi->handle,
+					(char *)smbus_methods.mt_info,
+					NULL, &buffer);
+		if (ACPI_FAILURE(status)) {
+			ACPI_ERROR((AE_INFO, "Evaluating %s: %i",
+				   smbus_methods.mt_info, status));
+			return -EIO;
+		}
+
+		obj = buffer.pointer;
+		if (obj && obj->type == ACPI_TYPE_PACKAGE)
+			obj = obj->package.elements;
+		else {
+			ACPI_ERROR((AE_INFO, "Invalid argument type"));
+			kfree(buffer.pointer);
+			return -EIO;
+		}
+
+		if (obj->type != ACPI_TYPE_INTEGER) {
+			ACPI_ERROR((AE_INFO, "Invalid argument type"));
+			return -EIO;
+		} else
+			ACPI_DEBUG_PRINT((ACPI_DB_INFO, "SMBus CMI Version %x"
+					  "\n", (int)obj->integer.value));
+
+		obj = obj->package.elements + 1;
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "SMB_INFO:\n"));
+		for (i = 0; i < obj->buffer.length; i++) {
+			ACPI_DEBUG_PRINT((ACPI_DB_INFO, "%02x ",
+						*(obj->buffer.pointer + i)));
+			if (i > 0 && (i % 16) == 0 && i != obj->buffer.length-1)
+				ACPI_DEBUG_PRINT((ACPI_DB_INFO, "\n"));
+		}
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "\n"));
+
+		kfree(buffer.pointer);
+		smbus_cmi->cap_info = 1;
+	} else if (!strcmp(name, smbus_methods.mt_sbr))
+		smbus_cmi->cap_read = 1;
+	else if (!strcmp(name, smbus_methods.mt_sbw))
+		smbus_cmi->cap_write = 1;
+	else
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "CMI Methods: %s\n", name));
+
+	return 0;
+}
+
+static acpi_status acpi_smbus_cmi_query_methods(acpi_handle handle, u32 level,
+			void *context, void **return_value)
+{
+	char node_name[5];
+	struct acpi_buffer buffer = { sizeof(node_name), node_name };
+	struct acpi_smbus_cmi *smbus_cmi = context;
+	acpi_status status;
+
+	status = acpi_get_name(handle, ACPI_SINGLE_NAME, &buffer);
+
+	if (ACPI_SUCCESS(status))
+		acpi_smbus_cmi_add_cap(smbus_cmi, node_name);
+
+	return AE_OK;
+}
+
+static int acpi_smbus_cmi_add(struct acpi_device *device)
+{
+	struct acpi_smbus_cmi *smbus_cmi;
+
+	smbus_cmi = kzalloc(sizeof(struct acpi_smbus_cmi), GFP_KERNEL);
+	if (!smbus_cmi)
+		return -ENOMEM;
+
+	smbus_cmi->handle = device->handle;
+	strcpy(acpi_device_name(device), ACPI_SMBUS_HC_DEVICE_NAME);
+	strcpy(acpi_device_class(device), ACPI_SMBUS_HC_CLASS);
+	device->driver_data = smbus_cmi;
+	smbus_cmi->cap_info = 0;
+	smbus_cmi->cap_read = 0;
+	smbus_cmi->cap_write = 0;
+
+	acpi_walk_namespace(ACPI_TYPE_METHOD, smbus_cmi->handle, 1,
+			    acpi_smbus_cmi_query_methods, smbus_cmi, NULL);
+
+	if (smbus_cmi->cap_info == 0)
+		goto err;
+
+	snprintf(smbus_cmi->adapter.name, sizeof(smbus_cmi->adapter.name),
+		"SMBus CMI adapter at %s-%s",
+		acpi_device_name(device),
+		acpi_device_uid(device));
+	smbus_cmi->adapter.owner = THIS_MODULE;
+	smbus_cmi->adapter.algo = &acpi_smbus_cmi_algorithm;
+	smbus_cmi->adapter.algo_data = smbus_cmi;
+	smbus_cmi->adapter.class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
+	smbus_cmi->adapter.dev.parent = &device->dev;
+
+	if (i2c_add_adapter(&smbus_cmi->adapter)) {
+		dev_err(&device->dev, "Couldn't register adapter!\n");
+		goto err;
+	}
+
+	return 0;
+
+err:
+	kfree(smbus_cmi);
+	device->driver_data = NULL;
+	return -EIO;
+}
+
+static int acpi_smbus_cmi_remove(struct acpi_device *device, int type)
+{
+	struct acpi_smbus_cmi *smbus_cmi = acpi_driver_data(device);
+
+	i2c_del_adapter(&smbus_cmi->adapter);
+	kfree(smbus_cmi);
+	smbus_cmi = NULL;
+
+	return 0;
+}
+
+static struct acpi_driver acpi_smbus_cmi_driver = {
+	.name = ACPI_SMBUS_HC_DEVICE_NAME,
+	.class = ACPI_SMBUS_HC_CLASS,
+	.ids = acpi_smbus_cmi_ids,
+	.ops = {
+		.add = acpi_smbus_cmi_add,
+		.remove = acpi_smbus_cmi_remove,
+	},
+};
+
+static int __init acpi_smbus_cmi_init(void)
+{
+	int result;
+
+	result = acpi_bus_register_driver(&acpi_smbus_cmi_driver);
+
+	return result;
+}
+
+static void __exit acpi_smbus_cmi_exit(void)
+{
+	acpi_bus_unregister_driver(&acpi_smbus_cmi_driver);
+}
+
+module_init(acpi_smbus_cmi_init);
+module_exit(acpi_smbus_cmi_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Crane Cai <crane.cai-5C7GfCeVMHo@public.gmane.org>");
+MODULE_DESCRIPTION("ACPI SMBus CMI driver");
-- 
1.6.0.4

Re: [PATCH v2] I2C: add driver for SMBus Control Method Interface

[Jean Delvare]

Hi Crane,

On Wed, 16 Sep 2009 17:18:25 +0800, Crane Cai wrote:
> This driver supports the SMBus Control Method Interface. It needs BIOS declare
> ACPI control methods which described in SMBus Control Method Interface Spec.
> http://smbus.org/specs/smbus_cmi10.pdf
> 
> Hi Jean,
> This driver is modified as you commented last time and it is verified OK. Could
> you put some time to review and apply it?

Here you go:

> Thanks.
> 
> Signed-off-by: Crane Cai <crane.cai-5C7GfCeVMHo@public.gmane.org>
> ---
>  drivers/i2c/busses/Kconfig   |   11 +
>  drivers/i2c/busses/Makefile  |    3 +
>  drivers/i2c/busses/i2c-cmi.c |  442 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 456 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/i2c/busses/i2c-cmi.c
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 8206442..afbc468 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -761,4 +761,15 @@ config SCx200_ACB
>  	  This support is also available as a module.  If so, the module
>  	  will be called scx200_acb.
>  
> +config I2C_CMI
> +	tristate "SMBus Control Method Interface"
> +	depends on ACPI
> +	help
> +	  This driver supports the SMBus Control Method Interface. It needs the
> +	  BIOS to declare ACPI control methods as described in the SMBus Control
> +	  Method Interface specification.
> +
> +	  To compile this driver as a module, choose M here:
> +	  the module will be called i2c-cmi.
> +

I believe we should create a specific part in the Kconfig menu for this
driver, as we just did for ACPI-based hardware monitoring drivers.

>  endmenu
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index e654263..4e83162 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -2,6 +2,9 @@
>  # Makefile for the i2c bus drivers.
>  #
>  
> +# SMBus CMI driver
> +obj-$(CONFIG_I2C_CMI)		+= i2c-cmi.o
> +
>  # PC SMBus host controller drivers
>  obj-$(CONFIG_I2C_ALI1535)	+= i2c-ali1535.o
>  obj-$(CONFIG_I2C_ALI1563)	+= i2c-ali1563.o
> diff --git a/drivers/i2c/busses/i2c-cmi.c b/drivers/i2c/busses/i2c-cmi.c
> new file mode 100644
> index 0000000..555b482
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-cmi.c
> @@ -0,0 +1,442 @@
> +/*
> + * SMBus driver for ACPI SMBus CMI
> + *
> + * Copyright (C) 2009 Crane Cai <crane.cai-5C7GfCeVMHo@public.gmane.org>
> + *
> + * 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 version 2.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/kernel.h>
> +#include <linux/stddef.h>
> +#include <linux/init.h>
> +#include <linux/i2c.h>
> +#include <linux/acpi.h>
> +
> +#define ACPI_SMBUS_HC_COMPONENT		0x00080000

This one isn't used anywhere. Is this OK?

> +#define ACPI_SMBUS_HC_CLASS		"smbus"
> +#define ACPI_SMBUS_HC_DEVICE_NAME	"cmi"
> +
> +ACPI_MODULE_NAME("smbus_cmi");
> +
> +struct smbus_methods_t {
> +	const char *mt_info;
> +	const char *mt_sbr;
> +	const char *mt_sbw;

I can see that adding const here required casts elsewhere in the code.
I didn't expect that, so it's probably better to remove the const here
to avoid the ugly casts.

> +};
> +
> +struct acpi_smbus_cmi {
> +	acpi_handle handle;
> +	struct i2c_adapter adapter;
> +	u8 cap_info:1;
> +	u8 cap_read:1;
> +	u8 cap_write:1;
> +};
> +
> +static const struct smbus_methods_t smbus_methods = {
> +	.mt_info = "_SBI",
> +	.mt_sbr  = "_SBR",
> +	.mt_sbw  = "_SBW",
> +};
> +
> +static const struct acpi_device_id acpi_smbus_cmi_ids[] = {
> +	{"SMBUS01", 0},
> +	{"", 0}
> +};
> +
> +#define ACPI_SMBUS_STATUS_OK			0x00
> +#define ACPI_SMBUS_STATUS_FAIL			0x07
> +#define ACPI_SMBUS_STATUS_DNAK			0x10
> +#define ACPI_SMBUS_STATUS_DERR			0x11
> +#define ACPI_SMBUS_STATUS_CMD_DENY		0x12
> +#define ACPI_SMBUS_STATUS_UNKNOWN		0x13
> +#define ACPI_SMBUS_STATUS_ACC_DENY		0x17
> +#define ACPI_SMBUS_STATUS_TIMEOUT		0x18
> +#define ACPI_SMBUS_STATUS_NOTSUP		0x19
> +#define ACPI_SMBUS_STATUS_BUSY			0x1A
> +#define ACPI_SMBUS_STATUS_PEC			0x1F
> +
> +#define ACPI_SMBUS_PRTCL_WRITE			0x00
> +#define ACPI_SMBUS_PRTCL_READ			0x01
> +#define ACPI_SMBUS_PRTCL_QUICK			0x02
> +#define ACPI_SMBUS_PRTCL_BYTE			0x04
> +#define ACPI_SMBUS_PRTCL_BYTE_DATA		0x06
> +#define ACPI_SMBUS_PRTCL_WORD_DATA		0x08
> +#define ACPI_SMBUS_PRTCL_BLOCK_DATA		0x0a
> +
> +
> +static int
> +acpi_smbus_cmi_access(struct i2c_adapter *adap, u16 addr, unsigned short flags,
> +		   char read_write, u8 command, int size,
> +		   union i2c_smbus_data *data)
> +{
> +	int result = 0;
> +	struct acpi_smbus_cmi *smbus_cmi = adap->algo_data;
> +	unsigned char protocol;
> +	acpi_status status = 0;
> +	struct acpi_object_list input;
> +	union acpi_object mt_params[5];
> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> +	union acpi_object *obj;
> +	union acpi_object *pkg;
> +	char *method;
> +	int len = 0;
> +
> +	dev_dbg(&adap->dev, "access size: %d %s\n", size,
> +		(read_write) ? "READ" : "WRITE");
> +	switch (size) {
> +	case I2C_SMBUS_QUICK:
> +		protocol = ACPI_SMBUS_PRTCL_QUICK;
> +		command = 0;
> +		if (read_write == I2C_SMBUS_WRITE) {
> +			mt_params[3].type = ACPI_TYPE_INTEGER;
> +			mt_params[3].integer.value = 0;
> +			mt_params[4].type = ACPI_TYPE_INTEGER;
> +			mt_params[4].integer.value = 0;
> +		}
> +		break;
> +
> +	case I2C_SMBUS_BYTE:
> +		protocol = ACPI_SMBUS_PRTCL_BYTE;
> +		if (read_write == I2C_SMBUS_WRITE) {
> +			mt_params[3].type = ACPI_TYPE_INTEGER;
> +			mt_params[3].integer.value = 0;
> +			mt_params[4].type = ACPI_TYPE_INTEGER;
> +			mt_params[4].integer.value = 0;
> +		} else {
> +			command = 0;
> +		}
> +		break;
> +
> +	case I2C_SMBUS_BYTE_DATA:
> +		protocol = ACPI_SMBUS_PRTCL_BYTE_DATA;
> +		if (read_write == I2C_SMBUS_WRITE) {
> +			mt_params[3].type = ACPI_TYPE_INTEGER;
> +			mt_params[3].integer.value = 1;
> +			mt_params[4].type = ACPI_TYPE_INTEGER;
> +			mt_params[4].integer.value = data->byte;
> +		}
> +		break;
> +
> +	case I2C_SMBUS_WORD_DATA:
> +		protocol = ACPI_SMBUS_PRTCL_WORD_DATA;
> +		if (read_write == I2C_SMBUS_WRITE) {
> +			mt_params[3].type = ACPI_TYPE_INTEGER;
> +			mt_params[3].integer.value = 2;
> +			mt_params[4].type = ACPI_TYPE_INTEGER;
> +			mt_params[4].integer.value = data->word;
> +		}
> +		break;
> +
> +	case I2C_SMBUS_BLOCK_DATA:
> +		protocol = ACPI_SMBUS_PRTCL_BLOCK_DATA;
> +		if (read_write == I2C_SMBUS_WRITE) {
> +			len = data->block[0];
> +			if (len == 0 || len > I2C_SMBUS_BLOCK_MAX)
> +				return -EINVAL;
> +			mt_params[3].type = ACPI_TYPE_INTEGER;
> +			mt_params[3].integer.value = len;
> +			mt_params[4].type = ACPI_TYPE_BUFFER;
> +			mt_params[4].buffer.pointer = data->block + 1;
> +		}
> +		break;
> +
> +	default:
> +		dev_warn(&adap->dev, "Unsupported transaction %d\n", size);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (read_write == I2C_SMBUS_READ) {
> +		protocol |= ACPI_SMBUS_PRTCL_READ;
> +		method = (char *)smbus_methods.mt_sbr;
> +		input.count = 3;
> +	} else {
> +		protocol |= ACPI_SMBUS_PRTCL_WRITE;
> +		method = (char *)smbus_methods.mt_sbw;
> +		input.count = 5;
> +	}
> +
> +	input.pointer = mt_params;
> +	mt_params[0].type = ACPI_TYPE_INTEGER;
> +	mt_params[0].integer.value = protocol;
> +	mt_params[1].type = ACPI_TYPE_INTEGER;
> +	mt_params[1].integer.value = addr;
> +	mt_params[2].type = ACPI_TYPE_INTEGER;
> +	mt_params[2].integer.value = command;
> +
> +	status = acpi_evaluate_object(smbus_cmi->handle, method, &input,
> +				      &buffer);
> +	if (ACPI_FAILURE(status)) {
> +		ACPI_ERROR((AE_INFO, "Evaluating %s: %i", method, status));
> +		return -EIO;
> +	}
> +
> +	pkg = buffer.pointer;
> +	if (pkg && pkg->type == ACPI_TYPE_PACKAGE)
> +		obj = pkg->package.elements;
> +	else {
> +		ACPI_ERROR((AE_INFO, "Invalid argument type"));
> +		result = -EIO;
> +		goto out;
> +	}
> +	if (obj == NULL || obj->type != ACPI_TYPE_INTEGER) {
> +		ACPI_ERROR((AE_INFO, "Invalid argument type"));
> +		result = -EIO;
> +		goto out;
> +	}
> +
> +	result = obj->integer.value;
> +	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "%s return status: %i\n",
> +			  method, result));
> +
> +	switch (result) {
> +	case ACPI_SMBUS_STATUS_OK:
> +		break;
> +	case ACPI_SMBUS_STATUS_BUSY:
> +		result = -EBUSY;
> +		goto out;
> +	case ACPI_SMBUS_STATUS_TIMEOUT:
> +		result = -ETIMEDOUT;
> +		goto out;
> +	case ACPI_SMBUS_STATUS_DNAK:
> +		result = -ENXIO;
> +		goto out;
> +	default:
> +		result = -EIO;
> +		goto out;
> +	}
> +
> +	if (read_write == I2C_SMBUS_WRITE || size == I2C_SMBUS_QUICK)
> +		goto out;
> +
> +	obj = pkg->package.elements + 1;
> +	if (obj == NULL || obj->type != ACPI_TYPE_INTEGER) {
> +		ACPI_ERROR((AE_INFO, "Invalid argument type"));
> +		result = -EIO;
> +		goto out;
> +	}
> +
> +	len = obj->integer.value;
> +	obj = pkg->package.elements + 2;
> +	switch (size) {
> +	case I2C_SMBUS_BYTE:
> +	case I2C_SMBUS_BYTE_DATA:
> +	case I2C_SMBUS_WORD_DATA:
> +		if (obj == NULL || obj->type != ACPI_TYPE_INTEGER) {
> +			ACPI_ERROR((AE_INFO, "Invalid argument type"));
> +			result = -EIO;
> +			goto out;
> +		}
> +		if (len == 2)
> +			data->word = obj->integer.value;
> +		else
> +			data->byte = obj->integer.value;
> +		break;
> +	case I2C_SMBUS_BLOCK_DATA:
> +		if (obj == NULL || obj->type != ACPI_TYPE_BUFFER) {
> +			ACPI_ERROR((AE_INFO, "Invalid argument type"));
> +			result = -EIO;
> +			goto out;
> +		}
> +		if (len == 0 || len > I2C_SMBUS_BLOCK_MAX)
> +			return -EPROTO;
> +		data->block[0] = len;
> +		memcpy(data->block + 1, obj->buffer.pointer, len);
> +		break;
> +	}
> +
> +out:
> +	kfree(buffer.pointer);
> +	dev_dbg(&adap->dev, "Transaction status: %i\n", result);
> +	return result;
> +}
> +
> +static u32 acpi_smbus_cmi_func(struct i2c_adapter *adapter)
> +{
> +	struct acpi_smbus_cmi *smbus_cmi = adapter->algo_data;
> +	u32 ret;
> +
> +	ret = smbus_cmi->cap_read | smbus_cmi->cap_write ?
> +		I2C_FUNC_SMBUS_QUICK : 0;
> +
> +	ret |= smbus_cmi->cap_read ?
> +		(I2C_FUNC_SMBUS_READ_BYTE |
> +		I2C_FUNC_SMBUS_READ_BYTE_DATA |
> +		I2C_FUNC_SMBUS_READ_WORD_DATA |
> +		I2C_FUNC_SMBUS_READ_BLOCK_DATA) : 0;
> +
> +	ret |= smbus_cmi->cap_write ?
> +		(I2C_FUNC_SMBUS_WRITE_BYTE |
> +		I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
> +		I2C_FUNC_SMBUS_WRITE_WORD_DATA |
> +		I2C_FUNC_SMBUS_WRITE_BLOCK_DATA) : 0;
> +
> +	return ret;
> +}
> +
> +static const struct i2c_algorithm acpi_smbus_cmi_algorithm = {
> +	.smbus_xfer = acpi_smbus_cmi_access,
> +	.functionality = acpi_smbus_cmi_func,
> +};
> +
> +static int acpi_smbus_cmi_add_cap(struct acpi_smbus_cmi *smbus_cmi,
> +				  const char *name)
> +{
> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> +	union acpi_object *obj;
> +	int i;
> +	acpi_status status;
> +
> +	if (!strcmp(name, smbus_methods.mt_info)) {
> +		status = acpi_evaluate_object(smbus_cmi->handle,
> +					(char *)smbus_methods.mt_info,
> +					NULL, &buffer);
> +		if (ACPI_FAILURE(status)) {
> +			ACPI_ERROR((AE_INFO, "Evaluating %s: %i",
> +				   smbus_methods.mt_info, status));
> +			return -EIO;
> +		}
> +
> +		obj = buffer.pointer;
> +		if (obj && obj->type == ACPI_TYPE_PACKAGE)
> +			obj = obj->package.elements;
> +		else {
> +			ACPI_ERROR((AE_INFO, "Invalid argument type"));
> +			kfree(buffer.pointer);
> +			return -EIO;
> +		}
> +
> +		if (obj->type != ACPI_TYPE_INTEGER) {
> +			ACPI_ERROR((AE_INFO, "Invalid argument type"));

Shouldn't you free buffer.pointer here?

> +			return -EIO;
> +		} else
> +			ACPI_DEBUG_PRINT((ACPI_DB_INFO, "SMBus CMI Version %x"
> +					  "\n", (int)obj->integer.value));
> +
> +		obj = obj->package.elements + 1;
> +		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "SMB_INFO:\n"));
> +		for (i = 0; i < obj->buffer.length; i++) {
> +			ACPI_DEBUG_PRINT((ACPI_DB_INFO, "%02x ",
> +						*(obj->buffer.pointer + i)));
> +			if (i > 0 && (i % 16) == 0 && i != obj->buffer.length-1)
> +				ACPI_DEBUG_PRINT((ACPI_DB_INFO, "\n"));

Did you try this? I suspect this will insert log level codes all around
the place and the output will be a mess. Fragmented logging is strongly
discouraged.

> +		}
> +		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "\n"));
> +
> +		kfree(buffer.pointer);
> +		smbus_cmi->cap_info = 1;
> +	} else if (!strcmp(name, smbus_methods.mt_sbr))
> +		smbus_cmi->cap_read = 1;
> +	else if (!strcmp(name, smbus_methods.mt_sbw))
> +		smbus_cmi->cap_write = 1;
> +	else
> +		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "CMI Methods: %s\n", name));

It won't be clear why this is printed. What about: "Unsupported CMI
method\n"?

> +
> +	return 0;
> +}
> +
> +static acpi_status acpi_smbus_cmi_query_methods(acpi_handle handle, u32 level,
> +			void *context, void **return_value)
> +{
> +	char node_name[5];
> +	struct acpi_buffer buffer = { sizeof(node_name), node_name };
> +	struct acpi_smbus_cmi *smbus_cmi = context;
> +	acpi_status status;
> +
> +	status = acpi_get_name(handle, ACPI_SINGLE_NAME, &buffer);
> +
> +	if (ACPI_SUCCESS(status))
> +		acpi_smbus_cmi_add_cap(smbus_cmi, node_name);
> +
> +	return AE_OK;
> +}
> +
> +static int acpi_smbus_cmi_add(struct acpi_device *device)
> +{
> +	struct acpi_smbus_cmi *smbus_cmi;
> +
> +	smbus_cmi = kzalloc(sizeof(struct acpi_smbus_cmi), GFP_KERNEL);
> +	if (!smbus_cmi)
> +		return -ENOMEM;
> +
> +	smbus_cmi->handle = device->handle;
> +	strcpy(acpi_device_name(device), ACPI_SMBUS_HC_DEVICE_NAME);
> +	strcpy(acpi_device_class(device), ACPI_SMBUS_HC_CLASS);
> +	device->driver_data = smbus_cmi;
> +	smbus_cmi->cap_info = 0;
> +	smbus_cmi->cap_read = 0;
> +	smbus_cmi->cap_write = 0;
> +
> +	acpi_walk_namespace(ACPI_TYPE_METHOD, smbus_cmi->handle, 1,
> +			    acpi_smbus_cmi_query_methods, smbus_cmi, NULL);
> +
> +	if (smbus_cmi->cap_info == 0)
> +		goto err;
> +
> +	snprintf(smbus_cmi->adapter.name, sizeof(smbus_cmi->adapter.name),
> +		"SMBus CMI adapter at %s-%s",
> +		acpi_device_name(device),
> +		acpi_device_uid(device));

"at" sounds strange, as what follows isn't an address. I'd rather use:

	"SMBus CMI adapter %s (%s)",
	acpi_device_name(device),
	acpi_device_uid(device)

> +	smbus_cmi->adapter.owner = THIS_MODULE;
> +	smbus_cmi->adapter.algo = &acpi_smbus_cmi_algorithm;
> +	smbus_cmi->adapter.algo_data = smbus_cmi;
> +	smbus_cmi->adapter.class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
> +	smbus_cmi->adapter.dev.parent = &device->dev;
> +
> +	if (i2c_add_adapter(&smbus_cmi->adapter)) {
> +		dev_err(&device->dev, "Couldn't register adapter!\n");
> +		goto err;
> +	}
> +
> +	return 0;
> +
> +err:
> +	kfree(smbus_cmi);
> +	device->driver_data = NULL;
> +	return -EIO;
> +}
> +
> +static int acpi_smbus_cmi_remove(struct acpi_device *device, int type)
> +{
> +	struct acpi_smbus_cmi *smbus_cmi = acpi_driver_data(device);
> +
> +	i2c_del_adapter(&smbus_cmi->adapter);
> +	kfree(smbus_cmi);
> +	smbus_cmi = NULL;

This is a no-op, smbus_cmi is a local variable. What you really want to
do is:

	device->driver_data = NULL;

> +
> +	return 0;
> +}
> +
> +static struct acpi_driver acpi_smbus_cmi_driver = {
> +	.name = ACPI_SMBUS_HC_DEVICE_NAME,
> +	.class = ACPI_SMBUS_HC_CLASS,
> +	.ids = acpi_smbus_cmi_ids,
> +	.ops = {
> +		.add = acpi_smbus_cmi_add,
> +		.remove = acpi_smbus_cmi_remove,
> +	},
> +};
> +
> +static int __init acpi_smbus_cmi_init(void)
> +{
> +	int result;
> +
> +	result = acpi_bus_register_driver(&acpi_smbus_cmi_driver);
> +
> +	return result;

This can be simplified to:

	return acpi_bus_register_driver(&acpi_smbus_cmi_driver);

> +}
> +
> +static void __exit acpi_smbus_cmi_exit(void)
> +{
> +	acpi_bus_unregister_driver(&acpi_smbus_cmi_driver);
> +}
> +
> +module_init(acpi_smbus_cmi_init);
> +module_exit(acpi_smbus_cmi_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Crane Cai <crane.cai-5C7GfCeVMHo@public.gmane.org>");
> +MODULE_DESCRIPTION("ACPI SMBus CMI driver");

If you are fine with my latest proposals, I'll implement them myself,
no need to resend a patch. Thanks a lot for writing the driver!

I am considering renaming the driver to i2c-scmi, too. "i2c-cmi"
seems somewhat short and I fear name collisions in the future.

-- 
Jean Delvare

Re: [PATCH v2] I2C: add driver for SMBus Control Method Interface

[Crane Cai]

Hi Jean,

On Wed, Sep 16, 2009 at 03:48:46PM +0200, Jean Delvare wrote:
> If you are fine with my latest proposals, I'll implement them myself,
> no need to resend a patch. Thanks a lot for writing the driver!
OK, I'm fine. Now it is the time in v2.6.32 merge window. I wish we can submit
it in time.

Considering my time zone 6 hours ahead of you. I modify the source as you said.
The discouraged print message I simple removed. For your reference.

---
 drivers/i2c/busses/i2c-cmi.c |   43 ++++++++++++++---------------------------
 1 files changed, 15 insertions(+), 28 deletions(-)

diff --git a/drivers/i2c/busses/i2c-cmi.c b/drivers/i2c/busses/i2c-cmi.c
index 555b482..e6cdefc 100644
--- a/drivers/i2c/busses/i2c-cmi.c
+++ b/drivers/i2c/busses/i2c-cmi.c
@@ -16,16 +16,15 @@
 #include <linux/i2c.h>
 #include <linux/acpi.h>
 
-#define ACPI_SMBUS_HC_COMPONENT		0x00080000
 #define ACPI_SMBUS_HC_CLASS		"smbus"
 #define ACPI_SMBUS_HC_DEVICE_NAME	"cmi"
 
 ACPI_MODULE_NAME("smbus_cmi");
 
 struct smbus_methods_t {
-	const char *mt_info;
-	const char *mt_sbr;
-	const char *mt_sbw;
+	char *mt_info;
+	char *mt_sbr;
+	char *mt_sbw;
 };
 
 struct acpi_smbus_cmi {
@@ -56,8 +55,8 @@ static const struct acpi_device_id acpi_smbus_cmi_ids[] = {
 #define ACPI_SMBUS_STATUS_ACC_DENY		0x17
 #define ACPI_SMBUS_STATUS_TIMEOUT		0x18
 #define ACPI_SMBUS_STATUS_NOTSUP		0x19
-#define ACPI_SMBUS_STATUS_BUSY			0x1A
-#define ACPI_SMBUS_STATUS_PEC			0x1F
+#define ACPI_SMBUS_STATUS_BUSY			0x1a
+#define ACPI_SMBUS_STATUS_PEC			0x1f
 
 #define ACPI_SMBUS_PRTCL_WRITE			0x00
 #define ACPI_SMBUS_PRTCL_READ			0x01
@@ -151,11 +150,11 @@ acpi_smbus_cmi_access(struct i2c_adapter *adap, u16 addr, unsigned short flags,
 
 	if (read_write == I2C_SMBUS_READ) {
 		protocol |= ACPI_SMBUS_PRTCL_READ;
-		method = (char *)smbus_methods.mt_sbr;
+		method = smbus_methods.mt_sbr;
 		input.count = 3;
 	} else {
 		protocol |= ACPI_SMBUS_PRTCL_WRITE;
-		method = (char *)smbus_methods.mt_sbw;
+		method = smbus_methods.mt_sbw;
 		input.count = 5;
 	}
 
@@ -282,17 +281,17 @@ static const struct i2c_algorithm acpi_smbus_cmi_algorithm = {
 	.functionality = acpi_smbus_cmi_func,
 };
 
+
 static int acpi_smbus_cmi_add_cap(struct acpi_smbus_cmi *smbus_cmi,
 				  const char *name)
 {
 	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
 	union acpi_object *obj;
-	int i;
 	acpi_status status;
 
 	if (!strcmp(name, smbus_methods.mt_info)) {
 		status = acpi_evaluate_object(smbus_cmi->handle,
-					(char *)smbus_methods.mt_info,
+					smbus_methods.mt_info,
 					NULL, &buffer);
 		if (ACPI_FAILURE(status)) {
 			ACPI_ERROR((AE_INFO, "Evaluating %s: %i",
@@ -311,21 +310,12 @@ static int acpi_smbus_cmi_add_cap(struct acpi_smbus_cmi *smbus_cmi,
 
 		if (obj->type != ACPI_TYPE_INTEGER) {
 			ACPI_ERROR((AE_INFO, "Invalid argument type"));
+			kfree(buffer.pointer);
 			return -EIO;
 		} else
 			ACPI_DEBUG_PRINT((ACPI_DB_INFO, "SMBus CMI Version %x"
 					  "\n", (int)obj->integer.value));
 
-		obj = obj->package.elements + 1;
-		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "SMB_INFO:\n"));
-		for (i = 0; i < obj->buffer.length; i++) {
-			ACPI_DEBUG_PRINT((ACPI_DB_INFO, "%02x ",
-						*(obj->buffer.pointer + i)));
-			if (i > 0 && (i % 16) == 0 && i != obj->buffer.length-1)
-				ACPI_DEBUG_PRINT((ACPI_DB_INFO, "\n"));
-		}
-		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "\n"));
-
 		kfree(buffer.pointer);
 		smbus_cmi->cap_info = 1;
 	} else if (!strcmp(name, smbus_methods.mt_sbr))
@@ -333,7 +323,8 @@ static int acpi_smbus_cmi_add_cap(struct acpi_smbus_cmi *smbus_cmi,
 	else if (!strcmp(name, smbus_methods.mt_sbw))
 		smbus_cmi->cap_write = 1;
 	else
-		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "CMI Methods: %s\n", name));
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Unsupported CMI Method: %s\n",
+				  name));
 
 	return 0;
 }
@@ -377,7 +368,7 @@ static int acpi_smbus_cmi_add(struct acpi_device *device)
 		goto err;
 
 	snprintf(smbus_cmi->adapter.name, sizeof(smbus_cmi->adapter.name),
-		"SMBus CMI adapter at %s-%s",
+		"SMBus CMI adapter %s (%s)",
 		acpi_device_name(device),
 		acpi_device_uid(device));
 	smbus_cmi->adapter.owner = THIS_MODULE;
@@ -405,7 +396,7 @@ static int acpi_smbus_cmi_remove(struct acpi_device *device, int type)
 
 	i2c_del_adapter(&smbus_cmi->adapter);
 	kfree(smbus_cmi);
-	smbus_cmi = NULL;
+	device->driver_data = NULL;
 
 	return 0;
 }
@@ -422,11 +413,7 @@ static struct acpi_driver acpi_smbus_cmi_driver = {
 
 static int __init acpi_smbus_cmi_init(void)
 {
-	int result;
-
-	result = acpi_bus_register_driver(&acpi_smbus_cmi_driver);
-
-	return result;
+	return acpi_bus_register_driver(&acpi_smbus_cmi_driver);
 }
 
 static void __exit acpi_smbus_cmi_exit(void)
-- 
1.6.0.4

-- 
Best Regards,
- Crane

Re: [PATCH v2] I2C: add driver for SMBus Control Method Interface

[Jean Delvare]

On Thu, 17 Sep 2009 12:46:47 +0800, Crane Cai wrote:
> Hi Jean,
> 
> On Wed, Sep 16, 2009 at 03:48:46PM +0200, Jean Delvare wrote:
> > If you are fine with my latest proposals, I'll implement them myself,
> > no need to resend a patch. Thanks a lot for writing the driver!
> OK, I'm fine. Now it is the time in v2.6.32 merge window. I wish we can submit
> it in time.

Yes, we are right in time for 2.6.32 :)

> 
> Considering my time zone 6 hours ahead of you. I modify the source as you said.
> The discouraged print message I simple removed. For your reference.
> 
> ---
>  drivers/i2c/busses/i2c-cmi.c |   43 ++++++++++++++---------------------------
>  1 files changed, 15 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-cmi.c b/drivers/i2c/busses/i2c-cmi.c
> index 555b482..e6cdefc 100644
> --- a/drivers/i2c/busses/i2c-cmi.c
> +++ b/drivers/i2c/busses/i2c-cmi.c
> @@ -16,16 +16,15 @@
>  #include <linux/i2c.h>
>  #include <linux/acpi.h>
>  
> -#define ACPI_SMBUS_HC_COMPONENT		0x00080000
>  #define ACPI_SMBUS_HC_CLASS		"smbus"
>  #define ACPI_SMBUS_HC_DEVICE_NAME	"cmi"
>  
>  ACPI_MODULE_NAME("smbus_cmi");
>  
>  struct smbus_methods_t {
> -	const char *mt_info;
> -	const char *mt_sbr;
> -	const char *mt_sbw;
> +	char *mt_info;
> +	char *mt_sbr;
> +	char *mt_sbw;
>  };
>  
>  struct acpi_smbus_cmi {
> @@ -56,8 +55,8 @@ static const struct acpi_device_id acpi_smbus_cmi_ids[] = {
>  #define ACPI_SMBUS_STATUS_ACC_DENY		0x17
>  #define ACPI_SMBUS_STATUS_TIMEOUT		0x18
>  #define ACPI_SMBUS_STATUS_NOTSUP		0x19
> -#define ACPI_SMBUS_STATUS_BUSY			0x1A
> -#define ACPI_SMBUS_STATUS_PEC			0x1F
> +#define ACPI_SMBUS_STATUS_BUSY			0x1a
> +#define ACPI_SMBUS_STATUS_PEC			0x1f
>  
>  #define ACPI_SMBUS_PRTCL_WRITE			0x00
>  #define ACPI_SMBUS_PRTCL_READ			0x01
> @@ -151,11 +150,11 @@ acpi_smbus_cmi_access(struct i2c_adapter *adap, u16 addr, unsigned short flags,
>  
>  	if (read_write == I2C_SMBUS_READ) {
>  		protocol |= ACPI_SMBUS_PRTCL_READ;
> -		method = (char *)smbus_methods.mt_sbr;
> +		method = smbus_methods.mt_sbr;
>  		input.count = 3;
>  	} else {
>  		protocol |= ACPI_SMBUS_PRTCL_WRITE;
> -		method = (char *)smbus_methods.mt_sbw;
> +		method = smbus_methods.mt_sbw;
>  		input.count = 5;
>  	}
>  
> @@ -282,17 +281,17 @@ static const struct i2c_algorithm acpi_smbus_cmi_algorithm = {
>  	.functionality = acpi_smbus_cmi_func,
>  };
>  
> +
>  static int acpi_smbus_cmi_add_cap(struct acpi_smbus_cmi *smbus_cmi,
>  				  const char *name)
>  {
>  	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
>  	union acpi_object *obj;
> -	int i;
>  	acpi_status status;
>  
>  	if (!strcmp(name, smbus_methods.mt_info)) {
>  		status = acpi_evaluate_object(smbus_cmi->handle,
> -					(char *)smbus_methods.mt_info,
> +					smbus_methods.mt_info,
>  					NULL, &buffer);
>  		if (ACPI_FAILURE(status)) {
>  			ACPI_ERROR((AE_INFO, "Evaluating %s: %i",
> @@ -311,21 +310,12 @@ static int acpi_smbus_cmi_add_cap(struct acpi_smbus_cmi *smbus_cmi,
>  
>  		if (obj->type != ACPI_TYPE_INTEGER) {
>  			ACPI_ERROR((AE_INFO, "Invalid argument type"));
> +			kfree(buffer.pointer);
>  			return -EIO;
>  		} else
>  			ACPI_DEBUG_PRINT((ACPI_DB_INFO, "SMBus CMI Version %x"
>  					  "\n", (int)obj->integer.value));
>  
> -		obj = obj->package.elements + 1;
> -		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "SMB_INFO:\n"));
> -		for (i = 0; i < obj->buffer.length; i++) {
> -			ACPI_DEBUG_PRINT((ACPI_DB_INFO, "%02x ",
> -						*(obj->buffer.pointer + i)));
> -			if (i > 0 && (i % 16) == 0 && i != obj->buffer.length-1)
> -				ACPI_DEBUG_PRINT((ACPI_DB_INFO, "\n"));
> -		}
> -		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "\n"));
> -
>  		kfree(buffer.pointer);
>  		smbus_cmi->cap_info = 1;
>  	} else if (!strcmp(name, smbus_methods.mt_sbr))
> @@ -333,7 +323,8 @@ static int acpi_smbus_cmi_add_cap(struct acpi_smbus_cmi *smbus_cmi,
>  	else if (!strcmp(name, smbus_methods.mt_sbw))
>  		smbus_cmi->cap_write = 1;
>  	else
> -		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "CMI Methods: %s\n", name));
> +		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Unsupported CMI Method: %s\n",
> +				  name));
>  
>  	return 0;
>  }
> @@ -377,7 +368,7 @@ static int acpi_smbus_cmi_add(struct acpi_device *device)
>  		goto err;
>  
>  	snprintf(smbus_cmi->adapter.name, sizeof(smbus_cmi->adapter.name),
> -		"SMBus CMI adapter at %s-%s",
> +		"SMBus CMI adapter %s (%s)",
>  		acpi_device_name(device),
>  		acpi_device_uid(device));
>  	smbus_cmi->adapter.owner = THIS_MODULE;
> @@ -405,7 +396,7 @@ static int acpi_smbus_cmi_remove(struct acpi_device *device, int type)
>  
>  	i2c_del_adapter(&smbus_cmi->adapter);
>  	kfree(smbus_cmi);
> -	smbus_cmi = NULL;
> +	device->driver_data = NULL;
>  
>  	return 0;
>  }
> @@ -422,11 +413,7 @@ static struct acpi_driver acpi_smbus_cmi_driver = {
>  
>  static int __init acpi_smbus_cmi_init(void)
>  {
> -	int result;
> -
> -	result = acpi_bus_register_driver(&acpi_smbus_cmi_driver);
> -
> -	return result;
> +	return acpi_bus_register_driver(&acpi_smbus_cmi_driver);
>  }
>  
>  static void __exit acpi_smbus_cmi_exit(void)

Changes merged, thanks.

-- 
Jean Delvare

Re: [PATCH v2] I2C: add driver for SMBus Control Method Interface

[Crane Cai]

Hi Jean,

On Thu, Sep 17, 2009 at 11:45:53AM +0200, Jean Delvare wrote:
> On Thu, 17 Sep 2009 12:46:47 +0800, Crane Cai wrote:
> > Hi Jean,
> > 
> > On Wed, Sep 16, 2009 at 03:48:46PM +0200, Jean Delvare wrote:
> > > If you are fine with my latest proposals, I'll implement them myself,
> > > no need to resend a patch. Thanks a lot for writing the driver!
> > OK, I'm fine. Now it is the time in v2.6.32 merge window. I wish we can submit
> > it in time.
> 
> Yes, we are right in time for 2.6.32 :)
> 
Thanks. It's my pleasure to co-work with you. I'd like to keep up.

-- 
Best Regards,
- Crane