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 B6120C43334 for ; Fri, 22 Jul 2022 10:13:37 +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-Type: Content-Transfer-Encoding:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:From:References:Cc:To:Subject: MIME-Version:Date:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=pJbd/AzoH37mA00CXcGqoYloZafskQcFjvsdk3ntx4g=; b=DXqUHKt/NQZKkJ RT3wPRvzB9TRCJ6HRVeFHCR0yF6RFIMYlMfJy7Mv7nN+rpSsvcrI5Y7nDj2AKVAtWb0IiNFsZuJ9W y3s5vtlQFPqQj0S883HijFEAoCaEscxW3IXkERZLwh6uyXywDkEn939CGBwo/eSxpiTnNeM2tX4HF eztoa5oWnMbrOgu+bVHGz3n/e4tay4uvSa/q3p15efreEqidjN+Ezxa1P21Z9wF+aFVteNEMhRi8V puPMOnmbGK/Gav1dC5ZTEsQ3lvuh4IlTeV/tRE8SJI/ajND/t2wwKwM2slDlVkWkAhIkPU0/+PugJ JOBZk4X9mCdvm7pWeQnw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oEpdo-002PGi-Sl; Fri, 22 Jul 2022 10:12:17 +0000 Received: from mail-wr1-x434.google.com ([2a00:1450:4864:20::434]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oEpdj-002Ox2-D2 for linux-arm-kernel@lists.infradead.org; Fri, 22 Jul 2022 10:12:14 +0000 Received: by mail-wr1-x434.google.com with SMTP id u5so5851466wrm.4 for ; Fri, 22 Jul 2022 03:12:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=baylibre-com.20210112.gappssmtp.com; s=20210112; h=message-id:date:mime-version:user-agent:subject:content-language:to :cc:references:from:in-reply-to:content-transfer-encoding; bh=Qxv2ZhAI5MpFdkuiCRvtOt/8KqoKWgf/252g6XKRWx8=; b=oJx0GRmFxA6+ajzy2KolNsEFTF1kyeoaTAQaRIzv5/SWIy0DM5VqveWvKM/STHymUF KKY88AZp0hIAYLfwsJmu3GJFWC+OMpla0TjqeSftOs/3df1Jq9De7Q2t8jPtSJFJ52+m JLylmGZF4WiPErIgDfrHfe/5lwAWk/jrf3YaIAyERe0oCkm3xCSCq5+WcBxZtRxaI03+ ZZDhW8KjvSgA5PBCzDWnEb6R/rO+xJEmlOfsCzHXWSgCrVBgUSd9lcyGyApvRLCw30vL vVziejAq4SrPkpQJEpAxoNZIBeZ1UTe1JaemfLhDCfIJ2Fhhe05zkXLq5K3P5tQ8jObN Tqng== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=Qxv2ZhAI5MpFdkuiCRvtOt/8KqoKWgf/252g6XKRWx8=; b=yeivmlQT2H6fgVG/1bVh7ulLxiM8wchMDllKufpAWaq/AiwKFDQOaKq54ndnhlHV0W D3NLOFT7BxEDZZDRQcBituBtGa6I5eFXhISdXS8ldmFFm7wmpoOqU+FsMdTw87D0zkdl MGtFL8/HuVb1waI0QaW26KEnD0C3ghYTYZ2+b20u+xbCXnd3IRpV/sMIYAPuXG8B8xr4 xlNon5+MZzSZHWufyeF+ocXZYTqbdVApI1kkUxwVQ0wB3BR6Yjwp2qmndCWmgmc0lqA3 ZqOyyBKl9a4fmI1n6C23pX/hniADtiEUk1194aMa1CTy8qvjtbuy9bYrLxddcUQVAMQN ZDow== X-Gm-Message-State: AJIora8sNyk09oqfFFOY3OHGomn+Itcl9p0nzOvJ1WWmn7laJZ0wBcuL LA7OzcaGJk9lOU7Az7piAtAPSg== X-Google-Smtp-Source: AGRyM1t92A+Ond+L5Gp2qLwoiAC/AfSjImgpKsz47kk5lRbpk6gCGR7r9lSleY/Svw+/yIe1tqxtcQ== X-Received: by 2002:a5d:67ca:0:b0:21e:5ba8:a067 with SMTP id n10-20020a5d67ca000000b0021e5ba8a067mr2011825wrw.650.1658484726796; Fri, 22 Jul 2022 03:12:06 -0700 (PDT) Received: from [10.2.4.117] (lfbn-nic-1-76-188.w2-15.abo.wanadoo.fr. [2.15.166.188]) by smtp.gmail.com with ESMTPSA id w10-20020adfde8a000000b0021e50971147sm4084202wrl.44.2022.07.22.03.12.05 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 22 Jul 2022 03:12:06 -0700 (PDT) Message-ID: Date: Fri, 22 Jul 2022 12:12:05 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH v1 08/14] regulator: drivers: Add TI TPS65219 PMIC regulators support Content-Language: en-US To: Mark Brown Cc: lgirdwood@gmail.com, robh+dt@kernel.org, nm@ti.com, kristo@kernel.org, khilman@baylibre.com, narmstrong@baylibre.com, msp@baylibre.com, j-keerthy@ti.c, lee.jones@linaro.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org References: <20220719091742.3221-1-jneanne@baylibre.com> <20220719091742.3221-9-jneanne@baylibre.com> From: jerome Neanne In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220722_031211_689151_534B081F X-CRM114-Status: GOOD ( 28.36 ) 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-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 19/07/2022 15:32, Mark Brown wrote: > On Tue, Jul 19, 2022 at 11:17:36AM +0200, Jerome Neanne wrote: > >> @@ -0,0 +1,414 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * tps65219-regulator.c >> + * > Please make the entire comment a C++ one so things look more > intentional. > >> +static int tps65219_pmic_set_voltage_sel(struct regulator_dev *dev, >> + unsigned int selector) >> +{ >> + int ret; >> + struct tps65219 *tps = rdev_get_drvdata(dev); >> + >> + /* Set the voltage based on vsel value */ >> + ret = regmap_update_bits(tps->regmap, dev->desc->vsel_reg, >> + dev->desc->vsel_mask, selector); >> + if (ret) { >> + dev_dbg(tps->dev, "%s failed for regulator %s: %d ", >> + __func__, dev->desc->name, ret); >> + } >> + return ret; >> +} > This should just be able to use the standard regmap helper, as should > the enable and disable operations? > >> +static int tps65219_set_mode(struct regulator_dev *dev, unsigned int mode) >> +{ >> + struct tps65219 *tps = rdev_get_drvdata(dev); >> + >> + switch (mode) { >> + case REGULATOR_MODE_NORMAL: >> + return regmap_set_bits(tps->regmap, TPS65219_REG_STBY_1_CONFIG, >> + dev->desc->enable_mask); >> + >> + case REGULATOR_MODE_STANDBY: >> + return regmap_clear_bits(tps->regmap, >> + TPS65219_REG_STBY_1_CONFIG, >> + dev->desc->enable_mask); >> + } >> + >> + return -EINVAL; > It'd be a little clearer to have that -EINVAL in a default statement. > >> +static irqreturn_t tps65219_regulator_irq_handler(int irq, void *data) >> +{ >> + struct tps65219_regulator_irq_data *irq_data = data; >> + >> + if (irq_data->type->event_name[0] == '\0') { >> + /* This is the timeout interrupt */ >> + dev_err(irq_data->dev, "System was put in shutdown during an active or standby transition.\n"); >> + return IRQ_HANDLED; >> + } >> + >> + dev_err(irq_data->dev, "Registered %s for %s\n", >> + irq_data->type->event_name, irq_data->type->regulator_name); > This should be reporting the events through the notification API, see > regulator_notifier_call_chain(). That will require a bit of refactoring > of the way the driver is registering interrupts unfortunately, at the > minute it doesn't have data joining them up with the > > I'd also reword that log message to be something more like "Error %s > reported for %s" - at the minute it looks more like a probe message. > > Otherwise this looks good. Thanks for your review. Refactoring the code with regulator_notifier_call_chain, I realized that some of the events in TPS65219 are not listed as standard REGULATOR_EVENT in consumer.h This is the case for below event list: REGULATOR_EVENT_SCG (ShortCut to Gnd) REGULATOR_EVENT_RV (Residual Voltage) REGULATOR_EVENT_RV_SD (Residual Voltage ShutDown) Should I add those events to the list of standard regulator events and assign a code? (if yes, any rule for the values?) Would it fit with some other predefined standard macro defined elsewhere? (if yes, could you point me to the right location?) > @@ -0,0 +1,414 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * tps65219-regulator.c > + * Please make the entire comment a C++ one so things look more intentional. checkpatch is complaining about that: --------------------------------------------------------------------- v5.19-rc6-PB-MSP1/0005-mfd-drivers-Add-TI-TPS65219-PMIC-support.patch --------------------------------------------------------------------- WARNING: Improper SPDX comment style for 'drivers/mfd/tps65219.c', please use '//' instead #91: FILE: drivers/mfd/tps65219.c:1: +/* SPDX-License-Identifier: GPL-2.0 Let me know if I should ignore checkpatch recommendations here. Regards, Jerome _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel