+Cc: Hans (he might give some advice regarding to the below) On Thu, Jun 29, 2017 at 3:10 PM, Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
