Re: [PATCH v2] platform/x86: intel_cht_int33fe: Work around BIOS bug on some devices

[Andy Shevchenko]

On Mon, Sep 4, 2017 at 3:47 PM, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> Hi,
>
> On 04-09-17 14:23, Andy Shevchenko wrote:
>>
>> On Sat, 2017-09-02 at 18:20 +0200, Hans de Goede wrote:
>>>
>>> At least one BIOS enumerates the max17047 both through the INT33FE
>>> ACPI
>>> device (it is right there in the resources table) as well as through a
>>> separate MAX17047 device.
>>>
>>> This commit checks for the max17047 already being enumerated through
>>> a separate MAX17047 ACPI device and if so it uses the i2c-client
>>> instantiated for this and attaches the device-props for the max17047
>>> to
>>> that i2c-client.
>>
>>
>> This one looks acceptable wrt buggy BIOS workaround, few comments below
>> (I can do it when applying)
>
>
> The suggested comments are fine with me, feel free to fix
> those up while applying.

Pushed to review-andy.

>
> Thanks & Regards,
>
> Hans
>
>
>
>
>>
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>>> ---
>>> Changes in v2:
>>> -Check acpi_companion HID instead of dev_name
>>> ---
>>>   drivers/platform/x86/intel_cht_int33fe.c | 71
>>> +++++++++++++++++++++++++++-----
>>>   1 file changed, 61 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/intel_cht_int33fe.c
>>> b/drivers/platform/x86/intel_cht_int33fe.c
>>> index da706e2c4232..a9cbc4b8ca63 100644
>>> --- a/drivers/platform/x86/intel_cht_int33fe.c
>>> +++ b/drivers/platform/x86/intel_cht_int33fe.c
>>> @@ -34,6 +34,42 @@ struct cht_int33fe_data {
>>>         struct i2c_client *pi3usb30532;
>>>   };
>>>   +/*
>>> + * Grrr I severly dislike buggy BIOS-es. At least one BIOS enumerates
>>> + * the max17047 both through the INT33FE ACPI device (it is right
>>> there
>>> + * in the resources table) as well as through a separate MAX17047
>>> device.
>>> + *
>>> + * These helpers are used to work around this by checking if an i2c-
>>> client
>>> + * for the max17047 has already been registered.
>>> + */
>>> +int cht_int33fe_check_for_max17047(struct device *dev, void *data)
>>> +{
>>> +       struct acpi_device *companion = ACPI_COMPANION(dev);
>>> +       struct i2c_client **max17047 = data;
>>> +       const char *hid;
>>> +
>>
>>
>> I would assign companion right here.
>>>
>>> +       if (!companion)
>>> +               return 0;
>>> +
>>> +       hid = acpi_device_hid(companion);
>>> +
>>> +       /* The MAX17047 ACPI node doesn't have an UID, so we don't
>>> check that */
>>
>>
>>> +       if (strcmp(hid, "MAX17047") == 0) {
>>
>>
>> if (strcmp())
>>   return 0;
>>
>> ?
>>
>>> +               *max17047 = to_i2c_client(dev);
>>> +               return 1;
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +struct i2c_client *cht_int33fe_find_max17047(void)
>>> +{
>>> +       struct i2c_client *max17047 = NULL;
>>> +
>>> +       i2c_for_each_dev(&max17047, cht_int33fe_check_for_max17047);
>>> +       return max17047;
>>> +}
>>> +
>>>   static const char * const max17047_suppliers[] = { "bq24190-charger"
>>> };
>>>     static const struct property_entry max17047_props[] = {
>>> @@ -46,9 +82,10 @@ static int cht_int33fe_probe(struct i2c_client
>>> *client)
>>>         struct device *dev = &client->dev;
>>>         struct i2c_board_info board_info;
>>>         struct cht_int33fe_data *data;
>>> +       struct i2c_client *max17047;
>>>         unsigned long long ptyp;
>>>         acpi_status status;
>>> -       int fusb302_irq;
>>> +       int ret, fusb302_irq;
>>>         status = acpi_evaluate_integer(ACPI_HANDLE(dev), "PTYP",
>>> NULL, &ptyp);
>>>         if (ACPI_FAILURE(status)) {
>>> @@ -75,13 +112,25 @@ static int cht_int33fe_probe(struct i2c_client
>>> *client)
>>>         if (!data)
>>>                 return -ENOMEM;
>>>   -     memset(&board_info, 0, sizeof(board_info));
>>> -       strlcpy(board_info.type, "max17047", I2C_NAME_SIZE);
>>> -       board_info.properties = max17047_props;
>>> -
>>> -       data->max17047 = i2c_acpi_new_device(dev, 1, &board_info);
>>> -       if (!data->max17047)
>>> -               return -EPROBE_DEFER; /* Wait for the i2c-adapter to
>>> load */
>>> +       /* Work around BIOS bug, see comment on
>>> cht_int33fe_find_max17047 */
>>> +       max17047 = cht_int33fe_find_max17047();
>>> +       if (max17047) {
>>> +               /* Pre-existing i2c-client for the max17047, add
>>> device-props */
>>> +               ret = device_add_properties(&max17047->dev,
>>> max17047_props);
>>> +               if (ret)
>>> +                       return ret;
>>> +               /* And re-probe to get the new device-props applied.
>>> */
>>> +               ret = device_reprobe(&max17047->dev);
>>> +               if (ret)
>>> +                       dev_warn(dev, "Reprobing max17047 error:
>>> %d\n", ret);
>>> +       } else {
>>> +               memset(&board_info, 0, sizeof(board_info));
>>> +               strlcpy(board_info.type, "max17047", I2C_NAME_SIZE);
>>> +               board_info.properties = max17047_props;
>>> +               data->max17047 = i2c_acpi_new_device(dev, 1,
>>> &board_info);
>>> +               if (!data->max17047)
>>> +                       return -EPROBE_DEFER; /* Wait for i2c-adapter
>>> to load */
>>> +       }
>>>         memset(&board_info, 0, sizeof(board_info));
>>>         strlcpy(board_info.type, "fusb302", I2C_NAME_SIZE);
>>> @@ -106,7 +155,8 @@ static int cht_int33fe_probe(struct i2c_client
>>> *client)
>>>         i2c_unregister_device(data->fusb302);
>>>     out_unregister_max17047:
>>> -       i2c_unregister_device(data->max17047);
>>> +       if (data->max17047)
>>> +               i2c_unregister_device(data->max17047);
>>>         return -EPROBE_DEFER; /* Wait for the i2c-adapter to load */
>>>   }
>>> @@ -117,7 +167,8 @@ static int cht_int33fe_remove(struct i2c_client
>>> *i2c)
>>>         i2c_unregister_device(data->pi3usb30532);
>>>         i2c_unregister_device(data->fusb302);
>>> -       i2c_unregister_device(data->max17047);
>>> +       if (data->max17047)
>>> +               i2c_unregister_device(data->max17047);
>>>         return 0;
>>>   }
>>
>>
>



-- 
With Best Regards,
Andy Shevchenko