| Message ID | 20170629121009.30234-1-benjamin.tissoires@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
+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
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
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
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
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 >
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
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
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
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.
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
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.
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
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
> 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
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
> -----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
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");