[PATCH v2] ACPI: surface3_power: MSHW0011 rev-eng implementation

[PATCH v2] ACPI: surface3_power: MSHW0011 rev-eng implementation

[Benjamin Tissoires]

MSHW0011 replaces the battery firmware by using ACPI operation regions.
The values have been obtained by reverse engineering, and are subject to
errors. Looks like it works on overall pretty well.

I couldn't manage to get the IRQ correctly triggered, so I am using a
good old polling thread to check for changes.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=106231

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Signed-off-by: Stephen Just <stephenjust@gmail.com>

---

Hi guys,

sorry for taking so long providing a v2 for this one, but it took me a while
to understand the acpica process and to get a proper PR done and merged[1].
(I must confess this felt through the crack too)
With ACPICA PR #277 merged, we now just need to have this patch included to
have proper battery reporting on the Surface 3.

I have included all the reviews Sebastian, Mika and Andy gave.

Cheers,
Benjamin

[1] https://github.com/acpica/acpica/pull/277

changes in v2:
- moved to drivers/acpi/ instead of power
- use uuid_le (guuid even)
- fix uper/lower case
- print_hex_dump() used in mshw0011_dump_registers() instead of custom dump
- removed "MSHW0011:00" in mshw0011_id
---
 drivers/acpi/Kconfig          |   6 +
 drivers/acpi/Makefile         |   2 +
 drivers/acpi/surface3_power.c | 702 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 710 insertions(+)
 create mode 100644 drivers/acpi/surface3_power.c

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 1ce52f8..ac29170 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -488,6 +488,12 @@ config ACPI_EXTLOG
 	  driver adds support for that functionality with corresponding
 	  tracepoint which carries that information to userspace.
 
+config ACPI_SURFACE3_POWER_OPREGION
+	tristate "Surface 3 battery platform operation region support"
+	help
+	  Select this option to enable support for ACPI operation
+	  region of the Surface 3 battery platform driver.
+
 menuconfig PMIC_OPREGION
 	bool "PMIC (Power Management Integrated Circuit) operation region support"
 	help
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index b1aacfc..459a1aa 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -98,6 +98,8 @@ obj-$(CONFIG_ACPI_APEI)		+= apei/
 
 obj-$(CONFIG_ACPI_EXTLOG)	+= acpi_extlog.o
 
+obj-$(CONFIG_ACPI_SURFACE3_POWER_OPREGION) += surface3_power.o
+
 obj-$(CONFIG_PMIC_OPREGION)	+= pmic/intel_pmic.o
 obj-$(CONFIG_CRC_PMIC_OPREGION) += pmic/intel_pmic_crc.o
 obj-$(CONFIG_XPOWER_PMIC_OPREGION) += pmic/intel_pmic_xpower.o
