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 99E11C43334 for ; Tue, 19 Jul 2022 13:34:07 +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: List-Subscribe:List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id: In-Reply-To:MIME-Version:References:Message-ID:Subject:Cc:To:From:Date: 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=qvXj2rHbNB/5/IGY8WaWIPyUZBFkQKYoVfoMyiOiaZ8=; b=ddhnQmypJgw/4eV+a/z9aFOvAD ZhArH665vLODp13MpQ4kcaVHVE0fCTd0pt5exfUOEUeYbyvZxNe2e01smjccP9p5kqmDvScthcDBt uhc4+uyx1Kh+GQTbZWlFSv97fuR96ZUN4cEIKnxtsv9SftNaGpGYke8cRqTwr+6PdJwKpp4ychJ/Z QwPlPG0O4Y2bdJOoSPxz7Is8Ir5y6bVOY1a3ldXl8/pxm58wNCSbkhbcClTQtv5EFYgNy7w09PkBO 03/1IqKsB7S37OAImWsr9cGrTulyqRQiD2LS7+meH1egsY4Mo/tu3WJrcKrP+GasE4M/Y7gQ3bdUM g8zziAMg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oDnLZ-009EU3-MN; Tue, 19 Jul 2022 13:33:09 +0000 Received: from dfw.source.kernel.org ([2604:1380:4641:c500::1]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oDnLW-009ESx-TX for linux-arm-kernel@lists.infradead.org; Tue, 19 Jul 2022 13:33:08 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 2ED2C615B1; Tue, 19 Jul 2022 13:33:06 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id C2780C341C6; Tue, 19 Jul 2022 13:33:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1658237585; bh=KmR8tfAQ9eE542jYNhTNtY7nfxX4kwpdPzlaxf3sLt8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=seW9K4at5nVQ/Q8nukFkt9W9kRQtMR/DXkKUUQYWp2yWcjv2awfL8XaVkAUhVnQUK bLb1vtVymoYpiAZPS56w+bnW3F8F5suJ+SpSk+Xc64YUFldgCcgsTLB9XPx3KkMZj1 WnO75Q16lSW3h2KxomnAiEznijzPykQ8dBt+kMuBeGHjwigJKoOA1Zx8d3EbgDFmfU CQ/66+TBMTNxDurp+nT0roRa8aa7PRg2UCTJN9ZSL4mtNc2AaZv2zc9tqsbivTnCls tzkE+1jw84sTk/nR5A6fFbq0iazB7yUtYnebQTNqOhXp7O0DMttpYv0c5Ej3d8bkT7 6/M76a8ifxdwQ== Date: Tue, 19 Jul 2022 14:32:58 +0100 From: Mark Brown To: Jerome Neanne 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 Subject: Re: [PATCH v1 08/14] regulator: drivers: Add TI TPS65219 PMIC regulators support Message-ID: References: <20220719091742.3221-1-jneanne@baylibre.com> <20220719091742.3221-9-jneanne@baylibre.com> MIME-Version: 1.0 In-Reply-To: <20220719091742.3221-9-jneanne@baylibre.com> X-Cookie: We have ears, earther...FOUR OF THEM! X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220719_063307_047977_B4AC4459 X-CRM114-Status: GOOD ( 23.50 ) 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: multipart/mixed; boundary="===============1729831618218907334==" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org --===============1729831618218907334== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="zBpfrOEWtgLbiYl8" Content-Disposition: inline --zBpfrOEWtgLbiYl8 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 =3D rdev_get_drvdata(dev); > + > + /* Set the voltage based on vsel value */ > + ret =3D 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 mod= e) > +{ > + struct tps65219 *tps =3D 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 =3D data; > + > + if (irq_data->type->event_name[0] =3D=3D '\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=20 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. --zBpfrOEWtgLbiYl8 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmLWsokACgkQJNaLcl1U h9AF+gf/QEBDuiPJXAhqxJOMjcTKQGe2EGbChi3FnnmfjZcl0uqL9RsiSnchsJcQ pleLjPyp1GlnRDwl/O0Vg2weZJRTNRlcKnfh9HhER9C0VKW91zIvbyNX78uCpPPw zXBAylmh26YknpAjtUz9gh7uO0x1TNnZX2TvI4+M5rd2ioDqrvyIhTWWKy5QCFt3 3oyzHQfCdlwWqS6lvIz+hl0dfY5R0/qJA408EIQ+VQrGWb0TIcWK5VrdshH4p1oy V8y91syEOGvOIXP1UnJBKe3EaAimcNmGtKLXPf1MVhDvC7dcTW/20sBpPeBzB0w4 /RnTiTspHVXIM1D3u4KcaD//S/WZ1A== =LDKf -----END PGP SIGNATURE----- --zBpfrOEWtgLbiYl8-- --===============1729831618218907334== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel --===============1729831618218907334==-- 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 60278CCA481 for ; Tue, 19 Jul 2022 14:12:09 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237632AbiGSOMI (ORCPT ); Tue, 19 Jul 2022 10:12:08 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53090 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S238608AbiGSOLt (ORCPT ); Tue, 19 Jul 2022 10:11:49 -0400 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 03044820FF; Tue, 19 Jul 2022 06:33:08 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id C101EB81B84; Tue, 19 Jul 2022 13:33:06 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id C2780C341C6; Tue, 19 Jul 2022 13:33:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1658237585; bh=KmR8tfAQ9eE542jYNhTNtY7nfxX4kwpdPzlaxf3sLt8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=seW9K4at5nVQ/Q8nukFkt9W9kRQtMR/DXkKUUQYWp2yWcjv2awfL8XaVkAUhVnQUK bLb1vtVymoYpiAZPS56w+bnW3F8F5suJ+SpSk+Xc64YUFldgCcgsTLB9XPx3KkMZj1 WnO75Q16lSW3h2KxomnAiEznijzPykQ8dBt+kMuBeGHjwigJKoOA1Zx8d3EbgDFmfU CQ/66+TBMTNxDurp+nT0roRa8aa7PRg2UCTJN9ZSL4mtNc2AaZv2zc9tqsbivTnCls tzkE+1jw84sTk/nR5A6fFbq0iazB7yUtYnebQTNqOhXp7O0DMttpYv0c5Ej3d8bkT7 6/M76a8ifxdwQ== Date: Tue, 19 Jul 2022 14:32:58 +0100 From: Mark Brown To: Jerome Neanne 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 Subject: Re: [PATCH v1 08/14] regulator: drivers: Add TI TPS65219 PMIC regulators support Message-ID: References: <20220719091742.3221-1-jneanne@baylibre.com> <20220719091742.3221-9-jneanne@baylibre.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="zBpfrOEWtgLbiYl8" Content-Disposition: inline In-Reply-To: <20220719091742.3221-9-jneanne@baylibre.com> X-Cookie: We have ears, earther...FOUR OF THEM! Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --zBpfrOEWtgLbiYl8 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 =3D rdev_get_drvdata(dev); > + > + /* Set the voltage based on vsel value */ > + ret =3D 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 mod= e) > +{ > + struct tps65219 *tps =3D 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 =3D data; > + > + if (irq_data->type->event_name[0] =3D=3D '\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=20 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. --zBpfrOEWtgLbiYl8 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmLWsokACgkQJNaLcl1U h9AF+gf/QEBDuiPJXAhqxJOMjcTKQGe2EGbChi3FnnmfjZcl0uqL9RsiSnchsJcQ pleLjPyp1GlnRDwl/O0Vg2weZJRTNRlcKnfh9HhER9C0VKW91zIvbyNX78uCpPPw zXBAylmh26YknpAjtUz9gh7uO0x1TNnZX2TvI4+M5rd2ioDqrvyIhTWWKy5QCFt3 3oyzHQfCdlwWqS6lvIz+hl0dfY5R0/qJA408EIQ+VQrGWb0TIcWK5VrdshH4p1oy V8y91syEOGvOIXP1UnJBKe3EaAimcNmGtKLXPf1MVhDvC7dcTW/20sBpPeBzB0w4 /RnTiTspHVXIM1D3u4KcaD//S/WZ1A== =LDKf -----END PGP SIGNATURE----- --zBpfrOEWtgLbiYl8--