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 87F33C52D7F for ; Wed, 14 Aug 2024 21:13:59 +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:In-Reply-To:Content-Type: MIME-Version:References:Subject:Cc:To:From:Date:Message-ID:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=E0PsU1V81otLoWVv/G1qLYZtjV9Tg/8agLNzDh9hSwo=; b=1Xb5gbis3U92JLtlGT2pUcyzDz 45hWuE5x51MZDcBFlYjjbaMIoY2ZO9TWBiV04xBEOzjO0n8qY7ad6+Qk3VezG1dofE2fhivdtj2jr KZO81I9HUYOklelZLbsB2dkyJ/zN36ao1EcDHVSL03w7Ibq5vLOH7aDch0ghbTlDXinXkgQrhEdL5 RVXVh2RPziFRhIkKN9AEflWaSo2EI2pSLnpqUVNIT/5TwVaU/Sc3i5ktNth3id9nsb0U7tDJxe84K V+L4v1s1m6UXc7PR/VCeTM5ny249NkDITXMlCmdlffc/D/rscdWJkt5F6HGjBWnUC+iNzhiI1o/KM ZXigJZLg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1seLJS-00000008Isx-3VGr; Wed, 14 Aug 2024 21:13:46 +0000 Received: from mail-oi1-x22c.google.com ([2607:f8b0:4864:20::22c]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1seLIo-00000008IlW-0eJP for linux-arm-kernel@lists.infradead.org; Wed, 14 Aug 2024 21:13:08 +0000 Received: by mail-oi1-x22c.google.com with SMTP id 5614622812f47-3dd16257b7bso129082b6e.1 for ; Wed, 14 Aug 2024 14:13:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1723669984; x=1724274784; darn=lists.infradead.org; h=in-reply-to:content-disposition:mime-version:references:subject:cc :to:from:date:message-id:from:to:cc:subject:date:message-id:reply-to; bh=E0PsU1V81otLoWVv/G1qLYZtjV9Tg/8agLNzDh9hSwo=; b=YwGkbu57d+n15nL81pvFaPj+wI/botzn3LwelFZ8riPF8ukjdOR5YWufiMLIzAOcG7 yCBVQqyZfbEhkgjMStums+JSOPSTSejRjtKU3lTckZb+ZLQdhdcJQKNHx1MrO8OOrx98 kfkAhCpTKvIhNSp8JyOKt1epSU2roRTPBz/5Ue8yVj32see/NCAHzj2gluDuS+pV4nav nhP3nSs0zbWQhA0PzDyRctaMYHZPsB1AFXkXjBBiJUqQ9jsMUBoCWamc5Q6UlzRK422e ObBcGAfwvuk6GTsLUhQaUFyCcqWq5EbXhOLlf+x9SC/k2xIEkVtFb07vyqJhWcBMZdWU dXTQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1723669984; x=1724274784; h=in-reply-to:content-disposition:mime-version:references:subject:cc :to:from:date:message-id:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=E0PsU1V81otLoWVv/G1qLYZtjV9Tg/8agLNzDh9hSwo=; b=cDOCnGAv1rKW7AYBc2QOGK/nm8pGXiUxF0St4zuwwYp9PHNqacXKDH47Q3MIf9wZ2e MTeViYPrfKPlVTEJlFoHX6FYtmIeyWJUrb28YP/+9BGGqzlDP6DFlMlZKS6BufiDJfyD EaYwFO9tz5YKN3K16Tmphq/iDPfT8pnMQUEfDhOcW7kGNhCrQKQEkXmKgLWVqYhCkOXW +8hgaLYtBpb1n9Tj/IceQFvevyVN4xgKddQySlJGA7/Pint6hkSPj+fBg5zb73nwN2UH KW9pT62HaTO7uMVi9B62wsaYJ2u8TBbIkc5K48GeqKpCS5AHamoeWivqyRsVZeKdxmFh FRog== X-Forwarded-Encrypted: i=1; AJvYcCXWFisMeoAUT/F95ta6J67+xsZvxNUgdRJcxnJjxRxcVksFS6d6ZLGgJDrZ9rFMm3AqmH+hwXcHlhonSgR6zcApsQIcg5K7+ySwPKVJzjLN53GZfig= X-Gm-Message-State: AOJu0YxnGnkxb2t+A13J/SKSrDNdEbbWAQTiVv/B+XdmY2+3UeDQ/SAK 6yZnakMrgQgl8TXdvUTV1/Br6r8dldbduPuhiqFaV+rqnziCnbqA X-Google-Smtp-Source: AGHT+IHPFbvzVVdUw502MWTJGOKLHnC1j3KggFd0N+p5LyKQWM3LNqxF2J0bV4X2lPo9bjtcCBrWNw== X-Received: by 2002:a05:6808:1789:b0:3d9:4163:654f with SMTP id 5614622812f47-3dd2997b5camr4570717b6e.32.1723669984114; Wed, 14 Aug 2024 14:13:04 -0700 (PDT) Received: from neuromancer. ([2600:1700:fb0:1bcf::54]) by smtp.gmail.com with ESMTPSA id 5614622812f47-3dd060b4e13sm2221748b6e.45.2024.08.14.14.13.03 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 14 Aug 2024 14:13:03 -0700 (PDT) Message-ID: <66bd1ddf.ca0a0220.13e5d4.8871@mx.google.com> X-Google-Original-Message-ID: Date: Wed, 14 Aug 2024 16:13:01 -0500 From: Chris Morgan To: Jonathan Cameron 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 References: <20240802192026.446344-1-macroalpha82@gmail.com> <20240802192026.446344-15-macroalpha82@gmail.com> <20240803121044.20481897@jic23-huawei> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240803121044.20481897@jic23-huawei> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240814_141306_224641_4A05E3E1 X-CRM114-Status: GOOD ( 37.12 ) 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 Sat, Aug 03, 2024 at 12:10:44PM +0100, Jonathan Cameron wrote: > 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. I'll take your other comments and apply them, but if it's okay with you I'll opt to not use FIELD_PREP/FIELD_GET for the moment, so the style remains the same. I will make sure to use those macros for other drivers I'm working on though as they seem handy. > > > > 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. Acknowledged. > > > +} > > + > > 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. Will do. > > + 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 > Nack, if that's okay (as mentioned above). If it's not let me know and I'll go back and redo this driver. > *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. > Will do. > > +} > > + > > 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. > Will do. > > + default: > > + return -EINVAL; > > + } > > +} > > + Thank you for your review, Chris