diff --git a/drivers/acpi/surface3_power.c b/drivers/acpi/surface3_power.c
new file mode 100644
index 0000000..6d59c7f
--- /dev/null
+++ b/drivers/acpi/surface3_power.c
@@ -0,0 +1,702 @@
+/*
+ * Supports for the power IC on the Surface 3 tablet.
+ *
+ * (C) Copyright 2016-2017 Red Hat, Inc
+ * (C) Copyright 2016-2017 Benjamin Tissoires <benjamin.tissoires@gmail.com>
+ * (C) Copyright 2016 Stephen Just <stephenjust@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; version 2
+ * of the License.
+ */
+
+/*
+ * This driver has been reverse-engineered by parsing the DSDT of the Surface 3
+ * and looking at the registers of the chips.
+ *
+ * The DSDT allowed to find out that:
+ * - the driver is required for the ACPI BAT0 device to communicate to the chip
+ *   through an operation region.
+ * - the various defines for the operation region functions to communicate with
+ *   this driver
+ * - the DSM 3f99e367-6220-4955-8b0f-06ef2ae79412 allows to trigger ACPI
+ *   events to BAT0 (the code is all available in the DSDT).
+ *
+ * Further findings regarding the 2 chips declared in the MSHW0011 are:
+ * - there are 2 chips declared:
+ *   . 0x22 seems to control the ADP1 line status (and probably the charger)
+ *   . 0x55 controls the battery directly
+ * - the battery chip uses a SMBus protocol (using plain SMBus allows non
+ *   destructive commands):
+ *   . the commands/registers used are in the range 0x00..0x7F
+ *   . if bit 8 (0x80) is set in the SMBus command, the returned value is the
+ *     same as when it is not set. There is a high chance this bit is the
+ *     read/write
+ *   . the various registers semantic as been deduced by observing the register
+ *     dumps.
+ */
+
+#include <asm/unaligned.h>
+#include <linux/acpi.h>
+#include <linux/freezer.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/kthread.h>
+#include <linux/slab.h>
+#include <linux/uuid.h>
+
+#define POLL_INTERVAL		(2 * HZ)
+
+static bool dump_registers;
+module_param_named(dump_registers, dump_registers, bool, 0644);
+MODULE_PARM_DESC(dump_registers,
+		 "Dump the SMBus register at probe (debugging only).");
+
+struct mshw0011_data {
+	struct i2c_client	*adp1;
+	struct i2c_client	*bat0;
+	unsigned short		notify_version;
+	struct task_struct	*poll_task;
+	bool			kthread_running;
+
+	bool			charging;
+	bool			bat_charging;
+	u8			trip_point;
+	s32			full_capacity;
+};
+
+struct mshw0011_lookup {
+	struct mshw0011_data	*cdata;
+	unsigned int		n;
+	unsigned int		index;
+	int			addr;
+};
+
+struct mshw0011_handler_data {
+	struct acpi_connection_info	info;
+	struct i2c_client		*client;
+};
+
+struct bix {
+	u32	revision;
+	u32	power_unit;
+	u32	design_capacity;
+	u32	last_full_charg_capacity;
+	u32	battery_technology;
+	u32	design_voltage;
+	u32	design_capacity_of_warning;
+	u32	design_capacity_of_low;
+	u32	cycle_count;
+	u32	measurement_accuracy;
+	u32	max_sampling_time;
+	u32	min_sampling_time;
+	u32	max_average_interval;
+	u32	min_average_interval;
+	u32	battery_capacity_granularity_1;
+	u32	battery_capacity_granularity_2;
+	char	model[10];
+	char	serial[10];
+	char	type[10];
+	char	OEM[10];
+} __packed;
+
+struct bst {
+	u32	battery_state;
+	s32	battery_present_rate;
+	u32	battery_remaining_capacity;
+	u32	battery_present_voltage;
+} __packed;
+
+struct gsb_command {
+	u8	arg0;
+	u8	arg1;
+	u8	arg2;
+} __packed;
+
+struct gsb_buffer {
+	u8	status;
+	u8	len;
+	u8	ret;
+	union {
+		struct gsb_command	cmd;
+		struct bst		bst;
+		struct bix		bix;
+	} __packed;
+} __packed;
+
+#define ACPI_BATTERY_STATE_DISCHARGING	0x1
+#define ACPI_BATTERY_STATE_CHARGING	0x2
+#define ACPI_BATTERY_STATE_CRITICAL	0x4
+
+#define MSHW0011_CMD_DEST_BAT0		0x01
+#define MSHW0011_CMD_DEST_ADP1		0x03
+
+#define MSHW0011_CMD_BAT0_STA		0x01
+#define MSHW0011_CMD_BAT0_BIX		0x02
+#define MSHW0011_CMD_BAT0_BCT		0x03
+#define MSHW0011_CMD_BAT0_BTM		0x04
+#define MSHW0011_CMD_BAT0_BST		0x05
+#define MSHW0011_CMD_BAT0_BTP		0x06
+#define MSHW0011_CMD_ADP1_PSR		0x07
+#define MSHW0011_CMD_BAT0_PSOC		0x09
+#define MSHW0011_CMD_BAT0_PMAX		0x0a
+#define MSHW0011_CMD_BAT0_PSRC		0x0b
+#define MSHW0011_CMD_BAT0_CHGI		0x0c
+#define MSHW0011_CMD_BAT0_ARTG		0x0d
+
+#define MSHW0011_NOTIFY_GET_VERSION	0x00
+#define MSHW0011_NOTIFY_ADP1		0x01
+#define MSHW0011_NOTIFY_BAT0_BST	0x02
+#define MSHW0011_NOTIFY_BAT0_BIX	0x05
+
+#define MSHW0011_ADP1_REG_PSR		0x04
+
+#define MSHW0011_BAT0_REG_CAPACITY		0x0c
+#define MSHW0011_BAT0_REG_FULL_CHG_CAPACITY	0x0e
+#define MSHW0011_BAT0_REG_DESIGN_CAPACITY	0x40
+#define MSHW0011_BAT0_REG_VOLTAGE	0x08
+#define MSHW0011_BAT0_REG_RATE		0x14
+#define MSHW0011_BAT0_REG_OEM		0x45
+#define MSHW0011_BAT0_REG_TYPE		0x4e
+#define MSHW0011_BAT0_REG_SERIAL_NO	0x56
+#define MSHW0011_BAT0_REG_CYCLE_CNT	0x6e
+
+#define MSHW0011_EV_2_5			0x1ff
+
+static int mshw0011_i2c_read_block(struct i2c_client *client, u8 reg, u8 *buf,
+				   int len)
+{
+	int status, i;
+
+	for (i = 0; i < len; i++) {
+		status = i2c_smbus_read_byte_data(client, reg + i);
+		if (status < 0) {
+			buf[i] = 0xff;
+			continue;
+		}
+
+		buf[i] = (u8)status;
+	}
+
+	return 0;
+}
+
+static int
+mshw0011_notify(struct mshw0011_data *cdata, u8 arg1, u8 arg2,
+		unsigned int *ret_value)
+{
+	static const uuid_le mshw0011_guid =
+		GUID_INIT(0x3F99E367, 0x6220, 0x4955,
+			  0x8B, 0x0F, 0x06, 0xEF, 0x2A, 0xE7, 0x94, 0x12);
+	union acpi_object *obj;
+	struct acpi_device *adev;
+	acpi_handle handle;
+	unsigned int i;
+
+	handle = ACPI_HANDLE(&cdata->adp1->dev);
+	if (!handle || acpi_bus_get_device(handle, &adev))
+		return -ENODEV;
+
+	obj = acpi_evaluate_dsm_typed(handle, &mshw0011_guid, arg1, arg2, NULL,
+				      ACPI_TYPE_BUFFER);
+	if (!obj) {
+		dev_err(&cdata->adp1->dev, "device _DSM execution failed\n");
+		return -ENODEV;
+	}
+
+	*ret_value = 0;
+	for (i = 0; i < obj->buffer.length; i++)
+		*ret_value |= obj->buffer.pointer[i] << (i * 8);
+
+	ACPI_FREE(obj);
+	return 0;
+}
+
+static const struct bix default_bix = {
+	.revision = 0x00,
+	.power_unit = 0x01,
+	.design_capacity = 0x1dca,
+	.last_full_charg_capacity = 0x1dca,
+	.battery_technology = 0x01,
+	.design_voltage = 0x10df,
+	.design_capacity_of_warning = 0x8f,
+	.design_capacity_of_low = 0x47,
+	.cycle_count = 0xffffffff,
+	.measurement_accuracy = 0x00015f90,
+	.max_sampling_time = 0x03e8,
+	.min_sampling_time = 0x03e8,
+	.max_average_interval = 0x03e8,
+	.min_average_interval = 0x03e8,
+	.battery_capacity_granularity_1 = 0x45,
+	.battery_capacity_granularity_2 = 0x11,
+	.model = "P11G8M",
+	.serial = "",
+	.type = "LION",
+	.OEM = "",
+};
+
+static int mshw0011_bix(struct mshw0011_data *cdata, struct bix *bix)
+{
+	struct i2c_client *client = cdata->bat0;
+	char buf[10];
+	int ret;
+
+	*bix = default_bix;
+
+	/* get design capacity */
+	ret = i2c_smbus_read_word_data(client,
+				       MSHW0011_BAT0_REG_DESIGN_CAPACITY);
+	if (ret < 0) {
+		dev_err(&client->dev, "Error reading design capacity: %d\n",
+			ret);
+		return ret;
+	}
+	bix->design_capacity = le16_to_cpu(ret);
+
+	/* get last full charge capacity */
+	ret = i2c_smbus_read_word_data(client,
+				       MSHW0011_BAT0_REG_FULL_CHG_CAPACITY);
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"Error reading last full charge capacity: %d\n", ret);
+		return ret;
+	}
+	bix->last_full_charg_capacity = le16_to_cpu(ret);
+
+	/* get serial number */
+	ret = mshw0011_i2c_read_block(client, MSHW0011_BAT0_REG_SERIAL_NO,
+				      buf, 10);
+	if (ret) {
+		dev_err(&client->dev, "Error reading serial no: %d\n", ret);
+		return ret;
+	}
+	memcpy(bix->serial, buf + 7, 3);
+	memcpy(bix->serial + 3, buf, 6);
+	bix->serial[9] = '\0';
+
+	/* get cycle count */
+	ret = i2c_smbus_read_word_data(client, MSHW0011_BAT0_REG_CYCLE_CNT);
+	if (ret < 0) {
+		dev_err(&client->dev, "Error reading cycle count: %d\n", ret);
+		return ret;
+	}
+	bix->cycle_count = le16_to_cpu(ret);
+
+	/* get OEM name */
+	ret = mshw0011_i2c_read_block(client, MSHW0011_BAT0_REG_OEM, buf, 4);
+	if (ret) {
+		dev_err(&client->dev, "Error reading cycle count: %d\n", ret);
+		return ret;
+	}
+	memcpy(bix->OEM, buf, 3);
+	bix->OEM[4] = '\0';
+
+	return 0;
+}
+
+static int mshw0011_bst(struct mshw0011_data *cdata, struct bst *bst)
+{
+	struct i2c_client *client = cdata->bat0;
+	int rate, capacity, voltage, state;
+	s16 tmp;
+
+	rate = i2c_smbus_read_word_data(client, MSHW0011_BAT0_REG_RATE);
+	if (rate < 0)
+		return rate;
+
+	capacity = i2c_smbus_read_word_data(client, MSHW0011_BAT0_REG_CAPACITY);
+	if (capacity < 0)
+		return capacity;
+
+	voltage = i2c_smbus_read_word_data(client, MSHW0011_BAT0_REG_VOLTAGE);
+	if (voltage < 0)
+		return voltage;
+
+	tmp = le16_to_cpu(rate);
+	bst->battery_present_rate = abs((s32)tmp);
+
+	state = 0;
+	if ((s32) tmp > 0)
+		state |= ACPI_BATTERY_STATE_CHARGING;
+	else if ((s32) tmp < 0)
+		state |= ACPI_BATTERY_STATE_DISCHARGING;
+	bst->battery_state = state;
+
+	bst->battery_remaining_capacity = le16_to_cpu(capacity);
+	bst->battery_present_voltage = le16_to_cpu(voltage);
+
+	return 0;
+}
+
+static int mshw0011_adp_psr(struct mshw0011_data *cdata)
+{
+	struct i2c_client *client = cdata->adp1;
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(client, MSHW0011_ADP1_REG_PSR);
+	if (ret < 0)
+		return ret;
+
+	return ret;
+}
+
+static int mshw0011_isr(struct mshw0011_data *cdata)
+{
+	struct bst bst;
+	struct bix bix;
+	int ret;
+	bool status, bat_status;
+
+	ret = mshw0011_adp_psr(cdata);
+	if (ret < 0)
+		return ret;
+
+	status = ret;
+
+	if (status != cdata->charging)
+		mshw0011_notify(cdata, cdata->notify_version,
+				MSHW0011_NOTIFY_ADP1, &ret);
+
+	cdata->charging = status;
+
+	ret = mshw0011_bst(cdata, &bst);
+	if (ret < 0)
+		return ret;
+
+	bat_status = bst.battery_state;
+
+	if (bat_status != cdata->bat_charging)
+		mshw0011_notify(cdata, cdata->notify_version,
+				MSHW0011_NOTIFY_BAT0_BST, &ret);
+
+	cdata->bat_charging = bat_status;
+
+	ret = mshw0011_bix(cdata, &bix);
+	if (ret < 0)
+		return ret;
+	if (bix.last_full_charg_capacity != cdata->full_capacity)
+		mshw0011_notify(cdata, cdata->notify_version,
+				MSHW0011_NOTIFY_BAT0_BIX, &ret);
+
+	cdata->full_capacity = bix.last_full_charg_capacity;
+
+	return 0;
+}
+
+static int mshw0011_poll_task(void *data)
+{
+	struct mshw0011_data *cdata = data;
+	int ret = 0;
+
+	cdata->kthread_running = true;
+
+	set_freezable();
+
+	while (!kthread_should_stop()) {
+		schedule_timeout_interruptible(POLL_INTERVAL);
+		try_to_freeze();
+		ret = mshw0011_isr(data);
+		if (ret)
+			break;
+	}
+
+	cdata->kthread_running = false;
+	return ret;
+}
+
+static acpi_status
+mshw0011_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 mshw0011_handler_data *data = handler_context;
+	struct acpi_connection_info *info = &data->info;
+	struct acpi_resource_i2c_serialbus *sb;
+	struct i2c_client *client = data->client;
+	struct mshw0011_data *cdata = i2c_get_clientdata(client);
+	struct acpi_resource *ares;
+	u32 accessor_type = function >> 16;
+	acpi_status ret;
+	int status = 1;
+
+	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;
+	}
+
+	if (accessor_type != ACPI_GSB_ACCESS_ATTRIB_RAW_PROCESS) {
+		ret = AE_BAD_PARAMETER;
+		goto err;
+	}
+
+	if (gsb->cmd.arg0 == MSHW0011_CMD_DEST_ADP1 &&
+	    gsb->cmd.arg1 == MSHW0011_CMD_ADP1_PSR) {
+		ret = mshw0011_adp_psr(cdata);
+		if (ret >= 0) {
+			status = ret;
+			ret = 0;
+		}
+		goto out;
+	}
+
+	if (gsb->cmd.arg0 != MSHW0011_CMD_DEST_BAT0) {
+		ret = AE_BAD_PARAMETER;
+		goto err;
+	}
+
+	switch (gsb->cmd.arg1) {
+	case MSHW0011_CMD_BAT0_STA:
+		ret = 0;
+		break;
+	case MSHW0011_CMD_BAT0_BIX:
+		ret = mshw0011_bix(cdata, &gsb->bix);
+		break;
+	case MSHW0011_CMD_BAT0_BTP:
+		ret = 0;
+		cdata->trip_point = gsb->cmd.arg2;
+		break;
+	case MSHW0011_CMD_BAT0_BST:
+		ret = mshw0011_bst(cdata, &gsb->bst);
+		break;
+	default:
+		pr_info("command(0x%02x) is not supported.\n", gsb->cmd.arg1);
+		ret = AE_BAD_PARAMETER;
+		goto err;
+	}
+
+ out:
+	gsb->ret = status;
+	gsb->status = 0;
+
+ err:
+	ACPI_FREE(ares);
+	return ret;
+}
+
+static int mshw0011_install_space_handler(struct i2c_client *client)
+{
+	acpi_handle handle;
+	struct mshw0011_handler_data *data;
+	acpi_status status;
+
+	handle = ACPI_HANDLE(&client->dev);
+
+	if (!handle)
+		return -ENODEV;
+
+	data = kzalloc(sizeof(struct mshw0011_handler_data),
+			    GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->client = client;
+	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,
+				&mshw0011_space_handler,
+				NULL,
+				data);
+	if (ACPI_FAILURE(status)) {
+		dev_err(&client->dev, "Error installing i2c space handler\n");
+		acpi_bus_detach_private_data(handle);
+		kfree(data);
+		return -ENOMEM;
+	}
+
+	acpi_walk_dep_device_list(handle);
+	return 0;
+}
+
+static void mshw0011_remove_space_handler(struct i2c_client *client)
+{
+	acpi_handle handle = ACPI_HANDLE(&client->dev);
+	struct mshw0011_handler_data *data;
+	acpi_status status;
+
+	if (!handle)
+		return;
+
+	acpi_remove_address_space_handler(handle,
+				ACPI_ADR_SPACE_GSBUS,
+				&mshw0011_space_handler);
+
+	status = acpi_bus_get_private_data(handle, (void **)&data);
+	if (ACPI_SUCCESS(status))
+		kfree(data);
+
+	acpi_bus_detach_private_data(handle);
+}
+
+static int acpi_find_i2c(struct acpi_resource *ares, void *data)
+{
+	struct mshw0011_lookup *lookup = data;
+
+	if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
+		return 1;
+
+	if (lookup->n++ == lookup->index && !lookup->addr)
+		lookup->addr = ares->data.i2c_serial_bus.slave_address;
+
+	return 1;
+}
+
+static int mshw0011_i2c_resource_lookup(struct mshw0011_data *cdata,
+					unsigned int index)
+{
+	struct i2c_client *client = cdata->adp1;
+	struct acpi_device *adev = ACPI_COMPANION(&client->dev);
+	struct mshw0011_lookup lookup = {
+		.cdata = cdata,
+		.index = index,
+	};
+	struct list_head res_list;
+	int ret;
+
+	INIT_LIST_HEAD(&res_list);
+
+	ret = acpi_dev_get_resources(adev, &res_list, acpi_find_i2c, &lookup);
+	if (ret < 0)
+		return ret;
+
+	acpi_dev_free_resource_list(&res_list);
+
+	if (!lookup.addr)
+		return -ENOENT;
+
+	return lookup.addr;
+}
+
+static void mshw0011_dump_registers(struct i2c_client *client,
+				    struct i2c_client *bat0, u8 end_register)
+{
+	char *rd_buf;
+	char prefix[128];
+	unsigned int length = end_register + 1;
+	int error;
+
+	snprintf(prefix, ARRAY_SIZE(prefix), "%s: ", bat0->name);
+	prefix[127] = '\0';
+
+	rd_buf = kzalloc(length, GFP_KERNEL);
+	error = mshw0011_i2c_read_block(bat0, 0, rd_buf, length);
+	print_hex_dump(KERN_INFO, prefix, DUMP_PREFIX_OFFSET, 16, 1,
+		       rd_buf, length, true);
+
+	kfree(rd_buf);
+}
+
+static int mshw0011_probe(struct i2c_client *client,
+			  const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	struct i2c_client *bat0;
+	struct mshw0011_data *data;
+	int error, version, addr;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->adp1 = client;
+	i2c_set_clientdata(client, data);
+
+	addr = mshw0011_i2c_resource_lookup(data, 1);
+	if (addr < 0)
+		return addr;
+
+	bat0 = i2c_new_dummy(client->adapter, addr);
+	if (!bat0)
+		return -ENOMEM;
+
+	data->bat0 = bat0;
+	i2c_set_clientdata(bat0, data);
+
+	if (dump_registers) {
+		mshw0011_dump_registers(client, client, 0xFF);
+		mshw0011_dump_registers(client, bat0, 0x80);
+	}
+
+	error = mshw0011_notify(data, 1, MSHW0011_NOTIFY_GET_VERSION, &version);
+	if (error)
+		goto out_err;
+
+	data->notify_version = version == MSHW0011_EV_2_5;
+
+	data->poll_task = kthread_run(mshw0011_poll_task, data, "mshw0011_adp");
+	if (IS_ERR(data->poll_task)) {
+		error = PTR_ERR(data->poll_task);
+		dev_err(&client->dev, "Unable to run kthread err %d\n", error);
+		goto out_err;
+	}
+
+	error = mshw0011_install_space_handler(client);
+	if (error)
+		goto out_err;
+
+	return 0;
+
+out_err:
+	if (data->kthread_running)
+		kthread_stop(data->poll_task);
+	i2c_unregister_device(data->bat0);
+	return error;
+}
+
+static int mshw0011_remove(struct i2c_client *client)
+{
+	struct mshw0011_data *cdata = i2c_get_clientdata(client);
+
+	mshw0011_remove_space_handler(client);
+
+	if (cdata->kthread_running)
+		kthread_stop(cdata->poll_task);
+
+	i2c_unregister_device(cdata->bat0);
+
+	return 0;
+}
+
+static const struct i2c_device_id mshw0011_id[] = {
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, mshw0011_id);
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id mshw0011_acpi_match[] = {
+	{ "MSHW0011", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, mshw0011_acpi_match);
+#endif
+
+static struct i2c_driver mshw0011_driver = {
+	.probe = mshw0011_probe,
+	.remove = mshw0011_remove,
+	.id_table = mshw0011_id,
+	.driver = {
+		.name = "mshw0011",
+		.acpi_match_table = ACPI_PTR(mshw0011_acpi_match),
+	},
+};
+module_i2c_driver(mshw0011_driver);
+
+MODULE_AUTHOR("Benjamin Tissoires <benjamin.tissoires@gmail.com>");
+MODULE_DESCRIPTION("mshw0011 driver");
+MODULE_LICENSE("GPL v2");
-- 
2.9.4

Re: [PATCH v2] ACPI: surface3_power: MSHW0011 rev-eng implementation

[Andy Shevchenko]

+Cc: Hans (he might give some advice regarding to the below)

On Thu, Jun 29, 2017 at 3:10 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> MSHW0011 replaces the battery firmware by using ACPI operation regions.
> The values have been obtained by reverse engineering, and are subject to
> errors. Looks like it works on overall pretty well.

What devices (laptops, tablets) have it?
Surface 3. What else?

> I couldn't manage to get the IRQ correctly triggered, so I am using a
> good old polling thread to check for changes.

It might be

>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=106231

> +config ACPI_SURFACE3_POWER_OPREGION
> +       tristate "Surface 3 battery platform operation region support"

depends on ACPI ?

> +       help
> +         Select this option to enable support for ACPI operation
> +         region of the Surface 3 battery platform driver.

> +/*
> + * Supports for the power IC on the Surface 3 tablet.

Shouldn't it go to drivers/acpi/pmic folder ?

And did you check if it have actual chip IP vendor name and model?
Most likely it's a TI (based?) solution.

> + */

> +/*
> + * Further findings regarding the 2 chips declared in the MSHW0011 are:
> + * - there are 2 chips declared:
> + *   . 0x22 seems to control the ADP1 line status (and probably the charger)
> + *   . 0x55 controls the battery directly
> + * - the battery chip uses a SMBus protocol (using plain SMBus allows non
> + *   destructive commands):
> + *   . the commands/registers used are in the range 0x00..0x7F
> + *   . if bit 8 (0x80) is set in the SMBus command, the returned value is the
> + *     same as when it is not set. There is a high chance this bit is the
> + *     read/write
> + *   . the various registers semantic as been deduced by observing the register
> + *     dumps.

All of this sounds familiar if look at what Hans discovered while was
doing INT33FE support.
Hans, does above ring any bell to you?

> + */

> +static bool dump_registers;
> +module_param_named(dump_registers, dump_registers, bool, 0644);
> +MODULE_PARM_DESC(dump_registers,
> +                "Dump the SMBus register at probe (debugging only).");

I'm not a fan of module parameter. Why not to use debugfs?

> +#define ACPI_BATTERY_STATE_DISCHARGING 0x1
> +#define ACPI_BATTERY_STATE_CHARGING    0x2
> +#define ACPI_BATTERY_STATE_CRITICAL    0x4

BIT() ?

> +#define MSHW0011_EV_2_5                        0x1ff

Is it mask? GENMASK() then.

> +
> +static int mshw0011_i2c_read_block(struct i2c_client *client, u8 reg, u8 *buf,
> +                                  int len)
> +{
> +       int status, i;
> +
> +       for (i = 0; i < len; i++) {
> +               status = i2c_smbus_read_byte_data(client, reg + i);
> +               if (status < 0) {
> +                       buf[i] = 0xff;
> +                       continue;
> +               }

Hmm... This looks weird. Why can you read entire block at once?

> +
> +               buf[i] = (u8)status;
> +       }
> +
> +       return 0;
> +}

> +static int
> +mshw0011_notify(struct mshw0011_data *cdata, u8 arg1, u8 arg2,
> +               unsigned int *ret_value)
> +{

> +       static const uuid_le mshw0011_guid =

guid_t, please :-)

> +               GUID_INIT(0x3F99E367, 0x6220, 0x4955,
> +                         0x8B, 0x0F, 0x06, 0xEF, 0x2A, 0xE7, 0x94, 0x12);

> +       *ret_value = 0;
> +       for (i = 0; i < obj->buffer.length; i++)
> +               *ret_value |= obj->buffer.pointer[i] << (i * 8);



> +
> +       ACPI_FREE(obj);
> +       return 0;
> +}

> +static int mshw0011_bix(struct mshw0011_data *cdata, struct bix *bix)
> +{

> +       memcpy(bix->serial, buf + 7, 3);
> +       memcpy(bix->serial + 3, buf, 6);
> +       bix->serial[9] = '\0';

snprintf()?

> +       bix->cycle_count = le16_to_cpu(ret);

non-x86 ?

> +       memcpy(bix->OEM, buf, 3);
> +       bix->OEM[4] = '\0';

snprintf() ?

> +
> +       return 0;
> +}

> +static int mshw0011_bst(struct mshw0011_data *cdata, struct bst *bst)
> +{
> +       struct i2c_client *client = cdata->bat0;
> +       int rate, capacity, voltage, state;
> +       s16 tmp;
> +
> +       rate = i2c_smbus_read_word_data(client, MSHW0011_BAT0_REG_RATE);
> +       if (rate < 0)
> +               return rate;
> +
> +       capacity = i2c_smbus_read_word_data(client, MSHW0011_BAT0_REG_CAPACITY);
> +       if (capacity < 0)
> +               return capacity;
> +
> +       voltage = i2c_smbus_read_word_data(client, MSHW0011_BAT0_REG_VOLTAGE);
> +       if (voltage < 0)
> +               return voltage;
> +

> +       tmp = le16_to_cpu(rate);

Do we need that on x86?

> +       bst->battery_present_rate = abs((s32)tmp);
> +
> +       state = 0;

> +       if ((s32) tmp > 0)

See above, perhaps using rate directly.

> +               state |= ACPI_BATTERY_STATE_CHARGING;
> +       else if ((s32) tmp < 0)

Ditto.

> +       bst->battery_remaining_capacity = le16_to_cpu(capacity);
> +       bst->battery_present_voltage = le16_to_cpu(voltage);

non-x86 ?

> +}
> +


