Received: by 2002:a25:6193:0:0:0:0:0 with SMTP id v141csp3133697ybb; Sun, 22 Mar 2020 16:26:46 -0700 (PDT) X-Google-Smtp-Source: ADFU+vuxjnHSHSNnPrFlwQJsFyGH79WogBAGjC1uABBGrToFiKVNtg4YG96rktYMhm4H6yv8bQid X-Received: by 2002:aca:488a:: with SMTP id v132mr15402881oia.166.1584919606330; Sun, 22 Mar 2020 16:26:46 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1584919606; cv=none; d=google.com; s=arc-20160816; b=Chv355o/MeGjw+2AeYC8qHMTkgHGI+w922dZmSwvp24GMCnv/TmejE4Q9du1Szfx7g +D+My63Q4Ku6dkgf9vYTzCcH00sHxcfBj7iR6Ha7HMTupLtsDHjdx12+vvEo3D0NOsi8 5ajDveMUGleQOLL0Zo94SS+m/jkmTfUA2BsiKR5bQnY/FfYI5Urx5B/+yiEI9AX/yYfz J59dpbZ6PdT8bOYRkSO09ertnuY0iyKs2Ni7zeZqF/9z1bO890uC78/6EvRW7dDbPrxV 0iY8wAfloOewr6sM9/CqVLuh+2N1IzjEUDYAyokaXb+gScFn6hakKmQh33sbEtgRT+LL ScOg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:dkim-signature; bh=ZDKQMZnLV2Qxu2G2wWsWQEZI3Xmwo2zYfXZksiF4wOY=; b=mwO6upiT97j6x6K+2FxbTx90mfxJRnhU4ulw7aXQrrrUx6AcTWcMatAdzt2iPsdPc0 XtzH/8wdLKwnQoepA3tXmImjAIJRvlwNJWSgBUvxakrfNggvzNUJUa/EF+AVXSDJp080 D6ByjDmhbSv0hVaMkvWp10ikzjJyzOSRbl3XmUOvu9ywTjiZFhwkywbpikx8oZTanOPF dKHO2f/T6mFBoJTkW/xkxuE15Ku8laqsb1WzFunx0AqS/hnGX6dnBEdOjwBYL61AkTKK kPYhEpJjHojG9jfReUbTrD4e9dZUplyK1Ka7rJZDUy7dxUCzDaDRFMSa1WvT8fI9rZIT 2BDw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=Mv5Fi2wt; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id g9si6806613otk.97.2020.03.22.16.26.32; Sun, 22 Mar 2020 16:26:46 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=Mv5Fi2wt; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726880AbgCVX0O (ORCPT + 99 others); Sun, 22 Mar 2020 19:26:14 -0400 Received: from mail-pj1-f66.google.com ([209.85.216.66]:34274 "EHLO mail-pj1-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726814AbgCVX0O (ORCPT ); Sun, 22 Mar 2020 19:26:14 -0400 Received: by mail-pj1-f66.google.com with SMTP id q16so5059761pje.1; Sun, 22 Mar 2020 16:26:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=ZDKQMZnLV2Qxu2G2wWsWQEZI3Xmwo2zYfXZksiF4wOY=; b=Mv5Fi2wtUcS1EUcl7QT0VXTLjfoS4onO9OZbnEUT04S9/8ccgjdFiycWWn52Ydw/iF yItaI8TiAWHFHOhM+9ga/M2EiSZTn4DuR6pMfmmJ5QjUfsvG3G8oNKigZYCjbkju4Qs1 BfimpVE9iFttTgJaeziTWulobv1XKLxmhnl7+HF0qhQ4c9C8vqyLfFqrlsEwg58bL7xu cUbThefH40SQSSrIkjr3N/s6TotsVqXy/QN4Vjo95a0M6k3Tor1QLvClkR5YKP+LLvBK y3pzJwob7QPVinRXQMY/HJVv1vbBSBeLf8Pbj8nmzapGsmUsrk6EQnk9HRZezxS4cpNy rsKg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=ZDKQMZnLV2Qxu2G2wWsWQEZI3Xmwo2zYfXZksiF4wOY=; b=GI6XFfwZbDVyiL6FxIsywY24KohmaYZMiNCOhofVF71UvYZDb0LPuhQAibnYIMz5Gf XD++i7Dwu1o9+GeqosbC0MXJNjuAHpReNOo86yzUUOBtxRBY/gQdTByGK5WQWzgPs/Z6 sRpf3TRjrT+hMx8oO5n3Fj1JHDseo733uJBojRvvCI/2cbsqH+NrMZP1CB8rc9+0SAO2 KHnfMOf2NObjePZbpXaiLBWQ30euJB2FxId1eMc4BkI7E5haGuBqJVPhXSDmnC3WPR1e vymEtzxW6jKSQcV4DvKtBybXNbUKPB/eisi2JY/p8Vl9YD0zOVtVttb3i2yZLDKpCyUZ xEtg== X-Gm-Message-State: ANhLgQ1T5jpC9S6C0uHni6JiHwlxsjN8C6e5n/jZetT8iyuH2rI0549W iAqJLFQ9legIY5Q/UxuRkA0r9C2hmBC+jRB2LBg= X-Received: by 2002:a17:90a:8403:: with SMTP id j3mr22305527pjn.8.1584919572031; Sun, 22 Mar 2020 16:26:12 -0700 (PDT) MIME-Version: 1.0 References: <20200322224626.13160-1-sravanhome@gmail.com> <20200322224626.13160-5-sravanhome@gmail.com> In-Reply-To: <20200322224626.13160-5-sravanhome@gmail.com> From: Andy Shevchenko Date: Mon, 23 Mar 2020 01:26:00 +0200 Message-ID: Subject: Re: [PATCH v4 4/5] power: supply: Add support for mps mp2629 battery charger To: Saravanan Sekar Cc: Lee Jones , Rob Herring , Jonathan Cameron , Hartmut Knaack , Lars-Peter Clausen , Peter Meerwald , Sebastian Reichel , devicetree , Linux Kernel Mailing List , linux-iio , Linux PM Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Mar 23, 2020 at 12:47 AM Saravanan Sekar wrote: > > The mp2629 provides switching-mode battery charge management for > single-cell Li-ion or Li-polymer battery. Driver supports the > access/control input source and battery charging parameters. ... > +#include > +#include > +#include Do you need this one? > +#include > +#include > +#include > +#include > +#include > +#include Perhaps put them in order? > +#include How this is being used? > +#include ... > +#define MP2629_MASK_INPUT_TYPE 0xe0 > +#define MP2629_MASK_CHARGE_TYPE 0x18 > +#define MP2629_MASK_CHARGE_CTRL 0x30 > +#define MP2629_MASK_WDOG_CTRL 0x30 > +#define MP2629_MASK_IMPEDANCE 0xf0 GENMASK()? ... > + struct regmap_field *regmap_fields[TERM_CURRENT + 1]; Hmm... Why not to have a definition to cover + 1? ... > +static int mp2629_get_prop(struct mp2629_charger *charger, > + enum mp2629_field fld, > + union power_supply_propval *val) > +{ > + int ret; > + unsigned int rval; > + > + ret = regmap_field_read(charger->regmap_fields[fld], &rval); > + if (!ret) > + val->intval = (rval * props[fld].step) + props[fld].min; > + > + return ret; Why not to use standard pattern, i.e. if (ret) return ret; ... return 0; ? > +} ... > +static int mp2629_charger_battery_set_prop(struct power_supply *psy, > + enum power_supply_property psp, > + const union power_supply_propval *val) > +{ > + struct mp2629_charger *charger = dev_get_drvdata(psy->dev.parent); > + int ret; You may replace it with in-place return statements. > + > + switch (psp) { > + case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT: > + ret = mp2629_set_prop(charger, TERM_CURRENT, val); > + break; > + > + case POWER_SUPPLY_PROP_PRECHARGE_CURRENT: > + ret = mp2629_set_prop(charger, PRECHARGE, val); > + break; > + > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE: > + ret = mp2629_set_prop(charger, CHARGE_VLIM, val); > + break; > + > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT: > + ret = mp2629_set_prop(charger, CHARGE_ILIM, val); > + break; > + > + default: > + return -EINVAL; > + } > + > + return ret; ...and drop this completely. > +} ... > + case POWER_SUPPLY_PROP_ONLINE: > + ret = regmap_read(charger->regmap, MP2629_REG_STATUS, &rval); > + if (!ret) > + val->intval = !!(rval & MP2629_MASK_INPUT_TYPE); > + break; Traditional pattern? ... > +static int mp2629_charger_usb_set_prop(struct power_supply *psy, > + enum power_supply_property psp, > + const union power_supply_propval *val) > +{ > + struct mp2629_charger *charger = dev_get_drvdata(psy->dev.parent); > + int ret; No need to have it. > + switch (psp) { > + case POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT: > + ret = mp2629_set_prop(charger, INPUT_VLIM, val); > + break; > + > + case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT: > + ret = mp2629_set_prop(charger, INPUT_ILIM, val); > + break; > + > + default: > + return -EINVAL; > + } > + > + return ret; > +} ... > + return (psp == POWER_SUPPLY_PROP_PRECHARGE_CURRENT || > + psp == POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT || > + psp == POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT || > + psp == POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE); Redundant parentheses. Ditto for similar cases in the driver. ... > +static ssize_t batt_impedance_compensation_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct mp2629_charger *charger = dev_get_drvdata(dev->parent); > + unsigned int rval; > + int ret; > + > + ret = regmap_read(charger->regmap, MP2629_REG_IMPEDANCE_COMP, &rval); > + if (ret < 0) ' < 0' is not needed. Ditto for other cases. > + return ret; > + > + rval = (rval >> 4) * 10; > + > + return scnprintf(buf, PAGE_SIZE, "%d mohm\n", rval); Simple sprintf(). > +} ... > +static ssize_t batt_impedance_compensation_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t count) > +{ > + struct mp2629_charger *charger = dev_get_drvdata(dev->parent); > + long val; > + int ret; > + > + ret = kstrtol(buf, 10, &val); > + if (ret < 0) No need to check for negative only. > + return ret; > + > + if (val < 0 && val > 140) > + return -ERANGE; And what the point then to use l instead of ul or even uint variant of the conversion above? > + /* multiples of 10 mohm so round off */ > + val = val / 10; > + ret = regmap_update_bits(charger->regmap, MP2629_REG_IMPEDANCE_COMP, > + MP2629_MASK_IMPEDANCE, val << 4); > + if (ret < 0) > + return ret; > + > + return count; > +} ... > +static int mp2629_charger_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + void **pdata = pdev->dev.platform_data; Why void? Why **? Why not to use dev_get_platdata()? Why do we need platform data at all? > + struct mp2629_charger *charger; > + struct power_supply_config psy_cfg = {0}; > + int ret, i; > + charger->regmap = *pdata; > + regmap_update_bits(charger->regmap, MP2629_REG_INTERRUPT, > + GENMASK(6, 5), (BIT(6) | BIT(5))); Too many parentheses. > +} -- With Best Regards, Andy Shevchenko