From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 0B5F1C3DA4A for ; Sat, 3 Aug 2024 11:11:43 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:MIME-Version:References:In-Reply-To:Message-ID:Subject:Cc:To: From:Date:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=nFXJaveFZCBAafa6RG3EmjW8pZhlQWLXnNt/G/aguwo=; b=grdcMI4zKcuwpsfDlRXtD37KDv T7oPrTEykbrirzIg2HrrdLocW04N1gm4xOTbP/4Heh9nzdC7wxqrviw55vcJYcLVKChrlE5VKalzO 4MkQZJSPu9g+ktyok6B7xYSZt9oL4tLQXXcuEhQuaLADy9WPeszfdTCwidhNzddepwA4ck5Kv513U 2ZsH5WVAuhAcIEfnyu/ex/0fVOj5garzk5k2LeX08GEF41Kxd9dvVxyYNnLwFfffa9LKWB7MXrJ1u rV33cjaXxYVv5paA8GDmOPklmx977L6IvCOxnHsFn6CeK3JTmTkWYMX3VP8N/Fblwgs6cxD4AQRyF Tj+wOcHg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1saCfY-0000000BHf0-1sVz; Sat, 03 Aug 2024 11:11:28 +0000 Received: from dfw.source.kernel.org ([139.178.84.217]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1saCf2-0000000BHbO-2bTN for linux-arm-kernel@lists.infradead.org; Sat, 03 Aug 2024 11:10:58 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id E719B6202C; Sat, 3 Aug 2024 11:10:55 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 22A6DC116B1; Sat, 3 Aug 2024 11:10:48 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1722683455; bh=CV4NXj2iWykP7jEOWQLFR0DMW5+IDOIAcyP5yA3ho9o=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=XIzAaZAR//PQ4bKlx5rRsx7sUN4I6HtToPXAHFNiafN05+IeOe2gXscYX7sKFn/zI PwfMn71bX7+72oCT+1EZxhcV+ebpGxo2cbxicvuWJ7hhYGtpyBAjldqDVd4nXytY5G IoJUr8qQX406E30TVo+G3ekDRUX9Bps/Lf/07f+zdkwPte/VMr7zf+AjwPOE+GSs0q pNtLhymBMWbu9WkJMV1SaRjU6bPQOdojkLMBpdVhNaj4wd3ngU7I6OchKxZhhRDbfw GGLN9p+EOspnqNPfLYFn7wyMR3YZSt69JN4E/ZQZMslfQJQYff6ypCSiK6bIV5pR3O L/1MNtzssE0xA== Date: Sat, 3 Aug 2024 12:10:44 +0100 From: Jonathan Cameron To: Chris Morgan Cc: linux-sunxi@lists.linux.dev, linux-pm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org, linux-iio@vger.kernel.org, quentin.schulz@free-electrons.com, mripard@kernel.org, tgamblin@baylibre.com, aidanmacdonald.0x0@gmail.com, u.kleine-koenig@pengutronix.de, lee@kernel.org, samuel@sholland.org, jernej.skrabec@gmail.com, sre@kernel.org, wens@csie.org, conor+dt@kernel.org, krzk+dt@kernel.org, robh@kernel.org, lars@metafoo.de, Chris Morgan , Philippe Simons Subject: Re: [PATCH V2 14/15] power: supply: axp20x_battery: add support for AXP717 Message-ID: <20240803121044.20481897@jic23-huawei> In-Reply-To: <20240802192026.446344-15-macroalpha82@gmail.com> References: <20240802192026.446344-1-macroalpha82@gmail.com> <20240802192026.446344-15-macroalpha82@gmail.com> X-Mailer: Claws Mail 4.3.0 (GTK 3.24.43; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240803_041056_828156_F3803132 X-CRM114-Status: GOOD ( 26.81 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Fri, 2 Aug 2024 14:20:25 -0500 Chris Morgan wrote: > From: Chris Morgan > > Add support for the AXP717 PMIC battery charger. The AXP717 differs > greatly from existing AXP battery chargers in that it cannot measure > the discharge current. The datasheet does not document the current > value's offset or scale, so the POWER_SUPPLY_PROP_CURRENT_NOW is left > unscaled. > > Tested-by: Philippe Simons > Signed-off-by: Chris Morgan Hi. A few drive by comments, Jonathan > --- > drivers/power/supply/axp20x_battery.c | 444 ++++++++++++++++++++++++++ > 1 file changed, 444 insertions(+) > > diff --git a/drivers/power/supply/axp20x_battery.c b/drivers/power/supply/axp20x_battery.c > index c903c588b361..53af4ad0549d 100644 > --- a/drivers/power/supply/axp20x_battery.c > +++ b/drivers/power/supply/axp20x_battery.c > @@ -32,9 +32,19 @@ > #include > > #define AXP20X_PWR_STATUS_BAT_CHARGING BIT(2) > +#define AXP717_PWR_STATUS_MASK GENMASK(6, 5) > +#define AXP717_PWR_STATUS_BAT_STANDBY (0 << 5) > +#define AXP717_PWR_STATUS_BAT_CHRG (1 << 5) > +#define AXP717_PWR_STATUS_BAT_DISCHRG (2 << 5) Fine to match local style in this patch, but just thought I'd comment that this driver would probably be more readable with use of FIELD_PREP and changing convention to not shift the defined values for contents of each field. To change to that it would either need to be before this patch, or done as a follow up. > struct axp20x_batt_ps; > > @@ -143,6 +176,41 @@ static int axp22x_battery_get_max_voltage(struct axp20x_batt_ps *axp20x_batt, > return 0; > } > > +static int axp717_battery_get_max_voltage(struct axp20x_batt_ps *axp20x_batt, > + int *val) > +{ > + int ret, reg; > + > + ret = regmap_read(axp20x_batt->regmap, AXP717_CV_CHG_SET, ®); > + if (ret) > + return ret; > + > + switch (reg & AXP717_CHRG_CV_VOLT_MASK) { > + case AXP717_CHRG_CV_4_0V: > + *val = 4000000; > + break; > + case AXP717_CHRG_CV_4_1V: > + *val = 4100000; > + break; > + case AXP717_CHRG_CV_4_2V: > + *val = 4200000; > + break; > + case AXP717_CHRG_CV_4_35V: > + *val = 4350000; > + break; > + case AXP717_CHRG_CV_4_4V: > + *val = 4400000; > + break; > + case AXP717_CHRG_CV_5_0V: > + *val = 5000000; > + break; > + default: > + return -EINVAL; > + } > + > + return 0; Could just return instead of breaking an save reader having to look to see if anything else happens after the switch finishes. > +} > + > static int axp813_battery_get_max_voltage(struct axp20x_batt_ps *axp20x_batt, > int *val) > { > @@ -188,6 +256,22 @@ static int axp20x_get_constant_charge_current(struct axp20x_batt_ps *axp, > return 0; > } > > +static int axp717_get_constant_charge_current(struct axp20x_batt_ps *axp, > + int *val) > +{ > + int ret; > + > + ret = regmap_read(axp->regmap, AXP717_ICC_CHG_SET, val); Trivial but I'd use a separate local variable for the register value. > + if (ret) > + return ret; > + > + *val &= AXP717_ICC_CHARGER_LIM_MASK; FIELD_GET() would be much more readable here as we'd not need to go check if LIM_MASK included bit 0 and it could be used directly inline with the below as *val = FIELD_GET(AXP717_IC_CHARGER_LIM_MASK, val) * axp->data->ccc_scale; > + > + *val = *val * axp->data->ccc_scale; > + > + return 0; > +} > + > static int axp20x_battery_get_prop(struct power_supply *psy, > enum power_supply_property psp, > union power_supply_propval *val) > @@ -340,6 +424,175 @@ static int axp20x_battery_get_prop(struct power_supply *psy, > return 0; > } > > +static int axp717_battery_get_prop(struct power_supply *psy, > + enum power_supply_property psp, > + union power_supply_propval *val) > +{ > + struct axp20x_batt_ps *axp20x_batt = power_supply_get_drvdata(psy); > + int ret = 0, reg; > + > + switch (psp) { > + case POWER_SUPPLY_PROP_PRESENT: > + case POWER_SUPPLY_PROP_ONLINE: > + ret = regmap_read(axp20x_batt->regmap, AXP717_ON_INDICATE, > + ®); > + if (ret) > + return ret; > + > + val->intval = !!(reg & AXP717_PWR_OP_BATT_PRESENT); FIELD_GET() here would be cleaner. > + break; > + >; > + } > + > + return 0; As nothing to do down here, I think early returns would make things more redabel. > +} > + > static int axp20x_battery_set_prop(struct power_supply *psy, > enum power_supply_property psp, > const union power_supply_propval *val) > @@ -492,6 +805,42 @@ static int axp20x_battery_set_prop(struct power_supply *psy, > } > } > > +static int axp717_battery_set_prop(struct power_supply *psy, > + enum power_supply_property psp, > + const union power_supply_propval *val) > +{ > + struct axp20x_batt_ps *axp20x_batt = power_supply_get_drvdata(psy); > + > + switch (psp) { > + case POWER_SUPPLY_PROP_VOLTAGE_MIN: > + return axp717_set_voltage_min_design(axp20x_batt, val->intval); > + > + case POWER_SUPPLY_PROP_VOLTAGE_MAX: > + return axp20x_batt->data->set_max_voltage(axp20x_batt, val->intval); > + > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX: > + return axp717_set_constant_charge_current(axp20x_batt, > + val->intval); > + case POWER_SUPPLY_PROP_STATUS: > + switch (val->intval) { > + case POWER_SUPPLY_STATUS_CHARGING: > + return regmap_update_bits(axp20x_batt->regmap, > + AXP717_MODULE_EN_CONTROL_2, > + AXP717_CHRG_ENABLE, > + AXP717_CHRG_ENABLE); > + > + case POWER_SUPPLY_STATUS_DISCHARGING: > + case POWER_SUPPLY_STATUS_NOT_CHARGING: > + return regmap_update_bits(axp20x_batt->regmap, > + AXP717_MODULE_EN_CONTROL_2, > + AXP717_CHRG_ENABLE, 0); > + } > + fallthrough; Why bother? Just return -EINVAL here. > + default: > + return -EINVAL; > + } > +} > +