ret = 0; ?
...
> +       switch (gsb->cmd.arg1) {
> +       case MSHW0011_CMD_BAT0_STA:

> +               ret = 0;

See above.

> +               break;
> +       case MSHW0011_CMD_BAT0_BIX:
> +               ret = mshw0011_bix(cdata, &gsb->bix);
> +               break;
> +       case MSHW0011_CMD_BAT0_BTP:

> +               ret = 0;

Ditto.

> +               cdata->trip_point = gsb->cmd.arg2;
> +               break;
> +       case MSHW0011_CMD_BAT0_BST:
> +               ret = mshw0011_bst(cdata, &gsb->bst);
> +               break;
> +       default:
> +               pr_info("command(0x%02x) is not supported.\n", gsb->cmd.arg1);
> +               ret = AE_BAD_PARAMETER;
> +               goto err;
> +       }
> +
> + out:
> +       gsb->ret = status;
> +       gsb->status = 0;
> +
> + err:
> +       ACPI_FREE(ares);
> +       return ret;
> +}
> +
> +static int mshw0011_install_space_handler(struct i2c_client *client)
> +{
> +       acpi_handle handle;
> +       struct mshw0011_handler_data *data;
> +       acpi_status status;
> +
> +       handle = ACPI_HANDLE(&client->dev);
> +
> +       if (!handle)
> +               return -ENODEV;
> +
> +       data = kzalloc(sizeof(struct mshw0011_handler_data),
> +                           GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       data->client = client;
> +       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,
> +                               &mshw0011_space_handler,
> +                               NULL,
> +                               data);
> +       if (ACPI_FAILURE(status)) {
> +               dev_err(&client->dev, "Error installing i2c space handler\n");
> +               acpi_bus_detach_private_data(handle);
> +               kfree(data);
> +               return -ENOMEM;
> +       }
> +
> +       acpi_walk_dep_device_list(handle);
> +       return 0;
> +}
> +
> +static void mshw0011_remove_space_handler(struct i2c_client *client)
> +{
> +       acpi_handle handle = ACPI_HANDLE(&client->dev);
> +       struct mshw0011_handler_data *data;
> +       acpi_status status;
> +
> +       if (!handle)
> +               return;
> +
> +       acpi_remove_address_space_handler(handle,
> +                               ACPI_ADR_SPACE_GSBUS,
> +                               &mshw0011_space_handler);
> +
> +       status = acpi_bus_get_private_data(handle, (void **)&data);
> +       if (ACPI_SUCCESS(status))
> +               kfree(data);
> +
> +       acpi_bus_detach_private_data(handle);
> +}
> +

> +static int acpi_find_i2c(struct acpi_resource *ares, void *data)
> +{
> +       struct mshw0011_lookup *lookup = data;
> +
> +       if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
> +               return 1;
> +
> +       if (lookup->n++ == lookup->index && !lookup->addr)
> +               lookup->addr = ares->data.i2c_serial_bus.slave_address;
> +
> +       return 1;
> +}
> +
> +static int mshw0011_i2c_resource_lookup(struct mshw0011_data *cdata,
> +                                       unsigned int index)
> +{
> +       struct i2c_client *client = cdata->adp1;
> +       struct acpi_device *adev = ACPI_COMPANION(&client->dev);
> +       struct mshw0011_lookup lookup = {
> +               .cdata = cdata,
> +               .index = index,
> +       };
> +       struct list_head res_list;
> +       int ret;
> +
> +       INIT_LIST_HEAD(&res_list);
> +
> +       ret = acpi_dev_get_resources(adev, &res_list, acpi_find_i2c, &lookup);
> +       if (ret < 0)
> +               return ret;
> +
> +       acpi_dev_free_resource_list(&res_list);
> +
> +       if (!lookup.addr)
> +               return -ENOENT;
> +
> +       return lookup.addr;
> +}

Strange you have above functions here. It's a copy paste from I2C
core. Please, think about way of deduplicating it.

> +static void mshw0011_dump_registers(struct i2c_client *client,
> +                                   struct i2c_client *bat0, u8 end_register)
> +{
> +       char *rd_buf;

> +       char prefix[128];

128 is too way big for a prefix.

> +       unsigned int length = end_register + 1;
> +       int error;
> +
> +       snprintf(prefix, ARRAY_SIZE(prefix), "%s: ", bat0->name);

> +       prefix[127] = '\0';

Why?

> +       rd_buf = kzalloc(length, GFP_KERNEL);

> +       error = mshw0011_i2c_read_block(bat0, 0, rd_buf, length);
> +       print_hex_dump(KERN_INFO, prefix, DUMP_PREFIX_OFFSET, 16, 1,
> +                      rd_buf, length, true);

If you switch to debugfs it makes things a bit more easier to handle I think.

> +
> +       kfree(rd_buf);
> +}

> +static int mshw0011_probe(struct i2c_client *client,
> +                         const struct i2c_device_id *id)
> +{

> +       data->notify_version = version == MSHW0011_EV_2_5;

0x1ff as version sounds hmm suspicious.

> +static const struct i2c_device_id mshw0011_id[] = {
> +       { }
> +};
> +MODULE_DEVICE_TABLE(i2c, mshw0011_id);

->probe_new(), please.

If I2C framework is _still_ broken we need to fix that part.

> +#ifdef CONFIG_ACPI

Is it going to be non-ACPI at all? See my proposal to Kconfig as well.

> +               .acpi_match_table = ACPI_PTR(mshw0011_acpi_match),

ACPI_PTR() might be gone

-- 
With Best Regards,
Andy Shevchenko

Re: [Devel] [PATCH v2] ACPI: surface3_power: MSHW0011 rev-eng implementation

[Andy Shevchenko]

[-- Attachment #1: Type: text/plain, Size: 10906 bytes --]

+Cc: Hans (he might give some advice regarding to the below)

On Thu, Jun 29, 2017 at 3:10 PM, Benjamin Tissoires
<benjamin.tissoires(a)redhat.com> wrote:
> MSHW0011 replaces the battery firmware by using ACPI operation regions.
> The values have been obtained by reverse engineering, and are subject to
> errors. Looks like it works on overall pretty well.

What devices (laptops, tablets) have it?
Surface 3. What else?

> I couldn't manage to get the IRQ correctly triggered, so I am using a
> good old polling thread to check for changes.

It might be

>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=106231

> +config ACPI_SURFACE3_POWER_OPREGION
> +       tristate "Surface 3 battery platform operation region support"

depends on ACPI ?

> +       help
> +         Select this option to enable support for ACPI operation
> +         region of the Surface 3 battery platform driver.

> +/*
> + * Supports for the power IC on the Surface 3 tablet.

Shouldn't it go to drivers/acpi/pmic folder ?

And did you check if it have actual chip IP vendor name and model?
Most likely it's a TI (based?) solution.

> + */

> +/*
> + * Further findings regarding the 2 chips declared in the MSHW0011 are:
> + * - there are 2 chips declared:
> + *   . 0x22 seems to control the ADP1 line status (and probably the charger)
> + *   . 0x55 controls the battery directly
> + * - the battery chip uses a SMBus protocol (using plain SMBus allows non
> + *   destructive commands):
> + *   . the commands/registers used are in the range 0x00..0x7F
> + *   . if bit 8 (0x80) is set in the SMBus command, the returned value is the
> + *     same as when it is not set. There is a high chance this bit is the
> + *     read/write
> + *   . the various registers semantic as been deduced by observing the register
> + *     dumps.

All of this sounds familiar if look at what Hans discovered while was
doing INT33FE support.
Hans, does above ring any bell to you?

> + */

> +static bool dump_registers;
> +module_param_named(dump_registers, dump_registers, bool, 0644);
> +MODULE_PARM_DESC(dump_registers,
> +                "Dump the SMBus register at probe (debugging only).");

I'm not a fan of module parameter. Why not to use debugfs?

> +#define ACPI_BATTERY_STATE_DISCHARGING 0x1
> +#define ACPI_BATTERY_STATE_CHARGING    0x2
> +#define ACPI_BATTERY_STATE_CRITICAL    0x4

BIT() ?

> +#define MSHW0011_EV_2_5                        0x1ff

Is it mask? GENMASK() then.

> +
> +static int mshw0011_i2c_read_block(struct i2c_client *client, u8 reg, u8 *buf,
> +                                  int len)
> +{
> +       int status, i;
> +
> +       for (i = 0; i < len; i++) {
> +               status = i2c_smbus_read_byte_data(client, reg + i);
> +               if (status < 0) {
> +                       buf[i] = 0xff;
> +                       continue;
> +               }

Hmm... This looks weird. Why can you read entire block at once?

> +
> +               buf[i] = (u8)status;
> +       }
> +
> +       return 0;
> +}

> +static int
> +mshw0011_notify(struct mshw0011_data *cdata, u8 arg1, u8 arg2,
> +               unsigned int *ret_value)
> +{

> +       static const uuid_le mshw0011_guid =

guid_t, please :-)

> +               GUID_INIT(0x3F99E367, 0x6220, 0x4955,
> +                         0x8B, 0x0F, 0x06, 0xEF, 0x2A, 0xE7, 0x94, 0x12);

> +       *ret_value = 0;
> +       for (i = 0; i < obj->buffer.length; i++)
> +               *ret_value |= obj->buffer.pointer[i] << (i * 8);



> +
> +       ACPI_FREE(obj);
> +       return 0;
> +}

> +static int mshw0011_bix(struct mshw0011_data *cdata, struct bix *bix)
> +{

> +       memcpy(bix->serial, buf + 7, 3);
> +       memcpy(bix->serial + 3, buf, 6);
> +       bix->serial[9] = '\0';

snprintf()?

> +       bix->cycle_count = le16_to_cpu(ret);

non-x86 ?

> +       memcpy(bix->OEM, buf, 3);
> +       bix->OEM[4] = '\0';

snprintf() ?

> +
> +       return 0;
> +}

> +static int mshw0011_bst(struct mshw0011_data *cdata, struct bst *bst)
> +{
> +       struct i2c_client *client = cdata->bat0;
> +       int rate, capacity, voltage, state;
> +       s16 tmp;
> +
> +       rate = i2c_smbus_read_word_data(client, MSHW0011_BAT0_REG_RATE);
> +       if (rate < 0)
> +               return rate;
> +
> +       capacity = i2c_smbus_read_word_data(client, MSHW0011_BAT0_REG_CAPACITY);
> +       if (capacity < 0)
> +               return capacity;
> +
> +       voltage = i2c_smbus_read_word_data(client, MSHW0011_BAT0_REG_VOLTAGE);
> +       if (voltage < 0)
> +               return voltage;
> +

> +       tmp = le16_to_cpu(rate);

Do we need that on x86?

> +       bst->battery_present_rate = abs((s32)tmp);
> +
> +       state = 0;

> +       if ((s32) tmp > 0)

See above, perhaps using rate directly.

> +               state |= ACPI_BATTERY_STATE_CHARGING;
> +       else if ((s32) tmp < 0)

Ditto.

> +       bst->battery_remaining_capacity = le16_to_cpu(capacity);
> +       bst->battery_present_voltage = le16_to_cpu(voltage);

non-x86 ?

> +}
> +


ret = 0; ?
...
> +       switch (gsb->cmd.arg1) {
> +       case MSHW0011_CMD_BAT0_STA:

> +               ret = 0;

See above.

> +               break;
> +       case MSHW0011_CMD_BAT0_BIX:
> +               ret = mshw0011_bix(cdata, &gsb->bix);
> +               break;
> +       case MSHW0011_CMD_BAT0_BTP:

> +               ret = 0;

Ditto.

> +               cdata->trip_point = gsb->cmd.arg2;
> +               break;
> +       case MSHW0011_CMD_BAT0_BST:
> +               ret = mshw0011_bst(cdata, &gsb->bst);
> +               break;
> +       default:
> +               pr_info("command(0x%02x) is not supported.\n", gsb->cmd.arg1);
> +               ret = AE_BAD_PARAMETER;
> +               goto err;
> +       }
> +
> + out:
> +       gsb->ret = status;
> +       gsb->status = 0;
> +
> + err:
> +       ACPI_FREE(ares);
> +       return ret;
> +}
> +
> +static int mshw0011_install_space_handler(struct i2c_client *client)
> +{
> +       acpi_handle handle;
> +       struct mshw0011_handler_data *data;
> +       acpi_status status;
> +
> +       handle = ACPI_HANDLE(&client->dev);
> +
> +       if (!handle)
> +               return -ENODEV;
> +
> +       data = kzalloc(sizeof(struct mshw0011_handler_data),
> +                           GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       data->client = client;
> +       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,
> +                               &mshw0011_space_handler,
> +                               NULL,
> +                               data);
> +       if (ACPI_FAILURE(status)) {
> +               dev_err(&client->dev, "Error installing i2c space handler\n");
> +               acpi_bus_detach_private_data(handle);
> +               kfree(data);
> +               return -ENOMEM;
> +       }
> +
> +       acpi_walk_dep_device_list(handle);
> +       return 0;
> +}
> +
> +static void mshw0011_remove_space_handler(struct i2c_client *client)
> +{
> +       acpi_handle handle = ACPI_HANDLE(&client->dev);
> +       struct mshw0011_handler_data *data;
> +       acpi_status status;
> +
> +       if (!handle)
> +               return;
> +
> +       acpi_remove_address_space_handler(handle,
> +                               ACPI_ADR_SPACE_GSBUS,
> +                               &mshw0011_space_handler);
> +
> +       status = acpi_bus_get_private_data(handle, (void **)&data);
> +       if (ACPI_SUCCESS(status))
> +               kfree(data);
> +
> +       acpi_bus_detach_private_data(handle);
> +}
> +

> +static int acpi_find_i2c(struct acpi_resource *ares, void *data)
> +{
> +       struct mshw0011_lookup *lookup = data;
> +
> +       if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
> +               return 1;
> +
> +       if (lookup->n++ == lookup->index && !lookup->addr)
> +               lookup->addr = ares->data.i2c_serial_bus.slave_address;
> +
> +       return 1;
> +}
> +
> +static int mshw0011_i2c_resource_lookup(struct mshw0011_data *cdata,
> +                                       unsigned int index)
> +{
> +       struct i2c_client *client = cdata->adp1;
> +       struct acpi_device *adev = ACPI_COMPANION(&client->dev);
> +       struct mshw0011_lookup lookup = {
> +               .cdata = cdata,
> +               .index = index,
> +       };
> +       struct list_head res_list;
> +       int ret;
> +
> +       INIT_LIST_HEAD(&res_list);
> +
> +       ret = acpi_dev_get_resources(adev, &res_list, acpi_find_i2c, &lookup);
> +       if (ret < 0)
> +               return ret;
> +
> +       acpi_dev_free_resource_list(&res_list);
> +
> +       if (!lookup.addr)
> +               return -ENOENT;
> +
> +       return lookup.addr;
> +}

Strange you have above functions here. It's a copy paste from I2C
core. Please, think about way of deduplicating it.

> +static void mshw0011_dump_registers(struct i2c_client *client,
> +                                   struct i2c_client *bat0, u8 end_register)
> +{
> +       char *rd_buf;

> +       char prefix[128];

128 is too way big for a prefix.

> +       unsigned int length = end_register + 1;
> +       int error;
> +
> +       snprintf(prefix, ARRAY_SIZE(prefix), "%s: ", bat0->name);

> +       prefix[127] = '\0';

Why?

> +       rd_buf = kzalloc(length, GFP_KERNEL);

> +       error = mshw0011_i2c_read_block(bat0, 0, rd_buf, length);
> +       print_hex_dump(KERN_INFO, prefix, DUMP_PREFIX_OFFSET, 16, 1,
> +                      rd_buf, length, true);

If you switch to debugfs it makes things a bit more easier to handle I think.

> +
> +       kfree(rd_buf);
> +}

