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

[Benjamin Tissoires <benjamin.tissoires@redhat.com>]

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