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

Andy Shevchenko
2017-06-29T14:22:38Z

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

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

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

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

It might be

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

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

depends on ACPI ?

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

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

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

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

> + */

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

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

> + */

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

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

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

BIT() ?

> +#define MSHW0011_EV_2_5 0x1ff

Is it mask? GENMASK() then.

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

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

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

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

> + static const uuid_le mshw0011_guid =

guid_t, please :-)

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

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

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

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

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

snprintf()?

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

non-x86 ?

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

snprintf() ?

> +
> + return 0;
> +}

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

> + tmp = le16_to_cpu(rate);

Do we need that on x86?

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

> + if ((s32) tmp > 0)

See above, perhaps using rate directly.

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

Ditto.

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

non-x86 ?

> +}
> +

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

> + ret = 0;

See above.

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

> + ret = 0;

Ditto.

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

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

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

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

> + char prefix[128];

128 is too way big for a prefix.

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

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

Why?

> + rd_buf = kzalloc(length, GFP_KERNEL);

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

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

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

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

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

0x1ff as version sounds hmm suspicious.

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

->probe_new(), please.

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

> +#ifdef CONFIG_ACPI

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

> + .acpi_match_table = ACPI_PTR(mshw0011_acpi_match),

ACPI_PTR() might be gone

--
With Best Regards,
Andy Shevchenko