> +static int mshw0011_probe(struct i2c_client *client,
> +                         const struct i2c_device_id *id)
> +{

> +       data->notify_version = version == MSHW0011_EV_2_5;

0x1ff as version sounds hmm suspicious.

> +static const struct i2c_device_id mshw0011_id[] = {
> +       { }
> +};
> +MODULE_DEVICE_TABLE(i2c, mshw0011_id);

->probe_new(), please.

If I2C framework is _still_ broken we need to fix that part.

> +#ifdef CONFIG_ACPI

Is it going to be non-ACPI at all? See my proposal to Kconfig as well.

> +               .acpi_match_table = ACPI_PTR(mshw0011_acpi_match),

ACPI_PTR() might be gone

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] ACPI: surface3_power: MSHW0011 rev-eng implementation

[Rafael J. Wysocki]

On Thu, Jun 29, 2017 at 4:22 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> +Cc: Hans (he might give some advice regarding to the below)
>
> On Thu, Jun 29, 2017 at 3:10 PM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
>> MSHW0011 replaces the battery firmware by using ACPI operation regions.
>> The values have been obtained by reverse engineering, and are subject to
>> errors. Looks like it works on overall pretty well.
>
> What devices (laptops, tablets) have it?
> Surface 3. What else?
>
>> I couldn't manage to get the IRQ correctly triggered, so I am using a
>> good old polling thread to check for changes.
>
> It might be
>
>>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=106231
>
>> +config ACPI_SURFACE3_POWER_OPREGION
>> +       tristate "Surface 3 battery platform operation region support"
>
> depends on ACPI ?
>
>> +       help
>> +         Select this option to enable support for ACPI operation
>> +         region of the Surface 3 battery platform driver.
>
>> +/*
>> + * Supports for the power IC on the Surface 3 tablet.
>
> Shouldn't it go to drivers/acpi/pmic folder ?

Surely not directly into drivers/acpi/ in any case.

Thanks,
Rafael

Re: [Devel] [PATCH v2] ACPI: surface3_power: MSHW0011 rev-eng implementation

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 1240 bytes --]

On Thu, Jun 29, 2017 at 4:22 PM, Andy Shevchenko
<andy.shevchenko(a)gmail.com> wrote:
> +Cc: Hans (he might give some advice regarding to the below)
>
> On Thu, Jun 29, 2017 at 3:10 PM, Benjamin Tissoires
> <benjamin.tissoires(a)redhat.com> wrote:
>> MSHW0011 replaces the battery firmware by using ACPI operation regions.
>> The values have been obtained by reverse engineering, and are subject to
>> errors. Looks like it works on overall pretty well.
>
> What devices (laptops, tablets) have it?
> Surface 3. What else?
>
>> I couldn't manage to get the IRQ correctly triggered, so I am using a
>> good old polling thread to check for changes.
>
> It might be
>
>>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=106231
>
>> +config ACPI_SURFACE3_POWER_OPREGION
>> +       tristate "Surface 3 battery platform operation region support"
>
> depends on ACPI ?
>
>> +       help
>> +         Select this option to enable support for ACPI operation
>> +         region of the Surface 3 battery platform driver.
>
>> +/*
>> + * Supports for the power IC on the Surface 3 tablet.
>
> Shouldn't it go to drivers/acpi/pmic folder ?

Surely not directly into drivers/acpi/ in any case.

Thanks,
Rafael

Re: [PATCH v2] ACPI: surface3_power: MSHW0011 rev-eng implementation

[Hans de Goede]

Hi,

On 29-06-17 16:22, Andy Shevchenko wrote:
> +Cc: Hans (he might give some advice regarding to the below)

Thank you for the Cc, so here we have the opposite situation as
with the devices with the AXP288 PMIC and the Cherry Trail
Whiskey Cove PMIC combined with the TI bq24292i charger and max17042
fuel-gauge. In those cases we have well working native drivers
for the used chips and we don't know the API of the custom ACPI
OpRegion the ACPI battery and ac interface implementing ACPI nodes
want. So for these devices we've disabled the ACPI battery and
ac drivers and are using power_supply drivers for the used chips
directly.

Here we've a similar setup where the ACPI nodes implementing the
ACPI battery and ac interfaces require a custom OpRegion, but
Benjamin has more or less figured out what the expected Opregion
API is (and implemented it) and we do not know which chips are
actually used.

Judging from the above I believe that implementing the ACPI
OpRegion support for the MSHW0011 devices is a good solution
in this case.

> On Thu, Jun 29, 2017 at 3:10 PM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
>> MSHW0011 replaces the battery firmware by using ACPI operation regions.
>> The values have been obtained by reverse engineering, and are subject to
>> errors. Looks like it works on overall pretty well.

<snip>

>> +/*
>> + * Further findings regarding the 2 chips declared in the MSHW0011 are:
>> + * - there are 2 chips declared:
>> + *   . 0x22 seems to control the ADP1 line status (and probably the charger)
>> + *   . 0x55 controls the battery directly
>> + * - the battery chip uses a SMBus protocol (using plain SMBus allows non
>> + *   destructive commands):
>> + *   . the commands/registers used are in the range 0x00..0x7F
>> + *   . if bit 8 (0x80) is set in the SMBus command, the returned value is the
>> + *     same as when it is not set. There is a high chance this bit is the
>> + *     read/write
>> + *   . the various registers semantic as been deduced by observing the register
>> + *     dumps.
> 
> All of this sounds familiar if look at what Hans discovered while was
> doing INT33FE support.
> Hans, does above ring any bell to you?

Familiar yes, but I actually already looked at this rev-eng driver while working
on my INT33FE support and it is for different chips, and I don't know for
which models exactly.

<snip>

>> +static int acpi_find_i2c(struct acpi_resource *ares, void *data)
>> +{
>> +       struct mshw0011_lookup *lookup = data;
>> +
>> +       if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
>> +               return 1;
>> +
>> +       if (lookup->n++ == lookup->index && !lookup->addr)
>> +               lookup->addr = ares->data.i2c_serial_bus.slave_address;
>> +
>> +       return 1;
>> +}
>> +
>> +static int mshw0011_i2c_resource_lookup(struct mshw0011_data *cdata,
>> +                                       unsigned int index)
>> +{
>> +       struct i2c_client *client = cdata->adp1;
>> +       struct acpi_device *adev = ACPI_COMPANION(&client->dev);
>> +       struct mshw0011_lookup lookup = {
>> +               .cdata = cdata,
>> +               .index = index,
>> +       };
>> +       struct list_head res_list;
>> +       int ret;
>> +
>> +       INIT_LIST_HEAD(&res_list);
>> +
>> +       ret = acpi_dev_get_resources(adev, &res_list, acpi_find_i2c, &lookup);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       acpi_dev_free_resource_list(&res_list);
>> +
>> +       if (!lookup.addr)
>> +               return -ENOENT;
>> +
>> +       return lookup.addr;
>> +}
> 
> Strange you have above functions here. It's a copy paste from I2C > core. Please, think about way of deduplicating it.

Right, this is something I can actually help with. When implementing the
INT33FE support (*) I also was dealing with an ACPI node with more then 1
I2cSerialBus type resource and I needed not just an i2c-client for the
first one (which the i2c-core gives us) but also for the others.

In 4.12 there is an i2c_acpi_new_device function which you can use
to create an i2c-client for the second i2c address you want to communicate
with, here is an example usage:

struct i2c_board_info board_info;

memset(&board_info, 0, sizeof(board_info));
strlcpy(board_info.type, "MSHW0011-bat0", I2C_NAME_SIZE);
bat0 = i2c_acpi_new_device(dev, 1, &board_info);

And then you can use bat0 to communicate to the other address,
as before, while dropping a whole bunch of copy-pasted code :)

Regards,

Hans



*) Well an implementation of INT33FE support really since there seem to be many
different INT33FE devices, each one for a different charger / fuel-gauge combi
and you can only differentiate which one you're actually dealing with by
checking the PTYPE APCI Object and my implementation only supports PTYPE 4

Re: [PATCH v2] ACPI: surface3_power: MSHW0011 rev-eng implementation

[Benjamin Tissoires]

On Jun 29 2017 or thereabouts, Rafael J. Wysocki wrote:
> On Thu, Jun 29, 2017 at 4:22 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > +Cc: Hans (he might give some advice regarding to the below)
> >
> > On Thu, Jun 29, 2017 at 3:10 PM, Benjamin Tissoires
> > <benjamin.tissoires@redhat.com> wrote:
> >> MSHW0011 replaces the battery firmware by using ACPI operation regions.
> >> The values have been obtained by reverse engineering, and are subject to
> >> errors. Looks like it works on overall pretty well.
> >
> > What devices (laptops, tablets) have it?
> > Surface 3. What else?
> >
> >> I couldn't manage to get the IRQ correctly triggered, so I am using a
> >> good old polling thread to check for changes.
> >
> > It might be
> >
> >>
> >> Link: https://bugzilla.kernel.org/show_bug.cgi?id=106231
> >
> >> +config ACPI_SURFACE3_POWER_OPREGION
> >> +       tristate "Surface 3 battery platform operation region support"
> >
> > depends on ACPI ?
> >
> >> +       help
> >> +         Select this option to enable support for ACPI operation
> >> +         region of the Surface 3 battery platform driver.
> >
> >> +/*
> >> + * Supports for the power IC on the Surface 3 tablet.
> >
> > Shouldn't it go to drivers/acpi/pmic folder ?
> 
> Surely not directly into drivers/acpi/ in any case.
> 

Yep, drivers/acpi/pmic seems like a good candidate. I will do that in
v3.

Cheers,
Benjamin

Re: [PATCH v2] ACPI: surface3_power: MSHW0011 rev-eng implementation

[Benjamin Tissoires]

On Jun 30 2017 or thereabouts, Hans de Goede wrote:
> Hi,
> 
> On 29-06-17 16:22, Andy Shevchenko wrote:
> > +Cc: Hans (he might give some advice regarding to the below)
> 
> Thank you for the Cc, so here we have the opposite situation as
> with the devices with the AXP288 PMIC and the Cherry Trail
> Whiskey Cove PMIC combined with the TI bq24292i charger and max17042
> fuel-gauge. In those cases we have well working native drivers
> for the used chips and we don't know the API of the custom ACPI
> OpRegion the ACPI battery and ac interface implementing ACPI nodes
> want. So for these devices we've disabled the ACPI battery and
> ac drivers and are using power_supply drivers for the used chips
> directly.
> 
> Here we've a similar setup where the ACPI nodes implementing the
> ACPI battery and ac interfaces require a custom OpRegion, but
> Benjamin has more or less figured out what the expected Opregion
> API is (and implemented it) and we do not know which chips are
> actually used.

Yeah, cracking open a Surface 3 voids the guarantee and it's a glue hell
from what I can understand. So no idea which chip is used in the end :(

> 
> Judging from the above I believe that implementing the ACPI
> OpRegion support for the MSHW0011 devices is a good solution
> in this case.

Thanks :)

> 
> > On Thu, Jun 29, 2017 at 3:10 PM, Benjamin Tissoires
> > <benjamin.tissoires@redhat.com> wrote:
> > > MSHW0011 replaces the battery firmware by using ACPI operation regions.
> > > The values have been obtained by reverse engineering, and are subject to
> > > errors. Looks like it works on overall pretty well.
> 
> <snip>
> 
> > > +/*
> > > + * Further findings regarding the 2 chips declared in the MSHW0011 are:
> > > + * - there are 2 chips declared:
> > > + *   . 0x22 seems to control the ADP1 line status (and probably the charger)
> > > + *   . 0x55 controls the battery directly
> > > + * - the battery chip uses a SMBus protocol (using plain SMBus allows non
> > > + *   destructive commands):
> > > + *   . the commands/registers used are in the range 0x00..0x7F
> > > + *   . if bit 8 (0x80) is set in the SMBus command, the returned value is the
> > > + *     same as when it is not set. There is a high chance this bit is the
> > > + *     read/write
> > > + *   . the various registers semantic as been deduced by observing the register
> > > + *     dumps.
> > 
> > All of this sounds familiar if look at what Hans discovered while was
> > doing INT33FE support.
> > Hans, does above ring any bell to you?
> 
> Familiar yes, but I actually already looked at this rev-eng driver while working
> on my INT33FE support and it is for different chips, and I don't know for
> which models exactly.
> 
> <snip>
> 
> > > +static int acpi_find_i2c(struct acpi_resource *ares, void *data)
> > > +{
> > > +       struct mshw0011_lookup *lookup = data;
> > > +
> > > +       if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
> > > +               return 1;
> > > +
> > > +       if (lookup->n++ == lookup->index && !lookup->addr)
> > > +               lookup->addr = ares->data.i2c_serial_bus.slave_address;
> > > +
> > > +       return 1;
> > > +}
> > > +
> > > +static int mshw0011_i2c_resource_lookup(struct mshw0011_data *cdata,
> > > +                                       unsigned int index)
> > > +{
> > > +       struct i2c_client *client = cdata->adp1;
> > > +       struct acpi_device *adev = ACPI_COMPANION(&client->dev);
> > > +       struct mshw0011_lookup lookup = {
> > > +               .cdata = cdata,
> > > +               .index = index,
> > > +       };
> > > +       struct list_head res_list;
> > > +       int ret;
> > > +
> > > +       INIT_LIST_HEAD(&res_list);
> > > +
> > > +       ret = acpi_dev_get_resources(adev, &res_list, acpi_find_i2c, &lookup);
> > > +       if (ret < 0)
> > > +               return ret;
> > > +
> > > +       acpi_dev_free_resource_list(&res_list);
> > > +
> > > +       if (!lookup.addr)
> > > +               return -ENOENT;
> > > +
> > > +       return lookup.addr;
> > > +}
> > 
> > Strange you have above functions here. It's a copy paste from I2C > core. Please, think about way of deduplicating it.
> 
> Right, this is something I can actually help with. When implementing the
> INT33FE support (*) I also was dealing with an ACPI node with more then 1
> I2cSerialBus type resource and I needed not just an i2c-client for the
> first one (which the i2c-core gives us) but also for the others.
> 
> In 4.12 there is an i2c_acpi_new_device function which you can use
> to create an i2c-client for the second i2c address you want to communicate
> with, here is an example usage:
> 
> struct i2c_board_info board_info;
> 
> memset(&board_info, 0, sizeof(board_info));
> strlcpy(board_info.type, "MSHW0011-bat0", I2C_NAME_SIZE);
> bat0 = i2c_acpi_new_device(dev, 1, &board_info);
> 
> And then you can use bat0 to communicate to the other address,
> as before, while dropping a whole bunch of copy-pasted code :)

Thanks for the tip. I wrote this code more than a year ago IIRC, and
there might not be those facilities at the time.
Anyway, I'll amend this in v3.

Cheers,
Benjamin

> 
> Regards,
> 
> Hans
> 
> 
> 
> *) Well an implementation of INT33FE support really since there seem to be many
> different INT33FE devices, each one for a different charger / fuel-gauge combi
> and you can only differentiate which one you're actually dealing with by
> checking the PTYPE APCI Object and my implementation only supports PTYPE 4
> 

Re: [PATCH v2] ACPI: surface3_power: MSHW0011 rev-eng implementation

[Hans de Goede]

HI,

On 30-06-17 17:24, Benjamin Tissoires wrote:
> On Jun 29 2017 or thereabouts, Rafael J. Wysocki wrote:
>> On Thu, Jun 29, 2017 at 4:22 PM, Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>> +Cc: Hans (he might give some advice regarding to the below)
>>>
>>> On Thu, Jun 29, 2017 at 3:10 PM, Benjamin Tissoires
>>> <benjamin.tissoires@redhat.com> wrote:
>>>> MSHW0011 replaces the battery firmware by using ACPI operation regions.
>>>> The values have been obtained by reverse engineering, and are subject to
>>>> errors. Looks like it works on overall pretty well.
>>>
>>> What devices (laptops, tablets) have it?
>>> Surface 3. What else?
>>>
>>>> I couldn't manage to get the IRQ correctly triggered, so I am using a
>>>> good old polling thread to check for changes.
>>>
>>> It might be
>>>
>>>>
>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=106231
>>>
>>>> +config ACPI_SURFACE3_POWER_OPREGION
>>>> +       tristate "Surface 3 battery platform operation region support"
>>>
>>> depends on ACPI ?
>>>
>>>> +       help
>>>> +         Select this option to enable support for ACPI operation
>>>> +         region of the Surface 3 battery platform driver.
>>>
>>>> +/*
>>>> + * Supports for the power IC on the Surface 3 tablet.
>>>
>>> Shouldn't it go to drivers/acpi/pmic folder ?
>>
>> Surely not directly into drivers/acpi/ in any case.
>>
> 
> Yep, drivers/acpi/pmic seems like a good candidate. I will do that in
> v3.

Sorry to add to the bikeshedding here, but IMHO drivers/acpi/pmic
is not a good location, that is for PMIC OpRegion drivers, and the
chips you're writing an OpRegion handler for are not PMICs they
are a charger and a fuel-gauge chip. As such I believe a better
location would be the catch all drivers/platform/x86 .

Anyways just my 2 cents if everyone else is happy with putting this
in drivers/acpi/pmic that is fine with me.

Regards,

Hans

Re: [PATCH v2] ACPI: surface3_power: MSHW0011 rev-eng implementation

[Hans de Goede]

Hi,

On 30-06-17 17:26, Benjamin Tissoires wrote:
> On Jun 30 2017 or thereabouts, Hans de Goede wrote:

<snip>

>>>> +static int mshw0011_i2c_resource_lookup(struct mshw0011_data *cdata,
>>>> +                                       unsigned int index)
>>>> +{
>>>> +       struct i2c_client *client = cdata->adp1;
>>>> +       struct acpi_device *adev = ACPI_COMPANION(&client->dev);
>>>> +       struct mshw0011_lookup lookup = {
>>>> +               .cdata = cdata,
>>>> +               .index = index,
>>>> +       };
>>>> +       struct list_head res_list;
>>>> +       int ret;
>>>> +
>>>> +       INIT_LIST_HEAD(&res_list);
>>>> +
>>>> +       ret = acpi_dev_get_resources(adev, &res_list, acpi_find_i2c, &lookup);
>>>> +       if (ret < 0)
>>>> +               return ret;
>>>> +
>>>> +       acpi_dev_free_resource_list(&res_list);
>>>> +
>>>> +       if (!lookup.addr)
>>>> +               return -ENOENT;
>>>> +
>>>> +       return lookup.addr;
>>>> +}
>>>
>>> Strange you have above functions here. It's a copy paste from I2C > core. Please, think about way of deduplicating it.
>>
>> Right, this is something I can actually help with. When implementing the
>> INT33FE support (*) I also was dealing with an ACPI node with more then 1
>> I2cSerialBus type resource and I needed not just an i2c-client for the
>> first one (which the i2c-core gives us) but also for the others.
>>
>> In 4.12 there is an i2c_acpi_new_device function which you can use
>> to create an i2c-client for the second i2c address you want to communicate
>> with, here is an example usage:
>>
>> struct i2c_board_info board_info;
>>
>> memset(&board_info, 0, sizeof(board_info));
>> strlcpy(board_info.type, "MSHW0011-bat0", I2C_NAME_SIZE);
>> bat0 = i2c_acpi_new_device(dev, 1, &board_info);
>>
>> And then you can use bat0 to communicate to the other address,
>> as before, while dropping a whole bunch of copy-pasted code :)
> 
> Thanks for the tip. I wrote this code more than a year ago IIRC, and
> there might not be those facilities at the time.

Correct I hit the same problem with the INT33FE device and decided
to fix this properly :)  The i2c_acpi_new_device function is new
in 4.12 .

Regards,

Hans

Re: [PATCH v2] ACPI: surface3_power: MSHW0011 rev-eng implementation

[Benjamin Tissoires]

Hi Andy,

Thanks for the review :)

On Jun 29 2017 or thereabouts, Andy Shevchenko wrote:
> +Cc: Hans (he might give some advice regarding to the below)
> 
> On Thu, Jun 29, 2017 at 3:10 PM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> > MSHW0011 replaces the battery firmware by using ACPI operation regions.
> > The values have been obtained by reverse engineering, and are subject to
> > errors. Looks like it works on overall pretty well.
> 
> What devices (laptops, tablets) have it?
> Surface 3. What else?

So far, Surface 3 only. It's a Microsoft PNPId, so I guess they control
which device has it. Maybe the model after the Surface 3 (reduced
platform) will have such chip, but for now, it's unknown.

> 
> > I couldn't manage to get the IRQ correctly triggered, so I am using a
> > good old polling thread to check for changes.
> 
> It might be
> 
> >
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=106231
> 
> > +config ACPI_SURFACE3_POWER_OPREGION
> > +       tristate "Surface 3 battery platform operation region support"
> 
> depends on ACPI ?

Good point

> 
> > +       help
> > +         Select this option to enable support for ACPI operation
> > +         region of the Surface 3 battery platform driver.
> 
> > +/*
> > + * Supports for the power IC on the Surface 3 tablet.
> 
> Shouldn't it go to drivers/acpi/pmic folder ?

Already answered later in the thread, so yes, I'll move it there.

> 
> And did you check if it have actual chip IP vendor name and model?
> Most likely it's a TI (based?) solution.

As mentioned, I have strictly no idea. I can not crack open the Surface
3 without breaking the warranty (I already had to return it once because
the disk crashed).

And I do not find anything brand-related under Windows either:
- it's called "Surface Platform Power Driver"
- and the driver is provided by Microsoft

> 
> > + */
> 
> > +/*
> > + * Further findings regarding the 2 chips declared in the MSHW0011 are:
> > + * - there are 2 chips declared:
> > + *   . 0x22 seems to control the ADP1 line status (and probably the charger)
> > + *   . 0x55 controls the battery directly
> > + * - the battery chip uses a SMBus protocol (using plain SMBus allows non
> > + *   destructive commands):
> > + *   . the commands/registers used are in the range 0x00..0x7F
> > + *   . if bit 8 (0x80) is set in the SMBus command, the returned value is the
> > + *     same as when it is not set. There is a high chance this bit is the
> > + *     read/write
> > + *   . the various registers semantic as been deduced by observing the register
> > + *     dumps.
> 
> All of this sounds familiar if look at what Hans discovered while was
> doing INT33FE support.
> Hans, does above ring any bell to you?
> 
> > + */
> 
> > +static bool dump_registers;
> > +module_param_named(dump_registers, dump_registers, bool, 0644);
> > +MODULE_PARM_DESC(dump_registers,
> > +                "Dump the SMBus register at probe (debugging only).");
> 
> I'm not a fan of module parameter. Why not to use debugfs?

We can probably remove this entirely actually. We used this for reverse
engineering, but now I think it's time for it to be removed.

> 
> > +#define ACPI_BATTERY_STATE_DISCHARGING 0x1
> > +#define ACPI_BATTERY_STATE_CHARGING    0x2
> > +#define ACPI_BATTERY_STATE_CRITICAL    0x4
> 
> BIT() ?

Yep.

> 
> > +#define MSHW0011_EV_2_5                        0x1ff
> 
> Is it mask? GENMASK() then.

I have strictly no idea (anymore) :)
I wrote this code too long ago, and I can't remember what this was.
Looking at the other piece of code that uses it, I am not so sure :/

> 
> > +
> > +static int mshw0011_i2c_read_block(struct i2c_client *client, u8 reg, u8 *buf,
> > +                                  int len)
> > +{
> > +       int status, i;
> > +
> > +       for (i = 0; i < len; i++) {
> > +               status = i2c_smbus_read_byte_data(client, reg + i);
> > +               if (status < 0) {
> > +                       buf[i] = 0xff;
> > +                       continue;
> > +               }
> 
> Hmm... This looks weird. Why can you read entire block at once?

Yeah, looks like the Windows driver reads up to 32 bytes at a time. So
we should be able to have the same here.

> 
> > +
> > +               buf[i] = (u8)status;
> > +       }
> > +
> > +       return 0;
> > +}
> 
> > +static int
> > +mshw0011_notify(struct mshw0011_data *cdata, u8 arg1, u8 arg2,
> > +               unsigned int *ret_value)
> > +{
> 
> > +       static const uuid_le mshw0011_guid =
> 
> guid_t, please :-)

oops :)

> 
> > +               GUID_INIT(0x3F99E367, 0x6220, 0x4955,
> > +                         0x8B, 0x0F, 0x06, 0xEF, 0x2A, 0xE7, 0x94, 0x12);
> 
> > +       *ret_value = 0;
> > +       for (i = 0; i < obj->buffer.length; i++)
> > +               *ret_value |= obj->buffer.pointer[i] << (i * 8);
> 
> 
> 
> > +
> > +       ACPI_FREE(obj);
> > +       return 0;
> > +}
> 
> > +static int mshw0011_bix(struct mshw0011_data *cdata, struct bix *bix)
> > +{
> 
> > +       memcpy(bix->serial, buf + 7, 3);
> > +       memcpy(bix->serial + 3, buf, 6);
> > +       bix->serial[9] = '\0';
> 
> snprintf()?

probably :)

> 
> > +       bix->cycle_count = le16_to_cpu(ret);
> 
> non-x86 ?

Well, nothing prevents this chip to be used on arm64 also, so I'd rather
have no-op operations but be big endian friendly.

> 
> > +       memcpy(bix->OEM, buf, 3);
> > +       bix->OEM[4] = '\0';
> 
> snprintf() ?
> 
> > +
> > +       return 0;
> > +}
> 
> > +static int mshw0011_bst(struct mshw0011_data *cdata, struct bst *bst)
> > +{
> > +       struct i2c_client *client = cdata->bat0;
> > +       int rate, capacity, voltage, state;
> > +       s16 tmp;
> > +
> > +       rate = i2c_smbus_read_word_data(client, MSHW0011_BAT0_REG_RATE);
> > +       if (rate < 0)
> > +               return rate;
> > +
> > +       capacity = i2c_smbus_read_word_data(client, MSHW0011_BAT0_REG_CAPACITY);
> > +       if (capacity < 0)
> > +               return capacity;
> > +
> > +       voltage = i2c_smbus_read_word_data(client, MSHW0011_BAT0_REG_VOLTAGE);
> > +       if (voltage < 0)
> > +               return voltage;
> > +
> 
> > +       tmp = le16_to_cpu(rate);
> 
> Do we need that on x86?
> 
> > +       bst->battery_present_rate = abs((s32)tmp);
> > +
> > +       state = 0;
> 
> > +       if ((s32) tmp > 0)
> 
> See above, perhaps using rate directly.
> 
> > +               state |= ACPI_BATTERY_STATE_CHARGING;
> > +       else if ((s32) tmp < 0)
> 
> Ditto.
> 
> > +       bst->battery_remaining_capacity = le16_to_cpu(capacity);
> > +       bst->battery_present_voltage = le16_to_cpu(voltage);
> 
> non-x86 ?

For all these, I'd rather keep the le16_to_cpu calls. Simply because
ACPI is not x86 only :)

> 
> > +}
> > +
> 
> 
> ret = 0; ?
> ...
> > +       switch (gsb->cmd.arg1) {
> > +       case MSHW0011_CMD_BAT0_STA:
> 
> > +               ret = 0;
> 
> See above.

Works too :)

> 
> > +               break;
> > +       case MSHW0011_CMD_BAT0_BIX:
> > +               ret = mshw0011_bix(cdata, &gsb->bix);
> > +               break;
> > +       case MSHW0011_CMD_BAT0_BTP:
> 
> > +               ret = 0;
> 
> Ditto.
> 
> > +               cdata->trip_point = gsb->cmd.arg2;
> > +               break;
> > +       case MSHW0011_CMD_BAT0_BST:
> > +               ret = mshw0011_bst(cdata, &gsb->bst);
> > +               break;
> > +       default:
> > +               pr_info("command(0x%02x) is not supported.\n", gsb->cmd.arg1);
> > +               ret = AE_BAD_PARAMETER;
> > +               goto err;
> > +       }
> > +
> > + out:
> > +       gsb->ret = status;
> > +       gsb->status = 0;
> > +
> > + err:
> > +       ACPI_FREE(ares);
> > +       return ret;
> > +}
> > +
> > +static int mshw0011_install_space_handler(struct i2c_client *client)
> > +{
> > +       acpi_handle handle;
> > +       struct mshw0011_handler_data *data;
> > +       acpi_status status;
> > +
> > +       handle = ACPI_HANDLE(&client->dev);
> > +
> > +       if (!handle)
> > +               return -ENODEV;
> > +
> > +       data = kzalloc(sizeof(struct mshw0011_handler_data),
> > +                           GFP_KERNEL);
> > +       if (!data)
> > +               return -ENOMEM;
> > +
> > +       data->client = client;
> > +       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,
> > +                               &mshw0011_space_handler,
> > +                               NULL,
> > +                               data);
> > +       if (ACPI_FAILURE(status)) {
> > +               dev_err(&client->dev, "Error installing i2c space handler\n");
> > +               acpi_bus_detach_private_data(handle);
> > +               kfree(data);
> > +               return -ENOMEM;
> > +       }
> > +
> > +       acpi_walk_dep_device_list(handle);
> > +       return 0;
> > +}
> > +
> > +static void mshw0011_remove_space_handler(struct i2c_client *client)
> > +{
> > +       acpi_handle handle = ACPI_HANDLE(&client->dev);
> > +       struct mshw0011_handler_data *data;
> > +       acpi_status status;
> > +
> > +       if (!handle)
> > +               return;
> > +
> > +       acpi_remove_address_space_handler(handle,
> > +                               ACPI_ADR_SPACE_GSBUS,
> > +                               &mshw0011_space_handler);
> > +
> > +       status = acpi_bus_get_private_data(handle, (void **)&data);
> > +       if (ACPI_SUCCESS(status))
> > +               kfree(data);
> > +
> > +       acpi_bus_detach_private_data(handle);
> > +}
> > +
> 
> > +static int acpi_find_i2c(struct acpi_resource *ares, void *data)
> > +{
> > +       struct mshw0011_lookup *lookup = data;
> > +
> > +       if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
> > +               return 1;
> > +
> > +       if (lookup->n++ == lookup->index && !lookup->addr)
> > +               lookup->addr = ares->data.i2c_serial_bus.slave_address;
> > +
> > +       return 1;
> > +}
> > +
> > +static int mshw0011_i2c_resource_lookup(struct mshw0011_data *cdata,
> > +                                       unsigned int index)
> > +{
> > +       struct i2c_client *client = cdata->adp1;
> > +       struct acpi_device *adev = ACPI_COMPANION(&client->dev);
> > +       struct mshw0011_lookup lookup = {
> > +               .cdata = cdata,
> > +               .index = index,
> > +       };
> > +       struct list_head res_list;
> > +       int ret;
> > +
> > +       INIT_LIST_HEAD(&res_list);
> > +
> > +       ret = acpi_dev_get_resources(adev, &res_list, acpi_find_i2c, &lookup);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       acpi_dev_free_resource_list(&res_list);
> > +
> > +       if (!lookup.addr)
> > +               return -ENOENT;
> > +
> > +       return lookup.addr;
> > +}
> 
> Strange you have above functions here. It's a copy paste from I2C
> core. Please, think about way of deduplicating it.

