Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753358AbcKYJua (ORCPT ); Fri, 25 Nov 2016 04:50:30 -0500 Received: from mail.free-electrons.com ([62.4.15.54]:60424 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751968AbcKYJuX (ORCPT ); Fri, 25 Nov 2016 04:50:23 -0500 Date: Fri, 25 Nov 2016 10:48:18 +0100 From: Thomas Petazzoni To: Quentin Schulz Cc: sre@kernel.org, robh+dt@kernel.org, mark.rutland@arm.com, wens@csie.org, linux@armlinux.org.uk, maxime.ripard@free-electrons.com, lee.jones@linaro.org, linux-pm@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 02/10] power: supply: axp20x_usb_power: set min voltage and max current from sysfs Message-ID: <20161125104818.597c0718@free-electrons.com> In-Reply-To: <20161125090921.23138-3-quentin.schulz@free-electrons.com> References: <20161125090921.23138-1-quentin.schulz@free-electrons.com> <20161125090921.23138-3-quentin.schulz@free-electrons.com> Organization: Free Electrons X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1538 Lines: 61 Quentin, On Fri, 25 Nov 2016 10:09:13 +0100, Quentin Schulz wrote: > +static int axp20x_usb_power_set_property(struct power_supply *psy, > + enum power_supply_property psp, > + const union power_supply_propval *val) > +{ > + struct axp20x_usb_power *power = power_supply_get_drvdata(psy); > + int ret, val1; > + > + switch (psp) { > + case POWER_SUPPLY_PROP_VOLTAGE_MIN: > + switch (val->intval) { This nested switch construct doesn't look very pretty. What about instead: switch(psp) { case POWER_SUPPLY_PROP_VOLTAGE_MIN: return axp20x_usb_power_set_prop_voltage_min(...); case POWER_SUPPLY_PROP_CURRENT_MAX: return axp20x_usb_power_set_prop_current_max(...); default: return -EINVAL; } > + case 4000000: > + case 4100000: > + case 4200000: > + case 4300000: > + case 4400000: > + case 4500000: > + case 4600000: > + case 4700000: > + val1 = (val->intval - 4000000) / 100000; > + ret = regmap_update_bits(power->regmap, > + AXP20X_VBUS_IPSOUT_MGMT, > + AXP20X_VBUS_VHOLD_MASK, > + val1 << 3); > + if (ret) > + return ret; > + > + return 0; Just do: return regmap_update_bits(...); The dance to test ret is useless, since you anyway return ret when non-zero, or return zero when ret was zero. While you're at it, maybe make val1 a u32, since I guess it's written to a 32-bit register (unless u32 is not commonly used in this driver already, of course). Thomas -- Thomas Petazzoni, CTO, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com