Re: [PATCH v4 2/4] power: bq24190_charger: Clean up extcon code

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 10-04-17 11:03, Liam Breck wrote:
> On Sat, Apr 8, 2017 at 1:05 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>>
>> On 08-04-17 21:09, Liam Breck wrote:
>>>
>>> On Sat, Apr 8, 2017 at 3:02 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 08-04-17 00:27, Liam Breck wrote:
>>>>>
>>>>>
>>>>> Hallo Hans,
>>>>>
>>>>> On Thu, Apr 6, 2017 at 12:17 AM, Hans de Goede <hdegoede@redhat.com>
>>>>> wrote:
>>>>
>>>>
>>>>
>>>> <snip>
>>>>
>>>>
>>>>>>> -       /*
>>>>>>> -        * If no charger has been detected and host mode is requested,
>>>>>>> activate
>>>>>>> -        * the 5V boost converter, otherwise deactivate it.
>>>>>>> -        */
>>>>>>> -       if (!iinlim && extcon_get_state(bdi->extcon, EXTCON_USB_HOST)
>>>>>>> ==
>>>>>>> 1) {
>>>>>>> -               ret = bq24190_write_mask(bdi, BQ24190_REG_POC,
>>>>>>> -
>>>>>>> BQ24190_REG_POC_CHG_CONFIG_MASK,
>>>>>>> -
>>>>>>> BQ24190_REG_POC_CHG_CONFIG_SHIFT,
>>>>>>> -
>>>>>>> BQ24190_REG_POC_CHG_CONFIG_OTG);
>>>>>>> -       } else {
>>>>>>> -               ret = bq24190_write_mask(bdi, BQ24190_REG_POC,
>>>>>>> -
>>>>>>> BQ24190_REG_POC_CHG_CONFIG_MASK,
>>>>>>> -
>>>>>>> BQ24190_REG_POC_CHG_CONFIG_SHIFT,
>>>>>>> -
>>>>>>> BQ24190_REG_POC_CHG_CONFIG_CHARGE);
>>>>>>> -       }
>>>>>>> -       if (ret)
>>>>>>> -               dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", ret);
>>>>>>> +       /* Set OTG 5V boost: on if no charger detected and in USB host
>>>>>>> mode, else off */
>>>>>>> +       error = bq24190_write_mask(bdi, BQ24190_REG_POC,
>>>>>>> +                                  BQ24190_REG_POC_CHG_CONFIG_MASK,
>>>>>>> +                                  BQ24190_REG_POC_CHG_CONFIG_SHIFT,
>>>>>>> +                       !iinlim && extcon_get_state(bdi->extcon,
>>>>>>> EXTCON_USB_HOST) == 1
>>>>>>> +                                ? BQ24190_REG_POC_CHG_CONFIG_OTG
>>>>>>> +                                : BQ24190_REG_POC_CHG_CONFIG_CHARGE);
>>>>>>> +       if (error < 0)
>>>>>>> +               dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n",
>>>>>>> error);
>>>>>>>
>>>>>>>         pm_runtime_mark_last_busy(bdi->dev);
>>>>>>>         pm_runtime_put_autosuspend(bdi->dev);
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> I still find the refactored code you're proposing here a lot less
>>>>>> readable
>>>>>> then the original code, and the same goes for the comment. If you
>>>>>> insist
>>>>>> in changing this, then I suggest changing it into this:
>>>>>
>>>>>
>>>>>
>>>>> OK. I'll collapse the comment to one line tho.
>>>>
>>>>
>>>>
>>>> Please don't having this as 2 lines really is not a problem in any way
>>>> and
>>>> I find the longer comment a lot easier to parse.
>>>>
>>>>
>>>>>
>>>>>>         /*
>>>>>>          * If no charger has been detected and host mode is requested,
>>>>>> activate
>>>>>>          * the 5V boost converter, otherwise deactivate it.
>>>>>>          */
>>>>>>         if (!iinlim && extcon_get_state(bdi->extcon, EXTCON_USB_HOST)
>>>>>> ==
>>>>>> 1)
>>>>>>                 chg_config = BQ24190_REG_POC_CHG_CONFIG_OTG;
>>>>>>         else
>>>>>>                 chg_config = BQ24190_REG_POC_CHG_CONFIG_CHARGE;
>>>>>>
>>>>>>         error = bq24190_write_mask(bdi, BQ24190_REG_POC,
>>>>>>                                    BQ24190_REG_POC_CHG_CONFIG_MASK,
>>>>>>                                    BQ24190_REG_POC_CHG_CONFIG_SHIFT,
>>>>>>                                    chg_config);
>>>>>>         if (error)
>>>>>>                 dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", error);
>>>>>>
>>>>>>> @@ -1432,8 +1427,8 @@ static int bq24190_probe(struct i2c_client
>>>>>>> *client,
>>>>>>>         }
>>>>>>>
>>>>>>>         /*
>>>>>>> -        * The extcon-name property is purely an in kernel interface
>>>>>>> for
>>>>>>> -        * x86/ACPI use, DT platforms should get extcon via phandle.
>>>>>>> +        * ACPI platforms should use device_property "extcon-name".
>>>>>>> +        * Devicetree platforms should get extcon via phandle (not yet
>>>>>>> supported).
>>>>>>>          */
>>>>>>>         if (device_property_read_string(dev, "extcon-name", &name) ==
>>>>>>> 0)
>>>>>>> {
>>>>>>>                 bdi->extcon = extcon_get_extcon_dev(name);
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> The new comment suggest that extcon-name is something which should
>>>>>> actually
>>>>>> be used inside ACPI DSDT tables, which it is not, please leave the
>>>>>> comment
>>>>>> as is.
>>>>>
>>>>>
>>>>>
>>>>> OK, right. But "in kernel interface" sounds official, and this is
>>>>> really a custom msg passing method. A platform-data struct is
>>>>> documented in a header, and a module parameter has its own docs
>>>>> scheme; and use of those methods is well documented. I can't tell how
>>>>> I would invoke this one. How about this...
>>>>>
>>>>> A Devicetree should provide extcon-name via phandle (not yet supported).
>>>>
>>>>
>>>>
>>>> No a devicetree should not provide a name it would provide a phandle
>>>> pointing to the extcon, no name (and name based lookup) should be
>>>> involved.
>>>
>>>
>>> OK.
>>>
>>>>> ACPI extcon drivers/clients[?] should insert[?] device_property
>>>>> extcon-name into our device with relevant_api_call(args).
>>>>
>>>>
>>>>
>>>> For an example of how this is used see:
>>>>
>>>>
>>>> https://github.com/jwrdegoede/linux-sunxi/commit/7fa4460e2a18a3f6ffb39ae4940fee1dd2a880a4
>>>>
>>>> Around line 214 of i2c-cht-wc.c and later.
>>>
>>>
>>> So a comment like...
>>>
>>> On ACPI platforms, extcon clients should invoke this driver with:
>>> struct i2c_adapter a = { ... };
>>> i2c_add_adapter(&a);
>>> struct property_entry pe[] = { PROPERTY_ENTRY_STRING("extcon-name",
>>> <name>), ... };
>>> struct i2c_board_info bi = { .type = "bq24190", .addr = 0x6b,
>>> .properties = pe, .irq = <irq> };
>>> i2c_new_device(&a, &bi);
>>
>>
>> Sure that works for me.
>
> Should we export a function from the charger driver to do the above,
> rather than requiring every client driver to replicate it?

If we end up needing to do this in more places, then we could consider
making iinlim a property and adding something generic for this to
the power_supply core. But that is something to do when we've at least
2 users, I've a feeling this will look different per use-case
(what about e.g. the 5v boost converter?) so we really need a better
idea of what a generic solution should look like before writing one.

Regards,

Hans