See Hans' reply and mine, I'll amend this in v3.

> 
> > +static void mshw0011_dump_registers(struct i2c_client *client,
> > +                                   struct i2c_client *bat0, u8 end_register)
> > +{
> > +       char *rd_buf;
> 
> > +       char prefix[128];
> 
> 128 is too way big for a prefix.

I know I should have used 1024 :)

Anyway, I think I'll drop the register dumps here, we don't need it
anymore (and I can now sniff the I2C communications under Windows, so we
can always doule check with those dumps).

> 
> > +       unsigned int length = end_register + 1;
> > +       int error;
> > +
> > +       snprintf(prefix, ARRAY_SIZE(prefix), "%s: ", bat0->name);
> 
> > +       prefix[127] = '\0';
> 
> Why?

Just me being paranoid in case the code doesn't follow the spec... Yeah, I'll
remove it.

> 
> > +       rd_buf = kzalloc(length, GFP_KERNEL);
> 
> > +       error = mshw0011_i2c_read_block(bat0, 0, rd_buf, length);
> > +       print_hex_dump(KERN_INFO, prefix, DUMP_PREFIX_OFFSET, 16, 1,
> > +                      rd_buf, length, true);
> 
> If you switch to debugfs it makes things a bit more easier to handle I think.
> 
> > +
> > +       kfree(rd_buf);
> > +}
> 
> > +static int mshw0011_probe(struct i2c_client *client,
> > +                         const struct i2c_device_id *id)
> > +{
> 
> > +       data->notify_version = version == MSHW0011_EV_2_5;
> 
> 0x1ff as version sounds hmm suspicious.

So after a little bit of digging, it appears those values were taken
from the DSDT:
https://bugzilla.kernel.org/attachment.cgi?id=187171 line 11694.

It appears 0x3F is EV 2.1 and before, and 0x1FF is EV 2.5 and above.
The returned value is not a version of the chip, just a flag to know
which path we are taking in the DSM.

The name is probably not the best.

> 
> > +static const struct i2c_device_id mshw0011_id[] = {
> > +       { }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, mshw0011_id);
> 
> ->probe_new(), please.

Correct

> 
> If I2C framework is _still_ broken we need to fix that part.

I haven't check, so let's see for v3.

> 
> > +#ifdef CONFIG_ACPI
> 
> Is it going to be non-ACPI at all? See my proposal to Kconfig as well.

Sounds good to me. I'll drop this #ifdef.

> 
> > +               .acpi_match_table = ACPI_PTR(mshw0011_acpi_match),
> 
> ACPI_PTR() might be gone


Ok.

Thanks again for the review, I'll try to get a v3 soon.

Cheers,
Benjamin

Re: [PATCH v2] ACPI: surface3_power: MSHW0011 rev-eng implementation

[Andy Shevchenko]

On Fri, Jun 30, 2017 at 6:57 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> On Jun 29 2017 or thereabouts, Andy Shevchenko wrote:
>> On Thu, Jun 29, 2017 at 3:10 PM, Benjamin Tissoires
>> <benjamin.tissoires@redhat.com> wrote:

>> What devices (laptops, tablets) have it?
>> Surface 3. What else?
>
> So far, Surface 3 only. It's a Microsoft PNPId, so I guess they control
> which device has it. Maybe the model after the Surface 3 (reduced
> platform) will have such chip, but for now, it's unknown.

Please, extend introduction in commit message to state above.

>> > I couldn't manage to get the IRQ correctly triggered, so I am using a
>> > good old polling thread to check for changes.
>>
>> It might be

It seems I didn't finished the sentence here.

I though it might be actually ACPI event, GPE or direct IRQ (when GPIO
chip should not disable it, though if it's the case it likely a BIOS
bug for this hardware).

>> > +       help
>> > +         Select this option to enable support for ACPI operation
>> > +         region of the Surface 3 battery platform driver.
>>
>> > +/*
>> > + * Supports for the power IC on the Surface 3 tablet.
>>
>> Shouldn't it go to drivers/acpi/pmic folder ?
>
> Already answered later in the thread, so yes, I'll move it there.

Actually Hans did a good point, so, feel free to use drivers/platform/x86.

>> And did you check if it have actual chip IP vendor name and model?
>> Most likely it's a TI (based?) solution.
>
> As mentioned, I have strictly no idea. I can not crack open the Surface
> 3 without breaking the warranty (I already had to return it once because
> the disk crashed).

We have one indeed cracked (screen is broken for good :-) ), so, I
would check what I can find there.

> And I do not find anything brand-related under Windows either:
> - it's called "Surface Platform Power Driver"
> - and the driver is provided by Microsoft

Fair enough.

>> > +static int mshw0011_bix(struct mshw0011_data *cdata, struct bix *bix)
>> > +{
>>
>> > +       memcpy(bix->serial, buf + 7, 3);
>> > +       memcpy(bix->serial + 3, buf, 6);
>> > +       bix->serial[9] = '\0';
>>
>> snprintf()?
>
> probably :)

I would do this until we have an evidence that it contains non-printable symbols
(or, in case you want to fix ahead, make the buffer 4 times bigger and use %*pE)

>> > +       memcpy(bix->OEM, buf, 3);
>> > +       bix->OEM[4] = '\0';
>>
>> snprintf() ?

Ditto.

>> > +       snprintf(prefix, ARRAY_SIZE(prefix), "%s: ", bat0->name);
>>
>> > +       prefix[127] = '\0';
>>
>> Why?
>
> Just me being paranoid in case the code doesn't follow the spec... Yeah, I'll
> remove it.

snprintf() despite n in the name takes care of terminating NUL.

>> > +static int mshw0011_probe(struct i2c_client *client,
>> > +                         const struct i2c_device_id *id)
>> > +{
>>
>> > +       data->notify_version = version == MSHW0011_EV_2_5;
>>
>> 0x1ff as version sounds hmm suspicious.
>
> So after a little bit of digging, it appears those values were taken
> from the DSDT:
> https://bugzilla.kernel.org/attachment.cgi?id=187171 line 11694.
>
> It appears 0x3F is EV 2.1 and before, and 0x1FF is EV 2.5 and above.
> The returned value is not a version of the chip, just a flag to know
> which path we are taking in the DSM.
>
> The name is probably not the best.

63 and 511 looks too suspicious to be a version. Rather block size, a
mask or alike.

>> > +static const struct i2c_device_id mshw0011_id[] = {
>> > +       { }
>> > +};
>> > +MODULE_DEVICE_TABLE(i2c, mshw0011_id);
>>
>> ->probe_new(), please.
>
> Correct
>
>>
>> If I2C framework is _still_ broken we need to fix that part.
>
> I haven't check, so let's see for v3.

Cc: Wolfram for v3 and ask him directly. Last time I checked it looks
like I2C core doesn't care about ACPI when ->probe_new() is used.

-- 
With Best Regards,
Andy Shevchenko

Re: [Devel] [PATCH v2] ACPI: surface3_power: MSHW0011 rev-eng implementation

[Andy Shevchenko]

[-- Attachment #1: Type: text/plain, Size: 4016 bytes --]

On Fri, Jun 30, 2017 at 6:57 PM, Benjamin Tissoires
<benjamin.tissoires(a)redhat.com> wrote:
> On Jun 29 2017 or thereabouts, Andy Shevchenko wrote:
>> On Thu, Jun 29, 2017 at 3:10 PM, Benjamin Tissoires
>> <benjamin.tissoires(a)redhat.com> wrote:

>> What devices (laptops, tablets) have it?
>> Surface 3. What else?
>
> So far, Surface 3 only. It's a Microsoft PNPId, so I guess they control
> which device has it. Maybe the model after the Surface 3 (reduced
> platform) will have such chip, but for now, it's unknown.

Please, extend introduction in commit message to state above.

>> > I couldn't manage to get the IRQ correctly triggered, so I am using a
>> > good old polling thread to check for changes.
>>
>> It might be

It seems I didn't finished the sentence here.

I though it might be actually ACPI event, GPE or direct IRQ (when GPIO
chip should not disable it, though if it's the case it likely a BIOS
bug for this hardware).

>> > +       help
>> > +         Select this option to enable support for ACPI operation
>> > +         region of the Surface 3 battery platform driver.
>>
>> > +/*
>> > + * Supports for the power IC on the Surface 3 tablet.
>>
>> Shouldn't it go to drivers/acpi/pmic folder ?
>
> Already answered later in the thread, so yes, I'll move it there.

Actually Hans did a good point, so, feel free to use drivers/platform/x86.

>> And did you check if it have actual chip IP vendor name and model?
>> Most likely it's a TI (based?) solution.
>
> As mentioned, I have strictly no idea. I can not crack open the Surface
> 3 without breaking the warranty (I already had to return it once because
> the disk crashed).

We have one indeed cracked (screen is broken for good :-) ), so, I
would check what I can find there.

> And I do not find anything brand-related under Windows either:
> - it's called "Surface Platform Power Driver"
> - and the driver is provided by Microsoft

Fair enough.

>> > +static int mshw0011_bix(struct mshw0011_data *cdata, struct bix *bix)
>> > +{
>>
>> > +       memcpy(bix->serial, buf + 7, 3);
>> > +       memcpy(bix->serial + 3, buf, 6);
>> > +       bix->serial[9] = '\0';
>>
>> snprintf()?
>
> probably :)

I would do this until we have an evidence that it contains non-printable symbols
(or, in case you want to fix ahead, make the buffer 4 times bigger and use %*pE)

>> > +       memcpy(bix->OEM, buf, 3);
>> > +       bix->OEM[4] = '\0';
>>
>> snprintf() ?

Ditto.

>> > +       snprintf(prefix, ARRAY_SIZE(prefix), "%s: ", bat0->name);
>>
>> > +       prefix[127] = '\0';
>>
>> Why?
>
> Just me being paranoid in case the code doesn't follow the spec... Yeah, I'll
> remove it.

snprintf() despite n in the name takes care of terminating NUL.

>> > +static int mshw0011_probe(struct i2c_client *client,
>> > +                         const struct i2c_device_id *id)
>> > +{
>>
>> > +       data->notify_version = version == MSHW0011_EV_2_5;
>>
>> 0x1ff as version sounds hmm suspicious.
>
> So after a little bit of digging, it appears those values were taken
> from the DSDT:
> https://bugzilla.kernel.org/attachment.cgi?id=187171 line 11694.
>
> It appears 0x3F is EV 2.1 and before, and 0x1FF is EV 2.5 and above.
> The returned value is not a version of the chip, just a flag to know
> which path we are taking in the DSM.
>
> The name is probably not the best.

63 and 511 looks too suspicious to be a version. Rather block size, a
mask or alike.

>> > +static const struct i2c_device_id mshw0011_id[] = {
>> > +       { }
>> > +};
>> > +MODULE_DEVICE_TABLE(i2c, mshw0011_id);
>>
>> ->probe_new(), please.
>
> Correct
>
>>
>> If I2C framework is _still_ broken we need to fix that part.
>
> I haven't check, so let's see for v3.

Cc: Wolfram for v3 and ask him directly. Last time I checked it looks
like I2C core doesn't care about ACPI when ->probe_new() is used.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] ACPI: surface3_power: MSHW0011 rev-eng implementation

[Hans de Goede]

Hi,

On 30-06-17 18:37, Andy Shevchenko wrote:
> On Fri, Jun 30, 2017 at 6:57 PM, Benjamin Tissoires

<snip>

>>>> +static const struct i2c_device_id mshw0011_id[] = {
>>>> +       { }
>>>> +};
>>>> +MODULE_DEVICE_TABLE(i2c, mshw0011_id);
>>>
>>> ->probe_new(), please.
>>
>> Correct
>>
>>>
>>> If I2C framework is _still_ broken we need to fix that part.
>>
>> I haven't check, so let's see for v3.
> 
> Cc: Wolfram for v3 and ask him directly. Last time I checked it looks
> like I2C core doesn't care about ACPI when ->probe_new() is used.

ACPI i2c drivers still need an empty i2c_device_id table I've
fixing this on my TODO but it has been buried in other stuff.

Benjamin if (not saying you should, but if) you want to take a look at
this, fixing the need for the empty table for ACPI devices should be
easy. The problem is these lines in drivers/i2c/i2c-core.c:
i2c_device_probe():

         /*
          * An I2C ID table is not mandatory, if and only if, a suitable Device
          * Tree match table entry is supplied for the probing device.
          */
         if (!driver->id_table &&
             !i2c_of_match_device(dev->driver->of_match_table, client))
                 return -ENODEV;

Which needs to be extended to also check for an ACPI match AFAIK
you can NOT just replace this with i2c_device_match because that would
break manually binding a driver through sysfs.

Regards,

Hans

Re: [PATCH v2] ACPI: surface3_power: MSHW0011 rev-eng implementation

[Andy Shevchenko]

On Fri, Jun 30, 2017 at 8:37 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> On 30-06-17 18:37, Andy Shevchenko wrote:
>> On Fri, Jun 30, 2017 at 6:57 PM, Benjamin Tissoires

> ACPI i2c drivers still need an empty i2c_device_id table I've
> fixing this on my TODO but it has been buried in other stuff.
>
> Benjamin if (not saying you should, but if) you want to take a look at
> this, fixing the need for the empty table for ACPI devices should be
> easy. The problem is these lines in drivers/i2c/i2c-core.c:
> i2c_device_probe():
>
>         /*
>          * An I2C ID table is not mandatory, if and only if, a suitable
> Device
>          * Tree match table entry is supplied for the probing device.
>          */
>         if (!driver->id_table &&
>             !i2c_of_match_device(dev->driver->of_match_table, client))
>                 return -ENODEV;
>
> Which needs to be extended to also check for an ACPI match AFAIK
> you can NOT just replace this with i2c_device_match because that would
> break manually binding a driver through sysfs.

I have a stashed change for that, just have no time to look closer.

-- 
With Best Regards,
Andy Shevchenko

Re: [Devel] [PATCH v2] ACPI: surface3_power: MSHW0011 rev-eng implementation

[Andy Shevchenko]

[-- Attachment #1: Type: text/plain, Size: 1192 bytes --]

On Fri, Jun 30, 2017 at 8:37 PM, Hans de Goede <hdegoede(a)redhat.com> wrote:
> On 30-06-17 18:37, Andy Shevchenko wrote:
>> On Fri, Jun 30, 2017 at 6:57 PM, Benjamin Tissoires

> ACPI i2c drivers still need an empty i2c_device_id table I've
> fixing this on my TODO but it has been buried in other stuff.
>
> Benjamin if (not saying you should, but if) you want to take a look at
> this, fixing the need for the empty table for ACPI devices should be
> easy. The problem is these lines in drivers/i2c/i2c-core.c:
> i2c_device_probe():
>
>         /*
>          * An I2C ID table is not mandatory, if and only if, a suitable
> Device
>          * Tree match table entry is supplied for the probing device.
>          */
>         if (!driver->id_table &&
>             !i2c_of_match_device(dev->driver->of_match_table, client))
>                 return -ENODEV;
>
> Which needs to be extended to also check for an ACPI match AFAIK
> you can NOT just replace this with i2c_device_match because that would
> break manually binding a driver through sysfs.

I have a stashed change for that, just have no time to look closer.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] ACPI: surface3_power: MSHW0011 rev-eng implementation

[Hans de Goede]

Hi,

On 30-06-17 19:40, Andy Shevchenko wrote:
> On Fri, Jun 30, 2017 at 8:37 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> On 30-06-17 18:37, Andy Shevchenko wrote:
>>> On Fri, Jun 30, 2017 at 6:57 PM, Benjamin Tissoires
> 
>> ACPI i2c drivers still need an empty i2c_device_id table I've
>> fixing this on my TODO but it has been buried in other stuff.
>>
>> Benjamin if (not saying you should, but if) you want to take a look at
>> this, fixing the need for the empty table for ACPI devices should be
>> easy. The problem is these lines in drivers/i2c/i2c-core.c:
>> i2c_device_probe():
>>
>>          /*
>>           * An I2C ID table is not mandatory, if and only if, a suitable
>> Device
>>           * Tree match table entry is supplied for the probing device.
>>           */
>>          if (!driver->id_table &&
>>              !i2c_of_match_device(dev->driver->of_match_table, client))
>>                  return -ENODEV;
>>
>> Which needs to be extended to also check for an ACPI match AFAIK
>> you can NOT just replace this with i2c_device_match because that would
>> break manually binding a driver through sysfs.
> 
> I have a stashed change for that, just have no time to look closer.

Care to share that? Between me and Benjamin one of us can hopefully
find the time to test / finish it (should be trivial really).

Regards,

Hans

Re: [PATCH v2] ACPI: surface3_power: MSHW0011 rev-eng implementation

[Andy Shevchenko]

[-- Attachment #1: Type: text/plain, Size: 502 bytes --]

On Fri, Jun 30, 2017 at 8:42 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> On 30-06-17 19:40, Andy Shevchenko wrote:
>> On Fri, Jun 30, 2017 at 8:37 PM, Hans de Goede <hdegoede@redhat.com>
>> wrote:
>>> On 30-06-17 18:37, Andy Shevchenko wrote:
>>>> On Fri, Jun 30, 2017 at 6:57 PM, Benjamin Tissoires

> Care to share that? Between me and Benjamin one of us can hopefully
> find the time to test / finish it (should be trivial really).

Not tested at all.



-- 
With Best Regards,
Andy Shevchenko

[-- Attachment #2: 0001-i2c-core-Allow-empty-id_table-in-ACPI-case-as-well.patch --]
[-- Type: text/x-patch, Size: 1188 bytes --]

From ea8aa2823410393bbf67a1182339aea439d8f81d Mon Sep 17 00:00:00 2001
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Date: Fri, 30 Jun 2017 20:53:00 +0300
Subject: [PATCH 1/1] i2c: core: Allow empty id_table in ACPI case as well

For now empty ID table is not allowed with ACPI and prevents driver to be
probed.

Add a check to allow empty ID table.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/i2c-core-base.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index c89dac7fd2e7..45231d2257ad 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -354,9 +354,10 @@ static int i2c_device_probe(struct device *dev)
 
 	/*
 	 * An I2C ID table is not mandatory, if and only if, a suitable Device
-	 * Tree match table entry is supplied for the probing device.
+	 * Tree or ACPI match table entry is supplied for the probing device.
 	 */
 	if (!driver->id_table &&
+	    !acpi_match_device(dev->driver->acpi_match_table, &client->dev) &&
 	    !i2c_of_match_device(dev->driver->of_match_table, client))
 		return -ENODEV;
 
-- 
2.11.0

Re: [Devel] [PATCH v2] ACPI: surface3_power: MSHW0011 rev-eng implementation

[Andy Shevchenko]

[-- Attachment #1: Type: text/plain, Size: 523 bytes --]

On Fri, Jun 30, 2017 at 8:42 PM, Hans de Goede <hdegoede(a)redhat.com> wrote:
> On 30-06-17 19:40, Andy Shevchenko wrote:
>> On Fri, Jun 30, 2017 at 8:37 PM, Hans de Goede <hdegoede(a)redhat.com>
>> wrote:
>>> On 30-06-17 18:37, Andy Shevchenko wrote:
>>>> On Fri, Jun 30, 2017 at 6:57 PM, Benjamin Tissoires

> Care to share that? Between me and Benjamin one of us can hopefully
> find the time to test / finish it (should be trivial really).

Not tested at all.



-- 
With Best Regards,
Andy Shevchenko

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-i2c-core-Allow-empty-id_table-in-ACPI-case-as-well.patch --]
[-- Type: text/x-patch, Size: 1188 bytes --]

From ea8aa2823410393bbf67a1182339aea439d8f81d Mon Sep 17 00:00:00 2001
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Date: Fri, 30 Jun 2017 20:53:00 +0300
Subject: [PATCH 1/1] i2c: core: Allow empty id_table in ACPI case as well

For now empty ID table is not allowed with ACPI and prevents driver to be
probed.

Add a check to allow empty ID table.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/i2c-core-base.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index c89dac7fd2e7..45231d2257ad 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -354,9 +354,10 @@ static int i2c_device_probe(struct device *dev)
 
 	/*
 	 * An I2C ID table is not mandatory, if and only if, a suitable Device
-	 * Tree match table entry is supplied for the probing device.
+	 * Tree or ACPI match table entry is supplied for the probing device.
 	 */
 	if (!driver->id_table &&
+	    !acpi_match_device(dev->driver->acpi_match_table, &client->dev) &&
 	    !i2c_of_match_device(dev->driver->of_match_table, client))
 		return -ENODEV;
 
-- 
2.11.0

Re: [PATCH v2] ACPI: surface3_power: MSHW0011 rev-eng implementation

[Andy Shevchenko]

On Fri, Jun 30, 2017 at 8:55 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Jun 30, 2017 at 8:42 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> On 30-06-17 19:40, Andy Shevchenko wrote:
>>> On Fri, Jun 30, 2017 at 8:37 PM, Hans de Goede <hdegoede@redhat.com>
>>> wrote:
>>>> On 30-06-17 18:37, Andy Shevchenko wrote:
>>>>> On Fri, Jun 30, 2017 at 6:57 PM, Benjamin Tissoires
>
>> Care to share that? Between me and Benjamin one of us can hopefully
>> find the time to test / finish it (should be trivial really).
>
> Not tested at all.

Should have

Fixes: da10c06a044b ("i2c: Make I2C ID tables non-mandatory for DT'ed devices")

also.

-- 
With Best Regards,
Andy Shevchenko

Re: [Devel] [PATCH v2] ACPI: surface3_power: MSHW0011 rev-eng implementation

[Andy Shevchenko]

[-- Attachment #1: Type: text/plain, Size: 727 bytes --]

On Fri, Jun 30, 2017 at 8:55 PM, Andy Shevchenko
<andy.shevchenko(a)gmail.com> wrote:
> On Fri, Jun 30, 2017 at 8:42 PM, Hans de Goede <hdegoede(a)redhat.com> wrote:
>> On 30-06-17 19:40, Andy Shevchenko wrote:
>>> On Fri, Jun 30, 2017 at 8:37 PM, Hans de Goede <hdegoede(a)redhat.com>
>>> wrote:
>>>> On 30-06-17 18:37, Andy Shevchenko wrote:
>>>>> On Fri, Jun 30, 2017 at 6:57 PM, Benjamin Tissoires
>
>> Care to share that? Between me and Benjamin one of us can hopefully
>> find the time to test / finish it (should be trivial really).
>
> Not tested at all.

Should have

Fixes: da10c06a044b ("i2c: Make I2C ID tables non-mandatory for DT'ed devices")

also.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] ACPI: surface3_power: MSHW0011 rev-eng implementation

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 703 bytes --]

Hi,

On Thu, Jun 29, 2017 at 02:10:09PM +0200, Benjamin Tissoires wrote:
> [...]
>
> +	/* get design capacity */
> +	ret = i2c_smbus_read_word_data(client,
> +				       MSHW0011_BAT0_REG_DESIGN_CAPACITY);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "Error reading design capacity: %d\n",
> +			ret);
> +		return ret;
> +	}
> +	bix->design_capacity = le16_to_cpu(ret);

