From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Subject: Re: [PATCH v4 2/4] power: bq24190_charger: Clean up extcon code Date: Mon, 10 Apr 2017 11:11:20 +0200 Message-ID: <2a8d1ae8-8cd8-7be2-3233-838d91c0127a@redhat.com> References: <20170406031048.12401-1-liam@networkimprov.net> <20170406031048.12401-3-liam@networkimprov.net> <06c84661-ac0d-105f-2896-2615eab7c093@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from mx1.redhat.com ([209.132.183.28]:38551 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751968AbdDJJLX (ORCPT ); Mon, 10 Apr 2017 05:11:23 -0400 In-Reply-To: Sender: linux-pm-owner@vger.kernel.org List-Id: linux-pm@vger.kernel.org To: Liam Breck Cc: Sebastian Reichel , linux-pm@vger.kernel.org, Tony Lindgren , Liam Breck Hi, On 10-04-17 11:03, Liam Breck wrote: > On Sat, Apr 8, 2017 at 1:05 PM, Hans de Goede wrote: >> Hi, >> >> >> On 08-04-17 21:09, Liam Breck wrote: >>> >>> On Sat, Apr 8, 2017 at 3:02 AM, Hans de Goede 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 >>>>> wrote: >>>> >>>> >>>> >>>> >>>> >>>> >>>>>>> - /* >>>>>>> - * 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", >>> ), ... }; >>> struct i2c_board_info bi = { .type = "bq24190", .addr = 0x6b, >>> .properties = pe, .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