Re: [PATCH v3] platform/x86: surface3_power: MSHW0011 rev-eng implementation

[Andy Shevchenko]

On Fri, Aug 31, 2018 at 6:18 PM Benjamin Tissoires
<benjamin.tissoires@xxxxxxxxxx> wrote:
>
> The MSHW0011 device is a chip that replaces the battery firmware
> by using ACPI operation regions on the Surface 3.
> It is unclear whether or not the chip will be reused somewhere else
> (under Windows, the chip is called "Surface Platform Power Driver"
> and the driver is provided by Microsoft).
>
> The values have been obtained by reverse engineering, and are subject to
> errors. Looks like it works on overall pretty well.
>
> I couldn't manage to get the IRQ correctly triggered, so I am using a
> good old polling thread to check for changes. This is something
> to be fixed in a later version.

I have pushed to my review and testing queue for bots playing with it!
But below my review.

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

Remove this blank line.

> +// SPDX-License-Identifier: GPL-2.0+

> +

No need for this blank line either.

> +/*
> + * Supports for the power IC on the Surface 3 tablet.
> + *
> + * (C) Copyright 2016-2018 Red Hat, Inc
> + * (C) Copyright 2016-2018 Benjamin Tissoires <benjamin.tissoires@xxxxxxxxx>
> + * (C) Copyright 2016 Stephen Just <stephenjust@xxxxxxxxx>

> + *
...and for this.

> + */

> +#include <asm/unaligned.h>

I'm pretty sure this should go after linux/*.h ones

> +#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>


> +       char    model[10];
> +       char    serial[10];
> +       char    type[10];
> +       char    OEM[10];

These 10:s is something worth to define:

#define SURFACE_3_STRLEN 10

or alike.

> +
> +

One blank line is enough

> +#define MSHW0011_EV_2_5                        0x1ff

Is it mask? Value? Some threshold?

> +       static const guid_t mshw0011_guid =
> +               GUID_INIT(0x3F99E367, 0x6220, 0x4955,
> +                         0x8B, 0x0F, 0x06, 0xEF, 0x2A, 0xE7, 0x94, 0x12);

Can you move this outside of the function?

> +       char buf[10];

Same definition as I mentioned above?


> +       /* get serial number */
> +       ret = i2c_smbus_read_i2c_block_data(client, MSHW0011_BAT0_REG_SERIAL_NO,
> +                                           10, buf);

10 -> sizeof() ?

> +       if (ret != 10) {

Ditto.

> +               dev_err(&client->dev, "Error reading serial no: %d\n", ret);
> +               return ret;
> +       }

> +       snprintf(bix->serial, ARRAY_SIZE(bix->serial),
> +                "%*pE%*pE", 3, buf + 7, 6, buf);

It would be one line with format string as "%3pE%6pE" followed by two pointers.

> +       /* get OEM name */
> +       ret = i2c_smbus_read_i2c_block_data(client, MSHW0011_BAT0_REG_OEM,
> +                                           4, buf);
> +       if (ret != 4) {

4 to be defined? Or separate buffer?

> +               dev_err(&client->dev, "Error reading cycle count: %d\n", ret);
> +               return ret;
> +       }
> +       snprintf(bix->OEM, ARRAY_SIZE(bix->OEM), "%*pE", 3, buf);

Depending on the above it might be either "%3pE" or '...sizeof(buf) -
1, buf...'.

> +       status = ret;

> +

No need to have this blank line.

> +       if (status != cdata->charging)
> +               mshw0011_notify(cdata, cdata->notify_mask,
> +                               MSHW0011_NOTIFY_ADP1, &ret);

> +       bat_status = bst.battery_state;

> +

Ditto.

> +       if (bat_status != cdata->bat_charging)
> +               mshw0011_notify(cdata, cdata->notify_mask,
> +                               MSHW0011_NOTIFY_BAT0_BST, &ret);

> +       ret = mshw0011_bix(cdata, &bix);
> +       if (ret < 0)
> +               return ret;

Unlike here, would be nice to add a blank line.

> +       if (bix.last_full_charg_capacity != cdata->full_capacity)
> +               mshw0011_notify(cdata, cdata->notify_mask,
> +                               MSHW0011_NOTIFY_BAT0_BIX, &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;
> +       }

Side note: It would be a good candidate to use my (not yet upstreamed) helper.

> +       handle = ACPI_HANDLE(&client->dev);

> +

Remove this blank line.

> +       if (!handle)
> +               return -ENODEV;

> +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;
> +

I would prefer to see handle assignment here followed by immediate check.

> +       if (!handle)
> +               return;

> +       struct i2c_board_info board_info;
> +       struct device *dev = &client->dev;
> +       struct i2c_client *bat0;

> +

Redundant.

> +       struct mshw0011_data *data;
> +       int error, mask;

> +       data->notify_mask = mask == MSHW0011_EV_2_5;

Since it's a mask, append _MASK and use GENMASK() above.

-- 
With Best Regards,
Andy Shevchenko