2014-04-16 13:24:51

by Lan Tianyu

[permalink] [raw]
Subject: [PATCH 0/9] I2C ACPI operation region handler support

ACPI 5.0 spec(5.5.2.4.5) defines GenericSerialBus(i2c, spi, uart) operation
region. It allows ACPI aml code able to access such kind of devices to
implement some ACPI standard method.

On the Asus T100TA, Bios use GenericSerialBus operation region to access
i2c device to get battery info. So battery function depends on the I2C
operation region support. Here is the bug link.
https://bugzilla.kernel.org/show_bug.cgi?id=69011

This patchset is to add I2C ACPI operation region handler support.

[PATCH 1/9] ACPICA: Executer: Fix buffer allocation issue for
[PATCH 2/9] ACPICA: Export acpi_buffer_to_resource symbol
[PATCH 3/9] ACPI: Add acpi_bus_attach_private_data() to facilitate to
[PATCH 4/9] ACPI/Thermal: Use acpi_bus_attach_private_data() to
[PATCH 5/9] I2C: Add smbus quick read/write helper function
[PATCH 6/9] I2C: Add smbus word/block process call helper function
[PATCH 7/9] I2C/ACPI: Add i2c ACPI operation region support
[PATCH 8/9] I2C/ACPI: Move ACPI related code to i2c-acpi.c
[PATCH 9/9] I2C/ACPI: Add CONFIG_I2C_ACPI config


2014-04-16 13:25:47

by Lan Tianyu

[permalink] [raw]
Subject: [PATCH 1/9] ACPICA: Executer: Fix buffer allocation issue for generic_serial_bus region field accesses.

From: Lv Zheng <[email protected]>

The size of the buffer allocated for generic_serial_bus region access
is not correct. This patch introduces acpi_ex_get_serial_access_length()
to be invoked to obtain correct data buffer length. Reported by
Lan Tianyu, Fixed by Lv Zheng.

Signed-off-by: Lv Zheng <[email protected]>
Signed-off-by: Lan Tianyu <[email protected]>
---
drivers/acpi/acpica/exfield.c | 104 +++++++++++++++++++++++++++++++++++++++---
1 file changed, 97 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/acpica/exfield.c b/drivers/acpi/acpica/exfield.c
index 68d9744..12878e1 100644
--- a/drivers/acpi/acpica/exfield.c
+++ b/drivers/acpi/acpica/exfield.c
@@ -45,10 +45,71 @@
#include "accommon.h"
#include "acdispat.h"
#include "acinterp.h"
+#include "amlcode.h"

#define _COMPONENT ACPI_EXECUTER
ACPI_MODULE_NAME("exfield")

