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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 26605CCA47C for ; Thu, 23 Jun 2022 19:28:39 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231339AbiFWT2i (ORCPT ); Thu, 23 Jun 2022 15:28:38 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49376 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230247AbiFWT2K (ORCPT ); Thu, 23 Jun 2022 15:28:10 -0400 Received: from mail-ed1-x536.google.com (mail-ed1-x536.google.com [IPv6:2a00:1450:4864:20::536]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0BCC18C7F7; Thu, 23 Jun 2022 11:56:48 -0700 (PDT) Received: by mail-ed1-x536.google.com with SMTP id o10so307977edi.1; Thu, 23 Jun 2022 11:56:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=0uXSZHdS3mJpQ7ecdn7ixmyaAYanZgmJ20U9iKB3LvM=; b=FqD779LMhl6ZXXPI+YNjr+WVLo9W4neYkIeUS2q9scpUhb/MJ5Wy8Ao0Qg0FJpaJ+t uOkV3nBwCI/RA0oiO53gNqpaZBlKcfcudajWmGUow3OEyErMDToEMcBU6CtoED0uvZKx RwYZiks9bKE/9SlYHs3OyWJqCmg5TU6CDPyPlZ9eCZcz6RleyuDcLhnXb4k0QCZMcwcJ RY/+wYPvzAo192SLwWyBkGRZse9PovdAQaeEBC23Y/d4A0fA2Ue8oObgemI4GySj84RS hlD+1ZWuMdfA8b38t4lpEpx3UG1buNlGfIar4YHj2bNgvOXYQhcwB+StNIKY7+h24Boq IxcA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=0uXSZHdS3mJpQ7ecdn7ixmyaAYanZgmJ20U9iKB3LvM=; b=E912BOO8IBJABquebymkLx3en9nGtIhCAjv3SAJH6iRI0F0mCstKs81tY1swy3LzP6 yYp3bowNvpJA+hvPO4Kj/c8Z+vYRKPwbUcVeXUsGKf3OixLpsNGEShlUqwPp8t6uKTYT cVC6rV+BcO8EgrilpZnVPY9i//UQ05LoSNARbRnC82RUgRjh5rCW/Cptxpo3vu+X/2QP cmOn0ZVdWMhsHLchXlkO02Yl4twgWunddeFU+wv4ocEmkYvP9JDihE6XOEnCwrY48Z/O yhzdUglRIE2fvpQ7xkY6q9efc1bgOUPw2KKZ2uYx9AKnOpzXTBhb8ArqGdQ52vEklQ2F jnqQ== X-Gm-Message-State: AJIora/7ApIHNMAOoEtBWHrtbmaMl26vcBrFG/ur9ALcHrdpunMaZeW2 1Cwqvr9J045BfrUmVV3S00cxH1dZs/1R8KAa8upvJAiKcjPPhg== X-Google-Smtp-Source: AGRyM1ubOzPazlKC5O9uY0fImpvLLf0ZLaU77Nrk9qWMXcLmh4hjsWTNf6/AOmeMsPByWzgewSmWWaXsLkwODbUFAW0= X-Received: by 2002:a05:6402:249e:b0:42d:bb88:865b with SMTP id q30-20020a056402249e00b0042dbb88865bmr12356084eda.141.1656010606271; Thu, 23 Jun 2022 11:56:46 -0700 (PDT) MIME-Version: 1.0 References: <20220623115631.22209-1-peterwu.pub@gmail.com> <20220623115631.22209-12-peterwu.pub@gmail.com> In-Reply-To: <20220623115631.22209-12-peterwu.pub@gmail.com> From: Andy Shevchenko Date: Thu, 23 Jun 2022 20:56:09 +0200 Message-ID: Subject: Re: [PATCH v3 11/14] power: supply: mt6370: Add Mediatek MT6370 charger driver To: ChiaEn Wu Cc: Lee Jones , Daniel Thompson , Jingoo Han , Pavel Machek , Rob Herring , Krzysztof Kozlowski , Matthias Brugger , Sebastian Reichel , Chunfeng Yun , Greg Kroah-Hartman , Jonathan Cameron , Lars-Peter Clausen , Liam Girdwood , Mark Brown , Guenter Roeck , "Krogerus, Heikki" , Helge Deller , chiaen_wu@richtek.com, alice_chen@richtek.com, cy_huang , dri-devel , Linux LED Subsystem , devicetree , linux-arm Mailing List , "moderated list:ARM/Mediatek SoC support" , Linux Kernel Mailing List , Linux PM , USB , linux-iio , "open list:FRAMEBUFFER LAYER" , szuni chen Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-leds@vger.kernel.org On Thu, Jun 23, 2022 at 2:00 PM ChiaEn Wu wrote: > > From: ChiaEn Wu > > Add Mediatek MT6370 charger driver. ... > +config CHARGER_MT6370 > + tristate "Mediatek MT6370 Charger Driver" > + depends on MFD_MT6370 > + depends on REGULATOR > + select LINEAR_RANGES > + help > + Say Y here to enable MT6370 Charger Part. > + The device supports High-Accuracy Voltage/Current Regulation, > + Average Input Current Regulation, Battery Temperature Sensing, > + Over-Temperature Protection, DPDM Detection for BC1.2. Module name? ... > +#include This usually goes after linux/* > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include ... > +#define MT6370_MIVR_IBUS_TH 100000 /* 100 mA */ Instead of comment, add proper units. ... > + MT6370_USB_STAT_DCP, > + MT6370_USB_STAT_CDP, > + MT6370_USB_STAT_MAX, No comma for a terminator line. ... > +static inline u32 mt6370_chg_val_to_reg(const struct mt6370_chg_range *range, > + u32 val) > +static inline u32 mt6370_chg_reg_to_val(const struct mt6370_chg_range *range, > + u8 reg) I'm wondering if you can use the https://elixir.bootlin.com/linux/v5.19-rc3/source/include/linux/linear_range.h APIs. ... > + int ret = 0; This seems a redundant assignment, see below. > + rcfg->ena_gpiod = fwnode_gpiod_get_index(of_fwnode_handle(of), > + "enable", 0, For index == 0 don't use _index API. > + GPIOD_OUT_LOW | > + GPIOD_FLAGS_BIT_NONEXCLUSIVE, > + rdesc->name); > + if (IS_ERR(rcfg->ena_gpiod)) { > + dev_err(priv->dev, "Failed to requeset OTG EN Pin\n"); request > + rcfg->ena_gpiod = NULL; So, use _optional and return any errors you got. > + } else { > + val = MT6370_OPA_MODE_MASK | MT6370_OTG_PIN_EN_MASK; > + ret = regmap_update_bits(priv->regmap, MT6370_REG_CHG_CTRL1, > + val, val); > + if (ret) > + dev_err(priv->dev, "Failed to set otg bits\n"); > + } ... > + irq_num = platform_get_irq_byname(pdev, irq_name); > + Unwanted blank line. > + if (irq_num < 0) { > + dev_err(priv->dev, "Failed to get platform resource\n"); Isn't it printed by the call? > + } else { > + if (en) > + enable_irq(irq_num); > + else > + disable_irq_nosync(irq_num); > + } ... > +toggle_cfo_exit: The useless label. > + return ret; > +} ... > + ret = mt6370_chg_get_online(priv, val); > + if (!val->intval) { No error check? > + val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > + return 0; > + } ... > +static int mt6370_chg_set_online(struct mt6370_priv *priv, > + const union power_supply_propval *val) > +{ > + int attach; > + u32 pwr_rdy = !!val->intval; > + > + mutex_lock(&priv->attach_lock); > + attach = atomic_read(&priv->attach); > + if (pwr_rdy == !!attach) { > + dev_err(priv->dev, "pwr_rdy is same(%d)\n", pwr_rdy); > + mutex_unlock(&priv->attach_lock); > + return 0; > + } > + > + atomic_set(&priv->attach, pwr_rdy); > + mutex_unlock(&priv->attach_lock); > + > + if (!queue_work(priv->wq, &priv->bc12_work)) > + dev_err(priv->dev, "bc12 work has already queued\n"); > + > + return 0; > + Unwanted blank line. > +} > +static int mt6370_chg_get_property(struct power_supply *psy, > + enum power_supply_property psp, > + union power_supply_propval *val) > +{ > + struct mt6370_priv *priv = power_supply_get_drvdata(psy); > + int ret = 0; > + > + switch (psp) { > + case POWER_SUPPLY_PROP_ONLINE: > + ret = mt6370_chg_get_online(priv, val); > + break; > + case POWER_SUPPLY_PROP_STATUS: > + ret = mt6370_chg_get_status(priv, val); > + break; > + case POWER_SUPPLY_PROP_CHARGE_TYPE: > + ret = mt6370_chg_get_charge_type(priv, val); > + break; > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT: > + ret = mt6370_chg_get_ichg(priv, val); > + break; > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX: > + ret = mt6370_chg_get_max_ichg(priv, val); > + break; > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE: > + ret = mt6370_chg_get_cv(priv, val); > + break; > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX: > + ret = mt6370_chg_get_max_cv(priv, val); > + break; > + case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT: > + ret = mt6370_chg_get_aicr(priv, val); > + break; > + case POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT: > + ret = mt6370_chg_get_mivr(priv, val); > + break; > + case POWER_SUPPLY_PROP_PRECHARGE_CURRENT: > + ret = mt6370_chg_get_iprechg(priv, val); > + break; > + case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT: > + ret = mt6370_chg_get_ieoc(priv, val); > + break; > + case POWER_SUPPLY_PROP_TYPE: > + val->intval = priv->psy_desc->type; > + break; > + case POWER_SUPPLY_PROP_USB_TYPE: > + val->intval = priv->psy_usb_type; > + break; > + default: > + ret = -EINVAL; > + break; > + } > + > + return ret; In all cases, return directly. > +} ... > + switch (psp) { > + case POWER_SUPPLY_PROP_ONLINE: > + ret = mt6370_chg_set_online(priv, val); > + break; > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT: > + ret = mt6370_chg_set_ichg(priv, val); > + break; > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE: > + ret = mt6370_chg_set_cv(priv, val); > + break; > + case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT: > + ret = mt6370_chg_set_aicr(priv, val); > + break; > + case POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT: > + ret = mt6370_chg_set_mivr(priv, val); > + break; > + case POWER_SUPPLY_PROP_PRECHARGE_CURRENT: > + ret = mt6370_chg_set_iprechg(priv, val); > + break; > + case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT: > + ret = mt6370_chg_set_ieoc(priv, val); > + break; > + default: > + ret = -EINVAL; > + } > + return ret; As per above. ... > + for (i = 0; i < F_MAX; i++) { > + priv->rmap_fields[i] = devm_regmap_field_alloc(priv->dev, > + priv->regmap, > + fds[i].field); > + if (IS_ERR(priv->rmap_fields[i])) { > + dev_err(priv->dev, > + "Failed to allocate regmap field [%s]\n", > + fds[i].name); > + return PTR_ERR(priv->rmap_fields[i]); return dev_err_probe(); > + } > + } ... > + mutex_init(&priv->attach_lock); > + atomic_set(&priv->attach, 0); Why not atomic_init() ? But yeah, usage of it and other locking mechanisms in this driver are questionable. ... > + /* ICHG/IEOC Workaroud, ICHG can not be set less than 900mA */ Workaround ... > + return IS_ERR(priv->rdev) ? PTR_ERR(priv->rdev) : 0; PTR_ERR_OR_ZERO() ... > + .of_node = priv->dev->of_node, dev_of_node() ? > + }; > + > + priv->psy_desc = &mt6370_chg_psy_desc; > + priv->psy_desc->name = dev_name(priv->dev); > + priv->psy = devm_power_supply_register(priv->dev, priv->psy_desc, &cfg); > + > + return IS_ERR(priv->psy) ? PTR_ERR(priv->psy) : 0; PTR_ERR_OR_ZERO() > +} ... > +static irqreturn_t mt6370_attach_i_handler(int irq, void *data) > +{ > + struct mt6370_priv *priv = data; > + u32 otg_en; > + int ret; > + > + /* Check in otg mode or not */ > + ret = mt6370_chg_field_get(priv, F_BOOST_STAT, &otg_en); > + if (ret < 0) { > + dev_err(priv->dev, "failed to get otg state\n"); > + return IRQ_HANDLED; Handled error? > + } > + > + if (otg_en) > + return IRQ_HANDLED; > + mutex_lock(&priv->attach_lock); > + atomic_set(&priv->attach, MT6370_ATTACH_STAT_ATTACH_BC12_DONE); > + mutex_unlock(&priv->attach_lock); Mutex around atomic?! It's interesting... > + if (!queue_work(priv->wq, &priv->bc12_work)) > + dev_err(priv->dev, "bc12 work has already queued\n"); > + > + return IRQ_HANDLED; > +} ... > + for (i = 0; i < ARRAY_SIZE(mt6370_chg_irqs); i++) { > + ret = platform_get_irq_byname(to_platform_device(priv->dev), > + mt6370_chg_irqs[i].name); > + if (ret < 0) { > + dev_err(priv->dev, "Failed to get irq %s\n", > + mt6370_chg_irqs[i].name); Isn't the same printed by the above call? > + return ret; > + } > + > + ret = devm_request_threaded_irq(priv->dev, ret, NULL, > + mt6370_chg_irqs[i].handler, > + IRQF_TRIGGER_FALLING, > + dev_name(priv->dev), > + priv); > + > + if (ret < 0) { > + dev_err(priv->dev, "Failed to request irq %s\n", > + mt6370_chg_irqs[i].name); > + return ret; return dev_err_probe(); > + } > + } ... > +static int mt6370_chg_probe(struct platform_device *pdev) > +{ Use return dev_err_probe(...); pattern. > +probe_out: > + destroy_workqueue(priv->wq); > + mutex_destroy(&priv->attach_lock); I don't see clearly the initialization of these in the ->probe(). Besides that, does destroy_workque() synchronize the actual queue(s)? Mixing devm_ and non-devm_ may lead to a wrong release order that's why it is better to see allocating and destroying resources in one function (they may be wrapped, but should be both of them, seems like you have done it only for the first parts). > + return ret; > +} ... > +static int mt6370_chg_remove(struct platform_device *pdev) > +{ > + struct mt6370_priv *priv = platform_get_drvdata(pdev); > + > + if (priv) { Can you describe when this condition can be false? > + mt6370_chg_enable_irq(priv, "mivr", false); > + cancel_delayed_work_sync(&priv->mivr_dwork); > + destroy_workqueue(priv->wq); > + mutex_destroy(&priv->attach_lock); > + } > + > + return 0; > +} ... > +static struct platform_driver mt6370_chg_driver = { > + .probe = mt6370_chg_probe, > + .remove = mt6370_chg_remove, > + .driver = { > + .name = "mt6370-charger", > + .of_match_table = of_match_ptr(mt6370_chg_of_match), No good use of of_match_ptr(), please drop it. > + }, > +}; -- With Best Regards, Andy Shevchenko 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 gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (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 5F1F9C43334 for ; Thu, 23 Jun 2022 18:56:50 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 5F6C110E7E4; Thu, 23 Jun 2022 18:56:49 +0000 (UTC) Received: from mail-ed1-x535.google.com (mail-ed1-x535.google.com [IPv6:2a00:1450:4864:20::535]) by gabe.freedesktop.org (Postfix) with ESMTPS id 1099710E5A5 for ; Thu, 23 Jun 2022 18:56:48 +0000 (UTC) Received: by mail-ed1-x535.google.com with SMTP id z11so252083edp.9 for ; Thu, 23 Jun 2022 11:56:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=0uXSZHdS3mJpQ7ecdn7ixmyaAYanZgmJ20U9iKB3LvM=; b=FqD779LMhl6ZXXPI+YNjr+WVLo9W4neYkIeUS2q9scpUhb/MJ5Wy8Ao0Qg0FJpaJ+t uOkV3nBwCI/RA0oiO53gNqpaZBlKcfcudajWmGUow3OEyErMDToEMcBU6CtoED0uvZKx RwYZiks9bKE/9SlYHs3OyWJqCmg5TU6CDPyPlZ9eCZcz6RleyuDcLhnXb4k0QCZMcwcJ RY/+wYPvzAo192SLwWyBkGRZse9PovdAQaeEBC23Y/d4A0fA2Ue8oObgemI4GySj84RS hlD+1ZWuMdfA8b38t4lpEpx3UG1buNlGfIar4YHj2bNgvOXYQhcwB+StNIKY7+h24Boq IxcA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=0uXSZHdS3mJpQ7ecdn7ixmyaAYanZgmJ20U9iKB3LvM=; b=AqnkhYdYprG9lSINluz5cXHD6cKBnqRHu7MyjNwv/YduOaCj8ToqzJ2sLC0emQxz+I dF152Dxpc4ga8/XVAl1tCHPdfvznznemd1ODaxPTokcYA/9xZhR6JoG3i1g3uAY601mG ju04Z0KY5Nkgn5HwFzsapXnTZxWOKQK7csWAg382/PQ8n2fTWsP/dpCZtqXYzq608jYz oMHmR8GPOP/KBebyKUzauEL7BWsuz9XNZLHADtOZsxc8DN3a/AwNylL6wpQohmG+Kjoo lX9wTJLkZlzXNNlluDynj6lAyu0pB10Q8dr2BQOQydzdl84d65cPTDsHIAdunP0nBUsV d1dw== X-Gm-Message-State: AJIora+c1Ypt9rD+9KaY+pWSmHQH++YPSQ79KQ8+71QOxaYBT+Eq1buU HhsTSWoQbhsdm4FGPChBr3n1j2r2Q//LcoODZEU= X-Google-Smtp-Source: AGRyM1ubOzPazlKC5O9uY0fImpvLLf0ZLaU77Nrk9qWMXcLmh4hjsWTNf6/AOmeMsPByWzgewSmWWaXsLkwODbUFAW0= X-Received: by 2002:a05:6402:249e:b0:42d:bb88:865b with SMTP id q30-20020a056402249e00b0042dbb88865bmr12356084eda.141.1656010606271; Thu, 23 Jun 2022 11:56:46 -0700 (PDT) MIME-Version: 1.0 References: <20220623115631.22209-1-peterwu.pub@gmail.com> <20220623115631.22209-12-peterwu.pub@gmail.com> In-Reply-To: <20220623115631.22209-12-peterwu.pub@gmail.com> From: Andy Shevchenko Date: Thu, 23 Jun 2022 20:56:09 +0200 Message-ID: Subject: Re: [PATCH v3 11/14] power: supply: mt6370: Add Mediatek MT6370 charger driver To: ChiaEn Wu Content-Type: text/plain; charset="UTF-8" X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "open list:FRAMEBUFFER LAYER" , "Krogerus, Heikki" , Krzysztof Kozlowski , alice_chen@richtek.com, linux-iio , dri-devel , Liam Girdwood , cy_huang , Pavel Machek , Lee Jones , Linux LED Subsystem , Daniel Thompson , Helge Deller , Rob Herring , Chunfeng Yun , Guenter Roeck , devicetree , Linux PM , szuni chen , Mark Brown , "moderated list:ARM/Mediatek SoC support" , Matthias Brugger , linux-arm Mailing List , Jingoo Han , USB , Sebastian Reichel , Linux Kernel Mailing List , chiaen_wu@richtek.com, Greg Kroah-Hartman , Jonathan Cameron Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On Thu, Jun 23, 2022 at 2:00 PM ChiaEn Wu wrote: > > From: ChiaEn Wu > > Add Mediatek MT6370 charger driver. ... > +config CHARGER_MT6370 > + tristate "Mediatek MT6370 Charger Driver" > + depends on MFD_MT6370 > + depends on REGULATOR > + select LINEAR_RANGES > + help > + Say Y here to enable MT6370 Charger Part. > + The device supports High-Accuracy Voltage/Current Regulation, > + Average Input Current Regulation, Battery Temperature Sensing, > + Over-Temperature Protection, DPDM Detection for BC1.2. Module name? ... > +#include This usually goes after linux/* > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include ... > +#define MT6370_MIVR_IBUS_TH 100000 /* 100 mA */ Instead of comment, add proper units. ... > + MT6370_USB_STAT_DCP, > + MT6370_USB_STAT_CDP, > + MT6370_USB_STAT_MAX, No comma for a terminator line. ... > +static inline u32 mt6370_chg_val_to_reg(const struct mt6370_chg_range *range, > + u32 val) > +static inline u32 mt6370_chg_reg_to_val(const struct mt6370_chg_range *range, > + u8 reg) I'm wondering if you can use the https://elixir.bootlin.com/linux/v5.19-rc3/source/include/linux/linear_range.h APIs. ... > + int ret = 0; This seems a redundant assignment, see below. > + rcfg->ena_gpiod = fwnode_gpiod_get_index(of_fwnode_handle(of), > + "enable", 0, For index == 0 don't use _index API. > + GPIOD_OUT_LOW | > + GPIOD_FLAGS_BIT_NONEXCLUSIVE, > + rdesc->name); > + if (IS_ERR(rcfg->ena_gpiod)) { > + dev_err(priv->dev, "Failed to requeset OTG EN Pin\n"); request > + rcfg->ena_gpiod = NULL; So, use _optional and return any errors you got. > + } else { > + val = MT6370_OPA_MODE_MASK | MT6370_OTG_PIN_EN_MASK; > + ret = regmap_update_bits(priv->regmap, MT6370_REG_CHG_CTRL1, > + val, val); > + if (ret) > + dev_err(priv->dev, "Failed to set otg bits\n"); > + } ... > + irq_num = platform_get_irq_byname(pdev, irq_name); > + Unwanted blank line. > + if (irq_num < 0) { > + dev_err(priv->dev, "Failed to get platform resource\n"); Isn't it printed by the call? > + } else { > + if (en) > + enable_irq(irq_num); > + else > + disable_irq_nosync(irq_num); > + } ... > +toggle_cfo_exit: The useless label. > + return ret; > +} ... > + ret = mt6370_chg_get_online(priv, val); > + if (!val->intval) { No error check? > + val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > + return 0; > + } ... > +static int mt6370_chg_set_online(struct mt6370_priv *priv, > + const union power_supply_propval *val) > +{ > + int attach; > + u32 pwr_rdy = !!val->intval; > + > + mutex_lock(&priv->attach_lock); > + attach = atomic_read(&priv->attach); > + if (pwr_rdy == !!attach) { > + dev_err(priv->dev, "pwr_rdy is same(%d)\n", pwr_rdy); > + mutex_unlock(&priv->attach_lock); > + return 0; > + } > + > + atomic_set(&priv->attach, pwr_rdy); > + mutex_unlock(&priv->attach_lock); > + > + if (!queue_work(priv->wq, &priv->bc12_work)) > + dev_err(priv->dev, "bc12 work has already queued\n"); > + > + return 0; > + Unwanted blank line. > +} > +static int mt6370_chg_get_property(struct power_supply *psy, > + enum power_supply_property psp, > + union power_supply_propval *val) > +{ > + struct mt6370_priv *priv = power_supply_get_drvdata(psy); > + int ret = 0; > + > + switch (psp) { > + case POWER_SUPPLY_PROP_ONLINE: > + ret = mt6370_chg_get_online(priv, val); > + break; > + case POWER_SUPPLY_PROP_STATUS: > + ret = mt6370_chg_get_status(priv, val); > + break; > + case POWER_SUPPLY_PROP_CHARGE_TYPE: > + ret = mt6370_chg_get_charge_type(priv, val); > + break; > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT: > + ret = mt6370_chg_get_ichg(priv, val); > + break; > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX: > + ret = mt6370_chg_get_max_ichg(priv, val); > + break; > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE: > + ret = mt6370_chg_get_cv(priv, val); > + break; > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX: > + ret = mt6370_chg_get_max_cv(priv, val); > + break; > + case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT: > + ret = mt6370_chg_get_aicr(priv, val); > + break; > + case POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT: > + ret = mt6370_chg_get_mivr(priv, val); > + break; > + case POWER_SUPPLY_PROP_PRECHARGE_CURRENT: > + ret = mt6370_chg_get_iprechg(priv, val); > + break; > + case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT: > + ret = mt6370_chg_get_ieoc(priv, val); > + break; > + case POWER_SUPPLY_PROP_TYPE: > + val->intval = priv->psy_desc->type; > + break; > + case POWER_SUPPLY_PROP_USB_TYPE: > + val->intval = priv->psy_usb_type; > + break; > + default: > + ret = -EINVAL; > + break; > + } > + > + return ret; In all cases, return directly. > +} ... > + switch (psp) { > + case POWER_SUPPLY_PROP_ONLINE: > + ret = mt6370_chg_set_online(priv, val); > + break; > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT: > + ret = mt6370_chg_set_ichg(priv, val); > + break; > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE: > + ret = mt6370_chg_set_cv(priv, val); > + break; > + case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT: > + ret = mt6370_chg_set_aicr(priv, val); > + break; > + case POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT: > + ret = mt6370_chg_set_mivr(priv, val); > + break; > + case POWER_SUPPLY_PROP_PRECHARGE_CURRENT: > + ret = mt6370_chg_set_iprechg(priv, val); > + break; > + case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT: > + ret = mt6370_chg_set_ieoc(priv, val); > + break; > + default: > + ret = -EINVAL; > + } > + return ret; As per above. ... > + for (i = 0; i < F_MAX; i++) { > + priv->rmap_fields[i] = devm_regmap_field_alloc(priv->dev, > + priv->regmap, > + fds[i].field); > + if (IS_ERR(priv->rmap_fields[i])) { > + dev_err(priv->dev, > + "Failed to allocate regmap field [%s]\n", > + fds[i].name); > + return PTR_ERR(priv->rmap_fields[i]); return dev_err_probe(); > + } > + } ... > + mutex_init(&priv->attach_lock); > + atomic_set(&priv->attach, 0); Why not atomic_init() ? But yeah, usage of it and other locking mechanisms in this driver are questionable. ... > + /* ICHG/IEOC Workaroud, ICHG can not be set less than 900mA */ Workaround ... > + return IS_ERR(priv->rdev) ? PTR_ERR(priv->rdev) : 0; PTR_ERR_OR_ZERO() ... > + .of_node = priv->dev->of_node, dev_of_node() ? > + }; > + > + priv->psy_desc = &mt6370_chg_psy_desc; > + priv->psy_desc->name = dev_name(priv->dev); > + priv->psy = devm_power_supply_register(priv->dev, priv->psy_desc, &cfg); > + > + return IS_ERR(priv->psy) ? PTR_ERR(priv->psy) : 0; PTR_ERR_OR_ZERO() > +} ... > +static irqreturn_t mt6370_attach_i_handler(int irq, void *data) > +{ > + struct mt6370_priv *priv = data; > + u32 otg_en; > + int ret; > + > + /* Check in otg mode or not */ > + ret = mt6370_chg_field_get(priv, F_BOOST_STAT, &otg_en); > + if (ret < 0) { > + dev_err(priv->dev, "failed to get otg state\n"); > + return IRQ_HANDLED; Handled error? > + } > + > + if (otg_en) > + return IRQ_HANDLED; > + mutex_lock(&priv->attach_lock); > + atomic_set(&priv->attach, MT6370_ATTACH_STAT_ATTACH_BC12_DONE); > + mutex_unlock(&priv->attach_lock); Mutex around atomic?! It's interesting... > + if (!queue_work(priv->wq, &priv->bc12_work)) > + dev_err(priv->dev, "bc12 work has already queued\n"); > + > + return IRQ_HANDLED; > +} ... > + for (i = 0; i < ARRAY_SIZE(mt6370_chg_irqs); i++) { > + ret = platform_get_irq_byname(to_platform_device(priv->dev), > + mt6370_chg_irqs[i].name); > + if (ret < 0) { > + dev_err(priv->dev, "Failed to get irq %s\n", > + mt6370_chg_irqs[i].name); Isn't the same printed by the above call? > + return ret; > + } > + > + ret = devm_request_threaded_irq(priv->dev, ret, NULL, > + mt6370_chg_irqs[i].handler, > + IRQF_TRIGGER_FALLING, > + dev_name(priv->dev), > + priv); > + > + if (ret < 0) { > + dev_err(priv->dev, "Failed to request irq %s\n", > + mt6370_chg_irqs[i].name); > + return ret; return dev_err_probe(); > + } > + } ... > +static int mt6370_chg_probe(struct platform_device *pdev) > +{ Use return dev_err_probe(...); pattern. > +probe_out: > + destroy_workqueue(priv->wq); > + mutex_destroy(&priv->attach_lock); I don't see clearly the initialization of these in the ->probe(). Besides that, does destroy_workque() synchronize the actual queue(s)? Mixing devm_ and non-devm_ may lead to a wrong release order that's why it is better to see allocating and destroying resources in one function (they may be wrapped, but should be both of them, seems like you have done it only for the first parts). > + return ret; > +} ... > +static int mt6370_chg_remove(struct platform_device *pdev) > +{ > + struct mt6370_priv *priv = platform_get_drvdata(pdev); > + > + if (priv) { Can you describe when this condition can be false? > + mt6370_chg_enable_irq(priv, "mivr", false); > + cancel_delayed_work_sync(&priv->mivr_dwork); > + destroy_workqueue(priv->wq); > + mutex_destroy(&priv->attach_lock); > + } > + > + return 0; > +} ... > +static struct platform_driver mt6370_chg_driver = { > + .probe = mt6370_chg_probe, > + .remove = mt6370_chg_remove, > + .driver = { > + .name = "mt6370-charger", > + .of_match_table = of_match_ptr(mt6370_chg_of_match), No good use of of_match_ptr(), please drop it. > + }, > +}; -- With Best Regards, Andy Shevchenko 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 AAA21C433EF for ; Thu, 23 Jun 2022 18:57:58 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Cc:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=kF66V3CAbWwPCmoO/WyiqYoTJ5Z/r3FQHpd2hWgOBaM=; b=EVm1Gzfgs3ANq4 iu81SXQ7QsnqHVXACZj/RZP0kDJcLG3AnVflPWaU7vo56As56mXUH23Cb/yUZ+Z7t5/XnWonnHKY+ vMkEBKjGahYgh8cUedbVipc3nyoaprXLSuvQbHQ7+KHXCPE42bJFp9pcUEWWD2Ca3UT0gsO1wUG3Q HDYXLg9kafRZ14i+8qoI9Foq0s4eG7/iWKlwAKRsoFZYLA473kwhP/ziVPZLY1ws1Z3p4TBOfefHZ OiMOPwglcxQhDW2Ia0jeFXlWQjhIrq9Zr9YFlk/UTmJPyfAEwQR3QZRjIgtNQWtFOJlYzd/4UAvtG cN5fmudqoeS7lYhS1wjQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1o4S0a-00GOq7-7F; Thu, 23 Jun 2022 18:56:52 +0000 Received: from mail-ed1-x52b.google.com ([2a00:1450:4864:20::52b]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1o4S0W-00GOns-Mm; Thu, 23 Jun 2022 18:56:50 +0000 Received: by mail-ed1-x52b.google.com with SMTP id ej4so267838edb.7; Thu, 23 Jun 2022 11:56:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=0uXSZHdS3mJpQ7ecdn7ixmyaAYanZgmJ20U9iKB3LvM=; b=FqD779LMhl6ZXXPI+YNjr+WVLo9W4neYkIeUS2q9scpUhb/MJ5Wy8Ao0Qg0FJpaJ+t uOkV3nBwCI/RA0oiO53gNqpaZBlKcfcudajWmGUow3OEyErMDToEMcBU6CtoED0uvZKx RwYZiks9bKE/9SlYHs3OyWJqCmg5TU6CDPyPlZ9eCZcz6RleyuDcLhnXb4k0QCZMcwcJ RY/+wYPvzAo192SLwWyBkGRZse9PovdAQaeEBC23Y/d4A0fA2Ue8oObgemI4GySj84RS hlD+1ZWuMdfA8b38t4lpEpx3UG1buNlGfIar4YHj2bNgvOXYQhcwB+StNIKY7+h24Boq IxcA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=0uXSZHdS3mJpQ7ecdn7ixmyaAYanZgmJ20U9iKB3LvM=; b=DDBXJFe1xX4Xbm3+WrDTE4wwIp8bF906HkWrvYpxdKEmal8a1Gj9t40srfYrwRSBxy tBNH+Il6pZwPbp+HwIxLT7Q55yvs7qmUwXiL+fcHumki5IuwKvfsqSYxvayNuXV9iKC9 5xJ4gm8i3s/XhDna1t8O5bDTPfidNZIyEyQCjEvCMpsVrXaeO9wJnPxopsA98wRiSih2 vEV09OORzxHaREUb48Wh0vKZKaiu8WMQ/kL0Pj4SNtK9q8qonXsfTXHpnN6yTykGcdfF +bVvrFOvXg46XGS7I5rpEbKDxGLtyHyDo32zg9+q2OYUs3M4zFHzmx0719uDjPxsLcgh DrJA== X-Gm-Message-State: AJIora8HqhDWAofon9JWod2OJJQI/nsebwl2kHJ5hfCnCn7sU+KCpe2G P7Acx3wJ1n0vK5LgbHWxGNSGYJ9dtnaigmxr2X0= X-Google-Smtp-Source: AGRyM1ubOzPazlKC5O9uY0fImpvLLf0ZLaU77Nrk9qWMXcLmh4hjsWTNf6/AOmeMsPByWzgewSmWWaXsLkwODbUFAW0= X-Received: by 2002:a05:6402:249e:b0:42d:bb88:865b with SMTP id q30-20020a056402249e00b0042dbb88865bmr12356084eda.141.1656010606271; Thu, 23 Jun 2022 11:56:46 -0700 (PDT) MIME-Version: 1.0 References: <20220623115631.22209-1-peterwu.pub@gmail.com> <20220623115631.22209-12-peterwu.pub@gmail.com> In-Reply-To: <20220623115631.22209-12-peterwu.pub@gmail.com> From: Andy Shevchenko Date: Thu, 23 Jun 2022 20:56:09 +0200 Message-ID: Subject: Re: [PATCH v3 11/14] power: supply: mt6370: Add Mediatek MT6370 charger driver To: ChiaEn Wu Cc: Lee Jones , Daniel Thompson , Jingoo Han , Pavel Machek , Rob Herring , Krzysztof Kozlowski , Matthias Brugger , Sebastian Reichel , Chunfeng Yun , Greg Kroah-Hartman , Jonathan Cameron , Lars-Peter Clausen , Liam Girdwood , Mark Brown , Guenter Roeck , "Krogerus, Heikki" , Helge Deller , chiaen_wu@richtek.com, alice_chen@richtek.com, cy_huang , dri-devel , Linux LED Subsystem , devicetree , linux-arm Mailing List , "moderated list:ARM/Mediatek SoC support" , Linux Kernel Mailing List , Linux PM , USB , linux-iio , "open list:FRAMEBUFFER LAYER" , szuni chen X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220623_115648_795951_DA0C89D8 X-CRM114-Status: GOOD ( 30.24 ) 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: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Jun 23, 2022 at 2:00 PM ChiaEn Wu wrote: > > From: ChiaEn Wu > > Add Mediatek MT6370 charger driver. ... > +config CHARGER_MT6370 > + tristate "Mediatek MT6370 Charger Driver" > + depends on MFD_MT6370 > + depends on REGULATOR > + select LINEAR_RANGES > + help > + Say Y here to enable MT6370 Charger Part. > + The device supports High-Accuracy Voltage/Current Regulation, > + Average Input Current Regulation, Battery Temperature Sensing, > + Over-Temperature Protection, DPDM Detection for BC1.2. Module name? ... > +#include This usually goes after linux/* > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include ... > +#define MT6370_MIVR_IBUS_TH 100000 /* 100 mA */ Instead of comment, add proper units. ... > + MT6370_USB_STAT_DCP, > + MT6370_USB_STAT_CDP, > + MT6370_USB_STAT_MAX, No comma for a terminator line. ... > +static inline u32 mt6370_chg_val_to_reg(const struct mt6370_chg_range *range, > + u32 val) > +static inline u32 mt6370_chg_reg_to_val(const struct mt6370_chg_range *range, > + u8 reg) I'm wondering if you can use the https://elixir.bootlin.com/linux/v5.19-rc3/source/include/linux/linear_range.h APIs. ... > + int ret = 0; This seems a redundant assignment, see below. > + rcfg->ena_gpiod = fwnode_gpiod_get_index(of_fwnode_handle(of), > + "enable", 0, For index == 0 don't use _index API. > + GPIOD_OUT_LOW | > + GPIOD_FLAGS_BIT_NONEXCLUSIVE, > + rdesc->name); > + if (IS_ERR(rcfg->ena_gpiod)) { > + dev_err(priv->dev, "Failed to requeset OTG EN Pin\n"); request > + rcfg->ena_gpiod = NULL; So, use _optional and return any errors you got. > + } else { > + val = MT6370_OPA_MODE_MASK | MT6370_OTG_PIN_EN_MASK; > + ret = regmap_update_bits(priv->regmap, MT6370_REG_CHG_CTRL1, > + val, val); > + if (ret) > + dev_err(priv->dev, "Failed to set otg bits\n"); > + } ... > + irq_num = platform_get_irq_byname(pdev, irq_name); > + Unwanted blank line. > + if (irq_num < 0) { > + dev_err(priv->dev, "Failed to get platform resource\n"); Isn't it printed by the call? > + } else { > + if (en) > + enable_irq(irq_num); > + else > + disable_irq_nosync(irq_num); > + } ... > +toggle_cfo_exit: The useless label. > + return ret; > +} ... > + ret = mt6370_chg_get_online(priv, val); > + if (!val->intval) { No error check? > + val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > + return 0; > + } ... > +static int mt6370_chg_set_online(struct mt6370_priv *priv, > + const union power_supply_propval *val) > +{ > + int attach; > + u32 pwr_rdy = !!val->intval; > + > + mutex_lock(&priv->attach_lock); > + attach = atomic_read(&priv->attach); > + if (pwr_rdy == !!attach) { > + dev_err(priv->dev, "pwr_rdy is same(%d)\n", pwr_rdy); > + mutex_unlock(&priv->attach_lock); > + return 0; > + } > + > + atomic_set(&priv->attach, pwr_rdy); > + mutex_unlock(&priv->attach_lock); > + > + if (!queue_work(priv->wq, &priv->bc12_work)) > + dev_err(priv->dev, "bc12 work has already queued\n"); > + > + return 0; > + Unwanted blank line. > +} > +static int mt6370_chg_get_property(struct power_supply *psy, > + enum power_supply_property psp, > + union power_supply_propval *val) > +{ > + struct mt6370_priv *priv = power_supply_get_drvdata(psy); > + int ret = 0; > + > + switch (psp) { > + case POWER_SUPPLY_PROP_ONLINE: > + ret = mt6370_chg_get_online(priv, val); > + break; > + case POWER_SUPPLY_PROP_STATUS: > + ret = mt6370_chg_get_status(priv, val); > + break; > + case POWER_SUPPLY_PROP_CHARGE_TYPE: > + ret = mt6370_chg_get_charge_type(priv, val); > + break; > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT: > + ret = mt6370_chg_get_ichg(priv, val); > + break; > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX: > + ret = mt6370_chg_get_max_ichg(priv, val); > + break; > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE: > + ret = mt6370_chg_get_cv(priv, val); > + break; > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX: > + ret = mt6370_chg_get_max_cv(priv, val); > + break; > + case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT: > + ret = mt6370_chg_get_aicr(priv, val); > + break; > + case POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT: > + ret = mt6370_chg_get_mivr(priv, val); > + break; > + case POWER_SUPPLY_PROP_PRECHARGE_CURRENT: > + ret = mt6370_chg_get_iprechg(priv, val); > + break; > + case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT: > + ret = mt6370_chg_get_ieoc(priv, val); > + break; > + case POWER_SUPPLY_PROP_TYPE: > + val->intval = priv->psy_desc->type; > + break; > + case POWER_SUPPLY_PROP_USB_TYPE: > + val->intval = priv->psy_usb_type; > + break; > + default: > + ret = -EINVAL; > + break; > + } > + > + return ret; In all cases, return directly. > +} ... > + switch (psp) { > + case POWER_SUPPLY_PROP_ONLINE: > + ret = mt6370_chg_set_online(priv, val); > + break; > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT: > + ret = mt6370_chg_set_ichg(priv, val); > + break; > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE: > + ret = mt6370_chg_set_cv(priv, val); > + break; > + case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT: > + ret = mt6370_chg_set_aicr(priv, val); > + break; > + case POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT: > + ret = mt6370_chg_set_mivr(priv, val); > + break; > + case POWER_SUPPLY_PROP_PRECHARGE_CURRENT: > + ret = mt6370_chg_set_iprechg(priv, val); > + break; > + case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT: > + ret = mt6370_chg_set_ieoc(priv, val); > + break; > + default: > + ret = -EINVAL; > + } > + return ret; As per above. ... > + for (i = 0; i < F_MAX; i++) { > + priv->rmap_fields[i] = devm_regmap_field_alloc(priv->dev, > + priv->regmap, > + fds[i].field); > + if (IS_ERR(priv->rmap_fields[i])) { > + dev_err(priv->dev, > + "Failed to allocate regmap field [%s]\n", > + fds[i].name); > + return PTR_ERR(priv->rmap_fields[i]); return dev_err_probe(); > + } > + } ... > + mutex_init(&priv->attach_lock); > + atomic_set(&priv->attach, 0); Why not atomic_init() ? But yeah, usage of it and other locking mechanisms in this driver are questionable. ... > + /* ICHG/IEOC Workaroud, ICHG can not be set less than 900mA */ Workaround ... > + return IS_ERR(priv->rdev) ? PTR_ERR(priv->rdev) : 0; PTR_ERR_OR_ZERO() ... > + .of_node = priv->dev->of_node, dev_of_node() ? > + }; > + > + priv->psy_desc = &mt6370_chg_psy_desc; > + priv->psy_desc->name = dev_name(priv->dev); > + priv->psy = devm_power_supply_register(priv->dev, priv->psy_desc, &cfg); > + > + return IS_ERR(priv->psy) ? PTR_ERR(priv->psy) : 0; PTR_ERR_OR_ZERO() > +} ... > +static irqreturn_t mt6370_attach_i_handler(int irq, void *data) > +{ > + struct mt6370_priv *priv = data; > + u32 otg_en; > + int ret; > + > + /* Check in otg mode or not */ > + ret = mt6370_chg_field_get(priv, F_BOOST_STAT, &otg_en); > + if (ret < 0) { > + dev_err(priv->dev, "failed to get otg state\n"); > + return IRQ_HANDLED; Handled error? > + } > + > + if (otg_en) > + return IRQ_HANDLED; > + mutex_lock(&priv->attach_lock); > + atomic_set(&priv->attach, MT6370_ATTACH_STAT_ATTACH_BC12_DONE); > + mutex_unlock(&priv->attach_lock); Mutex around atomic?! It's interesting... > + if (!queue_work(priv->wq, &priv->bc12_work)) > + dev_err(priv->dev, "bc12 work has already queued\n"); > + > + return IRQ_HANDLED; > +} ... > + for (i = 0; i < ARRAY_SIZE(mt6370_chg_irqs); i++) { > + ret = platform_get_irq_byname(to_platform_device(priv->dev), > + mt6370_chg_irqs[i].name); > + if (ret < 0) { > + dev_err(priv->dev, "Failed to get irq %s\n", > + mt6370_chg_irqs[i].name); Isn't the same printed by the above call? > + return ret; > + } > + > + ret = devm_request_threaded_irq(priv->dev, ret, NULL, > + mt6370_chg_irqs[i].handler, > + IRQF_TRIGGER_FALLING, > + dev_name(priv->dev), > + priv); > + > + if (ret < 0) { > + dev_err(priv->dev, "Failed to request irq %s\n", > + mt6370_chg_irqs[i].name); > + return ret; return dev_err_probe(); > + } > + } ... > +static int mt6370_chg_probe(struct platform_device *pdev) > +{ Use return dev_err_probe(...); pattern. > +probe_out: > + destroy_workqueue(priv->wq); > + mutex_destroy(&priv->attach_lock); I don't see clearly the initialization of these in the ->probe(). Besides that, does destroy_workque() synchronize the actual queue(s)? Mixing devm_ and non-devm_ may lead to a wrong release order that's why it is better to see allocating and destroying resources in one function (they may be wrapped, but should be both of them, seems like you have done it only for the first parts). > + return ret; > +} ... > +static int mt6370_chg_remove(struct platform_device *pdev) > +{ > + struct mt6370_priv *priv = platform_get_drvdata(pdev); > + > + if (priv) { Can you describe when this condition can be false? > + mt6370_chg_enable_irq(priv, "mivr", false); > + cancel_delayed_work_sync(&priv->mivr_dwork); > + destroy_workqueue(priv->wq); > + mutex_destroy(&priv->attach_lock); > + } > + > + return 0; > +} ... > +static struct platform_driver mt6370_chg_driver = { > + .probe = mt6370_chg_probe, > + .remove = mt6370_chg_remove, > + .driver = { > + .name = "mt6370-charger", > + .of_match_table = of_match_ptr(mt6370_chg_of_match), No good use of of_match_ptr(), please drop it. > + }, > +}; -- With Best Regards, Andy Shevchenko _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel