From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755533AbdCTWi1 (ORCPT ); Mon, 20 Mar 2017 18:38:27 -0400 Received: from mx1.redhat.com ([209.132.183.28]:47080 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755290AbdCTWiY (ORCPT ); Mon, 20 Mar 2017 18:38:24 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 3C1454DD55 Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=hdegoede@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 3C1454DD55 Subject: Re: [PATCH 10/15] power: supply: bq24190_charger: Use extcon to determine ilimit, 5v boost To: Andy Shevchenko , "Rafael J . Wysocki" , Len Brown , Wolfram Sang , Lee Jones , Sebastian Reichel , MyungJoo Ham , Chanwoo Choi References: <20170317095527.10487-1-hdegoede@redhat.com> <20170317095527.10487-11-hdegoede@redhat.com> <1489772011.19767.75.camel@linux.intel.com> Cc: linux-acpi@vger.kernel.org, Takashi Iwai , linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org From: Hans de Goede Message-ID: Date: Mon, 20 Mar 2017 23:38:19 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <1489772011.19767.75.camel@linux.intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Mon, 20 Mar 2017 22:38:24 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On 17-03-17 18:33, Andy Shevchenko wrote: > On Fri, 2017-03-17 at 10:55 +0100, Hans de Goede wrote: >> Add support for monitoring an extcon device with SDP/CDP/DCP and HOST >> cables and adjust ilimit and enable/disable the 5v boost converter >> accordingly. This is necessary on systems where the PSEL pin is >> hardwired >> high and ILIM needs to be set by software based on the detected >> charger >> type. >> > >> config CHARGER_BQ24190 >> tristate "TI BQ24190 battery charger driver" >> depends on I2C > >> + depends on EXTCON > > I dunno what is preferred here, but if we would like to keep > compatibility with previous configurations "select" should be used over > "depends on". select really should only be used for hidden options, using select in other scenarios leads to all sort of problems (hard to debug Kconfig dependency loops). > >> +static void bq24190_extcon_work(struct work_struct *work) >> +{ >> + struct bq24190_dev_info *bdi = >> + container_of(work, struct bq24190_dev_info, >> extcon_work); >> + int ret, iinlim = 0; >> + >> + if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_SDP) == 1) >> + iinlim = 500000; >> + else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_CDP) == >> 1 || >> + extcon_get_state(bdi->extcon, EXTCON_CHG_USB_ACA) == >> 1) >> + iinlim = 1500000; >> + else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_DCP) == >> 1) >> + iinlim = 2000000; >> + > >> + if (iinlim) { > > Could be possible to call below unconditionally here (use 0)? If there is no Vbus setting iinlim is not useful, the charger will reset it to a default as soon as Vbus comes up and i2c transactions are not free. > >> + ret = bq24190_set_field_val(bdi, BQ24190_REG_ISC, >> + BQ24190_REG_ISC_IINLIM_MASK, >> + BQ24190_REG_ISC_IINLIM_SHIFT, >> + bq24190_iinlim_values, >> + ARRAY_SIZE(bq24190_iinlim_values), >> + iinlim); >> + if (ret) >> + dev_err(bdi->dev, "Can't set IINLIM: %d\n", >> ret); >> + } > > Perhaps make above as a helper? > > In that case no need for "if (iinlim)" and perhaps switch-case might be > used instead of if-else-if (latter is up to you). I prefer to keep this as is. Thanks & Regards, Hans