i2c_smbus_read_word_data() returns native endianess for
little-endian bus (it basically has builtin le16_to_cpu).
Your conversion actually _breaks_ support on big endian
machines by converting it back.

That seems to be a common mistake in the kernel and it
might be a good idea to add some Coccinelle script for
it?

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] ACPI: surface3_power: MSHW0011 rev-eng implementation

[Julia Lawall]

> That seems to be a common mistake in the kernel and it
> might be a good idea to add some Coccinelle script for
> it?

Done.

julia

Re: [PATCH v2] ACPI: surface3_power: MSHW0011 rev-eng implementation

[Benjamin Tissoires]

Hi Andy,

I am resurrecting this thread now that ACPICA seemed to finally have
fixed the bug that prevent the driver to work.
The patch I submitted was reverted shortly after, which lead me to
ignore this review until ACPICA was fixed. It took a lot of effort
from Hans to have a fix accepted, so now we can hope to upstream this
driver.

On Fri, Jun 30, 2017 at 6:37 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Fri, Jun 30, 2017 at 6:57 PM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> > On Jun 29 2017 or thereabouts, Andy Shevchenko wrote:
> >> On Thu, Jun 29, 2017 at 3:10 PM, Benjamin Tissoires
> >> <benjamin.tissoires@redhat.com> wrote:
>
> >> What devices (laptops, tablets) have it?
> >> Surface 3. What else?
> >
> > So far, Surface 3 only. It's a Microsoft PNPId, so I guess they control
> > which device has it. Maybe the model after the Surface 3 (reduced
> > platform) will have such chip, but for now, it's unknown.
>
> Please, extend introduction in commit message to state above.

OK. On this note, I have been mentioned that the Surface Pro 2017 uses
a similar mechanism as in it's also using an operation region handler,
but this time over UART, not I2C :)

>
> >> > I couldn't manage to get the IRQ correctly triggered, so I am using a
> >> > good old polling thread to check for changes.
> >>
> >> It might be
>
> It seems I didn't finished the sentence here.
>
> I though it might be actually ACPI event, GPE or direct IRQ (when GPIO
> chip should not disable it, though if it's the case it likely a BIOS
> bug for this hardware).

If you don't mind, I'd rather have the polling version that seems to
be working first. I haven't touched the logs I had from Windows since
last year, so I am a little bit rusty on debugging this.
FWIW, /proc/interrupts doesn't change a bit when I unplug/replug the
power cable.

My guess is that the Windows driver initializes the chip in a
different way and this enables the cable detection.