+/* Local prototypes */
+static u32
+acpi_ex_get_serial_access_length(u32 accessor_type, u32 access_length);
+
+/*******************************************************************************
+ *
+ * FUNCTION: acpi_get_serial_access_bytes
+ *
+ * PARAMETERS: accessor_type - The type of the protocol indicated by region
+ * field access attributes
+ * access_length - The access length of the region field
+ *
+ * RETURN: Decoded access length
+ *
+ * DESCRIPTION: This routine returns the length of the generic_serial_bus
+ * protocol bytes
+ *
+ ******************************************************************************/
+
+static u32
+acpi_ex_get_serial_access_length(u32 accessor_type, u32 access_length)
+{
+ u32 length;
+
+ switch (accessor_type) {
+ case AML_FIELD_ATTRIB_QUICK:
+
+ length = 0;
+ break;
+
+ case AML_FIELD_ATTRIB_SEND_RCV:
+ case AML_FIELD_ATTRIB_BYTE:
+
+ length = 1;
+ break;
+
+ case AML_FIELD_ATTRIB_WORD:
+ case AML_FIELD_ATTRIB_WORD_CALL:
+
+ length = 2;
+ break;
+
+ case AML_FIELD_ATTRIB_MULTIBYTE:
+ case AML_FIELD_ATTRIB_RAW_BYTES:
+ case AML_FIELD_ATTRIB_RAW_PROCESS:
+
+ length = access_length;
+ break;
+
+ case AML_FIELD_ATTRIB_BLOCK:
+ case AML_FIELD_ATTRIB_BLOCK_CALL:
+ default:
+
+ length = ACPI_GSBUS_BUFFER_SIZE;
+ break;
+ }
+
+ return (length);
+}
+
/*******************************************************************************
*
* FUNCTION: acpi_ex_read_data_from_field
@@ -63,8 +124,9 @@ ACPI_MODULE_NAME("exfield")
* Buffer, depending on the size of the field.
*
******************************************************************************/
+
acpi_status
-acpi_ex_read_data_from_field(struct acpi_walk_state *walk_state,
+acpi_ex_read_data_from_field(struct acpi_walk_state * walk_state,
union acpi_operand_object *obj_desc,
union acpi_operand_object **ret_buffer_desc)
{
@@ -73,6 +135,7 @@ acpi_ex_read_data_from_field(struct acpi_walk_state *walk_state,
acpi_size length;
void *buffer;
u32 function;
+ u16 accessor_type;

ACPI_FUNCTION_TRACE_PTR(ex_read_data_from_field, obj_desc);

@@ -116,9 +179,22 @@ acpi_ex_read_data_from_field(struct acpi_walk_state *walk_state,
ACPI_READ | (obj_desc->field.attribute << 16);
} else if (obj_desc->field.region_obj->region.space_id ==
ACPI_ADR_SPACE_GSBUS) {
- length = ACPI_GSBUS_BUFFER_SIZE;
- function =
- ACPI_READ | (obj_desc->field.attribute << 16);
+ accessor_type = obj_desc->field.attribute;
+ length = acpi_ex_get_serial_access_length(accessor_type,
+ obj_desc->
+ field.
+ access_length);
+
+ /*
+ * Add additional 2 bytes for modeled generic_serial_bus data buffer:
+ * typedef struct {
+ * BYTEStatus; // Byte 0 of the data buffer
+ * BYTELength; // Byte 1 of the data buffer
+ * BYTE[x-1]Data; // Bytes 2-x of the arbitrary length data buffer,
+ * }
+ */
+ length += 2;
+ function = ACPI_READ | (accessor_type << 16);
} else { /* IPMI */

length = ACPI_IPMI_BUFFER_SIZE;
@@ -231,6 +307,7 @@ acpi_ex_write_data_to_field(union acpi_operand_object *source_desc,
void *buffer;
union acpi_operand_object *buffer_desc;
u32 function;
+ u16 accessor_type;

ACPI_FUNCTION_TRACE_PTR(ex_write_data_to_field, obj_desc);

@@ -284,9 +361,22 @@ acpi_ex_write_data_to_field(union acpi_operand_object *source_desc,
ACPI_WRITE | (obj_desc->field.attribute << 16);
} else if (obj_desc->field.region_obj->region.space_id ==
ACPI_ADR_SPACE_GSBUS) {
- length = ACPI_GSBUS_BUFFER_SIZE;
- function =
- ACPI_WRITE | (obj_desc->field.attribute << 16);
+ accessor_type = obj_desc->field.attribute;
+ length = acpi_ex_get_serial_access_length(accessor_type,
+ obj_desc->
+ field.
+ access_length);
+
+ /*
+ * Add additional 2 bytes for modeled generic_serial_bus data buffer:
+ * typedef struct {
+ * BYTEStatus; // Byte 0 of the data buffer
+ * BYTELength; // Byte 1 of the data buffer
+ * BYTE[x-1]Data; // Bytes 2-x of the arbitrary length data buffer,
+ * }
+ */
+ length += 2;
+ function = ACPI_WRITE | (accessor_type << 16);
} else { /* IPMI */

length = ACPI_IPMI_BUFFER_SIZE;
--
1.8.4.rc0.1.g8f6a3e5.dirty

2014-04-16 13:26:11

by Lan Tianyu

[permalink] [raw]
Subject: [PATCH 3/9] ACPI: Add acpi_bus_attach_private_data() to facilitate to attach data to ACPI handle

There is already acpi_bus_get_private_data() to get ACPI handle data
which is associated with acpi_bus_private_data_handler(). This patch
is to add acpi_bus_attach_private_data() to make a pair and facilitate
to attach and get data to/from ACPI handle.

Signed-off-by: Lan Tianyu <[email protected]>
---
drivers/acpi/bus.c | 18 +++++++++++++++++-
include/acpi/acpi_bus.h | 1 +
2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index e7e5844..4ed8d48 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -132,6 +132,22 @@ void acpi_bus_private_data_handler(acpi_handle handle,
}
EXPORT_SYMBOL(acpi_bus_private_data_handler);

+int acpi_bus_attach_private_data(acpi_handle handle, void *data)
+{
+ acpi_status status;
+
+ status = acpi_attach_data(handle,
+ acpi_bus_private_data_handler, data);
+ if (ACPI_FAILURE(status)) {
+ ACPI_ERROR((AE_INFO, "Error attaching device[%p] data\n",
+ handle));
+ return -ENODEV;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL(acpi_bus_attach_private_data);
+
int acpi_bus_get_private_data(acpi_handle handle, void **data)
{
acpi_status status;
@@ -141,7 +157,7 @@ int acpi_bus_get_private_data(acpi_handle handle, void **data)

status = acpi_get_data(handle, acpi_bus_private_data_handler, data);
if (ACPI_FAILURE(status) || !*data) {
- ACPI_DEBUG_PRINT((ACPI_DB_INFO, "No context for object [%p]\n",
+ ACPI_ERROR((AE_INFO, "No context for object [%p]\n",
handle));
return -ENODEV;
}
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 84a2e29..a1f6fec 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -406,6 +406,7 @@ extern struct kobject *acpi_kobj;
extern int acpi_bus_generate_netlink_event(const char*, const char*, u8, int);
void acpi_bus_private_data_handler(acpi_handle, void *);
int acpi_bus_get_private_data(acpi_handle, void **);
+int acpi_bus_attach_private_data(acpi_handle, void *);
void acpi_bus_no_hotplug(acpi_handle handle);
extern int acpi_notifier_call_chain(struct acpi_device *, u32, u32);
extern int register_acpi_notifier(struct notifier_block *);
--
1.8.4.rc0.1.g8f6a3e5.dirty

2014-04-16 13:26:29

by Lan Tianyu

[permalink] [raw]
Subject: [PATCH 6/9] I2C: Add smbus word/block process call helper function

Add i2c_smbus_word/block_proc_call() helper function. These will be used
in the implementation of i2c ACPI address space handler.

Signed-off-by: Lan Tianyu <[email protected]>
---
drivers/i2c/i2c-core.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/i2c.h | 4 ++++
2 files changed, 60 insertions(+)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 3bf0048..638befd 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -2306,6 +2306,30 @@ s32 i2c_smbus_write_word_data(const struct i2c_client *client, u8 command,
EXPORT_SYMBOL(i2c_smbus_write_word_data);

/**
+ * i2c_smbus_word_proc_call - SMBus "word proc call" protocol
+ * @client: Handle to slave device
+ * @command: Byte interpreted by slave
+ * @value: 16-bit "word" being written
+ *
+ * This executes the SMBus "word proc all" protocol, returning negative errno
+ * else a 16-bit unsigned "word" received from the device.
+ */
+s32 i2c_smbus_word_proc_call(const struct i2c_client *client, u8 command,
+ u16 value)
+{
+ union i2c_smbus_data data;
+ int status;
+
+ data.word = value;
+ status = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
+ I2C_SMBUS_READ, command,
+ I2C_SMBUS_PROC_CALL, &data);
+
+ return (status < 0) ? status : data.word;
+}
+EXPORT_SYMBOL(i2c_smbus_word_proc_call);
+
+/**
* i2c_smbus_read_block_data - SMBus "block read" protocol
* @client: Handle to slave device
* @command: Byte interpreted by slave
@@ -2362,6 +2386,38 @@ s32 i2c_smbus_write_block_data(const struct i2c_client *client, u8 command,
}
EXPORT_SYMBOL(i2c_smbus_write_block_data);

+/**
+ * i2c_smbus_block_proc_call - SMBus "block write" protocol
+ * @client: Handle to slave device
+ * @command: Byte interpreted by slave
+ * @length: Size of data block; SMBus allows at most 32 bytes
+ * @values: Byte array which will be written.
+ *
+ * This executes the SMBus "block proc call" protocol, returning negative errno
+ * else the number of read bytes.
+ */
+s32 i2c_smbus_block_proc_call(const struct i2c_client *client, u8 command,
+ u8 length, u8 *values)
+{
+ union i2c_smbus_data data;
+ int status;
+
+ if (length > I2C_SMBUS_BLOCK_MAX)
+ length = I2C_SMBUS_BLOCK_MAX;
+ data.block[0] = length;
+ memcpy(&data.block[1], values, length);
+ status = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
+ I2C_SMBUS_READ, command,
+ I2C_SMBUS_BLOCK_PROC_CALL, &data);
+
+ if (status < 0)
+ return status;
+
+ memcpy(values, &data.block[1], data.block[0]);
+ return data.block[0];
+}
+EXPORT_SYMBOL(i2c_smbus_block_proc_call);
+
/* Returns the number of read bytes */
s32 i2c_smbus_read_i2c_block_data(const struct i2c_client *client, u8 command,
u8 length, u8 *values)
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 3e6ea90..724440a 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -94,6 +94,10 @@ extern s32 i2c_smbus_read_word_data(const struct i2c_client *client,
u8 command);
extern s32 i2c_smbus_write_word_data(const struct i2c_client *client,
u8 command, u16 value);
+extern s32 i2c_smbus_word_proc_call(const struct i2c_client *client,
+ u8 command, u16 value);
+extern s32 i2c_smbus_block_proc_call(const struct i2c_client *client,
+ u8 command, u8 length, u8 *values);

static inline s32
i2c_smbus_read_word_swapped(const struct i2c_client *client, u8 command)
--
1.8.4.rc0.1.g8f6a3e5.dirty

2014-04-16 13:26:33

by Lan Tianyu

[permalink] [raw]
Subject: [PATCH 5/9] I2C: Add smbus quick read/write helper function

Add i2c_smbus_quick_write/read() helper function. These will be used
in the implementation of i2c ACPI address space handler.

Signed-off-by: Lan Tianyu <[email protected]>
---
drivers/i2c/i2c-core.c | 30 ++++++++++++++++++++++++++++++
include/linux/i2c.h | 2 ++
2 files changed, 32 insertions(+)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 7c7f4b8..3bf0048 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -2162,6 +2162,36 @@ static int i2c_smbus_check_pec(u8 cpec, struct i2c_msg *msg)
}

/**
+ * i2c_smbus_quick_write - SMBus "quick write" protocol
+ * @client: Handle to slave device
+ *
+ * This executes the SMBus "quick write" protocol, returning negative errno
+ * else zero on success.
+ */
+s32 i2c_smbus_quick_write(const struct i2c_client *client)
+{
+ return i2c_smbus_xfer(client->adapter, client->addr, client->flags,
+ I2C_SMBUS_WRITE, 0,
+ I2C_SMBUS_QUICK, NULL);
+}
+EXPORT_SYMBOL_GPL(i2c_smbus_quick_write);
+
+/**
+ * i2c_smbus_quick_read - SMBus "quick read" protocol
+ * @client: Handle to slave device
+ *
+ * This executes the SMBus "quick read" protocol, returning negative errno
+ * else zero on success.
+ */
+s32 i2c_smbus_quick_read(const struct i2c_client *client)
+{
+ return i2c_smbus_xfer(client->adapter, client->addr, client->flags,
+ I2C_SMBUS_READ, 0,
+ I2C_SMBUS_QUICK, NULL);
+}
+EXPORT_SYMBOL_GPL(i2c_smbus_quick_read);
+
+/**
* i2c_smbus_read_byte - SMBus "receive byte" protocol
* @client: Handle to slave device
*
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index b556e0a..3e6ea90 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -82,6 +82,8 @@ extern s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
/* Now follow the 'nice' access routines. These also document the calling
conventions of i2c_smbus_xfer. */

+extern s32 i2c_smbus_quick_write(const struct i2c_client *client);
+extern s32 i2c_smbus_quick_read(const struct i2c_client *client);
extern s32 i2c_smbus_read_byte(const struct i2c_client *client);
extern s32 i2c_smbus_write_byte(const struct i2c_client *client, u8 value);
extern s32 i2c_smbus_read_byte_data(const struct i2c_client *client,
--
1.8.4.rc0.1.g8f6a3e5.dirty

2014-04-16 13:26:42

by Lan Tianyu

[permalink] [raw]
Subject: [PATCH 9/9] I2C/ACPI: Add CONFIG_I2C_ACPI config

This patch is to add CONFIG_I2C_ACPI. Current there is a race between
removing I2C ACPI operation region and ACPI AML code accessing.
So make i2c core built-in if CONFIG_I2C_ACPI is set. The race will
be fixed later in the ACPICA.

Signed-off-by: Lan Tianyu <[email protected]>
---
drivers/i2c/Kconfig | 16 +++++++++++++++-
drivers/i2c/Makefile | 2 +-
include/linux/i2c.h | 2 +-
3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
index 7b7ea32..15d782e 100644
--- a/drivers/i2c/Kconfig
+++ b/drivers/i2c/Kconfig
@@ -2,7 +2,9 @@
# I2C subsystem configuration
#

-menuconfig I2C
+menu "I2C support"
+
+config I2C
tristate "I2C support"
select RT_MUTEXES
---help---
@@ -21,6 +23,16 @@ menuconfig I2C
This I2C support can also be built as a module. If so, the module
will be called i2c-core.

+config I2C_ACPI
+ bool "I2C ACPI support"
+ select I2C
+ depends on ACPI
+ help
+ Say Y here if you want to enable I2C ACPI function. ACPI table
+ provides I2C slave devices' information to enumerate these devices.
+ This option also allows ACPI AML code to access I2C slave devices
+ via I2C ACPI operation region to fulfill ACPI method.
+
if I2C

config I2C_BOARDINFO
@@ -124,3 +136,5 @@ config I2C_DEBUG_BUS
on.

endif # I2C
+
+endmenu
diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
index 80db307..37464ee 100644
--- a/drivers/i2c/Makefile
+++ b/drivers/i2c/Makefile
@@ -3,7 +3,7 @@
#

i2ccore-y := i2c-core.o
-i2ccore-$(CONFIG_ACPI) += i2c-acpi.o
+i2ccore-$(CONFIG_I2C_ACPI) += i2c-acpi.o

obj-$(CONFIG_I2C_BOARDINFO) += i2c-boardinfo.o
obj-$(CONFIG_I2C) += i2ccore.o
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index fc1ef42..d0ece9f 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -583,7 +583,7 @@ static inline struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node
}
#endif /* CONFIG_OF */

-#ifdef CONFIG_ACPI
+#ifdef CONFIG_I2C_ACPI
int acpi_i2c_install_space_handler(struct i2c_adapter *adapter);
void acpi_i2c_remove_space_handler(struct i2c_adapter *adapter);
void acpi_i2c_register_devices(struct i2c_adapter *adap);
--
1.8.4.rc0.1.g8f6a3e5.dirty

2014-04-16 13:26:44

by Lan Tianyu

[permalink] [raw]
Subject: [PATCH 8/9] I2C/ACPI: Move ACPI related code to i2c-acpi.c

Clean up ACPI related code in the i2c core.

Signed-off-by: Lan Tianyu <[email protected]>
---
drivers/i2c/i2c-acpi.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++
drivers/i2c/i2c-core.c | 95 --------------------------------------------------
include/linux/i2c.h | 3 ++
3 files changed, 92 insertions(+), 95 deletions(-)

diff --git a/drivers/i2c/i2c-acpi.c b/drivers/i2c/i2c-acpi.c
index 942abdf..a0ae867 100644
--- a/drivers/i2c/i2c-acpi.c
+++ b/drivers/i2c/i2c-acpi.c
@@ -37,6 +37,95 @@ struct gsb_buffer {
};
} __packed;

+static int acpi_i2c_add_resource(struct acpi_resource *ares, void *data)
+{
+ struct i2c_board_info *info = data;
+
+ if (ares->type == ACPI_RESOURCE_TYPE_SERIAL_BUS) {
+ struct acpi_resource_i2c_serialbus *sb;
+
+ sb = &ares->data.i2c_serial_bus;
+ if (sb->type == ACPI_RESOURCE_SERIAL_TYPE_I2C) {
+ info->addr = sb->slave_address;
+ if (sb->access_mode == ACPI_I2C_10BIT_MODE)
+ info->flags |= I2C_CLIENT_TEN;
+ }
+ } else if (info->irq < 0) {
+ struct resource r;
+
+ if (acpi_dev_resource_interrupt(ares, 0, &r))
+ info->irq = r.start;
+ }
+
+ /* Tell the ACPI core to skip this resource */
+ return 1;
+}
+
+static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
+ void *data, void **return_value)
+{
+ struct i2c_adapter *adapter = data;
+ struct list_head resource_list;
+ struct i2c_board_info info;
+ struct acpi_device *adev;
+ int ret;
+
+ if (acpi_bus_get_device(handle, &adev))
+ return AE_OK;
+ if (acpi_bus_get_status(adev) || !adev->status.present)
+ return AE_OK;
+
+ memset(&info, 0, sizeof(info));
+ info.acpi_node.companion = adev;
+ info.irq = -1;
+
+ INIT_LIST_HEAD(&resource_list);
+ ret = acpi_dev_get_resources(adev, &resource_list,
+ acpi_i2c_add_resource, &info);
+ acpi_dev_free_resource_list(&resource_list);
+
+ if (ret < 0 || !info.addr)
+ return AE_OK;
+
+ adev->power.flags.ignore_parent = true;
+ strlcpy(info.type, dev_name(&adev->dev), sizeof(info.type));
+ if (!i2c_new_device(adapter, &info)) {
+ adev->power.flags.ignore_parent = false;
+ dev_err(&adapter->dev,
+ "failed to add I2C device %s from ACPI\n",
+ dev_name(&adev->dev));
+ }
+
+ return AE_OK;
+}
+
+/**
+ * acpi_i2c_register_devices - enumerate I2C slave devices behind adapter
+ * @adap: pointer to adapter
+ *
+ * Enumerate all I2C slave devices behind this adapter by walking the ACPI
+ * namespace. When a device is found it will be added to the Linux device
+ * model and bound to the corresponding ACPI handle.
+ */
+void acpi_i2c_register_devices(struct i2c_adapter *adap)
+{
+ acpi_handle handle;
+ acpi_status status;
+
+ if (!adap->dev.parent)
+ return;
+
+ handle = ACPI_HANDLE(adap->dev.parent);
+ if (!handle)
+ return;
+
+ status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
+ acpi_i2c_add_device, NULL,
+ adap, NULL);
+ if (ACPI_FAILURE(status))
+ dev_warn(&adap->dev, "failed to enumerate I2C slaves\n");
+}
+
static int acpi_gsb_i2c_read_bytes(struct i2c_client *client,
u8 cmd, u8 *data, u8 data_len)
{
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index bb9dc56..9ca50c8 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1092,101 +1092,6 @@ EXPORT_SYMBOL(of_find_i2c_adapter_by_node);
static void of_i2c_register_devices(struct i2c_adapter *adap) { }
#endif /* CONFIG_OF */

-/* ACPI support code */
-
-#if IS_ENABLED(CONFIG_ACPI)
-static int acpi_i2c_add_resource(struct acpi_resource *ares, void *data)
-{
- struct i2c_board_info *info = data;
-
- if (ares->type == ACPI_RESOURCE_TYPE_SERIAL_BUS) {
- struct acpi_resource_i2c_serialbus *sb;
-
- sb = &ares->data.i2c_serial_bus;
- if (sb->type == ACPI_RESOURCE_SERIAL_TYPE_I2C) {
- info->addr = sb->slave_address;
- if (sb->access_mode == ACPI_I2C_10BIT_MODE)
- info->flags |= I2C_CLIENT_TEN;
- }
- } else if (info->irq < 0) {
- struct resource r;
-
- if (acpi_dev_resource_interrupt(ares, 0, &r))
- info->irq = r.start;
- }
-
- /* Tell the ACPI core to skip this resource */
- return 1;
-}
-
-static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
- void *data, void **return_value)
-{
- struct i2c_adapter *adapter = data;
- struct list_head resource_list;
- struct i2c_board_info info;
- struct acpi_device *adev;
- int ret;
-
- if (acpi_bus_get_device(handle, &adev))
- return AE_OK;
- if (acpi_bus_get_status(adev) || !adev->status.present)
- return AE_OK;
-
- memset(&info, 0, sizeof(info));
- info.acpi_node.companion = adev;
- info.irq = -1;
-
- INIT_LIST_HEAD(&resource_list);
- ret = acpi_dev_get_resources(adev, &resource_list,
- acpi_i2c_add_resource, &info);
- acpi_dev_free_resource_list(&resource_list);
-
- if (ret < 0 || !info.addr)
- return AE_OK;
-
- adev->power.flags.ignore_parent = true;
- strlcpy(info.type, dev_name(&adev->dev), sizeof(info.type));
- if (!i2c_new_device(adapter, &info)) {
- adev->power.flags.ignore_parent = false;
- dev_err(&adapter->dev,
- "failed to add I2C device %s from ACPI\n",
- dev_name(&adev->dev));
- }
-
- return AE_OK;
-}
-
-/**
- * acpi_i2c_register_devices - enumerate I2C slave devices behind adapter
- * @adap: pointer to adapter
- *
- * Enumerate all I2C slave devices behind this adapter by walking the ACPI
- * namespace. When a device is found it will be added to the Linux device
- * model and bound to the corresponding ACPI handle.
- */
-static void acpi_i2c_register_devices(struct i2c_adapter *adap)
-{
- acpi_handle handle;
- acpi_status status;
-
- if (!adap->dev.parent)
- return;
-
- handle = ACPI_HANDLE(adap->dev.parent);
- if (!handle)
- return;
-
- status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
- acpi_i2c_add_device, NULL,
- adap, NULL);
- if (ACPI_FAILURE(status))
- dev_warn(&adap->dev, "failed to enumerate I2C slaves\n");
-}
-#else
-static inline void acpi_i2c_register_devices(struct i2c_adapter *adap) {}
-#endif /* CONFIG_ACPI */
-
static int i2c_do_add_adapter(struct i2c_driver *driver,
struct i2c_adapter *adap)
{
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 681e689..fc1ef42 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -586,7 +586,10 @@ static inline struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node
#ifdef CONFIG_ACPI
int acpi_i2c_install_space_handler(struct i2c_adapter *adapter);
void acpi_i2c_remove_space_handler(struct i2c_adapter *adapter);
+void acpi_i2c_register_devices(struct i2c_adapter *adap);
#else
+static inline void acpi_i2c_register_devices(struct i2c_adapter *adap) {};
+
static inline void acpi_i2c_remove_space_handler(struct i2c_adapter *adapter)
{ }
static inline int acpi_i2c_install_space_handler(struct i2c_adapter *adapter)
--
1.8.4.rc0.1.g8f6a3e5.dirty

2014-04-16 13:27:53

by Lan Tianyu

[permalink] [raw]
Subject: [PATCH 7/9] I2C/ACPI: Add i2c ACPI operation region support

ACPI 5.0 spec(5.5.2.4.5) defines GenericSerialBus(i2c, spi, uart) operation region.
It allows ACPI aml code able to access such kind of devices to implement
some ACPI standard method.

ACPI Spec defines some access attribute to associate with i2c protocol.
AttribQuick Read/Write Quick Protocol
AttribSendReceive Send/Receive Byte Protocol
AttribByte Read/Write Byte Protocol
AttribWord Read/Write Word Protocol
AttribBlock Read/Write Block Protocol
AttribBytes Read/Write N-Bytes Protocol
AttribProcessCall Process Call Protocol
AttribBlockProcessCall Write Block-Read Block Process Call Protocol
AttribRawBytes Raw Read/Write N-BytesProtocol
AttribRawProcessBytes Raw Process Call Protocol

On the Asus T100TA, Bios use GenericSerialBus operation region to access
i2c device to get battery info.

Sample code From Asus T100TA

Scope (_SB.I2C1)
{
Name (UMPC, ResourceTemplate ()
{
I2cSerialBus (0x0066, ControllerInitiated, 0x00061A80,
AddressingMode7Bit, "\\_SB.I2C1",
0x00, ResourceConsumer, ,
)
})

...

OperationRegion (DVUM, GenericSerialBus, Zero, 0x0100)
Field (DVUM, BufferAcc, NoLock, Preserve)
{
Connection (UMPC),
Offset (0x81),
AccessAs (BufferAcc, AttribBytes (0x3E)),
FGC0, 8
}
...
}

Device (BATC)
{
Name (_HID, EisaId ("PNP0C0A")) // _HID: Hardware ID
Name (_UID, One) // _UID: Unique ID
...

Method (_BST, 0, NotSerialized) // _BST: Battery Status
{
If (LEqual (AVBL, One))
{
Store (FGC0, BFFG)
If (LNotEqual (STAT, One))
{
ShiftRight (CHST, 0x04, Local0)
And (Local0, 0x03, Local0)
If (LOr (LEqual (Local0, One), LEqual (Local0, 0x02)))
{
Store (0x02, Local1)
}
...

}

The i2c operation region is defined under I2C1 scope. _BST method under
battery device BATC read battery status from the field "FCG0". The request
would be sent to i2c operation region handler.

This patch is to add i2c ACPI operation region support. Due to there are
only "Byte" and "N-Bytes" protocol access on the Asus T100TA, other protocols
have not been tested.

About RawBytes and RawProcessBytes protocol, they needs specific drivers to interpret
reference data from AML code according ACPI 5.0 SPEC(5.5.2.4.5.3.9 and 5.5.2.4.5.3.10).
So far, not found such case and will add when find real case.

Signed-off-by: Lan Tianyu <[email protected]>
---
drivers/i2c/Makefile | 5 +-
drivers/i2c/i2c-acpi.c | 282 +++++++++++++++++++++++++++++++++++++++++++++++++
drivers/i2c/i2c-core.c | 2 +
include/linux/acpi.h | 11 ++
include/linux/i2c.h | 10 ++
5 files changed, 309 insertions(+), 1 deletion(-)
create mode 100644 drivers/i2c/i2c-acpi.c

diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
index 1722f50..80db307 100644
--- a/drivers/i2c/Makefile
+++ b/drivers/i2c/Makefile
@@ -2,8 +2,11 @@
# Makefile for the i2c core.
#

+i2ccore-y := i2c-core.o
+i2ccore-$(CONFIG_ACPI) += i2c-acpi.o
+
obj-$(CONFIG_I2C_BOARDINFO) += i2c-boardinfo.o
-obj-$(CONFIG_I2C) += i2c-core.o
+obj-$(CONFIG_I2C) += i2ccore.o
obj-$(CONFIG_I2C_SMBUS) += i2c-smbus.o
obj-$(CONFIG_I2C_CHARDEV) += i2c-dev.o
obj-$(CONFIG_I2C_MUX) += i2c-mux.o
diff --git a/drivers/i2c/i2c-acpi.c b/drivers/i2c/i2c-acpi.c
new file mode 100644
index 0000000..942abdf
--- /dev/null
+++ b/drivers/i2c/i2c-acpi.c
@@ -0,0 +1,282 @@
+/*
+ * I2C ACPI code
+ *
+ * Copyright (C) 2014 Intel Corp
+ *
+ * Author: Lan Tianyu <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * 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.
+ */
+#define pr_fmt(fmt) "I2C/ACPI : " fmt
+
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/acpi.h>
+
+struct acpi_i2c_handler_data {
+ struct acpi_connection_info info;
+ struct i2c_adapter *adapter;
+} __packed;
+
+struct gsb_buffer {
+ u8 status;
+ u8 len;
+ union {
+ u16 wdata;
+ u8 bdata;
+ u8 data[0];
+ };
+} __packed;
+
+static int acpi_gsb_i2c_read_bytes(struct i2c_client *client,
+ u8 cmd, u8 *data, u8 data_len)
+{
+
+ struct i2c_msg msgs[2];
+ int ret;
+ u8 *buffer;
+
+ buffer = kzalloc(data_len, GFP_KERNEL);
+ if (!buffer)
+ return AE_NO_MEMORY;
+
+ msgs[0].addr = client->addr;
+ msgs[0].flags = client->flags;
+ msgs[0].len = 1;
+ msgs[0].buf = &cmd;
+
+ msgs[1].addr = client->addr;
+ msgs[1].flags = client->flags | I2C_M_RD;
+ msgs[1].len = data_len;
+ msgs[1].buf = buffer;
+
+ ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
+ if (ret < 0)
+ pr_err("i2c read failed\n");
+ else
+ memcpy(data, buffer, data_len);
+
+ kfree(buffer);
+ return ret;
+}
+
+static int acpi_gsb_i2c_write_bytes(struct i2c_client *client,
+ u8 cmd, u8 *data, u8 data_len)
+{
+
+ struct i2c_msg msgs[1];
+ u8 *buffer;
+ int ret = AE_OK;
+
+ buffer = kzalloc(data_len + 1, GFP_KERNEL);
+ if (!buffer)
+ return AE_NO_MEMORY;
+
+ buffer[0] = cmd;
+ memcpy(buffer + 1, data, data_len);
+
+ msgs[0].addr = client->addr;
+ msgs[0].flags = client->flags;
+ msgs[0].len = data_len + 1;
+ msgs[0].buf = buffer;
+
+ ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
+ if (ret < 0)
+ pr_err("i2c write failed\n");
+
+ kfree(buffer);
+ return ret;
+}
+
+static acpi_status
+acpi_i2c_space_handler(u32 function, acpi_physical_address command,
+ u32 bits, u64 *value64,
+ void *handler_context, void *region_context)
+{
+ struct gsb_buffer *gsb = (struct gsb_buffer *)value64;
+ struct acpi_i2c_handler_data *data = handler_context;
+ struct acpi_connection_info *info = &data->info;
+ struct acpi_resource_i2c_serialbus *sb;
+ struct i2c_adapter *adapter = data->adapter;
+ struct i2c_client client;
+ struct acpi_resource *ares;
+ u32 accessor_type = function >> 16;
+ u8 action = function & ACPI_IO_MASK;
+ int status;
+
+ acpi_buffer_to_resource(info->connection, info->length, &ares);
+ if (!value64 || ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
+ return AE_BAD_PARAMETER;
+
+ sb = &ares->data.i2c_serial_bus;
+ if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_I2C)
+ return AE_BAD_PARAMETER;
+
+ client.adapter = adapter;
+ client.addr = sb->slave_address;
+ client.flags = 0;
+
+ if (sb->access_mode == ACPI_I2C_10BIT_MODE)
+ client.flags |= I2C_CLIENT_TEN;
+
+ switch (accessor_type) {
+ case ACPI_GSB_ACCESS_ATTRIB_QUICK:
+ if (action == ACPI_READ)
+ status = i2c_smbus_quick_read(&client);
+ else
+ status = i2c_smbus_quick_write(&client);
+ break;
+
+ case ACPI_GSB_ACCESS_ATTRIB_SEND_RCV:
+ if (action == ACPI_READ) {
+ status = i2c_smbus_read_byte(&client);
+ if (status >= 0) {
+ gsb->bdata = status;
+ status = 0;
+ }
+ } else {
+ status = i2c_smbus_write_byte(&client, gsb->bdata);
+ }
+ break;
+
+ case ACPI_GSB_ACCESS_ATTRIB_BYTE:
+ if (action == ACPI_READ) {
+ status = i2c_smbus_read_byte_data(&client, command);
+ if (status >= 0) {
+ gsb->bdata = status;
+ status = 0;
+ }
+ } else {
+ status = i2c_smbus_write_byte_data(&client, command,
+ gsb->bdata);
+ }
+ break;
+
+ case ACPI_GSB_ACCESS_ATTRIB_WORD:
+ if (action == ACPI_READ) {
+ status = i2c_smbus_read_word_data(&client, command);
+ if (status >= 0) {
+ gsb->wdata = status;
+ status = 0;
+ }
+ } else {
+ status = i2c_smbus_write_word_data(&client, command,
+ gsb->wdata);
+ }
+ break;
+
+ case ACPI_GSB_ACCESS_ATTRIB_BLOCK:
+ if (action == ACPI_READ) {
+ status = i2c_smbus_read_block_data(&client, command,
+ gsb->data);
+ if (status >= 0) {
+ gsb->len = status;
+ status = 0;
+ }
+ } else {
+ status = i2c_smbus_write_block_data(&client, command,
+ gsb->len, gsb->data);
+ }
+ break;
+
+ case ACPI_GSB_ACCESS_ATTRIB_MULTIBYTE:
+ if (action == ACPI_READ) {
+ status = acpi_gsb_i2c_read_bytes(&client, command,
+ gsb->data, info->access_length);
+ if (status > 0)
+ status = 0;
+ } else {
+ status = acpi_gsb_i2c_write_bytes(&client, command,
+ gsb->data, info->access_length);
+ }
+ break;
+
+ case ACPI_GSB_ACCESS_ATTRIB_WORD_CALL:
+ status = i2c_smbus_word_proc_call(&client, command, gsb->wdata);
+ if (status >= 0) {
+ gsb->wdata = status;
+ status = 0;
+ }
+ break;
+
+ case ACPI_GSB_ACCESS_ATTRIB_BLOCK_CALL:
+ status = i2c_smbus_block_proc_call(&client, command, gsb->len,
+ gsb->data);
+ if (status > 0) {
+ gsb->len = status;
+ status = 0;
+ }
+ break;
+
+ default:
+ pr_info("protocl(0x%02x) is not supported.\n", accessor_type);
+ return AE_BAD_PARAMETER;
+ }
+
+ gsb->status = status;
+ return AE_OK;
+}
+
+
+int acpi_i2c_install_space_handler(struct i2c_adapter *adapter)
+{
+ acpi_handle handle = ACPI_HANDLE(adapter->dev.parent);
+ struct acpi_i2c_handler_data *data;
+ acpi_status status;
+
+ if (!handle)
+ return -ENODEV;
+
+ data = kzalloc(sizeof(struct acpi_i2c_handler_data),
+ GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ data->adapter = adapter;
+ status = acpi_bus_attach_private_data(handle, (void *)data);
+ if (ACPI_FAILURE(status)) {
+ kfree(data);
+ return -ENOMEM;
+ }
+
+ status = acpi_install_address_space_handler(handle,
+ ACPI_ADR_SPACE_GSBUS,
+ &acpi_i2c_space_handler,
+ NULL,
+ data);
+ if (ACPI_FAILURE(status)) {
+ pr_err("Error installing i2c space handler\n");
+ acpi_bus_attach_private_data(handle, NULL);
+ kfree(data);
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
+void acpi_i2c_remove_space_handler(struct i2c_adapter *adapter)
+{
+ acpi_handle handle = ACPI_HANDLE(adapter->dev.parent);
+ struct acpi_i2c_handler_data *data;
+ acpi_status status;
+
+ if (!handle)
+ return;
+
+ acpi_remove_address_space_handler(handle,
+ ACPI_ADR_SPACE_GSBUS,
+ &acpi_i2c_space_handler);
+
+ status = acpi_bus_get_private_data(handle, (void **)&data);
+ if (ACPI_SUCCESS(status))
+ kfree(data);
+}
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 638befd..bb9dc56 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1293,6 +1293,7 @@ exit_recovery:
/* create pre-declared device nodes */
of_i2c_register_devices(adap);
acpi_i2c_register_devices(adap);
+ acpi_i2c_install_space_handler(adap);

if (adap->nr < __i2c_first_dynamic_bus_num)
i2c_scan_static_board_info(adap);
@@ -1466,6 +1467,7 @@ void i2c_del_adapter(struct i2c_adapter *adap)
return;
}

+ acpi_i2c_remove_space_handler(adap);
/* Tell drivers about this removal */
mutex_lock(&core_lock);
bus_for_each_drv(&i2c_bus_type, NULL, adap,
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 7a8f2cd..ea53b9b 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -361,6 +361,17 @@ extern bool osc_sb_apei_support_acked;
#define OSC_PCI_EXPRESS_CAPABILITY_CONTROL 0x00000010
#define OSC_PCI_CONTROL_MASKS 0x0000001f

+#define ACPI_GSB_ACCESS_ATTRIB_QUICK 0x00000002
+#define ACPI_GSB_ACCESS_ATTRIB_SEND_RCV 0x00000004
+#define ACPI_GSB_ACCESS_ATTRIB_BYTE 0x00000006
+#define ACPI_GSB_ACCESS_ATTRIB_WORD 0x00000008
+#define ACPI_GSB_ACCESS_ATTRIB_BLOCK 0x0000000A
+#define ACPI_GSB_ACCESS_ATTRIB_MULTIBYTE 0x0000000B
+#define ACPI_GSB_ACCESS_ATTRIB_WORD_CALL 0x0000000C
+#define ACPI_GSB_ACCESS_ATTRIB_BLOCK_CALL 0x0000000D
+#define ACPI_GSB_ACCESS_ATTRIB_RAW_BYTES 0x0000000E
+#define ACPI_GSB_ACCESS_ATTRIB_RAW_PROCESS 0x0000000F
+
extern acpi_status acpi_pci_osc_control_set(acpi_handle handle,
u32 *mask, u32 req);

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 724440a..681e689 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -583,4 +583,14 @@ static inline struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node
}
#endif /* CONFIG_OF */

+#ifdef CONFIG_ACPI
+int acpi_i2c_install_space_handler(struct i2c_adapter *adapter);
+void acpi_i2c_remove_space_handler(struct i2c_adapter *adapter);
+#else
+static inline void acpi_i2c_remove_space_handler(struct i2c_adapter *adapter)
+{ }
+static inline int acpi_i2c_install_space_handler(struct i2c_adapter *adapter)
+{ return 0; }
+#endif
+
#endif /* _LINUX_I2C_H */
--
1.8.4.rc0.1.g8f6a3e5.dirty

2014-04-16 13:26:08

by Lan Tianyu

[permalink] [raw]
Subject: [PATCH 4/9] ACPI/Thermal: Use acpi_bus_attach_private_data() to attach private data

Use acpi_bus_attach_private_data() to attach private data
instead of acpi_attach_data().

Signed-off-by: Lan Tianyu <[email protected]>
---
drivers/acpi/thermal.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
index c1e31a4..7ab9392 100644
--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -925,13 +925,10 @@ static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz)
if (result)
return result;

- status = acpi_attach_data(tz->device->handle,
- acpi_bus_private_data_handler,
- tz->thermal_zone);
- if (ACPI_FAILURE(status)) {
- pr_err(PREFIX "Error attaching device data\n");
+ status = acpi_bus_attach_private_data(tz->device->handle,
+ tz->thermal_zone);
+ if (ACPI_FAILURE(status))
return -ENODEV;
- }

tz->tz_enabled = 1;

--
1.8.4.rc0.1.g8f6a3e5.dirty

2014-04-16 13:26:05

by Lan Tianyu

[permalink] [raw]
Subject: [PATCH 2/9] ACPICA: Export acpi_buffer_to_resource symbol

The acpi_buffer_to_resource is needed in i2c module
to convert aml buffer to struct acpi_resource

Signed-off-by: Lan Tianyu <[email protected]>
---
drivers/acpi/acpica/rscreate.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/acpi/acpica/rscreate.c b/drivers/acpi/acpica/rscreate.c
index 75d3690..5828649 100644
--- a/drivers/acpi/acpica/rscreate.c
+++ b/drivers/acpi/acpica/rscreate.c
@@ -112,6 +112,7 @@ acpi_buffer_to_resource(u8 *aml_buffer,

return (status);
}
+ACPI_EXPORT_SYMBOL(acpi_buffer_to_resource);

/*******************************************************************************
*
--
1.8.4.rc0.1.g8f6a3e5.dirty

2014-04-16 13:36:12

by Lan Tianyu

[permalink] [raw]
Subject: Re: [PATCH 0/9] I2C ACPI operation region handler support

On 04/16/2014 09:24 PM, Lan Tianyu wrote:
> ACPI 5.0 spec(5.5.2.4.5) defines GenericSerialBus(i2c, spi, uart) operation
> region. It allows ACPI aml code able to access such kind of devices to
> implement some ACPI standard method.
>
> On the Asus T100TA, Bios use GenericSerialBus operation region to access
> i2c device to get battery info. So battery function depends on the I2C
> operation region support. Here is the bug link.
> https://bugzilla.kernel.org/show_bug.cgi?id=69011

Completely fixing battery issue on the Asus T100TA also needs ACPI _DEP
support. The feature is under developing. Attach a temporary patch
for test.

>
> This patchset is to add I2C ACPI operation region handler support.
>
> [PATCH 1/9] ACPICA: Executer: Fix buffer allocation issue for
> [PATCH 2/9] ACPICA: Export acpi_buffer_to_resource symbol
> [PATCH 3/9] ACPI: Add acpi_bus_attach_private_data() to facilitate to
> [PATCH 4/9] ACPI/Thermal: Use acpi_bus_attach_private_data() to
> [PATCH 5/9] I2C: Add smbus quick read/write helper function
> [PATCH 6/9] I2C: Add smbus word/block process call helper function
> [PATCH 7/9] I2C/ACPI: Add i2c ACPI operation region support
> [PATCH 8/9] I2C/ACPI: Move ACPI related code to i2c-acpi.c
> [PATCH 9/9] I2C/ACPI: Add CONFIG_I2C_ACPI config
>


Attachments:
0001-ACPI-temporary-dep-solution-for-battery-support.patch (4.09 kB)

2014-04-16 16:35:37

by Adam Williamson

[permalink] [raw]
Subject: Re: [PATCH 0/9] I2C ACPI operation region handler support

On Wed, 2014-04-16 at 21:24 +0800, Lan Tianyu wrote:
> ACPI 5.0 spec(5.5.2.4.5) defines GenericSerialBus(i2c, spi, uart) operation
> region. It allows ACPI aml code able to access such kind of devices to
> implement some ACPI standard method.
>
> On the Asus T100TA, Bios use GenericSerialBus operation region to access
> i2c device to get battery info. So battery function depends on the I2C
> operation region support. Here is the bug link.
> https://bugzilla.kernel.org/show_bug.cgi?id=69011
>
> This patchset is to add I2C ACPI operation region handler support.
>
> [PATCH 1/9] ACPICA: Executer: Fix buffer allocation issue for
> [PATCH 2/9] ACPICA: Export acpi_buffer_to_resource symbol
> [PATCH 3/9] ACPI: Add acpi_bus_attach_private_data() to facilitate to
> [PATCH 4/9] ACPI/Thermal: Use acpi_bus_attach_private_data() to
> [PATCH 5/9] I2C: Add smbus quick read/write helper function
> [PATCH 6/9] I2C: Add smbus word/block process call helper function
> [PATCH 7/9] I2C/ACPI: Add i2c ACPI operation region support
> [PATCH 8/9] I2C/ACPI: Move ACPI related code to i2c-acpi.c
> [PATCH 9/9] I2C/ACPI: Add CONFIG_I2C_ACPI config

I've tested the version of these changes that's available as
https://bugzilla.kernel.org/attachment.cgi?id=131321 on my Venue 8 Pro
and found it gives apparently accurate battery stats (and doesn't seem
to make anything else explode). I'll try this form of the change out
today if I can.
--
Adam Williamson
Fedora QA Community Monkey
IRC: adamw | Twitter: AdamW_Fedora | XMPP: adamw AT happyassassin . net
http://www.happyassassin.net

2014-04-21 21:21:50

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [PATCH 1/9] ACPICA: Executer: Fix buffer allocation issue for generic_serial_bus region field accesses.

Hi,

On Wednesday, April 16, 2014 09:24:34 PM Lan Tianyu wrote:
> From: Lv Zheng <[email protected]>
>
> The size of the buffer allocated for generic_serial_bus region access
> is not correct. This patch introduces acpi_ex_get_serial_access_length()
> to be invoked to obtain correct data buffer length. Reported by
> Lan Tianyu, Fixed by Lv Zheng.
>
> Signed-off-by: Lv Zheng <[email protected]>
> Signed-off-by: Lan Tianyu <[email protected]>

I'm queueing up this patch as a fix for 3.15, but can you please resend the
whole series with a CC to linux-acpi?


> ---
> drivers/acpi/acpica/exfield.c | 104 +++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 97 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/acpi/acpica/exfield.c b/drivers/acpi/acpica/exfield.c
> index 68d9744..12878e1 100644
> --- a/drivers/acpi/acpica/exfield.c
> +++ b/drivers/acpi/acpica/exfield.c
> @@ -45,10 +45,71 @@
> #include "accommon.h"
> #include "acdispat.h"
> #include "acinterp.h"
> +#include "amlcode.h"
>
> #define _COMPONENT ACPI_EXECUTER
> ACPI_MODULE_NAME("exfield")
>
> +/* Local prototypes */
> +static u32
> +acpi_ex_get_serial_access_length(u32 accessor_type, u32 access_length);
> +
> +/*******************************************************************************
> + *
> + * FUNCTION: acpi_get_serial_access_bytes
> + *
> + * PARAMETERS: accessor_type - The type of the protocol indicated by region
> + * field access attributes
> + * access_length - The access length of the region field
> + *
> + * RETURN: Decoded access length
> + *
> + * DESCRIPTION: This routine returns the length of the generic_serial_bus
> + * protocol bytes
> + *
> + ******************************************************************************/
> +
> +static u32
> +acpi_ex_get_serial_access_length(u32 accessor_type, u32 access_length)
> +{
> + u32 length;
> +
> + switch (accessor_type) {
> + case AML_FIELD_ATTRIB_QUICK:
> +
> + length = 0;
> + break;
> +
> + case AML_FIELD_ATTRIB_SEND_RCV:
> + case AML_FIELD_ATTRIB_BYTE:
> +
> + length = 1;
> + break;
> +
> + case AML_FIELD_ATTRIB_WORD:
> + case AML_FIELD_ATTRIB_WORD_CALL:
> +
> + length = 2;
> + break;
> +
> + case AML_FIELD_ATTRIB_MULTIBYTE:
> + case AML_FIELD_ATTRIB_RAW_BYTES:
> + case AML_FIELD_ATTRIB_RAW_PROCESS:
> +
> + length = access_length;
> + break;
> +
> + case AML_FIELD_ATTRIB_BLOCK:
> + case AML_FIELD_ATTRIB_BLOCK_CALL:
> + default:
> +
> + length = ACPI_GSBUS_BUFFER_SIZE;
> + break;
> + }
> +
> + return (length);
> +}
> +
> /*******************************************************************************
> *
> * FUNCTION: acpi_ex_read_data_from_field
> @@ -63,8 +124,9 @@ ACPI_MODULE_NAME("exfield")
> * Buffer, depending on the size of the field.
> *
> ******************************************************************************/
> +
> acpi_status
> -acpi_ex_read_data_from_field(struct acpi_walk_state *walk_state,
> +acpi_ex_read_data_from_field(struct acpi_walk_state * walk_state,
> union acpi_operand_object *obj_desc,
> union acpi_operand_object **ret_buffer_desc)
> {
> @@ -73,6 +135,7 @@ acpi_ex_read_data_from_field(struct acpi_walk_state *walk_state,
> acpi_size length;
> void *buffer;
> u32 function;
> + u16 accessor_type;
>
> ACPI_FUNCTION_TRACE_PTR(ex_read_data_from_field, obj_desc);
>
> @@ -116,9 +179,22 @@ acpi_ex_read_data_from_field(struct acpi_walk_state *walk_state,
> ACPI_READ | (obj_desc->field.attribute << 16);
> } else if (obj_desc->field.region_obj->region.space_id ==
> ACPI_ADR_SPACE_GSBUS) {
> - length = ACPI_GSBUS_BUFFER_SIZE;
> - function =
> - ACPI_READ | (obj_desc->field.attribute << 16);
> + accessor_type = obj_desc->field.attribute;
> + length = acpi_ex_get_serial_access_length(accessor_type,
> + obj_desc->
> + field.
> + access_length);
> +
> + /*
> + * Add additional 2 bytes for modeled generic_serial_bus data buffer:
> + * typedef struct {
> + * BYTEStatus; // Byte 0 of the data buffer
> + * BYTELength; // Byte 1 of the data buffer
> + * BYTE[x-1]Data; // Bytes 2-x of the arbitrary length data buffer,
> + * }
> + */
> + length += 2;
> + function = ACPI_READ | (accessor_type << 16);
> } else { /* IPMI */
>
> length = ACPI_IPMI_BUFFER_SIZE;
> @@ -231,6 +307,7 @@ acpi_ex_write_data_to_field(union acpi_operand_object *source_desc,
> void *buffer;
> union acpi_operand_object *buffer_desc;
> u32 function;
> + u16 accessor_type;
>
> ACPI_FUNCTION_TRACE_PTR(ex_write_data_to_field, obj_desc);
>
> @@ -284,9 +361,22 @@ acpi_ex_write_data_to_field(union acpi_operand_object *source_desc,
> ACPI_WRITE | (obj_desc->field.attribute << 16);
> } else if (obj_desc->field.region_obj->region.space_id ==
> ACPI_ADR_SPACE_GSBUS) {
> - length = ACPI_GSBUS_BUFFER_SIZE;
> - function =
> - ACPI_WRITE | (obj_desc->field.attribute << 16);
> + accessor_type = obj_desc->field.attribute;
> + length = acpi_ex_get_serial_access_length(accessor_type,
> + obj_desc->
> + field.
> + access_length);
> +
> + /*
> + * Add additional 2 bytes for modeled generic_serial_bus data buffer:
> + * typedef struct {
> + * BYTEStatus; // Byte 0 of the data buffer
> + * BYTELength; // Byte 1 of the data buffer
> + * BYTE[x-1]Data; // Bytes 2-x of the arbitrary length data buffer,
> + * }
> + */
> + length += 2;
> + function = ACPI_WRITE | (accessor_type << 16);
> } else { /* IPMI */
>
> length = ACPI_IPMI_BUFFER_SIZE;
>

--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

2014-04-22 01:22:31

by Lan Tianyu

[permalink] [raw]
Subject: Re: [PATCH 1/9] ACPICA: Executer: Fix buffer allocation issue for generic_serial_bus region field accesses.

On 2014年04月22日 05:38, Rafael J. Wysocki wrote:
> Hi,
>
> On Wednesday, April 16, 2014 09:24:34 PM Lan Tianyu wrote:
>> From: Lv Zheng <[email protected]>
>>
>> The size of the buffer allocated for generic_serial_bus region access
>> is not correct. This patch introduces acpi_ex_get_serial_access_length()
>> to be invoked to obtain correct data buffer length. Reported by
>> Lan Tianyu, Fixed by Lv Zheng.
>>
>> Signed-off-by: Lv Zheng <[email protected]>
>> Signed-off-by: Lan Tianyu <[email protected]>
>
> I'm queueing up this patch as a fix for 3.15, but can you please resend the
> whole series with a CC to linux-acpi?
>

Ok. I will do that.

>
>> ---
>> drivers/acpi/acpica/exfield.c | 104 +++++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 97 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/acpi/acpica/exfield.c b/drivers/acpi/acpica/exfield.c
>> index 68d9744..12878e1 100644
>> --- a/drivers/acpi/acpica/exfield.c
>> +++ b/drivers/acpi/acpica/exfield.c
>> @@ -45,10 +45,71 @@
>> #include "accommon.h"
>> #include "acdispat.h"
>> #include "acinterp.h"
>> +#include "amlcode.h"
>>
>> #define _COMPONENT ACPI_EXECUTER
>> ACPI_MODULE_NAME("exfield")
>>
>> +/* Local prototypes */
>> +static u32
>> +acpi_ex_get_serial_access_length(u32 accessor_type, u32 access_length);
>> +
>> +/*******************************************************************************
>> + *
>> + * FUNCTION: acpi_get_serial_access_bytes
>> + *
>> + * PARAMETERS: accessor_type - The type of the protocol indicated by region
>> + * field access attributes
>> + * access_length - The access length of the region field
>> + *
>> + * RETURN: Decoded access length
>> + *
>> + * DESCRIPTION: This routine returns the length of the generic_serial_bus
>> + * protocol bytes
>> + *
>> + ******************************************************************************/
>> +
>> +static u32
>> +acpi_ex_get_serial_access_length(u32 accessor_type, u32 access_length)
>> +{
>> + u32 length;
>> +
>> + switch (accessor_type) {
>> + case AML_FIELD_ATTRIB_QUICK:
>> +
>> + length = 0;
>> + break;
>> +
>> + case AML_FIELD_ATTRIB_SEND_RCV:
>> + case AML_FIELD_ATTRIB_BYTE:
>> +
>> + length = 1;
>> + break;
>> +
>> + case AML_FIELD_ATTRIB_WORD:
>> + case AML_FIELD_ATTRIB_WORD_CALL:
>> +
>> + length = 2;
>> + break;
>> +
>> + case AML_FIELD_ATTRIB_MULTIBYTE:
>> + case AML_FIELD_ATTRIB_RAW_BYTES:
>> + case AML_FIELD_ATTRIB_RAW_PROCESS:
>> +
>> + length = access_length;
>> + break;
>> +
>> + case AML_FIELD_ATTRIB_BLOCK:
>> + case AML_FIELD_ATTRIB_BLOCK_CALL:
>> + default:
>> +
>> + length = ACPI_GSBUS_BUFFER_SIZE;
>> + break;
>> + }
>> +
>> + return (length);
>> +}
>> +
>> /*******************************************************************************
>> *
>> * FUNCTION: acpi_ex_read_data_from_field
>> @@ -63,8 +124,9 @@ ACPI_MODULE_NAME("exfield")
>> * Buffer, depending on the size of the field.
>> *
>> ******************************************************************************/
>> +
>> acpi_status
>> -acpi_ex_read_data_from_field(struct acpi_walk_state *walk_state,
>> +acpi_ex_read_data_from_field(struct acpi_walk_state * walk_state,
>> union acpi_operand_object *obj_desc,
>> union acpi_operand_object **ret_buffer_desc)
>> {
>> @@ -73,6 +135,7 @@ acpi_ex_read_data_from_field(struct acpi_walk_state *walk_state,
>> acpi_size length;
>> void *buffer;
>> u32 function;
>> + u16 accessor_type;
>>
>> ACPI_FUNCTION_TRACE_PTR(ex_read_data_from_field, obj_desc);
>>
>> @@ -116,9 +179,22 @@ acpi_ex_read_data_from_field(struct acpi_walk_state *walk_state,
>> ACPI_READ | (obj_desc->field.attribute << 16);
>> } else if (obj_desc->field.region_obj->region.space_id ==
>> ACPI_ADR_SPACE_GSBUS) {
>> - length = ACPI_GSBUS_BUFFER_SIZE;
>> - function =
>> - ACPI_READ | (obj_desc->field.attribute << 16);
>> + accessor_type = obj_desc->field.attribute;
>> + length = acpi_ex_get_serial_access_length(accessor_type,
>> + obj_desc->
>> + field.
>> + access_length);
>> +
>> + /*
>> + * Add additional 2 bytes for modeled generic_serial_bus data buffer:
>> + * typedef struct {
>> + * BYTEStatus; // Byte 0 of the data buffer
>> + * BYTELength; // Byte 1 of the data buffer
>> + * BYTE[x-1]Data; // Bytes 2-x of the arbitrary length data buffer,
>> + * }
>> + */
>> + length += 2;
>> + function = ACPI_READ | (accessor_type << 16);
>> } else { /* IPMI */
>>
>> length = ACPI_IPMI_BUFFER_SIZE;
>> @@ -231,6 +307,7 @@ acpi_ex_write_data_to_field(union acpi_operand_object *source_desc,
>> void *buffer;
>> union acpi_operand_object *buffer_desc;
>> u32 function;
>> + u16 accessor_type;
>>
>> ACPI_FUNCTION_TRACE_PTR(ex_write_data_to_field, obj_desc);
>>
>> @@ -284,9 +361,22 @@ acpi_ex_write_data_to_field(union acpi_operand_object *source_desc,
>> ACPI_WRITE | (obj_desc->field.attribute << 16);
>> } else if (obj_desc->field.region_obj->region.space_id ==
>> ACPI_ADR_SPACE_GSBUS) {
>> - length = ACPI_GSBUS_BUFFER_SIZE;
>> - function =
>> - ACPI_WRITE | (obj_desc->field.attribute << 16);
>> + accessor_type = obj_desc->field.attribute;
>> + length = acpi_ex_get_serial_access_length(accessor_type,
>> + obj_desc->
>> + field.
>> + access_length);
>> +
>> + /*
>> + * Add additional 2 bytes for modeled generic_serial_bus data buffer:
>> + * typedef struct {
>> + * BYTEStatus; // Byte 0 of the data buffer
>> + * BYTELength; // Byte 1 of the data buffer
>> + * BYTE[x-1]Data; // Bytes 2-x of the arbitrary length data buffer,
>> + * }
>> + */
>> + length += 2;
>> + function = ACPI_WRITE | (accessor_type << 16);
>> } else { /* IPMI */
>>
>> length = ACPI_IPMI_BUFFER_SIZE;
>>
>


--
Best regards
Tianyu Lan

2014-04-22 06:32:11

by Lan Tianyu

[permalink] [raw]
Subject: [Resend Patch 0/9] I2C ACPI operation region handler support

ACPI 5.0 spec(5.5.2.4.5) defines GenericSerialBus(i2c, spi, uart) operation
region. It allows ACPI aml code able to access such kind of devices to
implement some ACPI standard method.

On the Asus T100TA, Bios use GenericSerialBus operation region to access
i2c device to get battery info. So battery function depends on the I2C
operation region support. Here is the bug link.
https://bugzilla.kernel.org/show_bug.cgi?id=69011

This patchset is to add I2C ACPI operation region handler support.

[Resend Patch 1/9] ACPICA: Executer: Fix buffer allocation issue for
[Resend Patch 2/9] ACPICA: Export acpi_buffer_to_resource symbol
[Resend Patch 3/9] ACPI: Add acpi_bus_attach_private_data() to
[Resend Patch 4/9] ACPI/Thermal: Use acpi_bus_attach_private_data()
[Resend Patch 5/9] I2C: Add smbus quick read/write helper function
[Resend Patch 6/9] I2C: Add smbus word/block process call helper
[Resend Patch 7/9] I2C/ACPI: Add i2c ACPI operation region support
[Resend Patch 8/9] I2C/ACPI: Move ACPI related code to i2c-acpi.c
[Resend Patch 9/9] I2C/ACPI: Add CONFIG_I2C_ACPI config

drivers/acpi/acpica/exfield.c | 104 +++++++++++++++++++++++++++++++++++++++++++++----
drivers/acpi/acpica/rscreate.c | 1 +
drivers/acpi/bus.c | 18 ++++++++-
drivers/acpi/thermal.c | 9 ++---
drivers/i2c/Kconfig | 17 +++++++-
drivers/i2c/Makefile | 5 ++-
drivers/i2c/i2c-acpi.c | 371 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
drivers/i2c/i2c-core.c | 183 +++++++++++++++++++++++++++++++++++++++++--------------------------------------------
include/acpi/acpi_bus.h | 1 +
include/linux/acpi.h | 11 ++++++
include/linux/i2c.h | 19 +++++++++
11 files changed, 628 insertions(+), 111 deletions(-)

2014-04-22 06:32:27

by Lan Tianyu

[permalink] [raw]
Subject: [Resend Patch 1/9] ACPICA: Executer: Fix buffer allocation issue for generic_serial_bus region field accesses.

From: Lv Zheng <[email protected]>

The size of the buffer allocated for generic_serial_bus region access
is not correct. This patch introduces acpi_ex_get_serial_access_length()
to be invoked to obtain correct data buffer length. Reported by
Lan Tianyu, Fixed by Lv Zheng.

Signed-off-by: Lv Zheng <[email protected]>
Signed-off-by: Lan Tianyu <[email protected]>
---
drivers/acpi/acpica/exfield.c | 104 +++++++++++++++++++++++++++++++++++++++---
1 file changed, 97 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/acpica/exfield.c b/drivers/acpi/acpica/exfield.c
index 68d9744..12878e1 100644
--- a/drivers/acpi/acpica/exfield.c
+++ b/drivers/acpi/acpica/exfield.c
@@ -45,10 +45,71 @@
#include "accommon.h"
#include "acdispat.h"
#include "acinterp.h"
+#include "amlcode.h"

#define _COMPONENT ACPI_EXECUTER
ACPI_MODULE_NAME("exfield")

+/* Local prototypes */
+static u32
+acpi_ex_get_serial_access_length(u32 accessor_type, u32 access_length);
+
+/*******************************************************************************
+ *
+ * FUNCTION: acpi_get_serial_access_bytes
+ *
+ * PARAMETERS: accessor_type - The type of the protocol indicated by region
+ * field access attributes
+ * access_length - The access length of the region field
+ *
+ * RETURN: Decoded access length
+ *
+ * DESCRIPTION: This routine returns the length of the generic_serial_bus
+ * protocol bytes
+ *
+ ******************************************************************************/
+
+static u32
+acpi_ex_get_serial_access_length(u32 accessor_type, u32 access_length)
+{
+ u32 length;
+
+ switch (accessor_type) {
+ case AML_FIELD_ATTRIB_QUICK:
+
+ length = 0;
+ break;
+
+ case AML_FIELD_ATTRIB_SEND_RCV:
+ case AML_FIELD_ATTRIB_BYTE:
+
+ length = 1;
+ break;
+
+ case AML_FIELD_ATTRIB_WORD:
+ case AML_FIELD_ATTRIB_WORD_CALL:
+
+ length = 2;
+ break;
+
+ case AML_FIELD_ATTRIB_MULTIBYTE:
+ case AML_FIELD_ATTRIB_RAW_BYTES:
+ case AML_FIELD_ATTRIB_RAW_PROCESS:
+
+ length = access_length;
+ break;
+
+ case AML_FIELD_ATTRIB_BLOCK:
+ case AML_FIELD_ATTRIB_BLOCK_CALL:
+ default:
+
+ length = ACPI_GSBUS_BUFFER_SIZE;
+ break;
+ }
+
+ return (length);
+}
+
/*******************************************************************************
*
* FUNCTION: acpi_ex_read_data_from_field
@@ -63,8 +124,9 @@ ACPI_MODULE_NAME("exfield")
* Buffer, depending on the size of the field.
*
******************************************************************************/
+
acpi_status
-acpi_ex_read_data_from_field(struct acpi_walk_state *walk_state,
+acpi_ex_read_data_from_field(struct acpi_walk_state * walk_state,
union acpi_operand_object *obj_desc,
union acpi_operand_object **ret_buffer_desc)
{
@@ -73,6 +135,7 @@ acpi_ex_read_data_from_field(struct acpi_walk_state *walk_state,
acpi_size length;
void *buffer;
u32 function;
+ u16 accessor_type;

ACPI_FUNCTION_TRACE_PTR(ex_read_data_from_field, obj_desc);

@@ -116,9 +179,22 @@ acpi_ex_read_data_from_field(struct acpi_walk_state *walk_state,
ACPI_READ | (obj_desc->field.attribute << 16);
} else if (obj_desc->field.region_obj->region.space_id ==
ACPI_ADR_SPACE_GSBUS) {
- length = ACPI_GSBUS_BUFFER_SIZE;
- function =
- ACPI_READ | (obj_desc->field.attribute << 16);
+ accessor_type = obj_desc->field.attribute;
+ length = acpi_ex_get_serial_access_length(accessor_type,
+ obj_desc->
+ field.
+ access_length);
+
+ /*
+ * Add additional 2 bytes for modeled generic_serial_bus data buffer:
+ * typedef struct {
+ * BYTEStatus; // Byte 0 of the data buffer
+ * BYTELength; // Byte 1 of the data buffer
+ * BYTE[x-1]Data; // Bytes 2-x of the arbitrary length data buffer,
+ * }
+ */
+ length += 2;
+ function = ACPI_READ | (accessor_type << 16);
} else { /* IPMI */

length = ACPI_IPMI_BUFFER_SIZE;
@@ -231,6 +307,7 @@ acpi_ex_write_data_to_field(union acpi_operand_object *source_desc,
void *buffer;
union acpi_operand_object *buffer_desc;
u32 function;
+ u16 accessor_type;

ACPI_FUNCTION_TRACE_PTR(ex_write_data_to_field, obj_desc);

@@ -284,9 +361,22 @@ acpi_ex_write_data_to_field(union acpi_operand_object *source_desc,
ACPI_WRITE | (obj_desc->field.attribute << 16);
} else if (obj_desc->field.region_obj->region.space_id ==
ACPI_ADR_SPACE_GSBUS) {
- length = ACPI_GSBUS_BUFFER_SIZE;
- function =
- ACPI_WRITE | (obj_desc->field.attribute << 16);
+ accessor_type = obj_desc->field.attribute;
+ length = acpi_ex_get_serial_access_length(accessor_type,
+ obj_desc->
+ field.
+ access_length);
+
+ /*
+ * Add additional 2 bytes for modeled generic_serial_bus data buffer:
+ * typedef struct {
+ * BYTEStatus; // Byte 0 of the data buffer
+ * BYTELength; // Byte 1 of the data buffer
+ * BYTE[x-1]Data; // Bytes 2-x of the arbitrary length data buffer,
+ * }
+ */
+ length += 2;
+ function = ACPI_WRITE | (accessor_type << 16);
} else { /* IPMI */

length = ACPI_IPMI_BUFFER_SIZE;
--
1.8.4.rc0.1.g8f6a3e5.dirty

2014-04-22 06:32:32

by Lan Tianyu

[permalink] [raw]
Subject: [Resend Patch 2/9] ACPICA: Export acpi_buffer_to_resource symbol

The acpi_buffer_to_resource is needed in i2c module
to convert aml buffer to struct acpi_resource

Signed-off-by: Lan Tianyu <[email protected]>
---
drivers/acpi/acpica/rscreate.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/acpi/acpica/rscreate.c b/drivers/acpi/acpica/rscreate.c
index 75d3690..5828649 100644
--- a/drivers/acpi/acpica/rscreate.c
+++ b/drivers/acpi/acpica/rscreate.c
@@ -112,6 +112,7 @@ acpi_buffer_to_resource(u8 *aml_buffer,

return (status);
}
+ACPI_EXPORT_SYMBOL(acpi_buffer_to_resource);

/*******************************************************************************
*
--
1.8.4.rc0.1.g8f6a3e5.dirty

2014-04-22 06:32:53

by Lan Tianyu

[permalink] [raw]
Subject: [Resend Patch 8/9] I2C/ACPI: Move ACPI related code to i2c-acpi.c

Clean up ACPI related code in the i2c core.

Signed-off-by: Lan Tianyu <[email protected]>
---
drivers/i2c/i2c-acpi.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++
drivers/i2c/i2c-core.c | 95 --------------------------------------------------
include/linux/i2c.h | 3 ++
3 files changed, 92 insertions(+), 95 deletions(-)

diff --git a/drivers/i2c/i2c-acpi.c b/drivers/i2c/i2c-acpi.c
index 942abdf..a0ae867 100644
--- a/drivers/i2c/i2c-acpi.c
+++ b/drivers/i2c/i2c-acpi.c
@@ -37,6 +37,95 @@ struct gsb_buffer {
};
} __packed;

+static int acpi_i2c_add_resource(struct acpi_resource *ares, void *data)
+{
+ struct i2c_board_info *info = data;
+
+ if (ares->type == ACPI_RESOURCE_TYPE_SERIAL_BUS) {
+ struct acpi_resource_i2c_serialbus *sb;
+
+ sb = &ares->data.i2c_serial_bus;
+ if (sb->type == ACPI_RESOURCE_SERIAL_TYPE_I2C) {
+ info->addr = sb->slave_address;
+ if (sb->access_mode == ACPI_I2C_10BIT_MODE)
+ info->flags |= I2C_CLIENT_TEN;
+ }
+ } else if (info->irq < 0) {
+ struct resource r;
+
+ if (acpi_dev_resource_interrupt(ares, 0, &r))
+ info->irq = r.start;
+ }
+
+ /* Tell the ACPI core to skip this resource */
+ return 1;
+}
+
+static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
+ void *data, void **return_value)
+{
+ struct i2c_adapter *adapter = data;
+ struct list_head resource_list;
+ struct i2c_board_info info;
+ struct acpi_device *adev;
+ int ret;
+
+ if (acpi_bus_get_device(handle, &adev))
+ return AE_OK;
+ if (acpi_bus_get_status(adev) || !adev->status.present)
+ return AE_OK;
+
+ memset(&info, 0, sizeof(info));
+ info.acpi_node.companion = adev;
+ info.irq = -1;
+
+ INIT_LIST_HEAD(&resource_list);
+ ret = acpi_dev_get_resources(adev, &resource_list,
+ acpi_i2c_add_resource, &info);
+ acpi_dev_free_resource_list(&resource_list);
+
+ if (ret < 0 || !info.addr)
+ return AE_OK;
+
+ adev->power.flags.ignore_parent = true;
+ strlcpy(info.type, dev_name(&adev->dev), sizeof(info.type));
+ if (!i2c_new_device(adapter, &info)) {
+ adev->power.flags.ignore_parent = false;
+ dev_err(&adapter->dev,
+ "failed to add I2C device %s from ACPI\n",
+ dev_name(&adev->dev));
+ }
+
+ return AE_OK;
+}
+
+/**
+ * acpi_i2c_register_devices - enumerate I2C slave devices behind adapter
+ * @adap: pointer to adapter
+ *
+ * Enumerate all I2C slave devices behind this adapter by walking the ACPI
+ * namespace. When a device is found it will be added to the Linux device
+ * model and bound to the corresponding ACPI handle.
+ */
+void acpi_i2c_register_devices(struct i2c_adapter *adap)
+{
+ acpi_handle handle;
+ acpi_status status;
+
+ if (!adap->dev.parent)
+ return;
+
+ handle = ACPI_HANDLE(adap->dev.parent);
+ if (!handle)
+ return;
+
+ status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
+ acpi_i2c_add_device, NULL,
+ adap, NULL);
+ if (ACPI_FAILURE(status))
+ dev_warn(&adap->dev, "failed to enumerate I2C slaves\n");
+}
+
static int acpi_gsb_i2c_read_bytes(struct i2c_client *client,
u8 cmd, u8 *data, u8 data_len)
{
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index bb9dc56..9ca50c8 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1092,101 +1092,6 @@ EXPORT_SYMBOL(of_find_i2c_adapter_by_node);
static void of_i2c_register_devices(struct i2c_adapter *adap) { }
#endif /* CONFIG_OF */

-/* ACPI support code */
-
-#if IS_ENABLED(CONFIG_ACPI)
-static int acpi_i2c_add_resource(struct acpi_resource *ares, void *data)
-{
- struct i2c_board_info *info = data;
-
- if (ares->type == ACPI_RESOURCE_TYPE_SERIAL_BUS) {
- struct acpi_resource_i2c_serialbus *sb;
-
- sb = &ares->data.i2c_serial_bus;
- if (sb->type == ACPI_RESOURCE_SERIAL_TYPE_I2C) {
- info->addr = sb->slave_address;
- if (sb->access_mode == ACPI_I2C_10BIT_MODE)
- info->flags |= I2C_CLIENT_TEN;
- }
- } else if (info->irq < 0) {
- struct resource r;
-
- if (acpi_dev_resource_interrupt(ares, 0, &r))
- info->irq = r.start;
- }
-
- /* Tell the ACPI core to skip this resource */
- return 1;
-}
-
-static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
- void *data, void **return_value)
-{
- struct i2c_adapter *adapter = data;
- struct list_head resource_list;
- struct i2c_board_info info;
- struct acpi_device *adev;
- int ret;
-
- if (acpi_bus_get_device(handle, &adev))
- return AE_OK;
- if (acpi_bus_get_status(adev) || !adev->status.present)
- return AE_OK;
-
- memset(&info, 0, sizeof(info));
- info.acpi_node.companion = adev;
- info.irq = -1;
-
- INIT_LIST_HEAD(&resource_list);
- ret = acpi_dev_get_resources(adev, &resource_list,
- acpi_i2c_add_resource, &info);
- acpi_dev_free_resource_list(&resource_list);
-
- if (ret < 0 || !info.addr)
- return AE_OK;
-
- adev->power.flags.ignore_parent = true;
- strlcpy(info.type, dev_name(&adev->dev), sizeof(info.type));
- if (!i2c_new_device(adapter, &info)) {
- adev->power.flags.ignore_parent = false;
- dev_err(&adapter->dev,
- "failed to add I2C device %s from ACPI\n",
- dev_name(&adev->dev));
- }
-
- return AE_OK;
-}
-
-/**
- * acpi_i2c_register_devices - enumerate I2C slave devices behind adapter
- * @adap: pointer to adapter
- *
- * Enumerate all I2C slave devices behind this adapter by walking the ACPI
- * namespace. When a device is found it will be added to the Linux device
- * model and bound to the corresponding ACPI handle.
- */
-static void acpi_i2c_register_devices(struct i2c_adapter *adap)
-{
- acpi_handle handle;
- acpi_status status;
-
- if (!adap->dev.parent)
- return;
-
- handle = ACPI_HANDLE(adap->dev.parent);
- if (!handle)
- return;
-
- status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
- acpi_i2c_add_device, NULL,
- adap, NULL);
- if (ACPI_FAILURE(status))
- dev_warn(&adap->dev, "failed to enumerate I2C slaves\n");
-}
-#else
-static inline void acpi_i2c_register_devices(struct i2c_adapter *adap) {}
-#endif /* CONFIG_ACPI */
-
static int i2c_do_add_adapter(struct i2c_driver *driver,
struct i2c_adapter *adap)
{
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 681e689..fc1ef42 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -586,7 +586,10 @@ static inline struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node
#ifdef CONFIG_ACPI
int acpi_i2c_install_space_handler(struct i2c_adapter *adapter);
void acpi_i2c_remove_space_handler(struct i2c_adapter *adapter);
+void acpi_i2c_register_devices(struct i2c_adapter *adap);
#else
+static inline void acpi_i2c_register_devices(struct i2c_adapter *adap) {};
+
static inline void acpi_i2c_remove_space_handler(struct i2c_adapter *adapter)
{ }
static inline int acpi_i2c_install_space_handler(struct i2c_adapter *adapter)
--
1.8.4.rc0.1.g8f6a3e5.dirty

2014-04-22 06:32:57

by Lan Tianyu

[permalink] [raw]
Subject: [Resend Patch 9/9] I2C/ACPI: Add CONFIG_I2C_ACPI config

This patch is to add CONFIG_I2C_ACPI. Current there is a race between
removing I2C ACPI operation region and ACPI AML code accessing.
So make i2c core built-in if CONFIG_I2C_ACPI is set.

Signed-off-by: Lan Tianyu <[email protected]>
---
drivers/i2c/Kconfig | 17 ++++++++++++++++-
drivers/i2c/Makefile | 2 +-
include/linux/i2c.h | 2 +-
3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
index 7b7ea32..c670d49 100644
--- a/drivers/i2c/Kconfig
+++ b/drivers/i2c/Kconfig
@@ -2,7 +2,9 @@
# I2C subsystem configuration
#

-menuconfig I2C
+menu "I2C support"
+
+config I2C
tristate "I2C support"
select RT_MUTEXES
---help---
@@ -21,6 +23,17 @@ menuconfig I2C
This I2C support can also be built as a module. If so, the module
will be called i2c-core.

+config I2C_ACPI
+ bool "I2C ACPI support"
+ select I2C
+ depends on ACPI
+ default y
+ help
+ Say Y here if you want to enable I2C ACPI function. ACPI table
+ provides I2C slave devices' information to enumerate these devices.
+ This option also allows ACPI AML code to access I2C slave devices
+ via I2C ACPI operation region to fulfill ACPI method.
+
if I2C

config I2C_BOARDINFO
@@ -124,3 +137,5 @@ config I2C_DEBUG_BUS
on.

endif # I2C
+
+endmenu
diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
index 80db307..37464ee 100644
--- a/drivers/i2c/Makefile
+++ b/drivers/i2c/Makefile
@@ -3,7 +3,7 @@
#

i2ccore-y := i2c-core.o
-i2ccore-$(CONFIG_ACPI) += i2c-acpi.o
+i2ccore-$(CONFIG_I2C_ACPI) += i2c-acpi.o

obj-$(CONFIG_I2C_BOARDINFO) += i2c-boardinfo.o
obj-$(CONFIG_I2C) += i2ccore.o
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index fc1ef42..d0ece9f 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -583,7 +583,7 @@ static inline struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node
}
#endif /* CONFIG_OF */

-#ifdef CONFIG_ACPI
+#ifdef CONFIG_I2C_ACPI
int acpi_i2c_install_space_handler(struct i2c_adapter *adapter);
void acpi_i2c_remove_space_handler(struct i2c_adapter *adapter);
void acpi_i2c_register_devices(struct i2c_adapter *adap);
--
1.8.4.rc0.1.g8f6a3e5.dirty

2014-04-22 06:32:48

by Lan Tianyu

[permalink] [raw]
Subject: [Resend Patch 4/9] ACPI/Thermal: Use acpi_bus_attach_private_data() to attach private data

Use acpi_bus_attach_private_data() to attach private data
instead of acpi_attach_data().

Signed-off-by: Lan Tianyu <[email protected]>
---
drivers/acpi/thermal.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
index c1e31a4..7ab9392 100644
--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -925,13 +925,10 @@ static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz)
if (result)
return result;

- status = acpi_attach_data(tz->device->handle,
- acpi_bus_private_data_handler,
- tz->thermal_zone);
- if (ACPI_FAILURE(status)) {
- pr_err(PREFIX "Error attaching device data\n");
+ status = acpi_bus_attach_private_data(tz->device->handle,
+ tz->thermal_zone);
+ if (ACPI_FAILURE(status))
return -ENODEV;
- }

tz->tz_enabled = 1;

--
1.8.4.rc0.1.g8f6a3e5.dirty

2014-04-22 06:33:34

by Lan Tianyu

[permalink] [raw]
Subject: [Resend Patch 7/9] I2C/ACPI: Add i2c ACPI operation region support

ACPI 5.0 spec(5.5.2.4.5) defines GenericSerialBus(i2c, spi, uart) operation region.
It allows ACPI aml code able to access such kind of devices to implement
some ACPI standard method.

ACPI Spec defines some access attribute to associate with i2c protocol.
AttribQuick Read/Write Quick Protocol
AttribSendReceive Send/Receive Byte Protocol
AttribByte Read/Write Byte Protocol
AttribWord Read/Write Word Protocol
AttribBlock Read/Write Block Protocol
AttribBytes Read/Write N-Bytes Protocol
AttribProcessCall Process Call Protocol
AttribBlockProcessCall Write Block-Read Block Process Call Protocol
AttribRawBytes Raw Read/Write N-BytesProtocol
AttribRawProcessBytes Raw Process Call Protocol

On the Asus T100TA, Bios use GenericSerialBus operation region to access
i2c device to get battery info.

Sample code From Asus T100TA

Scope (_SB.I2C1)
{
Name (UMPC, ResourceTemplate ()
{
I2cSerialBus (0x0066, ControllerInitiated, 0x00061A80,
AddressingMode7Bit, "\\_SB.I2C1",
0x00, ResourceConsumer, ,
)
})

...

OperationRegion (DVUM, GenericSerialBus, Zero, 0x0100)
Field (DVUM, BufferAcc, NoLock, Preserve)
{
Connection (UMPC),
Offset (0x81),
AccessAs (BufferAcc, AttribBytes (0x3E)),
FGC0, 8
}
...
}

Device (BATC)
{
Name (_HID, EisaId ("PNP0C0A")) // _HID: Hardware ID
Name (_UID, One) // _UID: Unique ID
...

Method (_BST, 0, NotSerialized) // _BST: Battery Status
{
If (LEqual (AVBL, One))
{
Store (FGC0, BFFG)
If (LNotEqual (STAT, One))
{
ShiftRight (CHST, 0x04, Local0)
And (Local0, 0x03, Local0)
If (LOr (LEqual (Local0, One), LEqual (Local0, 0x02)))
{
Store (0x02, Local1)
}
...

}

The i2c operation region is defined under I2C1 scope. _BST method under
battery device BATC read battery status from the field "FCG0". The request
would be sent to i2c operation region handler.

This patch is to add i2c ACPI operation region support. Due to there are
only "Byte" and "Bytes" protocol access on the Asus T100TA, other protocols
have not been tested.

About RawBytes and RawProcessBytes protocol, they needs specific drivers to interpret
reference data from AML code according ACPI 5.0 SPEC(5.5.2.4.5.3.9 and 5.5.2.4.5.3.10).
So far, not found such case and will add when find real case.

Signed-off-by: Lan Tianyu <[email protected]>
---
drivers/i2c/Makefile | 5 +-
drivers/i2c/i2c-acpi.c | 282 +++++++++++++++++++++++++++++++++++++++++++++++++
drivers/i2c/i2c-core.c | 2 +
include/linux/acpi.h | 11 ++
include/linux/i2c.h | 10 ++
5 files changed, 309 insertions(+), 1 deletion(-)
create mode 100644 drivers/i2c/i2c-acpi.c

diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
index 1722f50..80db307 100644
--- a/drivers/i2c/Makefile
+++ b/drivers/i2c/Makefile
@@ -2,8 +2,11 @@
# Makefile for the i2c core.
#

+i2ccore-y := i2c-core.o
+i2ccore-$(CONFIG_ACPI) += i2c-acpi.o
+
obj-$(CONFIG_I2C_BOARDINFO) += i2c-boardinfo.o
-obj-$(CONFIG_I2C) += i2c-core.o
+obj-$(CONFIG_I2C) += i2ccore.o
obj-$(CONFIG_I2C_SMBUS) += i2c-smbus.o
obj-$(CONFIG_I2C_CHARDEV) += i2c-dev.o
obj-$(CONFIG_I2C_MUX) += i2c-mux.o
diff --git a/drivers/i2c/i2c-acpi.c b/drivers/i2c/i2c-acpi.c
new file mode 100644
index 0000000..942abdf
--- /dev/null
+++ b/drivers/i2c/i2c-acpi.c
@@ -0,0 +1,282 @@
+/*
+ * I2C ACPI code
+ *
+ * Copyright (C) 2014 Intel Corp
+ *
+ * Author: Lan Tianyu <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * 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.
+ */
+#define pr_fmt(fmt) "I2C/ACPI : " fmt
+
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/acpi.h>
+
+struct acpi_i2c_handler_data {
+ struct acpi_connection_info info;
+ struct i2c_adapter *adapter;
+} __packed;
+
+struct gsb_buffer {
+ u8 status;
+ u8 len;
+ union {
+ u16 wdata;
+ u8 bdata;
+ u8 data[0];
+ };
+} __packed;
+
+static int acpi_gsb_i2c_read_bytes(struct i2c_client *client,
+ u8 cmd, u8 *data, u8 data_len)
+{
+
+ struct i2c_msg msgs[2];
+ int ret;
+ u8 *buffer;
+
+ buffer = kzalloc(data_len, GFP_KERNEL);
+ if (!buffer)
+ return AE_NO_MEMORY;
+
+ msgs[0].addr = client->addr;
+ msgs[0].flags = client->flags;
+ msgs[0].len = 1;
+ msgs[0].buf = &cmd;
+
+ msgs[1].addr = client->addr;
+ msgs[1].flags = client->flags | I2C_M_RD;
+ msgs[1].len = data_len;
+ msgs[1].buf = buffer;
+
+ ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
+ if (ret < 0)
+ pr_err("i2c read failed\n");
+ else
+ memcpy(data, buffer, data_len);
+
+ kfree(buffer);
+ return ret;
+}
+
+static int acpi_gsb_i2c_write_bytes(struct i2c_client *client,
+ u8 cmd, u8 *data, u8 data_len)
+{
+
+ struct i2c_msg msgs[1];
+ u8 *buffer;
+ int ret = AE_OK;
+
+ buffer = kzalloc(data_len + 1, GFP_KERNEL);
+ if (!buffer)
+ return AE_NO_MEMORY;
+
+ buffer[0] = cmd;
+ memcpy(buffer + 1, data, data_len);
+
+ msgs[0].addr = client->addr;
+ msgs[0].flags = client->flags;
+ msgs[0].len = data_len + 1;
+ msgs[0].buf = buffer;
+
+ ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
+ if (ret < 0)
+ pr_err("i2c write failed\n");
+
+ kfree(buffer);
+ return ret;
+}
+
+static acpi_status
+acpi_i2c_space_handler(u32 function, acpi_physical_address command,
+ u32 bits, u64 *value64,
+ void *handler_context, void *region_context)
+{
+ struct gsb_buffer *gsb = (struct gsb_buffer *)value64;
+ struct acpi_i2c_handler_data *data = handler_context;
+ struct acpi_connection_info *info = &data->info;
+ struct acpi_resource_i2c_serialbus *sb;
+ struct i2c_adapter *adapter = data->adapter;
+ struct i2c_client client;
+ struct acpi_resource *ares;
+ u32 accessor_type = function >> 16;
+ u8 action = function & ACPI_IO_MASK;
+ int status;
+
+ acpi_buffer_to_resource(info->connection, info->length, &ares);
+ if (!value64 || ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
+ return AE_BAD_PARAMETER;
+
+ sb = &ares->data.i2c_serial_bus;
+ if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_I2C)
+ return AE_BAD_PARAMETER;
+
+ client.adapter = adapter;
+ client.addr = sb->slave_address;
+ client.flags = 0;
+
+ if (sb->access_mode == ACPI_I2C_10BIT_MODE)
+ client.flags |= I2C_CLIENT_TEN;
+
+ switch (accessor_type) {
+ case ACPI_GSB_ACCESS_ATTRIB_QUICK:
+ if (action == ACPI_READ)
+ status = i2c_smbus_quick_read(&client);
+ else
+ status = i2c_smbus_quick_write(&client);
+ break;
+
+ case ACPI_GSB_ACCESS_ATTRIB_SEND_RCV:
+ if (action == ACPI_READ) {
+ status = i2c_smbus_read_byte(&client);
+ if (status >= 0) {
+ gsb->bdata = status;
+ status = 0;
+ }
+ } else {
+ status = i2c_smbus_write_byte(&client, gsb->bdata);
+ }
+ break;
+
+ case ACPI_GSB_ACCESS_ATTRIB_BYTE:
+ if (action == ACPI_READ) {
+ status = i2c_smbus_read_byte_data(&client, command);
+ if (status >= 0) {
+ gsb->bdata = status;
+ status = 0;
+ }
+ } else {
+ status = i2c_smbus_write_byte_data(&client, command,
+ gsb->bdata);
+ }
+ break;
+
+ case ACPI_GSB_ACCESS_ATTRIB_WORD:
+ if (action == ACPI_READ) {
+ status = i2c_smbus_read_word_data(&client, command);
+ if (status >= 0) {
+ gsb->wdata = status;
+ status = 0;
+ }
+ } else {
+ status = i2c_smbus_write_word_data(&client, command,
+ gsb->wdata);
+ }
+ break;
+
+ case ACPI_GSB_ACCESS_ATTRIB_BLOCK:
+ if (action == ACPI_READ) {
+ status = i2c_smbus_read_block_data(&client, command,
+ gsb->data);
+ if (status >= 0) {
+ gsb->len = status;
+ status = 0;
+ }
+ } else {
+ status = i2c_smbus_write_block_data(&client, command,
+ gsb->len, gsb->data);
+ }
+ break;
+
+ case ACPI_GSB_ACCESS_ATTRIB_MULTIBYTE:
+ if (action == ACPI_READ) {
+ status = acpi_gsb_i2c_read_bytes(&client, command,
+ gsb->data, info->access_length);
+ if (status > 0)
+ status = 0;
+ } else {
+ status = acpi_gsb_i2c_write_bytes(&client, command,
+ gsb->data, info->access_length);
+ }
+ break;
+
+ case ACPI_GSB_ACCESS_ATTRIB_WORD_CALL:
+ status = i2c_smbus_word_proc_call(&client, command, gsb->wdata);
+ if (status >= 0) {
+ gsb->wdata = status;
+ status = 0;
+ }
+ break;
+
+ case ACPI_GSB_ACCESS_ATTRIB_BLOCK_CALL:
+ status = i2c_smbus_block_proc_call(&client, command, gsb->len,
+ gsb->data);
+ if (status > 0) {
+ gsb->len = status;
+ status = 0;
+ }
+ break;
+
+ default:
+ pr_info("protocl(0x%02x) is not supported.\n", accessor_type);
+ return AE_BAD_PARAMETER;
+ }
+
+ gsb->status = status;
+ return AE_OK;
+}
+
+
+int acpi_i2c_install_space_handler(struct i2c_adapter *adapter)
+{
+ acpi_handle handle = ACPI_HANDLE(adapter->dev.parent);
+ struct acpi_i2c_handler_data *data;
+ acpi_status status;
+
+ if (!handle)
+ return -ENODEV;
+
+ data = kzalloc(sizeof(struct acpi_i2c_handler_data),
+ GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ data->adapter = adapter;
+ status = acpi_bus_attach_private_data(handle, (void *)data);
+ if (ACPI_FAILURE(status)) {
+ kfree(data);
+ return -ENOMEM;
+ }
+
+ status = acpi_install_address_space_handler(handle,
+ ACPI_ADR_SPACE_GSBUS,
+ &acpi_i2c_space_handler,
+ NULL,
+ data);
+ if (ACPI_FAILURE(status)) {
+ pr_err("Error installing i2c space handler\n");
+ acpi_bus_attach_private_data(handle, NULL);
+ kfree(data);
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
+void acpi_i2c_remove_space_handler(struct i2c_adapter *adapter)
+{
+ acpi_handle handle = ACPI_HANDLE(adapter->dev.parent);
+ struct acpi_i2c_handler_data *data;
+ acpi_status status;
+
+ if (!handle)
+ return;
+
+ acpi_remove_address_space_handler(handle,
+ ACPI_ADR_SPACE_GSBUS,
+ &acpi_i2c_space_handler);
+
+ status = acpi_bus_get_private_data(handle, (void **)&data);
+ if (ACPI_SUCCESS(status))
+ kfree(data);
+}
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 638befd..bb9dc56 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1293,6 +1293,7 @@ exit_recovery:
/* create pre-declared device nodes */
of_i2c_register_devices(adap);
acpi_i2c_register_devices(adap);
+ acpi_i2c_install_space_handler(adap);

if (adap->nr < __i2c_first_dynamic_bus_num)
i2c_scan_static_board_info(adap);
@@ -1466,6 +1467,7 @@ void i2c_del_adapter(struct i2c_adapter *adap)
return;
}

+ acpi_i2c_remove_space_handler(adap);
/* Tell drivers about this removal */
mutex_lock(&core_lock);
bus_for_each_drv(&i2c_bus_type, NULL, adap,
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 7a8f2cd..ea53b9b 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -361,6 +361,17 @@ extern bool osc_sb_apei_support_acked;
#define OSC_PCI_EXPRESS_CAPABILITY_CONTROL 0x00000010
#define OSC_PCI_CONTROL_MASKS 0x0000001f

+#define ACPI_GSB_ACCESS_ATTRIB_QUICK 0x00000002
+#define ACPI_GSB_ACCESS_ATTRIB_SEND_RCV 0x00000004
+#define ACPI_GSB_ACCESS_ATTRIB_BYTE 0x00000006
+#define ACPI_GSB_ACCESS_ATTRIB_WORD 0x00000008
+#define ACPI_GSB_ACCESS_ATTRIB_BLOCK 0x0000000A
+#define ACPI_GSB_ACCESS_ATTRIB_MULTIBYTE 0x0000000B
+#define ACPI_GSB_ACCESS_ATTRIB_WORD_CALL 0x0000000C
+#define ACPI_GSB_ACCESS_ATTRIB_BLOCK_CALL 0x0000000D
+#define ACPI_GSB_ACCESS_ATTRIB_RAW_BYTES 0x0000000E
+#define ACPI_GSB_ACCESS_ATTRIB_RAW_PROCESS 0x0000000F
+
extern acpi_status acpi_pci_osc_control_set(acpi_handle handle,
u32 *mask, u32 req);

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 724440a..681e689 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -583,4 +583,14 @@ static inline struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node
}
#endif /* CONFIG_OF */

+#ifdef CONFIG_ACPI
+int acpi_i2c_install_space_handler(struct i2c_adapter *adapter);
+void acpi_i2c_remove_space_handler(struct i2c_adapter *adapter);
+#else
+static inline void acpi_i2c_remove_space_handler(struct i2c_adapter *adapter)
+{ }
+static inline int acpi_i2c_install_space_handler(struct i2c_adapter *adapter)
+{ return 0; }
+#endif
+
#endif /* _LINUX_I2C_H */
--
1.8.4.rc0.1.g8f6a3e5.dirty

2014-04-22 06:34:21

by Lan Tianyu

[permalink] [raw]
Subject: [Resend Patch 6/9] I2C: Add smbus word/block process call helper function

Add i2c_smbus_word/block_proc_call() helper function. These will be used
in the implementation of i2c ACPI address space handler.

Signed-off-by: Lan Tianyu <[email protected]>
---
drivers/i2c/i2c-core.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/i2c.h | 4 ++++
2 files changed, 60 insertions(+)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 3bf0048..638befd 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -2306,6 +2306,30 @@ s32 i2c_smbus_write_word_data(const struct i2c_client *client, u8 command,
EXPORT_SYMBOL(i2c_smbus_write_word_data);

/**
+ * i2c_smbus_word_proc_call - SMBus "word proc call" protocol
+ * @client: Handle to slave device
+ * @command: Byte interpreted by slave
+ * @value: 16-bit "word" being written
+ *
+ * This executes the SMBus "word proc all" protocol, returning negative errno
+ * else a 16-bit unsigned "word" received from the device.
+ */
+s32 i2c_smbus_word_proc_call(const struct i2c_client *client, u8 command,
+ u16 value)
+{
+ union i2c_smbus_data data;
+ int status;
+
+ data.word = value;
+ status = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
+ I2C_SMBUS_READ, command,
+ I2C_SMBUS_PROC_CALL, &data);
+
+ return (status < 0) ? status : data.word;
+}
+EXPORT_SYMBOL(i2c_smbus_word_proc_call);
+
+/**
* i2c_smbus_read_block_data - SMBus "block read" protocol
* @client: Handle to slave device
* @command: Byte interpreted by slave
@@ -2362,6 +2386,38 @@ s32 i2c_smbus_write_block_data(const struct i2c_client *client, u8 command,
}
EXPORT_SYMBOL(i2c_smbus_write_block_data);

+/**
+ * i2c_smbus_block_proc_call - SMBus "block write" protocol
+ * @client: Handle to slave device
+ * @command: Byte interpreted by slave
+ * @length: Size of data block; SMBus allows at most 32 bytes
+ * @values: Byte array which will be written.
+ *
+ * This executes the SMBus "block proc call" protocol, returning negative errno
+ * else the number of read bytes.
+ */
+s32 i2c_smbus_block_proc_call(const struct i2c_client *client, u8 command,
+ u8 length, u8 *values)
+{
+ union i2c_smbus_data data;
+ int status;
+
+ if (length > I2C_SMBUS_BLOCK_MAX)
+ length = I2C_SMBUS_BLOCK_MAX;
+ data.block[0] = length;
+ memcpy(&data.block[1], values, length);
+ status = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
+ I2C_SMBUS_READ, command,
+ I2C_SMBUS_BLOCK_PROC_CALL, &data);
+
+ if (status < 0)
+ return status;
+
+ memcpy(values, &data.block[1], data.block[0]);
+ return data.block[0];
+}
+EXPORT_SYMBOL(i2c_smbus_block_proc_call);
+
/* Returns the number of read bytes */
s32 i2c_smbus_read_i2c_block_data(const struct i2c_client *client, u8 command,
u8 length, u8 *values)
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 3e6ea90..724440a 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -94,6 +94,10 @@ extern s32 i2c_smbus_read_word_data(const struct i2c_client *client,
u8 command);
extern s32 i2c_smbus_write_word_data(const struct i2c_client *client,
u8 command, u16 value);
+extern s32 i2c_smbus_word_proc_call(const struct i2c_client *client,
+ u8 command, u16 value);
+extern s32 i2c_smbus_block_proc_call(const struct i2c_client *client,
+ u8 command, u8 length, u8 *values);

static inline s32
i2c_smbus_read_word_swapped(const struct i2c_client *client, u8 command)
--
1.8.4.rc0.1.g8f6a3e5.dirty

2014-04-22 06:32:42

by Lan Tianyu

[permalink] [raw]
Subject: [Resend Patch 5/9] I2C: Add smbus quick read/write helper function

Add i2c_smbus_quick_write/read() helper function. These will be used
in the implementation of i2c ACPI address space handler.

Signed-off-by: Lan Tianyu <[email protected]>
---
drivers/i2c/i2c-core.c | 30 ++++++++++++++++++++++++++++++
include/linux/i2c.h | 2 ++
2 files changed, 32 insertions(+)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 7c7f4b8..3bf0048 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -2162,6 +2162,36 @@ static int i2c_smbus_check_pec(u8 cpec, struct i2c_msg *msg)
}

/**
+ * i2c_smbus_quick_write - SMBus "quick write" protocol
+ * @client: Handle to slave device
+ *
+ * This executes the SMBus "quick write" protocol, returning negative errno
+ * else zero on success.
+ */
+s32 i2c_smbus_quick_write(const struct i2c_client *client)
+{
+ return i2c_smbus_xfer(client->adapter, client->addr, client->flags,
+ I2C_SMBUS_WRITE, 0,
+ I2C_SMBUS_QUICK, NULL);
+}
+EXPORT_SYMBOL_GPL(i2c_smbus_quick_write);
+
+/**
+ * i2c_smbus_quick_read - SMBus "quick read" protocol
+ * @client: Handle to slave device
+ *
+ * This executes the SMBus "quick read" protocol, returning negative errno
+ * else zero on success.
+ */
+s32 i2c_smbus_quick_read(const struct i2c_client *client)
+{
+ return i2c_smbus_xfer(client->adapter, client->addr, client->flags,
+ I2C_SMBUS_READ, 0,
+ I2C_SMBUS_QUICK, NULL);
+}
+EXPORT_SYMBOL_GPL(i2c_smbus_quick_read);
+
+/**
* i2c_smbus_read_byte - SMBus "receive byte" protocol
* @client: Handle to slave device
*
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index b556e0a..3e6ea90 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -82,6 +82,8 @@ extern s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
/* Now follow the 'nice' access routines. These also document the calling
conventions of i2c_smbus_xfer. */

+extern s32 i2c_smbus_quick_write(const struct i2c_client *client);
+extern s32 i2c_smbus_quick_read(const struct i2c_client *client);
extern s32 i2c_smbus_read_byte(const struct i2c_client *client);
extern s32 i2c_smbus_write_byte(const struct i2c_client *client, u8 value);
extern s32 i2c_smbus_read_byte_data(const struct i2c_client *client,
--
1.8.4.rc0.1.g8f6a3e5.dirty

2014-04-22 06:35:07

by Lan Tianyu

[permalink] [raw]
Subject: [Resend Patch 3/9] ACPI: Add acpi_bus_attach_private_data() to facilitate to attach data to ACPI handle

There is already acpi_bus_get_private_data() to get ACPI handle data
which is associated with acpi_bus_private_data_handler(). This patch
is to add acpi_bus_attach_private_data() to make a pair and facilitate
to attach and get data to/from ACPI handle.

Signed-off-by: Lan Tianyu <[email protected]>
---
drivers/acpi/bus.c | 18 +++++++++++++++++-
include/acpi/acpi_bus.h | 1 +
2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index e7e5844..4ed8d48 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -132,6 +132,22 @@ void acpi_bus_private_data_handler(acpi_handle handle,
}
EXPORT_SYMBOL(acpi_bus_private_data_handler);

+int acpi_bus_attach_private_data(acpi_handle handle, void *data)
+{
+ acpi_status status;
+
+ status = acpi_attach_data(handle,
+ acpi_bus_private_data_handler, data);
+ if (ACPI_FAILURE(status)) {
+ ACPI_ERROR((AE_INFO, "Error attaching device[%p] data\n",
+ handle));
+ return -ENODEV;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL(acpi_bus_attach_private_data);
+
int acpi_bus_get_private_data(acpi_handle handle, void **data)
{
acpi_status status;
@@ -141,7 +157,7 @@ int acpi_bus_get_private_data(acpi_handle handle, void **data)

status = acpi_get_data(handle, acpi_bus_private_data_handler, data);
if (ACPI_FAILURE(status) || !*data) {
- ACPI_DEBUG_PRINT((ACPI_DB_INFO, "No context for object [%p]\n",
+ ACPI_ERROR((AE_INFO, "No context for object [%p]\n",
handle));
return -ENODEV;
}
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 84a2e29..a1f6fec 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -406,6 +406,7 @@ extern struct kobject *acpi_kobj;
extern int acpi_bus_generate_netlink_event(const char*, const char*, u8, int);
void acpi_bus_private_data_handler(acpi_handle, void *);
int acpi_bus_get_private_data(acpi_handle, void **);
+int acpi_bus_attach_private_data(acpi_handle, void *);
void acpi_bus_no_hotplug(acpi_handle handle);
extern int acpi_notifier_call_chain(struct acpi_device *, u32, u32);
extern int register_acpi_notifier(struct notifier_block *);
--
1.8.4.rc0.1.g8f6a3e5.dirty

2014-04-22 11:13:50

by Mika Westerberg

[permalink] [raw]
Subject: Re: [Resend Patch 1/9] ACPICA: Executer: Fix buffer allocation issue for generic_serial_bus region field accesses.

On Tue, Apr 22, 2014 at 02:24:07PM +0800, Lan Tianyu wrote:
> From: Lv Zheng <[email protected]>
>
> The size of the buffer allocated for generic_serial_bus region access
> is not correct. This patch introduces acpi_ex_get_serial_access_length()
> to be invoked to obtain correct data buffer length. Reported by
> Lan Tianyu, Fixed by Lv Zheng.
>
> Signed-off-by: Lv Zheng <[email protected]>
> Signed-off-by: Lan Tianyu <[email protected]>

Reviewed-by: Mika Westerberg <[email protected]>

2014-04-22 11:14:19

by Mika Westerberg

[permalink] [raw]
Subject: Re: [Resend Patch 2/9] ACPICA: Export acpi_buffer_to_resource symbol

On Tue, Apr 22, 2014 at 02:24:08PM +0800, Lan Tianyu wrote:
> The acpi_buffer_to_resource is needed in i2c module
> to convert aml buffer to struct acpi_resource
>
> Signed-off-by: Lan Tianyu <[email protected]>

Reviewed-by: Mika Westerberg <[email protected]>

2014-04-22 11:16:05

by Mika Westerberg

[permalink] [raw]
Subject: Re: [Resend Patch 3/9] ACPI: Add acpi_bus_attach_private_data() to facilitate to attach data to ACPI handle

On Tue, Apr 22, 2014 at 02:24:09PM +0800, Lan Tianyu wrote:
> There is already acpi_bus_get_private_data() to get ACPI handle data
> which is associated with acpi_bus_private_data_handler(). This patch
> is to add acpi_bus_attach_private_data() to make a pair and facilitate
> to attach and get data to/from ACPI handle.
>
> Signed-off-by: Lan Tianyu <[email protected]>
> ---
> drivers/acpi/bus.c | 18 +++++++++++++++++-
> include/acpi/acpi_bus.h | 1 +
> 2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index e7e5844..4ed8d48 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -132,6 +132,22 @@ void acpi_bus_private_data_handler(acpi_handle handle,
> }
> EXPORT_SYMBOL(acpi_bus_private_data_handler);
>
> +int acpi_bus_attach_private_data(acpi_handle handle, void *data)
> +{
> + acpi_status status;
> +
> + status = acpi_attach_data(handle,
> + acpi_bus_private_data_handler, data);
> + if (ACPI_FAILURE(status)) {
> + ACPI_ERROR((AE_INFO, "Error attaching device[%p] data\n",
> + handle));
> + return -ENODEV;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(acpi_bus_attach_private_data);

When I added GPIO operation region support, Rafael mentioned that we might
want to add this private data to the struct acpi_device instead.

Either way, looks good to me,

Reviewed-by: Mika Westerberg <[email protected]>

2014-04-22 11:16:50

by Mika Westerberg

[permalink] [raw]
Subject: Re: [Resend Patch 4/9] ACPI/Thermal: Use acpi_bus_attach_private_data() to attach private data

On Tue, Apr 22, 2014 at 02:24:10PM +0800, Lan Tianyu wrote:
> Use acpi_bus_attach_private_data() to attach private data
> instead of acpi_attach_data().
>
> Signed-off-by: Lan Tianyu <[email protected]>

Reviewed-by: Mika Westerberg <[email protected]>

2014-04-22 11:17:43

by Mika Westerberg

[permalink] [raw]
Subject: Re: [Resend Patch 5/9] I2C: Add smbus quick read/write helper function

On Tue, Apr 22, 2014 at 02:24:11PM +0800, Lan Tianyu wrote:
> Add i2c_smbus_quick_write/read() helper function. These will be used
> in the implementation of i2c ACPI address space handler.
>
> Signed-off-by: Lan Tianyu <[email protected]>

Reviewed-by: Mika Westerberg <[email protected]>

2014-04-22 11:19:00

by Mika Westerberg

[permalink] [raw]
Subject: Re: [Resend Patch 6/9] I2C: Add smbus word/block process call helper function

On Tue, Apr 22, 2014 at 02:24:12PM +0800, Lan Tianyu wrote:
> Add i2c_smbus_word/block_proc_call() helper function. These will be used
> in the implementation of i2c ACPI address space handler.
>
> Signed-off-by: Lan Tianyu <[email protected]>

Reviewed-by: Mika Westerberg <[email protected]>

2014-04-22 11:29:07

by Mika Westerberg

[permalink] [raw]
Subject: Re: [Resend Patch 7/9] I2C/ACPI: Add i2c ACPI operation region support

On Tue, Apr 22, 2014 at 02:24:13PM +0800, Lan Tianyu wrote:
> diff --git a/drivers/i2c/i2c-acpi.c b/drivers/i2c/i2c-acpi.c
> new file mode 100644
> index 0000000..942abdf
> --- /dev/null
> +++ b/drivers/i2c/i2c-acpi.c
> @@ -0,0 +1,282 @@
> +/*
> + * I2C ACPI code
> + *
> + * Copyright (C) 2014 Intel Corp
> + *
> + * Author: Lan Tianyu <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * 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.
> + */
> +#define pr_fmt(fmt) "I2C/ACPI : " fmt
> +
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/acpi.h>
> +
> +struct acpi_i2c_handler_data {
> + struct acpi_connection_info info;
> + struct i2c_adapter *adapter;
> +} __packed;

Why __packed?

> +
> +struct gsb_buffer {
> + u8 status;
> + u8 len;
> + union {
> + u16 wdata;
> + u8 bdata;
> + u8 data[0];
> + };
> +} __packed;
> +
> +static int acpi_gsb_i2c_read_bytes(struct i2c_client *client,
> + u8 cmd, u8 *data, u8 data_len)
> +{
> +
> + struct i2c_msg msgs[2];
> + int ret;
> + u8 *buffer;

You seem to have two spaces here.

> +
> + buffer = kzalloc(data_len, GFP_KERNEL);
> + if (!buffer)
> + return AE_NO_MEMORY;
> +
> + msgs[0].addr = client->addr;
> + msgs[0].flags = client->flags;
> + msgs[0].len = 1;
> + msgs[0].buf = &cmd;
> +
> + msgs[1].addr = client->addr;
> + msgs[1].flags = client->flags | I2C_M_RD;
> + msgs[1].len = data_len;
> + msgs[1].buf = buffer;
> +
> + ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> + if (ret < 0)
> + pr_err("i2c read failed\n");
> + else
> + memcpy(data, buffer, data_len);
> +
> + kfree(buffer);
> + return ret;
> +}
> +
> +static int acpi_gsb_i2c_write_bytes(struct i2c_client *client,
> + u8 cmd, u8 *data, u8 data_len)
> +{
> +
> + struct i2c_msg msgs[1];
> + u8 *buffer;

Ditto.

> + int ret = AE_OK;
> +
> + buffer = kzalloc(data_len + 1, GFP_KERNEL);
> + if (!buffer)
> + return AE_NO_MEMORY;
> +
> + buffer[0] = cmd;
> + memcpy(buffer + 1, data, data_len);
> +
> + msgs[0].addr = client->addr;
> + msgs[0].flags = client->flags;
> + msgs[0].len = data_len + 1;
> + msgs[0].buf = buffer;
> +
> + ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> + if (ret < 0)
> + pr_err("i2c write failed\n");

Since you have pointer to the adapter, can't you use dev_err() instead?

> +
> + kfree(buffer);
> + return ret;
> +}
> +
> +static acpi_status
> +acpi_i2c_space_handler(u32 function, acpi_physical_address command,
> + u32 bits, u64 *value64,
> + void *handler_context, void *region_context)
> +{
> + struct gsb_buffer *gsb = (struct gsb_buffer *)value64;
> + struct acpi_i2c_handler_data *data = handler_context;
> + struct acpi_connection_info *info = &data->info;
> + struct acpi_resource_i2c_serialbus *sb;
> + struct i2c_adapter *adapter = data->adapter;
> + struct i2c_client client;
> + struct acpi_resource *ares;
> + u32 accessor_type = function >> 16;
> + u8 action = function & ACPI_IO_MASK;
> + int status;
> +
> + acpi_buffer_to_resource(info->connection, info->length, &ares);
> + if (!value64 || ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
> + return AE_BAD_PARAMETER;
> +
> + sb = &ares->data.i2c_serial_bus;
> + if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_I2C)
> + return AE_BAD_PARAMETER;

Do you leak the resource buffer here?

> +
> + client.adapter = adapter;
> + client.addr = sb->slave_address;
> + client.flags = 0;

I'm not sure if this is the correct way to use struct i2c_client
(allocating it from stack and passing forward to functions that might
expect a real i2c_client structure). It has ->dev and all.

I'm?wondering if you can use i2c_transfer() and i2c_smbus_xfer() here
instead, passing just the adapter pointer?

> +
> + if (sb->access_mode == ACPI_I2C_10BIT_MODE)
> + client.flags |= I2C_CLIENT_TEN;
> +
> + switch (accessor_type) {
> + case ACPI_GSB_ACCESS_ATTRIB_QUICK:
> + if (action == ACPI_READ)
> + status = i2c_smbus_quick_read(&client);
> + else
> + status = i2c_smbus_quick_write(&client);
> + break;
> +
> + case ACPI_GSB_ACCESS_ATTRIB_SEND_RCV:
> + if (action == ACPI_READ) {
> + status = i2c_smbus_read_byte(&client);
> + if (status >= 0) {
> + gsb->bdata = status;
> + status = 0;
> + }
> + } else {
> + status = i2c_smbus_write_byte(&client, gsb->bdata);
> + }
> + break;
> +
> + case ACPI_GSB_ACCESS_ATTRIB_BYTE:
> + if (action == ACPI_READ) {
> + status = i2c_smbus_read_byte_data(&client, command);
> + if (status >= 0) {
> + gsb->bdata = status;
> + status = 0;
> + }
> + } else {
> + status = i2c_smbus_write_byte_data(&client, command,
> + gsb->bdata);
> + }
> + break;
> +
> + case ACPI_GSB_ACCESS_ATTRIB_WORD:
> + if (action == ACPI_READ) {
> + status = i2c_smbus_read_word_data(&client, command);
> + if (status >= 0) {
> + gsb->wdata = status;
> + status = 0;
> + }
> + } else {
> + status = i2c_smbus_write_word_data(&client, command,
> + gsb->wdata);
> + }
> + break;
> +
> + case ACPI_GSB_ACCESS_ATTRIB_BLOCK:
> + if (action == ACPI_READ) {
> + status = i2c_smbus_read_block_data(&client, command,
> + gsb->data);
> + if (status >= 0) {
> + gsb->len = status;
> + status = 0;
> + }
> + } else {
> + status = i2c_smbus_write_block_data(&client, command,
> + gsb->len, gsb->data);
> + }
> + break;
> +
> + case ACPI_GSB_ACCESS_ATTRIB_MULTIBYTE:
> + if (action == ACPI_READ) {
> + status = acpi_gsb_i2c_read_bytes(&client, command,
> + gsb->data, info->access_length);
> + if (status > 0)
> + status = 0;
> + } else {
> + status = acpi_gsb_i2c_write_bytes(&client, command,
> + gsb->data, info->access_length);
> + }
> + break;
> +
> + case ACPI_GSB_ACCESS_ATTRIB_WORD_CALL:
> + status = i2c_smbus_word_proc_call(&client, command, gsb->wdata);
> + if (status >= 0) {
> + gsb->wdata = status;
> + status = 0;
> + }
> + break;
> +
> + case ACPI_GSB_ACCESS_ATTRIB_BLOCK_CALL:
> + status = i2c_smbus_block_proc_call(&client, command, gsb->len,
> + gsb->data);
> + if (status > 0) {
> + gsb->len = status;
> + status = 0;
> + }
> + break;
> +
> + default:
> + pr_info("protocl(0x%02x) is not supported.\n", accessor_type);

protocl -> protocol

> + return AE_BAD_PARAMETER;
> + }
> +
> + gsb->status = status;
> + return AE_OK;
> +}

2014-04-22 11:30:59

by Mika Westerberg

[permalink] [raw]
Subject: Re: [Resend Patch 8/9] I2C/ACPI: Move ACPI related code to i2c-acpi.c

On Tue, Apr 22, 2014 at 02:24:14PM +0800, Lan Tianyu wrote:
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 681e689..fc1ef42 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -586,7 +586,10 @@ static inline struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node
> #ifdef CONFIG_ACPI
> int acpi_i2c_install_space_handler(struct i2c_adapter *adapter);
> void acpi_i2c_remove_space_handler(struct i2c_adapter *adapter);
> +void acpi_i2c_register_devices(struct i2c_adapter *adap);
> #else
> +static inline void acpi_i2c_register_devices(struct i2c_adapter *adap) {};

Semicolon is not needed here.

Apart from that,

Reviewed-by: Mika Westerberg <[email protected]>

2014-04-22 11:37:39

by Mika Westerberg

[permalink] [raw]
Subject: Re: [Resend Patch 9/9] I2C/ACPI: Add CONFIG_I2C_ACPI config

On Tue, Apr 22, 2014 at 02:24:15PM +0800, Lan Tianyu wrote:
> This patch is to add CONFIG_I2C_ACPI. Current there is a race between
> removing I2C ACPI operation region and ACPI AML code accessing.
> So make i2c core built-in if CONFIG_I2C_ACPI is set.
>
> Signed-off-by: Lan Tianyu <[email protected]>
> ---
> drivers/i2c/Kconfig | 17 ++++++++++++++++-
> drivers/i2c/Makefile | 2 +-
> include/linux/i2c.h | 2 +-
> 3 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
> index 7b7ea32..c670d49 100644
> --- a/drivers/i2c/Kconfig
> +++ b/drivers/i2c/Kconfig
> @@ -2,7 +2,9 @@
> # I2C subsystem configuration
> #
>
> -menuconfig I2C
> +menu "I2C support"
> +
> +config I2C
> tristate "I2C support"
> select RT_MUTEXES
> ---help---
> @@ -21,6 +23,17 @@ menuconfig I2C
> This I2C support can also be built as a module. If so, the module
> will be called i2c-core.
>
> +config I2C_ACPI
> + bool "I2C ACPI support"
> + select I2C
> + depends on ACPI
> + default y
> + help
> + Say Y here if you want to enable I2C ACPI function. ACPI table
> + provides I2C slave devices' information to enumerate these devices.
> + This option also allows ACPI AML code to access I2C slave devices
> + via I2C ACPI operation region to fulfill ACPI method.
> +

I'm wondering, can we provide some sort of wrapper function from ACPI core
that is guaranteed to be built in to the kernel image and use it instead of
adding new Kconfig options?

2014-04-23 02:01:05

by Lan Tianyu

[permalink] [raw]
Subject: Re: [Resend Patch 7/9] I2C/ACPI: Add i2c ACPI operation region support

Hi Mika:
Thanks a lot for your review.

On 2014年04月22日 19:36, Mika Westerberg wrote:
>> > +
>> > + client.adapter = adapter;
>> > + client.addr = sb->slave_address;
>> > + client.flags = 0;
> I'm not sure if this is the correct way to use struct i2c_client
> (allocating it from stack and passing forward to functions that might
> expect a real i2c_client structure). It has ->dev and all.

I check all i2c_smbus_xxx helper functions and they are simple wrappers
of i2c_smbus_xfer(). Only adapter, addr and flags of struct client are
used in these functions.

>
> I'm wondering if you can use i2c_transfer() and i2c_smbus_xfer() here
> instead, passing just the adapter pointer?
>

Yes, I can do that but this needs to copy all codes of i2c_smbus_xxx
helper function in the i2c-core.c here. This seems duplicated?

--
Best regards
Tianyu Lan

2014-04-23 05:47:38

by Lan Tianyu

[permalink] [raw]
Subject: Re: [Resend Patch 9/9] I2C/ACPI: Add CONFIG_I2C_ACPI config

On 2014年04月22日 19:45, Mika Westerberg wrote:
> On Tue, Apr 22, 2014 at 02:24:15PM +0800, Lan Tianyu wrote:
>> This patch is to add CONFIG_I2C_ACPI. Current there is a race between
>> removing I2C ACPI operation region and ACPI AML code accessing.
>> So make i2c core built-in if CONFIG_I2C_ACPI is set.
>>
>> Signed-off-by: Lan Tianyu <[email protected]>
>> ---
>> drivers/i2c/Kconfig | 17 ++++++++++++++++-
>> drivers/i2c/Makefile | 2 +-
>> include/linux/i2c.h | 2 +-
>> 3 files changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
>> index 7b7ea32..c670d49 100644
>> --- a/drivers/i2c/Kconfig
>> +++ b/drivers/i2c/Kconfig
>> @@ -2,7 +2,9 @@
>> # I2C subsystem configuration
>> #
>>
>> -menuconfig I2C
>> +menu "I2C support"
>> +
>> +config I2C
>> tristate "I2C support"
>> select RT_MUTEXES
>> ---help---
>> @@ -21,6 +23,17 @@ menuconfig I2C
>> This I2C support can also be built as a module. If so, the module
>> will be called i2c-core.
>>
>> +config I2C_ACPI
>> + bool "I2C ACPI support"
>> + select I2C
>> + depends on ACPI
>> + default y
>> + help
>> + Say Y here if you want to enable I2C ACPI function. ACPI table
>> + provides I2C slave devices' information to enumerate these devices.
>> + This option also allows ACPI AML code to access I2C slave devices
>> + via I2C ACPI operation region to fulfill ACPI method.
>> +
>
> I'm wondering, can we provide some sort of wrapper function from ACPI core
> that is guaranteed to be built in to the kernel image and use it instead of
> adding new Kconfig options?
>
Cc: LV

LV tried to fix the issue via wrapper solution in the ACPI code before.
https://lkml.org/lkml/2013/7/23/87

He has a plan to resolve the issue in ACPICA later.

Other choice is to increase the i2c-core module count to prevent it
being unloaded when i2c operation region handler is installed. Remove
the code When LV finish his job.

--
Best regards
Tianyu Lan

2014-04-23 06:47:50

by Zheng, Lv

[permalink] [raw]
Subject: RE: [Resend Patch 9/9] I2C/ACPI: Add CONFIG_I2C_ACPI config

Hi, Tianyu

> From: Lan, Tianyu
> Sent: Wednesday, April 23, 2014 1:40 PM
>
> On 2014年04月22日 19:45, Mika Westerberg wrote:
> > On Tue, Apr 22, 2014 at 02:24:15PM +0800, Lan Tianyu wrote:
> >> This patch is to add CONFIG_I2C_ACPI. Current there is a race between
> >> removing I2C ACPI operation region and ACPI AML code accessing.
> >> So make i2c core built-in if CONFIG_I2C_ACPI is set.
> >>
> >> Signed-off-by: Lan Tianyu <[email protected]>
> >> ---
> >> drivers/i2c/Kconfig | 17 ++++++++++++++++-
> >> drivers/i2c/Makefile | 2 +-
> >> include/linux/i2c.h | 2 +-
> >> 3 files changed, 18 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
> >> index 7b7ea32..c670d49 100644
> >> --- a/drivers/i2c/Kconfig
> >> +++ b/drivers/i2c/Kconfig
> >> @@ -2,7 +2,9 @@
> >> # I2C subsystem configuration
> >> #
> >>
> >> -menuconfig I2C
> >> +menu "I2C support"
> >> +
> >> +config I2C
> >> tristate "I2C support"
> >> select RT_MUTEXES
> >> ---help---
> >> @@ -21,6 +23,17 @@ menuconfig I2C
> >> This I2C support can also be built as a module. If so, the module
> >> will be called i2c-core.
> >>
> >> +config I2C_ACPI
> >> + bool "I2C ACPI support"
> >> + select I2C
> >> + depends on ACPI
> >> + default y
> >> + help
> >> + Say Y here if you want to enable I2C ACPI function. ACPI table
> >> + provides I2C slave devices' information to enumerate these devices.
> >> + This option also allows ACPI AML code to access I2C slave devices
> >> + via I2C ACPI operation region to fulfill ACPI method.
> >> +
> >
> > I'm wondering, can we provide some sort of wrapper function from ACPI core
> > that is guaranteed to be built in to the kernel image and use it instead of
> > adding new Kconfig options?
> >
> Cc: LV
>
> LV tried to fix the issue via wrapper solution in the ACPI code before.
> https://lkml.org/lkml/2013/7/23/87
>
> He has a plan to resolve the issue in ACPICA later.
>
> Other choice is to increase the i2c-core module count to prevent it
> being unloaded when i2c operation region handler is installed. Remove
> the code When LV finish his job.

You may see it implemented in ACPICA after several release.
If you need a fix for now, you can use the patch pointed to by the link you've provided,
Or you could find an updated one here:
acpi-ipmi13.patch archived in (https://bugzilla.kernel.org/attachment.cgi?id=112611)

I think the solution you've provided in this patch is also reasonable for now.
IPMI also uses a similar solution to solve this issue.
Please refer to the CONFIG_ACPI_IPMI.

The story can be found at:
http://www.spinics.net/lists/linux-acpi/msg49044.html
And the similar solution can be found at:
http://www.spinics.net/lists/linux-acpi/msg49184.html

Thanks and best regards
-Lv

>
> --
> Best regards
> Tianyu Lan
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2014-04-23 07:22:32

by Mika Westerberg

[permalink] [raw]
Subject: Re: [Resend Patch 7/9] I2C/ACPI: Add i2c ACPI operation region support

On Wed, Apr 23, 2014 at 09:53:21AM +0800, Lan Tianyu wrote:
> Hi Mika:
> Thanks a lot for your review.
>
> On 2014年04月22日 19:36, Mika Westerberg wrote:
> >> > +
> >> > + client.adapter = adapter;
> >> > + client.addr = sb->slave_address;
> >> > + client.flags = 0;
> > I'm not sure if this is the correct way to use struct i2c_client
> > (allocating it from stack and passing forward to functions that might
> > expect a real i2c_client structure). It has ->dev and all.
>
> I check all i2c_smbus_xxx helper functions and they are simple wrappers
> of i2c_smbus_xfer(). Only adapter, addr and flags of struct client are
> used in these functions.

In that case I suppose it is OK to (ab)use the client structure like this.

I think you still want to initialize the whole structure (unused fields
with zero) before passing it to the wrappers.

2014-04-23 07:25:35

by Lan Tianyu

[permalink] [raw]
Subject: Re: [Resend Patch 7/9] I2C/ACPI: Add i2c ACPI operation region support

On 2014年04月23日 15:28, Mika Westerberg wrote:
> On Wed, Apr 23, 2014 at 09:53:21AM +0800, Lan Tianyu wrote:
>> Hi Mika:
>> Thanks a lot for your review.
>>
>> On 2014年04月22日 19:36, Mika Westerberg wrote:
>>>>> +
>>>>> + client.adapter = adapter;
>>>>> + client.addr = sb->slave_address;
>>>>> + client.flags = 0;
>>> I'm not sure if this is the correct way to use struct i2c_client
>>> (allocating it from stack and passing forward to functions that might
>>> expect a real i2c_client structure). It has ->dev and all.
>>
>> I check all i2c_smbus_xxx helper functions and they are simple wrappers
>> of i2c_smbus_xfer(). Only adapter, addr and flags of struct client are
>> used in these functions.
>
> In that case I suppose it is OK to (ab)use the client structure like this.
>
> I think you still want to initialize the whole structure (unused fields
> with zero) before passing it to the wrappers.
>

Ok. I will do that.

--
Best regards
Tianyu Lan

2014-04-23 07:34:05

by Mika Westerberg

[permalink] [raw]
Subject: Re: [Resend Patch 9/9] I2C/ACPI: Add CONFIG_I2C_ACPI config

On Wed, Apr 23, 2014 at 06:47:05AM +0000, Zheng, Lv wrote:
> Hi, Tianyu
>
> > From: Lan, Tianyu
> > Sent: Wednesday, April 23, 2014 1:40 PM
> >
> > On 2014年04月22日 19:45, Mika Westerberg wrote:
> > > On Tue, Apr 22, 2014 at 02:24:15PM +0800, Lan Tianyu wrote:
> > >> This patch is to add CONFIG_I2C_ACPI. Current there is a race between
> > >> removing I2C ACPI operation region and ACPI AML code accessing.
> > >> So make i2c core built-in if CONFIG_I2C_ACPI is set.
> > >>
> > >> Signed-off-by: Lan Tianyu <[email protected]>
> > >> ---
> > >> drivers/i2c/Kconfig | 17 ++++++++++++++++-
> > >> drivers/i2c/Makefile | 2 +-
> > >> include/linux/i2c.h | 2 +-
> > >> 3 files changed, 18 insertions(+), 3 deletions(-)
> > >>
> > >> diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
> > >> index 7b7ea32..c670d49 100644
> > >> --- a/drivers/i2c/Kconfig
> > >> +++ b/drivers/i2c/Kconfig
> > >> @@ -2,7 +2,9 @@
> > >> # I2C subsystem configuration
> > >> #
> > >>
> > >> -menuconfig I2C
> > >> +menu "I2C support"
> > >> +
> > >> +config I2C
> > >> tristate "I2C support"
> > >> select RT_MUTEXES
> > >> ---help---
> > >> @@ -21,6 +23,17 @@ menuconfig I2C
> > >> This I2C support can also be built as a module. If so, the module
> > >> will be called i2c-core.
> > >>
> > >> +config I2C_ACPI
> > >> + bool "I2C ACPI support"
> > >> + select I2C
> > >> + depends on ACPI
> > >> + default y
> > >> + help
> > >> + Say Y here if you want to enable I2C ACPI function. ACPI table
> > >> + provides I2C slave devices' information to enumerate these devices.
> > >> + This option also allows ACPI AML code to access I2C slave devices
> > >> + via I2C ACPI operation region to fulfill ACPI method.
> > >> +
> > >
> > > I'm wondering, can we provide some sort of wrapper function from ACPI core
> > > that is guaranteed to be built in to the kernel image and use it instead of
> > > adding new Kconfig options?
> > >
> > Cc: LV
> >
> > LV tried to fix the issue via wrapper solution in the ACPI code before.
> > https://lkml.org/lkml/2013/7/23/87
> >
> > He has a plan to resolve the issue in ACPICA later.
> >
> > Other choice is to increase the i2c-core module count to prevent it
> > being unloaded when i2c operation region handler is installed. Remove
> > the code When LV finish his job.
>
> You may see it implemented in ACPICA after several release.
> If you need a fix for now, you can use the patch pointed to by the link you've provided,
> Or you could find an updated one here:
> acpi-ipmi13.patch archived in (https://bugzilla.kernel.org/attachment.cgi?id=112611)
>
> I think the solution you've provided in this patch is also reasonable for now.
> IPMI also uses a similar solution to solve this issue.
> Please refer to the CONFIG_ACPI_IPMI.
>
> The story can be found at:
> http://www.spinics.net/lists/linux-acpi/msg49044.html
> And the similar solution can be found at:
> http://www.spinics.net/lists/linux-acpi/msg49184.html

Thanks for the pointers.

Given that the IPMI problem was solved like this I guess I2C operation
regions can follow the same pattern if there is no better solution
available.

Out of curiousity: how did you plan to fix this in ACPICA?

2014-04-23 23:03:40

by Adam Williamson

[permalink] [raw]
Subject: Re: [Resend Patch 0/9] I2C ACPI operation region handler support

On Tue, 2014-04-22 at 14:24 +0800, Lan Tianyu wrote:
> ACPI 5.0 spec(5.5.2.4.5) defines GenericSerialBus(i2c, spi, uart) operation
> region. It allows ACPI aml code able to access such kind of devices to
> implement some ACPI standard method.
>
> On the Asus T100TA, Bios use GenericSerialBus operation region to access
> i2c device to get battery info. So battery function depends on the I2C
> operation region support. Here is the bug link.
> https://bugzilla.kernel.org/show_bug.cgi?id=69011
>
> This patchset is to add I2C ACPI operation region handler support.

For what it's worth, I've tested this patchset with the exception of
patch 9/9 - which I leave out because it would require annoying fiddling
with my kernel builds - and it gives apparently-accurate battery/AC
information on my Venue 8 Pro.

It doesn't *obviously* cause any other problems, though there are a few
other issues I'm dealing with ATM. If any of them turn out to be related
to this patch set, I'll send a follow-up.
--
Adam Williamson
Fedora QA Community Monkey
IRC: adamw | Twitter: AdamW_Fedora | XMPP: adamw AT happyassassin . net
http://www.happyassassin.net

2014-04-28 14:27:32

by Lan Tianyu

[permalink] [raw]
Subject: [Patch V2 1/9] ACPICA: Executer: Fix buffer allocation issue for generic_serial_bus region field accesses.

From: Lv Zheng <[email protected]>

The size of the buffer allocated for generic_serial_bus region access
is not correct. This patch introduces acpi_ex_get_serial_access_length()
to be invoked to obtain correct data buffer length. Reported by
Lan Tianyu, Fixed by Lv Zheng.

Reviewed-by: Mika Westerberg <[email protected]>
Signed-off-by: Lv Zheng <[email protected]>
Signed-off-by: Lan Tianyu <[email protected]>
---
drivers/acpi/acpica/exfield.c | 104 +++++++++++++++++++++++++++++++++++++++---
1 file changed, 97 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/acpica/exfield.c b/drivers/acpi/acpica/exfield.c
index 68d9744..12878e1 100644
--- a/drivers/acpi/acpica/exfield.c
+++ b/drivers/acpi/acpica/exfield.c
@@ -45,10 +45,71 @@
#include "accommon.h"
#include "acdispat.h"
#include "acinterp.h"
+#include "amlcode.h"

#define _COMPONENT ACPI_EXECUTER
ACPI_MODULE_NAME("exfield")

+/* Local prototypes */
+static u32
+acpi_ex_get_serial_access_length(u32 accessor_type, u32 access_length);
+
+/*******************************************************************************
+ *
+ * FUNCTION: acpi_get_serial_access_bytes
+ *
+ * PARAMETERS: accessor_type - The type of the protocol indicated by region
+ * field access attributes
+ * access_length - The access length of the region field
+ *
+ * RETURN: Decoded access length
+ *
+ * DESCRIPTION: This routine returns the length of the generic_serial_bus
+ * protocol bytes
+ *
+ ******************************************************************************/
+
+static u32
+acpi_ex_get_serial_access_length(u32 accessor_type, u32 access_length)
+{
+ u32 length;
+
+ switch (accessor_type) {
+ case AML_FIELD_ATTRIB_QUICK:
+
+ length = 0;
+ break;
+
+ case AML_FIELD_ATTRIB_SEND_RCV:
+ case AML_FIELD_ATTRIB_BYTE:
+
+ length = 1;
+ break;
+
+ case AML_FIELD_ATTRIB_WORD:
+ case AML_FIELD_ATTRIB_WORD_CALL:
+
+ length = 2;
+ break;
+
+ case AML_FIELD_ATTRIB_MULTIBYTE:
+ case AML_FIELD_ATTRIB_RAW_BYTES:
+ case AML_FIELD_ATTRIB_RAW_PROCESS:
+
+ length = access_length;
+ break;
+
+ case AML_FIELD_ATTRIB_BLOCK:
+ case AML_FIELD_ATTRIB_BLOCK_CALL:
+ default:
+
+ length = ACPI_GSBUS_BUFFER_SIZE;
+ break;
+ }
+
+ return (length);
+}
+
/*******************************************************************************
*
* FUNCTION: acpi_ex_read_data_from_field
@@ -63,8 +124,9 @@ ACPI_MODULE_NAME("exfield")
* Buffer, depending on the size of the field.
*
******************************************************************************/
+
acpi_status
-acpi_ex_read_data_from_field(struct acpi_walk_state *walk_state,
+acpi_ex_read_data_from_field(struct acpi_walk_state * walk_state,
union acpi_operand_object *obj_desc,
union acpi_operand_object **ret_buffer_desc)
{
@@ -73,6 +135,7 @@ acpi_ex_read_data_from_field(struct acpi_walk_state *walk_state,
acpi_size length;
void *buffer;
u32 function;
+ u16 accessor_type;

ACPI_FUNCTION_TRACE_PTR(ex_read_data_from_field, obj_desc);

@@ -116,9 +179,22 @@ acpi_ex_read_data_from_field(struct acpi_walk_state *walk_state,
ACPI_READ | (obj_desc->field.attribute << 16);
} else if (obj_desc->field.region_obj->region.space_id ==
ACPI_ADR_SPACE_GSBUS) {
- length = ACPI_GSBUS_BUFFER_SIZE;
- function =
- ACPI_READ | (obj_desc->field.attribute << 16);
+ accessor_type = obj_desc->field.attribute;
+ length = acpi_ex_get_serial_access_length(accessor_type,
+ obj_desc->
+ field.
+ access_length);
+
+ /*
+ * Add additional 2 bytes for modeled generic_serial_bus data buffer:
+ * typedef struct {
+ * BYTEStatus; // Byte 0 of the data buffer
+ * BYTELength; // Byte 1 of the data buffer
+ * BYTE[x-1]Data; // Bytes 2-x of the arbitrary length data buffer,
+ * }
+ */
+ length += 2;
+ function = ACPI_READ | (accessor_type << 16);
} else { /* IPMI */

length = ACPI_IPMI_BUFFER_SIZE;
@@ -231,6 +307,7 @@ acpi_ex_write_data_to_field(union acpi_operand_object *source_desc,
void *buffer;
union acpi_operand_object *buffer_desc;
u32 function;
+ u16 accessor_type;

ACPI_FUNCTION_TRACE_PTR(ex_write_data_to_field, obj_desc);

@@ -284,9 +361,22 @@ acpi_ex_write_data_to_field(union acpi_operand_object *source_desc,
ACPI_WRITE | (obj_desc->field.attribute << 16);
} else if (obj_desc->field.region_obj->region.space_id ==
ACPI_ADR_SPACE_GSBUS) {
- length = ACPI_GSBUS_BUFFER_SIZE;
- function =
- ACPI_WRITE | (obj_desc->field.attribute << 16);
+ accessor_type = obj_desc->field.attribute;
+ length = acpi_ex_get_serial_access_length(accessor_type,
+ obj_desc->
+ field.
+ access_length);
+
+ /*
+ * Add additional 2 bytes for modeled generic_serial_bus data buffer:
+ * typedef struct {
+ * BYTEStatus; // Byte 0 of the data buffer
+ * BYTELength; // Byte 1 of the data buffer
+ * BYTE[x-1]Data; // Bytes 2-x of the arbitrary length data buffer,
+ * }
+ */
+ length += 2;
+ function = ACPI_WRITE | (accessor_type << 16);
} else { /* IPMI */

length = ACPI_IPMI_BUFFER_SIZE;
--
1.8.3.1

2014-04-28 14:27:42

by Lan Tianyu

[permalink] [raw]
Subject: [Patch V2 2/9] ACPICA: Export acpi_buffer_to_resource symbol

The acpi_buffer_to_resource is needed in i2c module
to convert aml buffer to struct acpi_resource

Reviewed-by: Mika Westerberg <[email protected]>
Signed-off-by: Lan Tianyu <[email protected]>
---
drivers/acpi/acpica/rscreate.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/acpi/acpica/rscreate.c b/drivers/acpi/acpica/rscreate.c
index 75d3690..5828649 100644
--- a/drivers/acpi/acpica/rscreate.c
+++ b/drivers/acpi/acpica/rscreate.c
@@ -112,6 +112,7 @@ acpi_buffer_to_resource(u8 *aml_buffer,

return (status);
}
+ACPI_EXPORT_SYMBOL(acpi_buffer_to_resource);

/*******************************************************************************
*
--
1.8.3.1

2014-04-28 14:27:54

by Lan Tianyu

[permalink] [raw]
Subject: [Patch V2 5/9] I2C: Add smbus quick read/write helper function

Add i2c_smbus_quick_write/read() helper function. These will be used
in the implementation of i2c ACPI address space handler.

Reviewed-by: Mika Westerberg <[email protected]>
Signed-off-by: Lan Tianyu <[email protected]>
---
drivers/i2c/i2c-core.c | 30 ++++++++++++++++++++++++++++++
include/linux/i2c.h | 2 ++
2 files changed, 32 insertions(+)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 7c7f4b8..3bf0048 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -2162,6 +2162,36 @@ static int i2c_smbus_check_pec(u8 cpec, struct i2c_msg *msg)
}

/**
+ * i2c_smbus_quick_write - SMBus "quick write" protocol
+ * @client: Handle to slave device
+ *
+ * This executes the SMBus "quick write" protocol, returning negative errno
+ * else zero on success.
+ */
+s32 i2c_smbus_quick_write(const struct i2c_client *client)
+{
+ return i2c_smbus_xfer(client->adapter, client->addr, client->flags,
+ I2C_SMBUS_WRITE, 0,
+ I2C_SMBUS_QUICK, NULL);
+}
+EXPORT_SYMBOL_GPL(i2c_smbus_quick_write);
+
+/**
+ * i2c_smbus_quick_read - SMBus "quick read" protocol
+ * @client: Handle to slave device
+ *
+ * This executes the SMBus "quick read" protocol, returning negative errno
+ * else zero on success.
+ */
+s32 i2c_smbus_quick_read(const struct i2c_client *client)
+{
+ return i2c_smbus_xfer(client->adapter, client->addr, client->flags,
+ I2C_SMBUS_READ, 0,
+ I2C_SMBUS_QUICK, NULL);
+}
+EXPORT_SYMBOL_GPL(i2c_smbus_quick_read);
+
+/**
* i2c_smbus_read_byte - SMBus "receive byte" protocol
* @client: Handle to slave device
*
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index b556e0a..3e6ea90 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -82,6 +82,8 @@ extern s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
/* Now follow the 'nice' access routines. These also document the calling
conventions of i2c_smbus_xfer. */

+extern s32 i2c_smbus_quick_write(const struct i2c_client *client);
+extern s32 i2c_smbus_quick_read(const struct i2c_client *client);
extern s32 i2c_smbus_read_byte(const struct i2c_client *client);
extern s32 i2c_smbus_write_byte(const struct i2c_client *client, u8 value);
extern s32 i2c_smbus_read_byte_data(const struct i2c_client *client,
--
1.8.3.1

2014-04-28 14:28:14

by Lan Tianyu

[permalink] [raw]
Subject: [Patch V2 3/9] ACPI: Add acpi_bus_attach_private_data() to facilitate to attach data to ACPI handle

There is already acpi_bus_get_private_data() to get ACPI handle data
which is associated with acpi_bus_private_data_handler(). This patch
is to add acpi_bus_attach_private_data() to make a pair and facilitate
to attach and get data to/from ACPI handle.

Reviewed-by: Mika Westerberg <[email protected]>
Signed-off-by: Lan Tianyu <[email protected]>
---
drivers/acpi/bus.c | 18 +++++++++++++++++-
include/acpi/acpi_bus.h | 1 +
2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index e7e5844..4ed8d48 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -132,6 +132,22 @@ void acpi_bus_private_data_handler(acpi_handle handle,
}
EXPORT_SYMBOL(acpi_bus_private_data_handler);

+int acpi_bus_attach_private_data(acpi_handle handle, void *data)
+{
+ acpi_status status;
+
+ status = acpi_attach_data(handle,
+ acpi_bus_private_data_handler, data);
+ if (ACPI_FAILURE(status)) {
+ ACPI_ERROR((AE_INFO, "Error attaching device[%p] data\n",
+ handle));
+ return -ENODEV;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL(acpi_bus_attach_private_data);
+
int acpi_bus_get_private_data(acpi_handle handle, void **data)
{
acpi_status status;
@@ -141,7 +157,7 @@ int acpi_bus_get_private_data(acpi_handle handle, void **data)

status = acpi_get_data(handle, acpi_bus_private_data_handler, data);
if (ACPI_FAILURE(status) || !*data) {
- ACPI_DEBUG_PRINT((ACPI_DB_INFO, "No context for object [%p]\n",
+ ACPI_ERROR((AE_INFO, "No context for object [%p]\n",
handle));
return -ENODEV;
}
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 84a2e29..a1f6fec 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -406,6 +406,7 @@ extern struct kobject *acpi_kobj;
extern int acpi_bus_generate_netlink_event(const char*, const char*, u8, int);
void acpi_bus_private_data_handler(acpi_handle, void *);
int acpi_bus_get_private_data(acpi_handle, void **);
+int acpi_bus_attach_private_data(acpi_handle, void *);
void acpi_bus_no_hotplug(acpi_handle handle);
extern int acpi_notifier_call_chain(struct acpi_device *, u32, u32);
extern int register_acpi_notifier(struct notifier_block *);
--
1.8.3.1

2014-04-28 14:28:31

by Lan Tianyu

[permalink] [raw]
Subject: [Patch V2 7/9] I2C/ACPI: Add i2c ACPI operation region support

ACPI 5.0 spec(5.5.2.4.5) defines GenericSerialBus(i2c, spi, uart) operation region.
It allows ACPI aml code able to access such kind of devices to implement
some ACPI standard method.

ACPI Spec defines some access attribute to associate with i2c protocol.
AttribQuick Read/Write Quick Protocol
AttribSendReceive Send/Receive Byte Protocol
AttribByte Read/Write Byte Protocol
AttribWord Read/Write Word Protocol
AttribBlock Read/Write Block Protocol
AttribBytes Read/Write N-Bytes Protocol
AttribProcessCall Process Call Protocol
AttribBlockProcessCall Write Block-Read Block Process Call Protocol
AttribRawBytes Raw Read/Write N-BytesProtocol
AttribRawProcessBytes Raw Process Call Protocol

On the Asus T100TA, Bios use GenericSerialBus operation region to access
i2c device to get battery info.

Sample code From Asus T100TA

Scope (_SB.I2C1)
{
Name (UMPC, ResourceTemplate ()
{
I2cSerialBus (0x0066, ControllerInitiated, 0x00061A80,
AddressingMode7Bit, "\\_SB.I2C1",
0x00, ResourceConsumer, ,
)
})

...

OperationRegion (DVUM, GenericSerialBus, Zero, 0x0100)
Field (DVUM, BufferAcc, NoLock, Preserve)
{
Connection (UMPC),
Offset (0x81),
AccessAs (BufferAcc, AttribBytes (0x3E)),
FGC0, 8
}
...
}

Device (BATC)
{
Name (_HID, EisaId ("PNP0C0A")) // _HID: Hardware ID
Name (_UID, One) // _UID: Unique ID
...

Method (_BST, 0, NotSerialized) // _BST: Battery Status
{
If (LEqual (AVBL, One))
{
Store (FGC0, BFFG)
If (LNotEqual (STAT, One))
{
ShiftRight (CHST, 0x04, Local0)
And (Local0, 0x03, Local0)
If (LOr (LEqual (Local0, One), LEqual (Local0, 0x02)))
{
Store (0x02, Local1)
}
...

}

The i2c operation region is defined under I2C1 scope. _BST method under
battery device BATC read battery status from the field "FCG0". The request
would be sent to i2c operation region handler.

This patch is to add i2c ACPI operation region support. Due to there are
only "Byte" and "Bytes" protocol access on the Asus T100TA, other protocols
have not been tested.

About RawBytes and RawProcessBytes protocol, they needs specific drivers to interpret
reference data from AML code according ACPI 5.0 SPEC(5.5.2.4.5.3.9 and 5.5.2.4.5.3.10).
So far, not found such case and will add when find real case.

Signed-off-by: Lan Tianyu <[email protected]>
---
drivers/i2c/Makefile | 5 +-
drivers/i2c/i2c-acpi.c | 295 +++++++++++++++++++++++++++++++++++++++++++++++++
drivers/i2c/i2c-core.c | 2 +
include/linux/acpi.h | 11 ++
include/linux/i2c.h | 10 ++
5 files changed, 322 insertions(+), 1 deletion(-)
create mode 100644 drivers/i2c/i2c-acpi.c

diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
index 1722f50..80db307 100644
--- a/drivers/i2c/Makefile
+++ b/drivers/i2c/Makefile
@@ -2,8 +2,11 @@
# Makefile for the i2c core.
#

+i2ccore-y := i2c-core.o
+i2ccore-$(CONFIG_ACPI) += i2c-acpi.o
+
obj-$(CONFIG_I2C_BOARDINFO) += i2c-boardinfo.o
-obj-$(CONFIG_I2C) += i2c-core.o
+obj-$(CONFIG_I2C) += i2ccore.o
obj-$(CONFIG_I2C_SMBUS) += i2c-smbus.o
obj-$(CONFIG_I2C_CHARDEV) += i2c-dev.o
obj-$(CONFIG_I2C_MUX) += i2c-mux.o
diff --git a/drivers/i2c/i2c-acpi.c b/drivers/i2c/i2c-acpi.c
new file mode 100644
index 0000000..ab0272e
--- /dev/null
+++ b/drivers/i2c/i2c-acpi.c
@@ -0,0 +1,295 @@
+/*
+ * I2C ACPI code
+ *
+ * Copyright (C) 2014 Intel Corp
+ *
+ * Author: Lan Tianyu <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * 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.
+ */
+#define pr_fmt(fmt) "I2C/ACPI : " fmt
+
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/acpi.h>
+
+struct acpi_i2c_handler_data {
+ struct acpi_connection_info info;
+ struct i2c_adapter *adapter;
+};
+
+struct gsb_buffer {
+ u8 status;
+ u8 len;
+ union {
+ u16 wdata;
+ u8 bdata;
+ u8 data[0];
+ };
+} __packed;
+
+static int acpi_gsb_i2c_read_bytes(struct i2c_client *client,
+ u8 cmd, u8 *data, u8 data_len)
+{
+
+ struct i2c_msg msgs[2];
+ int ret;
+ u8 *buffer;
+
+ buffer = kzalloc(data_len, GFP_KERNEL);
+ if (!buffer)
+ return AE_NO_MEMORY;
+
+ msgs[0].addr = client->addr;
+ msgs[0].flags = client->flags;
+ msgs[0].len = 1;
+ msgs[0].buf = &cmd;
+
+ msgs[1].addr = client->addr;
+ msgs[1].flags = client->flags | I2C_M_RD;
+ msgs[1].len = data_len;
+ msgs[1].buf = buffer;
+
+ ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
+ if (ret < 0)
+ dev_err(&client->adapter->dev, "i2c read failed\n");
+ else
+ memcpy(data, buffer, data_len);
+
+ kfree(buffer);
+ return ret;
+}
+
+static int acpi_gsb_i2c_write_bytes(struct i2c_client *client,
+ u8 cmd, u8 *data, u8 data_len)
+{
+
+ struct i2c_msg msgs[1];
+ u8 *buffer;
+ int ret = AE_OK;
+
+ buffer = kzalloc(data_len + 1, GFP_KERNEL);
+ if (!buffer)
+ return AE_NO_MEMORY;
+
+ buffer[0] = cmd;
+ memcpy(buffer + 1, data, data_len);
+
+ msgs[0].addr = client->addr;
+ msgs[0].flags = client->flags;
+ msgs[0].len = data_len + 1;
+ msgs[0].buf = buffer;
+
+ ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
+ if (ret < 0)
+ dev_err(&client->adapter->dev, "i2c write failed\n");
+
+ kfree(buffer);
+ return ret;
+}
+
+static acpi_status
+acpi_i2c_space_handler(u32 function, acpi_physical_address command,
+ u32 bits, u64 *value64,
+ void *handler_context, void *region_context)
+{
+ struct gsb_buffer *gsb = (struct gsb_buffer *)value64;
+ struct acpi_i2c_handler_data *data = handler_context;
+ struct acpi_connection_info *info = &data->info;
+ struct acpi_resource_i2c_serialbus *sb;
+ struct i2c_adapter *adapter = data->adapter;
+ struct i2c_client client;
+ struct acpi_resource *ares;
+ u32 accessor_type = function >> 16;
+ u8 action = function & ACPI_IO_MASK;
+ acpi_status ret = AE_OK;
+ int status;
+
+ ret = acpi_buffer_to_resource(info->connection, info->length, &ares);
+ if (ACPI_FAILURE(ret))
+ return ret;
+
+ if (!value64 || ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS) {
+ ret = AE_BAD_PARAMETER;
+ goto err;
+ }
+
+ sb = &ares->data.i2c_serial_bus;
+ if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_I2C) {
+ ret = AE_BAD_PARAMETER;
+ goto err;
+ }
+
+ memset(&client, 0, sizeof(client));
+ client.adapter = adapter;
+ client.addr = sb->slave_address;
+ client.flags = 0;
+
+ if (sb->access_mode == ACPI_I2C_10BIT_MODE)
+ client.flags |= I2C_CLIENT_TEN;
+
+ switch (accessor_type) {
+ case ACPI_GSB_ACCESS_ATTRIB_QUICK:
+ if (action == ACPI_READ)
+ status = i2c_smbus_quick_read(&client);
+ else
+ status = i2c_smbus_quick_write(&client);
+ break;
+
+ case ACPI_GSB_ACCESS_ATTRIB_SEND_RCV:
+ if (action == ACPI_READ) {
+ status = i2c_smbus_read_byte(&client);
+ if (status >= 0) {
+ gsb->bdata = status;
+ status = 0;
+ }
+ } else {
+ status = i2c_smbus_write_byte(&client, gsb->bdata);
+ }
+ break;
+
+ case ACPI_GSB_ACCESS_ATTRIB_BYTE:
+ if (action == ACPI_READ) {
+ status = i2c_smbus_read_byte_data(&client, command);
+ if (status >= 0) {
+ gsb->bdata = status;
+ status = 0;
+ }
+ } else {
+ status = i2c_smbus_write_byte_data(&client, command,
+ gsb->bdata);
+ }
+ break;
+
+ case ACPI_GSB_ACCESS_ATTRIB_WORD:
+ if (action == ACPI_READ) {
+ status = i2c_smbus_read_word_data(&client, command);
+ if (status >= 0) {
+ gsb->wdata = status;
+ status = 0;
+ }
+ } else {
+ status = i2c_smbus_write_word_data(&client, command,
+ gsb->wdata);
+ }
+ break;
+
+ case ACPI_GSB_ACCESS_ATTRIB_BLOCK:
+ if (action == ACPI_READ) {
+ status = i2c_smbus_read_block_data(&client, command,
+ gsb->data);
+ if (status >= 0) {
+ gsb->len = status;
+ status = 0;
+ }
+ } else {
+ status = i2c_smbus_write_block_data(&client, command,
+ gsb->len, gsb->data);
+ }
+ break;
+
+ case ACPI_GSB_ACCESS_ATTRIB_MULTIBYTE:
+ if (action == ACPI_READ) {
+ status = acpi_gsb_i2c_read_bytes(&client, command,
+ gsb->data, info->access_length);
+ if (status > 0)
+ status = 0;
+ } else {
+ status = acpi_gsb_i2c_write_bytes(&client, command,
+ gsb->data, info->access_length);
+ }
+ break;
+
+ case ACPI_GSB_ACCESS_ATTRIB_WORD_CALL:
+ status = i2c_smbus_word_proc_call(&client, command, gsb->wdata);
+ if (status >= 0) {
+ gsb->wdata = status;
+ status = 0;
+ }
+ break;
+
+ case ACPI_GSB_ACCESS_ATTRIB_BLOCK_CALL:
+ status = i2c_smbus_block_proc_call(&client, command, gsb->len,
+ gsb->data);
+ if (status > 0) {
+ gsb->len = status;
+ status = 0;
+ }
+ break;
+
+ default:
+ pr_info("protocol(0x%02x) is not supported.\n", accessor_type);
+ ret = AE_BAD_PARAMETER;
+ goto err;
+ }
+
+ gsb->status = status;
+
+ err:
+ ACPI_FREE(ares);
+ return ret;
+}
+
+
+int acpi_i2c_install_space_handler(struct i2c_adapter *adapter)
+{
+ acpi_handle handle = ACPI_HANDLE(adapter->dev.parent);
+ struct acpi_i2c_handler_data *data;
+ acpi_status status;
+
+ if (!handle)
+ return -ENODEV;
+
+ data = kzalloc(sizeof(struct acpi_i2c_handler_data),
+ GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ data->adapter = adapter;
+ status = acpi_bus_attach_private_data(handle, (void *)data);
+ if (ACPI_FAILURE(status)) {
+ kfree(data);
+ return -ENOMEM;
+ }
+
+ status = acpi_install_address_space_handler(handle,
+ ACPI_ADR_SPACE_GSBUS,
+ &acpi_i2c_space_handler,
+ NULL,
+ data);
+ if (ACPI_FAILURE(status)) {
+ dev_err(&adapter->dev, "Error installing i2c space handler\n");
+ acpi_bus_attach_private_data(handle, NULL);
+ kfree(data);
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
+void acpi_i2c_remove_space_handler(struct i2c_adapter *adapter)
+{
+ acpi_handle handle = ACPI_HANDLE(adapter->dev.parent);
+ struct acpi_i2c_handler_data *data;
+ acpi_status status;
+
+ if (!handle)
+ return;
+
+ acpi_remove_address_space_handler(handle,
+ ACPI_ADR_SPACE_GSBUS,
+ &acpi_i2c_space_handler);
+
+ status = acpi_bus_get_private_data(handle, (void **)&data);
+ if (ACPI_SUCCESS(status))
+ kfree(data);
+}
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 638befd..bb9dc56 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1293,6 +1293,7 @@ exit_recovery:
/* create pre-declared device nodes */
of_i2c_register_devices(adap);
acpi_i2c_register_devices(adap);
+ acpi_i2c_install_space_handler(adap);

if (adap->nr < __i2c_first_dynamic_bus_num)
i2c_scan_static_board_info(adap);
@@ -1466,6 +1467,7 @@ void i2c_del_adapter(struct i2c_adapter *adap)
return;
}

+ acpi_i2c_remove_space_handler(adap);
/* Tell drivers about this removal */
mutex_lock(&core_lock);
bus_for_each_drv(&i2c_bus_type, NULL, adap,
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 7a8f2cd..ea53b9b 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -361,6 +361,17 @@ extern bool osc_sb_apei_support_acked;
#define OSC_PCI_EXPRESS_CAPABILITY_CONTROL 0x00000010
#define OSC_PCI_CONTROL_MASKS 0x0000001f

+#define ACPI_GSB_ACCESS_ATTRIB_QUICK 0x00000002
+#define ACPI_GSB_ACCESS_ATTRIB_SEND_RCV 0x00000004
+#define ACPI_GSB_ACCESS_ATTRIB_BYTE 0x00000006
+#define ACPI_GSB_ACCESS_ATTRIB_WORD 0x00000008
+#define ACPI_GSB_ACCESS_ATTRIB_BLOCK 0x0000000A
+#define ACPI_GSB_ACCESS_ATTRIB_MULTIBYTE 0x0000000B
+#define ACPI_GSB_ACCESS_ATTRIB_WORD_CALL 0x0000000C
+#define ACPI_GSB_ACCESS_ATTRIB_BLOCK_CALL 0x0000000D
+#define ACPI_GSB_ACCESS_ATTRIB_RAW_BYTES 0x0000000E
+#define ACPI_GSB_ACCESS_ATTRIB_RAW_PROCESS 0x0000000F
+
extern acpi_status acpi_pci_osc_control_set(acpi_handle handle,
u32 *mask, u32 req);

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 724440a..681e689 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -583,4 +583,14 @@ static inline struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node
}
#endif /* CONFIG_OF */

+#ifdef CONFIG_ACPI
+int acpi_i2c_install_space_handler(struct i2c_adapter *adapter);
+void acpi_i2c_remove_space_handler(struct i2c_adapter *adapter);
+#else
+static inline void acpi_i2c_remove_space_handler(struct i2c_adapter *adapter)
+{ }
+static inline int acpi_i2c_install_space_handler(struct i2c_adapter *adapter)
+{ return 0; }
+#endif
+
#endif /* _LINUX_I2C_H */
--
1.8.3.1

2014-04-28 14:28:48

by Lan Tianyu

[permalink] [raw]
Subject: [Patch V2 8/9] I2C/ACPI: Move ACPI related code to i2c-acpi.c

Clean up ACPI related code in the i2c core.

Reviewed-by: Mika Westerberg <[email protected]>
Signed-off-by: Lan Tianyu <[email protected]>
---
drivers/i2c/i2c-acpi.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++
drivers/i2c/i2c-core.c | 95 --------------------------------------------------
include/linux/i2c.h | 2 ++
3 files changed, 91 insertions(+), 95 deletions(-)

diff --git a/drivers/i2c/i2c-acpi.c b/drivers/i2c/i2c-acpi.c
index ab0272e..e2ad72d 100644
--- a/drivers/i2c/i2c-acpi.c
+++ b/drivers/i2c/i2c-acpi.c
@@ -37,6 +37,95 @@ struct gsb_buffer {
};
} __packed;

+static int acpi_i2c_add_resource(struct acpi_resource *ares, void *data)
+{
+ struct i2c_board_info *info = data;
+
+ if (ares->type == ACPI_RESOURCE_TYPE_SERIAL_BUS) {
+ struct acpi_resource_i2c_serialbus *sb;
+
+ sb = &ares->data.i2c_serial_bus;
+ if (sb->type == ACPI_RESOURCE_SERIAL_TYPE_I2C) {
+ info->addr = sb->slave_address;
+ if (sb->access_mode == ACPI_I2C_10BIT_MODE)
+ info->flags |= I2C_CLIENT_TEN;
+ }
+ } else if (info->irq < 0) {
+ struct resource r;
+
+ if (acpi_dev_resource_interrupt(ares, 0, &r))
+ info->irq = r.start;
+ }
+
+ /* Tell the ACPI core to skip this resource */
+ return 1;
+}
+
+static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
+ void *data, void **return_value)
+{
+ struct i2c_adapter *adapter = data;
+ struct list_head resource_list;
+ struct i2c_board_info info;
+ struct acpi_device *adev;
+ int ret;
+
+ if (acpi_bus_get_device(handle, &adev))
+ return AE_OK;
+ if (acpi_bus_get_status(adev) || !adev->status.present)
+ return AE_OK;
+
+ memset(&info, 0, sizeof(info));
+ info.acpi_node.companion = adev;
+ info.irq = -1;
+
+ INIT_LIST_HEAD(&resource_list);
+ ret = acpi_dev_get_resources(adev, &resource_list,
+ acpi_i2c_add_resource, &info);
+ acpi_dev_free_resource_list(&resource_list);
+
+ if (ret < 0 || !info.addr)
+ return AE_OK;
+
+ adev->power.flags.ignore_parent = true;
+ strlcpy(info.type, dev_name(&adev->dev), sizeof(info.type));
+ if (!i2c_new_device(adapter, &info)) {
+ adev->power.flags.ignore_parent = false;
+ dev_err(&adapter->dev,
+ "failed to add I2C device %s from ACPI\n",
+ dev_name(&adev->dev));
+ }
+
+ return AE_OK;
+}
+
+/**
+ * acpi_i2c_register_devices - enumerate I2C slave devices behind adapter
+ * @adap: pointer to adapter
+ *
+ * Enumerate all I2C slave devices behind this adapter by walking the ACPI
+ * namespace. When a device is found it will be added to the Linux device
+ * model and bound to the corresponding ACPI handle.
+ */
+void acpi_i2c_register_devices(struct i2c_adapter *adap)
+{
+ acpi_handle handle;
+ acpi_status status;
+
+ if (!adap->dev.parent)
+ return;
+
+ handle = ACPI_HANDLE(adap->dev.parent);
+ if (!handle)
+ return;
+
+ status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
+ acpi_i2c_add_device, NULL,
+ adap, NULL);
+ if (ACPI_FAILURE(status))
+ dev_warn(&adap->dev, "failed to enumerate I2C slaves\n");
+}
+
static int acpi_gsb_i2c_read_bytes(struct i2c_client *client,
u8 cmd, u8 *data, u8 data_len)
{
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index bb9dc56..9ca50c8 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1092,101 +1092,6 @@ EXPORT_SYMBOL(of_find_i2c_adapter_by_node);
static void of_i2c_register_devices(struct i2c_adapter *adap) { }
#endif /* CONFIG_OF */

-/* ACPI support code */
-
-#if IS_ENABLED(CONFIG_ACPI)
-static int acpi_i2c_add_resource(struct acpi_resource *ares, void *data)
-{
- struct i2c_board_info *info = data;
-
- if (ares->type == ACPI_RESOURCE_TYPE_SERIAL_BUS) {
- struct acpi_resource_i2c_serialbus *sb;
-
- sb = &ares->data.i2c_serial_bus;
- if (sb->type == ACPI_RESOURCE_SERIAL_TYPE_I2C) {
- info->addr = sb->slave_address;
- if (sb->access_mode == ACPI_I2C_10BIT_MODE)
- info->flags |= I2C_CLIENT_TEN;
- }
- } else if (info->irq < 0) {
- struct resource r;
-
- if (acpi_dev_resource_interrupt(ares, 0, &r))
- info->irq = r.start;
- }
-
- /* Tell the ACPI core to skip this resource */
- return 1;
-}
-
-static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
- void *data, void **return_value)
-{
- struct i2c_adapter *adapter = data;
- struct list_head resource_list;
- struct i2c_board_info info;
- struct acpi_device *adev;
- int ret;
-
- if (acpi_bus_get_device(handle, &adev))
- return AE_OK;
- if (acpi_bus_get_status(adev) || !adev->status.present)
- return AE_OK;
-
- memset(&info, 0, sizeof(info));
- info.acpi_node.companion = adev;
- info.irq = -1;
-
- INIT_LIST_HEAD(&resource_list);
- ret = acpi_dev_get_resources(adev, &resource_list,
- acpi_i2c_add_resource, &info);
- acpi_dev_free_resource_list(&resource_list);
-
- if (ret < 0 || !info.addr)
- return AE_OK;
-
- adev->power.flags.ignore_parent = true;
- strlcpy(info.type, dev_name(&adev->dev), sizeof(info.type));
- if (!i2c_new_device(adapter, &info)) {
- adev->power.flags.ignore_parent = false;
- dev_err(&adapter->dev,
- "failed to add I2C device %s from ACPI\n",
- dev_name(&adev->dev));
- }
-
- return AE_OK;
-}
-
-/**
- * acpi_i2c_register_devices - enumerate I2C slave devices behind adapter
- * @adap: pointer to adapter
- *
- * Enumerate all I2C slave devices behind this adapter by walking the ACPI
- * namespace. When a device is found it will be added to the Linux device
- * model and bound to the corresponding ACPI handle.
- */
-static void acpi_i2c_register_devices(struct i2c_adapter *adap)
-{
- acpi_handle handle;
- acpi_status status;
-
- if (!adap->dev.parent)
- return;
-
- handle = ACPI_HANDLE(adap->dev.parent);
- if (!handle)
- return;
-
- status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
- acpi_i2c_add_device, NULL,
- adap, NULL);
- if (ACPI_FAILURE(status))
- dev_warn(&adap->dev, "failed to enumerate I2C slaves\n");
-}
-#else
-static inline void acpi_i2c_register_devices(struct i2c_adapter *adap) {}
-#endif /* CONFIG_ACPI */
-
static int i2c_do_add_adapter(struct i2c_driver *driver,
struct i2c_adapter *adap)
{
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 681e689..4b6fcdb 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -586,7 +586,9 @@ static inline struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node
#ifdef CONFIG_ACPI
int acpi_i2c_install_space_handler(struct i2c_adapter *adapter);
void acpi_i2c_remove_space_handler(struct i2c_adapter *adapter);
+void acpi_i2c_register_devices(struct i2c_adapter *adap);
#else
+static inline void acpi_i2c_register_devices(struct i2c_adapter *adap) { }
static inline void acpi_i2c_remove_space_handler(struct i2c_adapter *adapter)
{ }
static inline int acpi_i2c_install_space_handler(struct i2c_adapter *adapter)
--
1.8.3.1

2014-04-28 14:28:54

by Lan Tianyu

[permalink] [raw]
Subject: [Patch V2 9/9] I2C/ACPI: Add CONFIG_I2C_ACPI config

This patch is to add CONFIG_I2C_ACPI. Current there is a race between
removing I2C ACPI operation region and ACPI AML code accessing.
So make i2c core built-in if CONFIG_I2C_ACPI is set.

Signed-off-by: Lan Tianyu <[email protected]>
---
drivers/i2c/Kconfig | 17 ++++++++++++++++-
drivers/i2c/Makefile | 2 +-
include/linux/i2c.h | 2 +-
3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
index 7b7ea32..c670d49 100644
--- a/drivers/i2c/Kconfig
+++ b/drivers/i2c/Kconfig
@@ -2,7 +2,9 @@
# I2C subsystem configuration
#

-menuconfig I2C
+menu "I2C support"
+
+config I2C
tristate "I2C support"
select RT_MUTEXES
---help---
@@ -21,6 +23,17 @@ menuconfig I2C
This I2C support can also be built as a module. If so, the module
will be called i2c-core.

+config I2C_ACPI
+ bool "I2C ACPI support"
+ select I2C
+ depends on ACPI
+ default y
+ help
+ Say Y here if you want to enable I2C ACPI function. ACPI table
+ provides I2C slave devices' information to enumerate these devices.
+ This option also allows ACPI AML code to access I2C slave devices
+ via I2C ACPI operation region to fulfill ACPI method.
+
if I2C

config I2C_BOARDINFO
@@ -124,3 +137,5 @@ config I2C_DEBUG_BUS
on.

endif # I2C
+
+endmenu
diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
index 80db307..37464ee 100644
--- a/drivers/i2c/Makefile
+++ b/drivers/i2c/Makefile
@@ -3,7 +3,7 @@
#

i2ccore-y := i2c-core.o
-i2ccore-$(CONFIG_ACPI) += i2c-acpi.o
+i2ccore-$(CONFIG_I2C_ACPI) += i2c-acpi.o

obj-$(CONFIG_I2C_BOARDINFO) += i2c-boardinfo.o
obj-$(CONFIG_I2C) += i2ccore.o
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 4b6fcdb..8c2d9b5 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -583,7 +583,7 @@ static inline struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node
}
#endif /* CONFIG_OF */

-#ifdef CONFIG_ACPI
+#ifdef CONFIG_I2C_ACPI
int acpi_i2c_install_space_handler(struct i2c_adapter *adapter);
void acpi_i2c_remove_space_handler(struct i2c_adapter *adapter);
void acpi_i2c_register_devices(struct i2c_adapter *adap);
--
1.8.3.1

2014-04-28 14:28:30

by Lan Tianyu

[permalink] [raw]
Subject: [Patch V2 4/9] ACPI/Thermal: Use acpi_bus_attach_private_data() to attach private data

Use acpi_bus_attach_private_data() to attach private data
instead of acpi_attach_data().

Reviewed-by: Mika Westerberg <[email protected]>
Signed-off-by: Lan Tianyu <[email protected]>
---
drivers/acpi/thermal.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
index c1e31a4..7ab9392 100644
--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -925,13 +925,10 @@ static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz)
if (result)
return result;

- status = acpi_attach_data(tz->device->handle,
- acpi_bus_private_data_handler,
- tz->thermal_zone);
- if (ACPI_FAILURE(status)) {
- pr_err(PREFIX "Error attaching device data\n");
+ status = acpi_bus_attach_private_data(tz->device->handle,
+ tz->thermal_zone);
+ if (ACPI_FAILURE(status))
return -ENODEV;
- }

tz->tz_enabled = 1;

--
1.8.3.1

2014-04-28 14:31:24

by Lan Tianyu

[permalink] [raw]
Subject: [Patch V2 6/9] I2C: Add smbus word/block process call helper function

Add i2c_smbus_word/block_proc_call() helper function. These will be used
in the implementation of i2c ACPI address space handler.

Reviewed-by: Mika Westerberg <[email protected]>
Signed-off-by: Lan Tianyu <[email protected]>
---
drivers/i2c/i2c-core.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/i2c.h | 4 ++++
2 files changed, 60 insertions(+)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 3bf0048..638befd 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -2306,6 +2306,30 @@ s32 i2c_smbus_write_word_data(const struct i2c_client *client, u8 command,
EXPORT_SYMBOL(i2c_smbus_write_word_data);

/**
+ * i2c_smbus_word_proc_call - SMBus "word proc call" protocol
+ * @client: Handle to slave device
+ * @command: Byte interpreted by slave
+ * @value: 16-bit "word" being written
+ *
+ * This executes the SMBus "word proc all" protocol, returning negative errno
+ * else a 16-bit unsigned "word" received from the device.
+ */
+s32 i2c_smbus_word_proc_call(const struct i2c_client *client, u8 command,
+ u16 value)
+{
+ union i2c_smbus_data data;
+ int status;
+
+ data.word = value;
+ status = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
+ I2C_SMBUS_READ, command,
+ I2C_SMBUS_PROC_CALL, &data);
+
+ return (status < 0) ? status : data.word;
+}
+EXPORT_SYMBOL(i2c_smbus_word_proc_call);
+
+/**
* i2c_smbus_read_block_data - SMBus "block read" protocol
* @client: Handle to slave device
* @command: Byte interpreted by slave
@@ -2362,6 +2386,38 @@ s32 i2c_smbus_write_block_data(const struct i2c_client *client, u8 command,
}
EXPORT_SYMBOL(i2c_smbus_write_block_data);

+/**
+ * i2c_smbus_block_proc_call - SMBus "block write" protocol
+ * @client: Handle to slave device
+ * @command: Byte interpreted by slave
+ * @length: Size of data block; SMBus allows at most 32 bytes
+ * @values: Byte array which will be written.
+ *
+ * This executes the SMBus "block proc call" protocol, returning negative errno
+ * else the number of read bytes.
+ */
+s32 i2c_smbus_block_proc_call(const struct i2c_client *client, u8 command,
+ u8 length, u8 *values)
+{
+ union i2c_smbus_data data;
+ int status;
+
+ if (length > I2C_SMBUS_BLOCK_MAX)
+ length = I2C_SMBUS_BLOCK_MAX;
+ data.block[0] = length;
+ memcpy(&data.block[1], values, length);
+ status = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
+ I2C_SMBUS_READ, command,
+ I2C_SMBUS_BLOCK_PROC_CALL, &data);
+
+ if (status < 0)
+ return status;
+
+ memcpy(values, &data.block[1], data.block[0]);
+ return data.block[0];
+}
+EXPORT_SYMBOL(i2c_smbus_block_proc_call);
+
/* Returns the number of read bytes */
s32 i2c_smbus_read_i2c_block_data(const struct i2c_client *client, u8 command,
u8 length, u8 *values)
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 3e6ea90..724440a 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -94,6 +94,10 @@ extern s32 i2c_smbus_read_word_data(const struct i2c_client *client,
u8 command);
extern s32 i2c_smbus_write_word_data(const struct i2c_client *client,
u8 command, u16 value);
+extern s32 i2c_smbus_word_proc_call(const struct i2c_client *client,
+ u8 command, u16 value);
+extern s32 i2c_smbus_block_proc_call(const struct i2c_client *client,
+ u8 command, u8 length, u8 *values);

static inline s32
i2c_smbus_read_word_swapped(const struct i2c_client *client, u8 command)
--
1.8.3.1

2014-04-28 14:27:28

by Lan Tianyu

[permalink] [raw]
Subject: [Patch V2 0/9] I2C ACPI operation region handler support

ACPI 5.0 spec(5.5.2.4.5) defines GenericSerialBus(i2c, spi, uart) operation
region. It allows ACPI aml code able to access such kind of devices to
implement some ACPI standard method.

On the Asus T100TA, Bios use GenericSerialBus operation region to access
i2c device to get battery info. So battery function depends on the I2C
operation region support. Here is the bug link.
https://bugzilla.kernel.org/show_bug.cgi?id=69011

This patchset is to add I2C ACPI operation region handler support.

Change Since V1:
Fix some code style and memory leak issues in Patch 7

[Patch V2 1/9] ACPICA: Executer: Fix buffer allocation issue for
[Patch V2 2/9] ACPICA: Export acpi_buffer_to_resource symbol
[Patch V2 3/9] ACPI: Add acpi_bus_attach_private_data() to facilitate
[Patch V2 4/9] ACPI/Thermal: Use acpi_bus_attach_private_data() to
[Patch V2 5/9] I2C: Add smbus quick read/write helper function
[Patch V2 6/9] I2C: Add smbus word/block process call helper function
[Patch V2 7/9] I2C/ACPI: Add i2c ACPI operation region support
[Patch V2 8/9] I2C/ACPI: Move ACPI related code to i2c-acpi.c
[Patch V2 9/9] I2C/ACPI: Add CONFIG_I2C_ACPI config

drivers/acpi/acpica/exfield.c | 104 ++++++++++++++++++++++++++++++++++---
drivers/acpi/acpica/rscreate.c | 1 +
drivers/acpi/bus.c | 18 ++++++-
drivers/acpi/thermal.c | 9 ++--
drivers/i2c/Kconfig | 17 ++++++-
drivers/i2c/Makefile | 5 +-
drivers/i2c/i2c-acpi.c | 384 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
drivers/i2c/i2c-core.c | 183 ++++++++++++++++++++++++++++++++---------------------------------
include/acpi/acpi_bus.h | 1 +
include/linux/acpi.h | 11 ++++
include/linux/i2c.h | 18 +++++++
11 files changed, 640 insertions(+), 111 deletions(-)

2014-04-28 19:25:21

by Adam Williamson

[permalink] [raw]
Subject: Re: [Patch V2 1/9] ACPICA: Executer: Fix buffer allocation issue for generic_serial_bus region field accesses.

On Mon, 2014-04-28 at 10:52 -0700, Adam Williamson wrote:
> On Mon, 2014-04-28 at 22:27 +0800, Lan Tianyu wrote:
> > From: Lv Zheng <[email protected]>
> >
> > The size of the buffer allocated for generic_serial_bus region access
> > is not correct. This patch introduces acpi_ex_get_serial_access_length()
> > to be invoked to obtain correct data buffer length. Reported by
> > Lan Tianyu, Fixed by Lv Zheng.
> >
> > Reviewed-by: Mika Westerberg <[email protected]>
> > Signed-off-by: Lv Zheng <[email protected]>
> > Signed-off-by: Lan Tianyu <[email protected]>
>
> Very superficial issue, and sorry I forgot to mention it for v1, but the
> summary line for this patch (1/9) ends with a period - "...region field
> accesses." - which I think is not the correct style?

In fact, it seems this patch already got merged into Linus' tree
somehow:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/acpi/acpica/exfield.c?id=6273f00e6ecbd60494a979033b7e5271a29a0436

the others do not seem to have been merged yet.
--
Adam Williamson
Fedora QA Community Monkey
IRC: adamw | Twitter: AdamW_Fedora | XMPP: adamw AT happyassassin . net
http://www.happyassassin.net

2014-04-28 19:34:12

by Adam Williamson

[permalink] [raw]
Subject: Re: [Patch V2 1/9] ACPICA: Executer: Fix buffer allocation issue for generic_serial_bus region field accesses.

On Mon, 2014-04-28 at 22:27 +0800, Lan Tianyu wrote:
> From: Lv Zheng <[email protected]>
>
> The size of the buffer allocated for generic_serial_bus region access
> is not correct. This patch introduces acpi_ex_get_serial_access_length()
> to be invoked to obtain correct data buffer length. Reported by
> Lan Tianyu, Fixed by Lv Zheng.
>
> Reviewed-by: Mika Westerberg <[email protected]>
> Signed-off-by: Lv Zheng <[email protected]>
> Signed-off-by: Lan Tianyu <[email protected]>

Very superficial issue, and sorry I forgot to mention it for v1, but the
summary line for this patch (1/9) ends with a period - "...region field
accesses." - which I think is not the correct style?
--
Adam Williamson
Fedora QA Community Monkey
IRC: adamw | Twitter: AdamW_Fedora | XMPP: adamw AT happyassassin . net
http://www.happyassassin.net

2014-04-28 22:33:52

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [Patch V2 1/9] ACPICA: Executer: Fix buffer allocation issue for generic_serial_bus region field accesses.

On Monday, April 28, 2014 11:08:25 AM Adam Williamson wrote:
> On Mon, 2014-04-28 at 10:52 -0700, Adam Williamson wrote:
> > On Mon, 2014-04-28 at 22:27 +0800, Lan Tianyu wrote:
> > > From: Lv Zheng <[email protected]>
> > >
> > > The size of the buffer allocated for generic_serial_bus region access
> > > is not correct. This patch introduces acpi_ex_get_serial_access_length()
> > > to be invoked to obtain correct data buffer length. Reported by
> > > Lan Tianyu, Fixed by Lv Zheng.
> > >
> > > Reviewed-by: Mika Westerberg <[email protected]>
> > > Signed-off-by: Lv Zheng <[email protected]>
> > > Signed-off-by: Lan Tianyu <[email protected]>
> >
> > Very superficial issue, and sorry I forgot to mention it for v1, but the
> > summary line for this patch (1/9) ends with a period - "...region field
> > accesses." - which I think is not the correct style?

Well, some people do that, but is it really a problem?

> In fact, it seems this patch already got merged into Linus' tree
> somehow:
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/acpi/acpica/exfield.c?id=6273f00e6ecbd60494a979033b7e5271a29a0436

Yes, I wanted to have it in there now.

> the others do not seem to have been merged yet.

The rest has been queued up for 3.16.

--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

2014-04-28 22:34:59

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [Patch V2 0/9] I2C ACPI operation region handler support

On Monday, April 28, 2014 10:27:39 PM Lan Tianyu wrote:
> ACPI 5.0 spec(5.5.2.4.5) defines GenericSerialBus(i2c, spi, uart) operation
> region. It allows ACPI aml code able to access such kind of devices to
> implement some ACPI standard method.
>
> On the Asus T100TA, Bios use GenericSerialBus operation region to access
> i2c device to get battery info. So battery function depends on the I2C
> operation region support. Here is the bug link.
> https://bugzilla.kernel.org/show_bug.cgi?id=69011
>
> This patchset is to add I2C ACPI operation region handler support.
>
> Change Since V1:
> Fix some code style and memory leak issues in Patch 7

Is it the only patch that has changed from v1?

Rafael

2014-04-29 02:03:01

by Lan Tianyu

[permalink] [raw]
Subject: Re: [Patch V2 0/9] I2C ACPI operation region handler support

On 2014年04月29日 06:51, Rafael J. Wysocki wrote:
> On Monday, April 28, 2014 10:27:39 PM Lan Tianyu wrote:
>> ACPI 5.0 spec(5.5.2.4.5) defines GenericSerialBus(i2c, spi, uart) operation
>> region. It allows ACPI aml code able to access such kind of devices to
>> implement some ACPI standard method.
>>
>> On the Asus T100TA, Bios use GenericSerialBus operation region to access
>> i2c device to get battery info. So battery function depends on the I2C
>> operation region support. Here is the bug link.
>> https://bugzilla.kernel.org/show_bug.cgi?id=69011
>>
>> This patchset is to add I2C ACPI operation region handler support.
>>
>> Change Since V1:
>> Fix some code style and memory leak issues in Patch 7
>
> Is it the only patch that has changed from v1?


I also remove a redundant semicolon in the PATCH 8. Sorry. I didn't
notice these patches are already in your tree. I will produce divergence
patches based on your bleeding-edge branch.


>
> Rafael
>


--
Best regards
Tianyu Lan

2014-04-29 08:02:58

by Mika Westerberg

[permalink] [raw]
Subject: Re: [Patch V2 7/9] I2C/ACPI: Add i2c ACPI operation region support

On Mon, Apr 28, 2014 at 10:27:46PM +0800, Lan Tianyu wrote:
> ACPI 5.0 spec(5.5.2.4.5) defines GenericSerialBus(i2c, spi, uart) operation region.
> It allows ACPI aml code able to access such kind of devices to implement
> some ACPI standard method.

Looks good to me now,

Reviewed-by: Mika Westerberg <[email protected]>

2014-04-29 08:16:42

by Mika Westerberg

[permalink] [raw]
Subject: Re: [Patch V2 9/9] I2C/ACPI: Add CONFIG_I2C_ACPI config

On Mon, Apr 28, 2014 at 10:27:48PM +0800, Lan Tianyu wrote:
> This patch is to add CONFIG_I2C_ACPI. Current there is a race between
> removing I2C ACPI operation region and ACPI AML code accessing.
> So make i2c core built-in if CONFIG_I2C_ACPI is set.
>
> Signed-off-by: Lan Tianyu <[email protected]>
> ---
> drivers/i2c/Kconfig | 17 ++++++++++++++++-
> drivers/i2c/Makefile | 2 +-
> include/linux/i2c.h | 2 +-
> 3 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
> index 7b7ea32..c670d49 100644
> --- a/drivers/i2c/Kconfig
> +++ b/drivers/i2c/Kconfig
> @@ -2,7 +2,9 @@
> # I2C subsystem configuration
> #
>
> -menuconfig I2C
> +menu "I2C support"
> +
> +config I2C
> tristate "I2C support"
> select RT_MUTEXES
> ---help---
> @@ -21,6 +23,17 @@ menuconfig I2C
> This I2C support can also be built as a module. If so, the module
> will be called i2c-core.
>
> +config I2C_ACPI
> + bool "I2C ACPI support"
> + select I2C
> + depends on ACPI
> + default y
> + help
> + Say Y here if you want to enable I2C ACPI function. ACPI table
> + provides I2C slave devices' information to enumerate these devices.
> + This option also allows ACPI AML code to access I2C slave devices
> + via I2C ACPI operation region to fulfill ACPI method.

I would prefer something like:

Say Y here if you want to enable ACPI I2C support. This includes support
for automatic enumeration of I2C slave devices and support for ACPI I2C
Operation Regions. Operation Regions allow firmware (BIOS) code to
access I2C slave devices, such as smart batteries through an I2C host
controller driver.

But it is really up to you so,

Reviewed-by: Mika Westerberg <[email protected]>

2014-04-29 11:31:41

by Wolfram Sang

[permalink] [raw]
Subject: Re: [Patch V2 1/9] ACPICA: Executer: Fix buffer allocation issue for generic_serial_bus region field accesses.


> The rest has been queued up for 3.16.

I also aim for 3.16, yet it may take 1 or 2 weeks more until I'll be
able to review the I2C part of those patches.


Attachments:
(No filename) (158.00 B)
signature.asc (836.00 B)
Digital signature
Download all attachments

2014-04-29 15:30:44

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [Patch V2 0/9] I2C ACPI operation region handler support

On Tuesday, April 29, 2014 09:54:46 AM Lan Tianyu wrote:
> On 2014年04月29日 06:51, Rafael J. Wysocki wrote:
> > On Monday, April 28, 2014 10:27:39 PM Lan Tianyu wrote:
> >> ACPI 5.0 spec(5.5.2.4.5) defines GenericSerialBus(i2c, spi, uart) operation
> >> region. It allows ACPI aml code able to access such kind of devices to
> >> implement some ACPI standard method.
> >>
> >> On the Asus T100TA, Bios use GenericSerialBus operation region to access
> >> i2c device to get battery info. So battery function depends on the I2C
> >> operation region support. Here is the bug link.
> >> https://bugzilla.kernel.org/show_bug.cgi?id=69011
> >>
> >> This patchset is to add I2C ACPI operation region handler support.
> >>
> >> Change Since V1:
> >> Fix some code style and memory leak issues in Patch 7
> >
> > Is it the only patch that has changed from v1?
>
>
> I also remove a redundant semicolon in the PATCH 8. Sorry. I didn't
> notice these patches are already in your tree. I will produce divergence
> patches based on your bleeding-edge branch.

No need for that, I'll use the new versions.

--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

2014-04-29 21:21:30

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [Patch V2 1/9] ACPICA: Executer: Fix buffer allocation issue for generic_serial_bus region field accesses.

On Tuesday, April 29, 2014 01:31:28 PM Wolfram Sang wrote:
>
> --3MwIy2ne0vdjdPXF
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
>
>
> > The rest has been queued up for 3.16.
>
> I also aim for 3.16, yet it may take 1 or 2 weeks more until I'll be
> able to review the I2C part of those patches.

That's fine, they can stay in my bleeding-edge branch till then.

--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.