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 616B9C7EE37 for ; Fri, 9 Jun 2023 08:59:05 +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=CLVq0RWcLWMJSMN6vgBJH7uINugvo0ueofkukzglEbg=; b=kV6IiLoiSVIbb5 HLM9HYh3ljaH0hsva6qMPbUteix+L7Po/pVsltnPfOaZXxEBKAGA89XDB2dJLVp0hl/fB8tCHX1pE /DFEHWD99sII2NYbTMPn4lQRFaFGugRWXcWOlf4zvd15eGOquN2bM1nurcVe8xBUO2ttKf2RDFLzA fZrm9rEhUPozEv+zlaSnhWhdVAnU02uGwcQmBcJLZ++bk9Ea5nLbKrV3tQvY15pEz5y5nvR6RPRyA QpILY3ucr2NoXIMOt8FtIArmW5TS9sWWJdrd/NUGXekKZKdATTfhn1dSIDMz1kod5DhdrRyF4BqP2 h3dCPIU6sM81etObngPQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1q7XxE-00CKY8-1Q; Fri, 09 Jun 2023 08:58:44 +0000 Received: from madras.collabora.co.uk ([46.235.227.172]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1q7XxB-00CKXZ-1B; Fri, 09 Jun 2023 08:58:43 +0000 Received: from [IPV6:2001:b07:2ed:14ed:a962:cd4d:a84:1eab] (unknown [IPv6:2001:b07:2ed:14ed:a962:cd4d:a84:1eab]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits)) (No client certificate requested) (Authenticated sender: kholk11) by madras.collabora.co.uk (Postfix) with ESMTPSA id 34FB66606F1D; Fri, 9 Jun 2023 09:58:39 +0100 (BST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1686301119; bh=v/JdB+WJ+9dqXMcQEztInK7JcbHhmK32bd12IwZ2XZ8=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=ISgHSdGDTZht+Ps3nFiTgRpH+EaXVLVnGhe24ABfj2hoAOaSXBkGzXnlQHg7qdDoh kPzn2oDAQiy8haBmBpnF/to1QyKJ0GmNIf70PEtL5/4xnYlZ0bnIxCFXNZDQUkXPWC I595ilSykteItF+2cG4lWJD8WWXEar1wtbR1z8iTKj/XDkjxz8YNri3ZHuKUB9xyyj ZSO3W2TEOSXK4B/spCZsLQIG/ffnubKLnZXOd9XmC4683+lHWxEAOujRtsAHRzLtfB 2YUKltfgutIkGl7l+jZQvtYipOyz4DPeyFC7dvVUU64fPuPymhx5G3JVJ/GfQYebFM Q3MVP8q6uvHoQ== Message-ID: <07816a63-0d54-aced-a109-209c446f6bfa@collabora.com> Date: Fri, 9 Jun 2023 10:58:37 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.2 Subject: Re: [PATCH 3/9] regulator: mt6358: Merge VCN33_* regulators Content-Language: en-US To: Chen-Yu Tsai , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Mark Brown , Liam Girdwood , Matthias Brugger Cc: devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org References: <20230609083009.2822259-1-wenst@chromium.org> <20230609083009.2822259-4-wenst@chromium.org> From: AngeloGioacchino Del Regno In-Reply-To: <20230609083009.2822259-4-wenst@chromium.org> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230609_015841_668524_ABB9E393 X-CRM114-Status: GOOD ( 30.11 ) 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 Il 09/06/23 10:30, Chen-Yu Tsai ha scritto: > The VCN33_BT and VCN33_WIFI regulators are actually the same regulator, > having the same voltage setting and output pin. There are simply two > enable bits that are ORed together to enable the regulator. > > Having two regulators representing the same output pin is misleading > from a design matching standpoint, and also error-prone in driver > implementations. If consumers try to set different voltages on either > regulator, the one set later would override the one set before. There > are ways around this, such as chaining them together and having the > downstream one act as a switch. But given there's only one output pin, > such a workaround doesn't match reality. > > Remove the VCN33_WIFI regulator. During the probe phase, have the driver > sync the enable status of VCN33_WIFI to VCN33_BT. Also drop the suffix > so that the regulator name matches the pin name in the datasheet. > > Signed-off-by: Chen-Yu Tsai > --- > drivers/regulator/mt6358-regulator.c | 65 +++++++++++++++++----- > include/linux/regulator/mt6358-regulator.h | 6 +- > 2 files changed, 52 insertions(+), 19 deletions(-) > > diff --git a/drivers/regulator/mt6358-regulator.c b/drivers/regulator/mt6358-regulator.c > index c9e16bd092f6..faf6b0757019 100644 > --- a/drivers/regulator/mt6358-regulator.c > +++ b/drivers/regulator/mt6358-regulator.c > @@ -277,7 +277,7 @@ static const unsigned int vcama_voltages[] = { > 2800000, 2900000, 3000000, > }; > > -static const unsigned int vcn33_bt_wifi_voltages[] = { > +static const unsigned int vcn33_voltages[] = { > 3300000, 3400000, 3500000, > }; > > @@ -321,7 +321,7 @@ static const u32 vcama_idx[] = { > 0, 7, 9, 10, 11, 12, > }; > > -static const u32 vcn33_bt_wifi_idx[] = { > +static const u32 vcn33_idx[] = { > 1, 2, 3, > }; > > @@ -566,12 +566,8 @@ static struct mt6358_regulator_info mt6358_regulators[] = { > MT6358_LDO_VCAMA1_CON0, 0, MT6358_VCAMA1_ANA_CON0, 0xf00), > MT6358_LDO("ldo_vemc", VEMC, vmch_vemc_voltages, vmch_vemc_idx, > MT6358_LDO_VEMC_CON0, 0, MT6358_VEMC_ANA_CON0, 0x700), > - MT6358_LDO("ldo_vcn33_bt", VCN33_BT, vcn33_bt_wifi_voltages, > - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_0, > - 0, MT6358_VCN33_ANA_CON0, 0x300), > - MT6358_LDO("ldo_vcn33_wifi", VCN33_WIFI, vcn33_bt_wifi_voltages, > - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_1, > - 0, MT6358_VCN33_ANA_CON0, 0x300), > + MT6358_LDO("ldo_vcn33", VCN33, vcn33_voltages, vcn33_idx, > + MT6358_LDO_VCN33_CON0_0, 0, MT6358_VCN33_ANA_CON0, 0x300), > MT6358_LDO("ldo_vcama2", VCAMA2, vcama_voltages, vcama_idx, > MT6358_LDO_VCAMA2_CON0, 0, MT6358_VCAMA2_ANA_CON0, 0xf00), > MT6358_LDO("ldo_vmc", VMC, vmc_voltages, vmc_idx, > @@ -662,12 +658,8 @@ static struct mt6358_regulator_info mt6366_regulators[] = { > MT6358_LDO_VMCH_CON0, 0, MT6358_VMCH_ANA_CON0, 0x700), > MT6366_LDO("ldo_vemc", VEMC, vmch_vemc_voltages, vmch_vemc_idx, > MT6358_LDO_VEMC_CON0, 0, MT6358_VEMC_ANA_CON0, 0x700), > - MT6366_LDO("ldo_vcn33_bt", VCN33_BT, vcn33_bt_wifi_voltages, > - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_0, > - 0, MT6358_VCN33_ANA_CON0, 0x300), > - MT6366_LDO("ldo_vcn33_wifi", VCN33_WIFI, vcn33_bt_wifi_voltages, > - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_1, > - 0, MT6358_VCN33_ANA_CON0, 0x300), > + MT6366_LDO("ldo_vcn33", VCN33, vcn33_voltages, vcn33_idx, > + MT6358_LDO_VCN33_CON0_0, 0, MT6358_VCN33_ANA_CON0, 0x300), > MT6366_LDO("ldo_vmc", VMC, vmc_voltages, vmc_idx, > MT6358_LDO_VMC_CON0, 0, MT6358_VMC_ANA_CON0, 0xf00), > MT6366_LDO("ldo_vsim2", VSIM2, vsim_voltages, vsim_idx, > @@ -690,13 +682,56 @@ static struct mt6358_regulator_info mt6366_regulators[] = { > MT6358_LDO_VSRAM_CON1, 0x7f), > }; > > +static int mt6358_sync_vcn33_setting(struct device *dev) > +{ > + struct mt6397_chip *mt6397 = dev_get_drvdata(dev->parent); > + unsigned int val; > + int ret; > + > + /* > + * VCN33_WIFI and VCN33_BT are two separate enable bits for the same > + * regulator. They share the same voltage setting and output pin. > + * Instead of having two potentially conflicting regulators, just have > + * one VCN33 regulator. Sync the two enable bits and only use one in > + * the regulator device. > + */ > + ret = regmap_read(mt6397->regmap, MT6358_LDO_VCN33_CON0_1, &val); > + if (ret) { > + dev_err(dev, "Failed to read VCN33_WIFI setting\n"); > + return ret; > + } > + > + if (!(val & BIT(0))) > + return 0; > + > + /* Sync VCN33_WIFI enable status to VCN33_BT */ > + ret = regmap_update_bits(mt6397->regmap, MT6358_LDO_VCN33_CON0_0, BIT(0), BIT(0)); > + if (ret) { > + dev_err(dev, "Failed to sync VCN33_WIFI setting to VCN33_BT\n"); > + return ret; > + } > + > + /* Disable VCN33_WIFI */ > + ret = regmap_update_bits(mt6397->regmap, MT6358_LDO_VCN33_CON0_1, BIT(0), 0); > + if (ret) { > + dev_err(dev, "Failed to disable VCN33_BT\n"); > + return ret; > + } > + > + return 0; > +} > + > static int mt6358_regulator_probe(struct platform_device *pdev) > { > struct mt6397_chip *mt6397 = dev_get_drvdata(pdev->dev.parent); > struct regulator_config config = {}; > struct regulator_dev *rdev; > struct mt6358_regulator_info *mt6358_info; > - int i, max_regulator; > + int i, max_regulator, ret; > + > + ret = mt6358_sync_vcn33_setting(&pdev->dev); > + if (ret) > + return ret; I'd put this after the chip_id check, and I would also add a safety check for that... switch (mt6397->chip_id) { case MT6366_CHIP_ID: max_regulator = MT6366_MAX_REGULATOR; mt6358_info = mt6366_regulators; break; case MT6358_CHIP_ID: max_regulator = MT6358_MAX_REGULATOR; mt6358_info = mt6358_regulators; break; default: return -EINVAL; } ret = mt6358_sync_vcn33_setting(....) ...but I agree with your point here about this being a strange design and also with your way of fixing the driver. Regards, Angelo _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel