From mboxrd@z Thu Jan 1 00:00:00 1970 From: Liam Breck Subject: Re: [PATCH v4 2/4] power: bq24190_charger: Clean up extcon code Date: Mon, 10 Apr 2017 02:03:48 -0700 Message-ID: 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 Return-path: Received: from mail-io0-f171.google.com ([209.85.223.171]:35065 "EHLO mail-io0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752945AbdDJJDt (ORCPT ); Mon, 10 Apr 2017 05:03:49 -0400 Received: by mail-io0-f171.google.com with SMTP id r16so33517001ioi.2 for ; Mon, 10 Apr 2017 02:03:49 -0700 (PDT) In-Reply-To: Sender: linux-pm-owner@vger.kernel.org List-Id: linux-pm@vger.kernel.org To: Hans de Goede Cc: Sebastian Reichel , linux-pm@vger.kernel.org, Tony Lindgren , Liam Breck 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?