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

Benjamin Tissoires
2017-06-30T15:57:19Z

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