>
> >> > +       help
> >> > +         Select this option to enable support for ACPI operation
> >> > +         region of the Surface 3 battery platform driver.
> >>
> >> > +/*
> >> > + * Supports for the power IC on the Surface 3 tablet.
> >>
> >> Shouldn't it go to drivers/acpi/pmic folder ?
> >
> > Already answered later in the thread, so yes, I'll move it there.
>
> Actually Hans did a good point, so, feel free to use drivers/platform/x86.

Roger that!

>
> >> And did you check if it have actual chip IP vendor name and model?
> >> Most likely it's a TI (based?) solution.
> >
> > As mentioned, I have strictly no idea. I can not crack open the Surface
> > 3 without breaking the warranty (I already had to return it once because
> > the disk crashed).
>
> We have one indeed cracked (screen is broken for good :-) ), so, I
> would check what I can find there.
>
> > And I do not find anything brand-related under Windows either:
> > - it's called "Surface Platform Power Driver"
> > - and the driver is provided by Microsoft
>
> Fair enough.
>
> >> > +static int mshw0011_bix(struct mshw0011_data *cdata, struct bix *bix)
> >> > +{
> >>
> >> > +       memcpy(bix->serial, buf + 7, 3);
> >> > +       memcpy(bix->serial + 3, buf, 6);
> >> > +       bix->serial[9] = '\0';
> >>
> >> snprintf()?
> >
> > probably :)
>
> I would do this until we have an evidence that it contains non-printable symbols
> (or, in case you want to fix ahead, make the buffer 4 times bigger and use %*pE)

I can't really make the buffer 4 time bigger. The buffer is then used
by the DSDT table to report the _BIX status, so the length of 10 is
mandatory.
It doesn't seem to hurt, and worse case, we will just strip the
serial, not a big deal IMO.

>
> >> > +       memcpy(bix->OEM, buf, 3);
> >> > +       bix->OEM[4] = '\0';
> >>
> >> snprintf() ?
>
> Ditto.
>
> >> > +       snprintf(prefix, ARRAY_SIZE(prefix), "%s: ", bat0->name);
> >>
> >> > +       prefix[127] = '\0';
> >>
> >> Why?
> >
> > Just me being paranoid in case the code doesn't follow the spec... Yeah, I'll
> > remove it.
>
> snprintf() despite n in the name takes care of terminating NUL.
>
> >> > +static int mshw0011_probe(struct i2c_client *client,
> >> > +                         const struct i2c_device_id *id)
> >> > +{
> >>
> >> > +       data->notify_version = version == MSHW0011_EV_2_5;
> >>
> >> 0x1ff as version sounds hmm suspicious.
> >
> > So after a little bit of digging, it appears those values were taken
> > from the DSDT:
> > https://bugzilla.kernel.org/attachment.cgi?id=187171 line 11694.
> >
> > It appears 0x3F is EV 2.1 and before, and 0x1FF is EV 2.5 and above.
> > The returned value is not a version of the chip, just a flag to know
> > which path we are taking in the DSM.
> >
> > The name is probably not the best.
>
> 63 and 511 looks too suspicious to be a version. Rather block size, a
> mask or alike.

I replaced the 'version' by 'mask' in v3. It doesn't hurt to do so.

>
> >> > +static const struct i2c_device_id mshw0011_id[] = {
> >> > +       { }
> >> > +};
> >> > +MODULE_DEVICE_TABLE(i2c, mshw0011_id);
> >>
> >> ->probe_new(), please.
> >
> > Correct
> >
> >>
> >> If I2C framework is _still_ broken we need to fix that part.
> >
> > I haven't check, so let's see for v3.
>
> Cc: Wolfram for v3 and ask him directly. Last time I checked it looks
> like I2C core doesn't care about ACPI when ->probe_new() is used.

Looks like things are working fine now. So I can just submit the
driver without bothering the I2C core team :)

Cheers,
Benjamin

RE: [PATCH v2] ACPI: surface3_power: MSHW0011 rev-eng implementation

[Moore, Robert]

> -----Original Message-----
> From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com]
> Sent: Friday, August 31, 2018 7:55 AM
> To: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: Hans de Goede <hdegoede@redhat.com>; Bastien Nocera
> <hadess@hadess.net>; stephenjust@gmail.com; Sebastian Reichel
> <sre@kernel.org>; Wysocki, Rafael J <rafael.j.wysocki@intel.com>;
> lenb@kernel.org; Moore, Robert <robert.moore@intel.com>;
> lv.zheng@intel.com; mika.westerberg@linux.intel.com; linux-
> acpi@vger.kernel.org; devel@acpica.org; linux-pm@vger.kernel.org; lkml
> <linux-kernel@vger.kernel.org>
> Subject: Re: [PATCH v2] ACPI: surface3_power: MSHW0011 rev-eng
> implementation
> 
> Hi Andy,
> 
> I am resurrecting this thread now that ACPICA seemed to finally have
> fixed the bug that prevent the driver to work.
> The patch I submitted was reverted shortly after, which lead me to
> ignore this review until ACPICA was fixed. It took a lot of effort from
> Hans to have a fix accepted, so now we can hope to upstream this driver.
> 
[Moore, Robert] 

The worst part of all this is that the ACPI specification is so ambiguous in this area, that we were forced to *guess* at certain elements of the implementation.

So, if anyone knows of any ASL/machines that use the serial bus stuff, please forward the code to me.

This includes:
    GenericSerialBus
    SMBus
    IPMI

And maybe:
    GeneralPurposeIo

For completeness.



> On Fri, Jun 30, 2017 at 6:37 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Fri, Jun 30, 2017 at 6:57 PM, Benjamin Tissoires
> > <benjamin.tissoires@redhat.com> wrote:
> > > On Jun 29 2017 or thereabouts, Andy Shevchenko wrote:
> > >> On Thu, Jun 29, 2017 at 3:10 PM, Benjamin Tissoires
> > >> <benjamin.tissoires@redhat.com> wrote:
> >
> > >> What devices (laptops, tablets) have it?
> > >> Surface 3. What else?
> > >
> > > So far, Surface 3 only. It's a Microsoft PNPId, so I guess they
> > > control which device has it. Maybe the model after the Surface 3
> > > (reduced
> > > platform) will have such chip, but for now, it's unknown.
> >
> > Please, extend introduction in commit message to state above.
> 
> OK. On this note, I have been mentioned that the Surface Pro 2017 uses a
> similar mechanism as in it's also using an operation region handler, but
> this time over UART, not I2C :)
> 
> >
> > >> > I couldn't manage to get the IRQ correctly triggered, so I am
> > >> > using a good old polling thread to check for changes.
> > >>
> > >> It might be
> >
> > It seems I didn't finished the sentence here.
> >
> > I though it might be actually ACPI event, GPE or direct IRQ (when GPIO
> > chip should not disable it, though if it's the case it likely a BIOS
> > bug for this hardware).
> 
> If you don't mind, I'd rather have the polling version that seems to be
> working first. I haven't touched the logs I had from Windows since last
> year, so I am a little bit rusty on debugging this.
> FWIW, /proc/interrupts doesn't change a bit when I unplug/replug the
> power cable.
> 
> My guess is that the Windows driver initializes the chip in a different
> way and this enables the cable detection.
> 
> >
> > >> > +       help
> > >> > +         Select this option to enable support for ACPI operation
> > >> > +         region of the Surface 3 battery platform driver.
> > >>
> > >> > +/*
> > >> > + * Supports for the power IC on the Surface 3 tablet.
> > >>
> > >> Shouldn't it go to drivers/acpi/pmic folder ?
> > >
> > > Already answered later in the thread, so yes, I'll move it there.
> >
> > Actually Hans did a good point, so, feel free to use
> drivers/platform/x86.
> 
> Roger that!
> 
> >
> > >> And did you check if it have actual chip IP vendor name and model?
> > >> Most likely it's a TI (based?) solution.
> > >
> > > As mentioned, I have strictly no idea. I can not crack open the
> > > Surface
> > > 3 without breaking the warranty (I already had to return it once
> > > because the disk crashed).
> >
> > We have one indeed cracked (screen is broken for good :-) ), so, I
> > would check what I can find there.
> >
> > > And I do not find anything brand-related under Windows either:
> > > - it's called "Surface Platform Power Driver"
> > > - and the driver is provided by Microsoft
> >
> > Fair enough.
> >
> > >> > +static int mshw0011_bix(struct mshw0011_data *cdata, struct bix
> > >> > +*bix) {
> > >>
> > >> > +       memcpy(bix->serial, buf + 7, 3);
> > >> > +       memcpy(bix->serial + 3, buf, 6);
> > >> > +       bix->serial[9] = '\0';
> > >>
> > >> snprintf()?
> > >
> > > probably :)
> >
> > I would do this until we have an evidence that it contains
> > non-printable symbols (or, in case you want to fix ahead, make the
> > buffer 4 times bigger and use %*pE)
> 
> I can't really make the buffer 4 time bigger. The buffer is then used by
> the DSDT table to report the _BIX status, so the length of 10 is
> mandatory.
> It doesn't seem to hurt, and worse case, we will just strip the serial,
> not a big deal IMO.
> 
> >
> > >> > +       memcpy(bix->OEM, buf, 3);
> > >> > +       bix->OEM[4] = '\0';
> > >>
> > >> snprintf() ?
> >
> > Ditto.
> >
> > >> > +       snprintf(prefix, ARRAY_SIZE(prefix), "%s: ", bat0->name);
> > >>
> > >> > +       prefix[127] = '\0';
> > >>
> > >> Why?
> > >
> > > Just me being paranoid in case the code doesn't follow the spec...
> > > Yeah, I'll remove it.
> >
> > snprintf() despite n in the name takes care of terminating NUL.
> >
> > >> > +static int mshw0011_probe(struct i2c_client *client,
> > >> > +                         const struct i2c_device_id *id) {
> > >>
> > >> > +       data->notify_version = version == MSHW0011_EV_2_5;
> > >>
> > >> 0x1ff as version sounds hmm suspicious.
> > >
> > > So after a little bit of digging, it appears those values were taken
> > > from the DSDT:
> > > https://bugzilla.kernel.org/attachment.cgi?id=187171 line 11694.
> > >
> > > It appears 0x3F is EV 2.1 and before, and 0x1FF is EV 2.5 and above.
> > > The returned value is not a version of the chip, just a flag to know
> > > which path we are taking in the DSM.
> > >
> > > The name is probably not the best.
> >
> > 63 and 511 looks too suspicious to be a version. Rather block size, a
> > mask or alike.
> 
> I replaced the 'version' by 'mask' in v3. It doesn't hurt to do so.
> 
> >
> > >> > +static const struct i2c_device_id mshw0011_id[] = {
> > >> > +       { }
> > >> > +};
> > >> > +MODULE_DEVICE_TABLE(i2c, mshw0011_id);
> > >>
> > >> ->probe_new(), please.
> > >
> > > Correct
> > >
> > >>
> > >> If I2C framework is _still_ broken we need to fix that part.
> > >
> > > I haven't check, so let's see for v3.
> >
> > Cc: Wolfram for v3 and ask him directly. Last time I checked it looks
> > like I2C core doesn't care about ACPI when ->probe_new() is used.
> 
> Looks like things are working fine now. So I can just submit the driver
> without bothering the I2C core team :)
> 
> Cheers,
> Benjamin

Re: [Devel] [PATCH v2] ACPI: surface3_power: MSHW0011 rev-eng implementation

[Moore, Robert]

[-- Attachment #1: Type: text/plain, Size: 7198 bytes --]



> -----Original Message-----
> From: Benjamin Tissoires [mailto:benjamin.tissoires(a)redhat.com]
> Sent: Friday, August 31, 2018 7:55 AM
> To: Andy Shevchenko <andy.shevchenko(a)gmail.com>
> Cc: Hans de Goede <hdegoede(a)redhat.com>; Bastien Nocera
> <hadess(a)hadess.net>; stephenjust(a)gmail.com; Sebastian Reichel
> <sre(a)kernel.org>; Wysocki, Rafael J <rafael.j.wysocki(a)intel.com>;
> lenb(a)kernel.org; Moore, Robert <robert.moore(a)intel.com>;
> lv.zheng(a)intel.com; mika.westerberg(a)linux.intel.com; linux-
> acpi(a)vger.kernel.org; devel(a)acpica.org; linux-pm(a)vger.kernel.org; lkml
> <linux-kernel(a)vger.kernel.org>
> Subject: Re: [PATCH v2] ACPI: surface3_power: MSHW0011 rev-eng
> implementation
> 
> Hi Andy,
> 
> I am resurrecting this thread now that ACPICA seemed to finally have
> fixed the bug that prevent the driver to work.
> The patch I submitted was reverted shortly after, which lead me to
> ignore this review until ACPICA was fixed. It took a lot of effort from
> Hans to have a fix accepted, so now we can hope to upstream this driver.
> 
[Moore, Robert] 

The worst part of all this is that the ACPI specification is so ambiguous in this area, that we were forced to *guess* at certain elements of the implementation.

So, if anyone knows of any ASL/machines that use the serial bus stuff, please forward the code to me.

This includes:
    GenericSerialBus
    SMBus
    IPMI

And maybe:
    GeneralPurposeIo

For completeness.



> On Fri, Jun 30, 2017 at 6:37 PM Andy Shevchenko
> <andy.shevchenko(a)gmail.com> wrote:
> >
> > On Fri, Jun 30, 2017 at 6:57 PM, Benjamin Tissoires
> > <benjamin.tissoires(a)redhat.com> wrote:
> > > On Jun 29 2017 or thereabouts, Andy Shevchenko wrote:
> > >> On Thu, Jun 29, 2017 at 3:10 PM, Benjamin Tissoires
> > >> <benjamin.tissoires(a)redhat.com> wrote:
> >
> > >> What devices (laptops, tablets) have it?
> > >> Surface 3. What else?
> > >
> > > So far, Surface 3 only. It's a Microsoft PNPId, so I guess they
> > > control which device has it. Maybe the model after the Surface 3
> > > (reduced
> > > platform) will have such chip, but for now, it's unknown.
> >
> > Please, extend introduction in commit message to state above.
> 
> OK. On this note, I have been mentioned that the Surface Pro 2017 uses a
> similar mechanism as in it's also using an operation region handler, but
> this time over UART, not I2C :)
> 
> >
> > >> > I couldn't manage to get the IRQ correctly triggered, so I am
> > >> > using a good old polling thread to check for changes.
> > >>
> > >> It might be
> >
> > It seems I didn't finished the sentence here.
> >
> > I though it might be actually ACPI event, GPE or direct IRQ (when GPIO
> > chip should not disable it, though if it's the case it likely a BIOS
> > bug for this hardware).
> 
> If you don't mind, I'd rather have the polling version that seems to be
> working first. I haven't touched the logs I had from Windows since last
> year, so I am a little bit rusty on debugging this.
> FWIW, /proc/interrupts doesn't change a bit when I unplug/replug the
> power cable.
> 
> My guess is that the Windows driver initializes the chip in a different
> way and this enables the cable detection.
> 
> >
> > >> > +       help
> > >> > +         Select this option to enable support for ACPI operation
> > >> > +         region of the Surface 3 battery platform driver.
> > >>
> > >> > +/*
> > >> > + * Supports for the power IC on the Surface 3 tablet.
> > >>
> > >> Shouldn't it go to drivers/acpi/pmic folder ?
> > >
> > > Already answered later in the thread, so yes, I'll move it there.
> >
> > Actually Hans did a good point, so, feel free to use
> drivers/platform/x86.
> 
> Roger that!
> 
> >
> > >> And did you check if it have actual chip IP vendor name and model?
> > >> Most likely it's a TI (based?) solution.
> > >
> > > As mentioned, I have strictly no idea. I can not crack open the
> > > Surface
> > > 3 without breaking the warranty (I already had to return it once
> > > because the disk crashed).
> >
> > We have one indeed cracked (screen is broken for good :-) ), so, I
> > would check what I can find there.
> >
> > > And I do not find anything brand-related under Windows either:
> > > - it's called "Surface Platform Power Driver"
> > > - and the driver is provided by Microsoft
> >
> > Fair enough.
> >
> > >> > +static int mshw0011_bix(struct mshw0011_data *cdata, struct bix
> > >> > +*bix) {
> > >>
> > >> > +       memcpy(bix->serial, buf + 7, 3);
> > >> > +       memcpy(bix->serial + 3, buf, 6);
> > >> > +       bix->serial[9] = '\0';
> > >>
> > >> snprintf()?
> > >
> > > probably :)
> >
> > I would do this until we have an evidence that it contains
> > non-printable symbols (or, in case you want to fix ahead, make the
> > buffer 4 times bigger and use %*pE)
> 
> I can't really make the buffer 4 time bigger. The buffer is then used by
> the DSDT table to report the _BIX status, so the length of 10 is
> mandatory.
> It doesn't seem to hurt, and worse case, we will just strip the serial,
> not a big deal IMO.
> 
> >
> > >> > +       memcpy(bix->OEM, buf, 3);
> > >> > +       bix->OEM[4] = '\0';
> > >>
> > >> snprintf() ?
> >
> > Ditto.
> >
> > >> > +       snprintf(prefix, ARRAY_SIZE(prefix), "%s: ", bat0->name);
> > >>
> > >> > +       prefix[127] = '\0';
> > >>
> > >> Why?
> > >
> > > Just me being paranoid in case the code doesn't follow the spec...
> > > Yeah, I'll remove it.
> >
> > snprintf() despite n in the name takes care of terminating NUL.
> >
> > >> > +static int mshw0011_probe(struct i2c_client *client,
> > >> > +                         const struct i2c_device_id *id) {
> > >>
> > >> > +       data->notify_version = version == MSHW0011_EV_2_5;
> > >>
> > >> 0x1ff as version sounds hmm suspicious.
> > >
> > > So after a little bit of digging, it appears those values were taken
> > > from the DSDT:
> > > https://bugzilla.kernel.org/attachment.cgi?id=187171 line 11694.
> > >
> > > It appears 0x3F is EV 2.1 and before, and 0x1FF is EV 2.5 and above.
> > > The returned value is not a version of the chip, just a flag to know
> > > which path we are taking in the DSM.
> > >
> > > The name is probably not the best.
> >
> > 63 and 511 looks too suspicious to be a version. Rather block size, a
> > mask or alike.
> 
> I replaced the 'version' by 'mask' in v3. It doesn't hurt to do so.
> 
> >
> > >> > +static const struct i2c_device_id mshw0011_id[] = {
> > >> > +       { }
> > >> > +};
> > >> > +MODULE_DEVICE_TABLE(i2c, mshw0011_id);
> > >>
> > >> ->probe_new(), please.
> > >
> > > Correct
> > >
> > >>
> > >> If I2C framework is _still_ broken we need to fix that part.
> > >
> > > I haven't check, so let's see for v3.
> >
> > Cc: Wolfram for v3 and ask him directly. Last time I checked it looks
> > like I2C core doesn't care about ACPI when ->probe_new() is used.
> 
> Looks like things are working fine now. So I can just submit the driver
> without bothering the I2C core team :)
> 
> Cheers,
> Benjamin