Received: by 2002:a05:6358:d09b:b0:dc:cd0c:909e with SMTP id jc27csp2573325rwb; Fri, 16 Dec 2022 04:04:01 -0800 (PST) X-Google-Smtp-Source: AA0mqf58DCpljQsnHTVaoQrrCuIGtvctuNGV55JSkEKIMTahvvpCK/OOK9ca76I0sZ1uY3mgRGCY X-Received: by 2002:a17:907:d40f:b0:7c4:f402:9769 with SMTP id vi15-20020a170907d40f00b007c4f4029769mr14814287ejc.76.1671192241410; Fri, 16 Dec 2022 04:04:01 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1671192241; cv=none; d=google.com; s=arc-20160816; b=StwY66OGMlN9liscxCKbNIypQJqlyTB1kvnuO2qwkcEO++a+N9mbhzQfrgnMVBscic 8eR2wjtTNj7z3N/m0ZzhYPnMCqqOpVy0Yy2CWGUVW/dKSi3IVo/WV/aa5JjCKDKEAsJM d1pp1hX9kamOWNyEXSORAGpPeGG39dV3jCVdpjkuiX3gT60DEI/P5aibfLQwXgBeTd7N vsL3o5xCwbe5ViVUGupcflgrFhuooEtc+SqxqhLxApcjRw7Pl1h5UnIofpo/z5VH6LNv pZjmptquBRx7e1O6SFOHV9tPsQhIvENW55TZDKQzlYEZorQmSBG+uwumlG6NdJjmmPPk W5GA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :organization:references:in-reply-to:message-id:subject:cc:to:from :date; bh=NkJhaG4g6NPwLkrfpbnZAjwR/Su1hEQcApb7bOyzc/E=; b=WwAALVPqDVvUeZVSSdbp9JUBYPtZ7KC9ftTODtbQnnMF238C3nlS5E7faTyJuYTSEh mO7Z7CYKtByDb1mc8goAwhKvnHWKUgp+agZRuHHTVOoH/toqXBAaQbs0CVRMFR+oq1U2 PKaPwMVm7BVhuzufzk7tPXdBgrADCu+DknJ2IiQ9vQOvILZnmWQxnBJQKQ7dfnOEQhtB jjSMMfApv1qcmBymtxum+4l/Uxi5k8bnkjWSp4aUFWMOyGbnpb+qEwWKs16Nx5G4vRKx oltuFunq32jFDqMiKrP0pdzXTl0gfZw8yg9dQ2zVA6RjQk6igJbzl169bHGBZDC7xDm6 5UxQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id xe9-20020a170907318900b007ae2b7df929si2423814ejb.72.2022.12.16.04.03.43; Fri, 16 Dec 2022 04:04:01 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230233AbiLPLwY convert rfc822-to-8bit (ORCPT + 68 others); Fri, 16 Dec 2022 06:52:24 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48192 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229495AbiLPLwW (ORCPT ); Fri, 16 Dec 2022 06:52:22 -0500 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 0C1D22B604; Fri, 16 Dec 2022 03:52:21 -0800 (PST) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 786E01042; Fri, 16 Dec 2022 03:53:01 -0800 (PST) Received: from donnerap.cambridge.arm.com (usa-sjc-imap-foss1.foss.arm.com [10.121.207.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 741E13F73B; Fri, 16 Dec 2022 03:52:18 -0800 (PST) Date: Fri, 16 Dec 2022 11:52:14 +0000 From: Andre Przywara To: Martin Botka Cc: martin.botka1@gmail.com, Konrad Dybcio , AngeloGioacchino Del Regno , Marijn Suijten , Jami Kettunen , Paul Bouchara , Jan Trmal , Lee Jones , Rob Herring , Krzysztof Kozlowski , Chen-Yu Tsai , Liam Girdwood , Mark Brown , devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Samuel Holland , Jernej =?UTF-8?B?xaBrcmFiZWM=?= , linux-sunxi Subject: Re: [PATCH v5 3/3] regulator: axp20x: Add support for AXP1530 variant Message-ID: <20221216115214.1be53742@donnerap.cambridge.arm.com> In-Reply-To: References: <20221214190305.3354669-1-martin.botka@somainline.org> <20221214190305.3354669-4-martin.botka@somainline.org> <20221215231615.6a4fa710@slackpad.lan> Organization: ARM X-Mailer: Claws Mail 3.18.0 (GTK+ 2.24.32; aarch64-unknown-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 16 Dec 2022 06:26:38 +0100 Martin Botka wrote: Hi Martin, > On December 16, 2022 12:16:15 AM GMT+01:00, Andre Przywara wrote: > >On Wed, 14 Dec 2022 20:03:05 +0100 > >Martin Botka wrote: > > > >Hi Martin, > > > >> AXP1530 has a few regulators that are controlled via I2C Bus. > >> > >> Add support for them. > > > >thanks for putting this together! > >After coming up with a very similar patch based on the AXP313A313 > >datasheet, I realised that those two must indeed be *somewhat* > >compatible, so I am going to compare my patch with yours ;-) > > > Hello Andre, > Thanks so much for looking at this. > >> > >> Signed-off-by: Martin Botka > >> --- > >> drivers/regulator/axp20x-regulator.c | 44 ++++++++++++++++++++++++++++ > >> 1 file changed, 44 insertions(+) > >> > >> diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c > >> index d260c442b788..9420839ff4f9 100644 > >> --- a/drivers/regulator/axp20x-regulator.c > >> +++ b/drivers/regulator/axp20x-regulator.c > >> @@ -1001,6 +1001,40 @@ static const struct regulator_desc axp813_regulators[] = { > >> AXP22X_PWR_OUT_CTRL2, AXP22X_PWR_OUT_DC1SW_MASK), > >> }; > >> > >> +static const struct linear_range axp1530_dcdc1_ranges[] = { > >> + REGULATOR_LINEAR_RANGE(500000, 0x0, 0x46, 10000), > > > >The AXP313A manual mentions "steps", in decimal > >(0.5~1.2V,10mV/step,71steps), so I wonder if we should follow suit > >here and describe the min_sel and max_sel members in decimal? > > > Ack. We definitely can :) > >> + REGULATOR_LINEAR_RANGE(1220000, 0x47, 0x57, 20000), > >> + REGULATOR_LINEAR_RANGE(1600000, 0x58, 0x6A, 100000), > >> +}; > >> + > >> +static const struct linear_range axp1530_dcdc2_ranges[] = { > >> + REGULATOR_LINEAR_RANGE(500000, 0x0, 0x46, 10000), > >> + REGULATOR_LINEAR_RANGE(1220000, 0x47, 0x57, 20000), > >> +}; > > > >The values up till here match exactly what I extracted from the AXP313A > >manual. > > > >> + > >> +static const struct linear_range axp1530_dcdc3_ranges[] = { > >> + REGULATOR_LINEAR_RANGE(500000, 0x0, 0x46, 10000), > >> + REGULATOR_LINEAR_RANGE(1220000, 0x47, 0x66, 20000), > >> +}; > > > >Can you double check that those are the values for DCDC3? > >The AXP313A manual uses different ranges, essentially: > > REGULATOR_LINEAR_RANGE(800000, 0, 32, 10000), > > REGULATOR_LINEAR_RANGE(1140000, 33, 68, 20000), > >So starting from 800mV, and using a slightly different split point. > > > >I would just hope that's this doesn't turn out to be an incompatible register. > > > Interesting. The unfortunate thing with 1530 is that i could not find any datasheet referencing it. The actual PMIC that is on the device i have here is 313A. Do i think it would be best if i rename this driver to 313A and follow the 313A datasheet which is public. So where does the 1530 name and the other bits come from? Was there some statement somewhere that AXP1530 is the canonical chip, and the 313A is just a variant? Or are there devices using this chip, and you just happen to not have them, but a "compatible enough" 313A instead? > This was already proposed in one of my device tree series. > What do you think of this idea Andre ? In general, I think it's too dangerous to develop against something you cannot test against. So I would always call this driver after the chip I have access to, especially if there is also a datasheet for it. If someone later finds the AXP1530 to be compatible, they can always use: compatible = "x-powers,axp1530", "x-powers,axp313a"; Or later on add explicit support for that chip if it turns out to be not fully compatible. The only reason to not do it this way around would be if one was a strict subset of the other. So if the 313A is the 1530 plus RTCLDO, for instance, it would make sense to reflect that in the compatible strings (by swapping them in the above example). But then again there is really no legacy here, so we could just upstream support for both variants in one go, using separate compatible strings, and let them just share some data structures internally. That is probably cleaner, and if both are going it at the same time, there is no downside, really. > >> +static const struct regulator_desc axp1530_regulators[] = { > >> + AXP_DESC_RANGES(AXP1530, DCDC1, "dcdc1", "vin1", axp1530_dcdc1_ranges, > >> + 0x6B, AXP1530_DCDC1_CONRTOL, 0x7f, AXP1530_OUTPUT_CONTROL, > > > >Again I would code the steps in decimal. The other regulators use a > >preprocessor constant, which helps the reader to get its meaning. > >And please use at least GENMASK(6, 0) instead of 0x7f, or #define this > >(can be shared for all DCDCs and the LDOs). > > > Ack. Will use GENMASK. > >> + BIT(0)), > >> + AXP_DESC_RANGES(AXP1530, DCDC2, "dcdc2", "vin2", axp1530_dcdc2_ranges, > >> + 0x58, AXP1530_DCDC2_CONRTOL, 0x7f, AXP1530_OUTPUT_CONTROL, > >> + BIT(1)), > >> + AXP_DESC_RANGES(AXP1530, DCDC3, "dcdc3", "vin3", axp1530_dcdc3_ranges, > >> + 0x58, AXP1530_DCDC3_CONRTOL, 0x7f, AXP1530_OUTPUT_CONTROL, > >> + BIT(2)), > >> + AXP_DESC(AXP1530, LDO1, "ldo1", "ldo1in", 500, 3500, 100, > >> + AXP1530_ALDO1_CONRTOL, 0x1f, AXP1530_OUTPUT_CONTROL, > >> + BIT(3)), > >> + AXP_DESC(AXP1530, LDO2, "ldo2", "ldo2in", 500, 3500, 100, > >> + AXP1530_DLDO1_CONRTOL, 0x1f, AXP1530_OUTPUT_CONTROL, > >> + BIT(4)), > > > >Does this miss the fixed RTC-LDO? Or does the AXP1530 not have that? > > AXP_DESC_FIXED(AXP313, RTC_LDO, "rtc-ldo", "ips", 1800), > >The AXP313A manual mentions that the voltage is customisable, either > >1.8V or 3.3V. I don't know how to model that, exactly. Should this be a > >DT property, then? Or do we fix it to one voltage, covering the value > >that's used out there? > > > This is what always struck me as weird. This driver is based upon downstream version it indeed does miss the rtc-ldo. Well, in my experience downstream (BSP) drivers cannot be really trusted in this regard. So they could just papered over it by modelling this as a separate "regulator-fixed"? Or maybe this was their solution for the "customise" problem? So instead of having this as part of the AXP, they just put a per-board fixed regulator in and set the voltage there? Cheers, Andre > Afaik this may be the only difference between 1530 and 313 (other then what you pointed out the dcdc3 registers) > >> +}; > >> + > >> static int axp20x_set_dcdc_freq(struct platform_device *pdev, u32 dcdcfreq) > >> { > >> struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent); > >> @@ -1040,6 +1074,12 @@ static int axp20x_set_dcdc_freq(struct platform_device *pdev, u32 dcdcfreq) > >> def = 3000; > >> step = 150; > >> break; > >> + case AXP1530_ID: > >> + /* > >> + * Do not set the DCDC frequency on AXP1530 > > > >This should say that the frequency is fixed and cannot be programmed. > >I also added a warning if the frequency is not 3 MHz. > >Either this, or we make the "x-powers,dcdc-freq" DT property optional. > > > Ack. Will reword and add warning. > >Cheers, > >Andre > > > Cheers, > Martin > >> + */ > >> + return 0; > >> + break; > >> default: > >> dev_err(&pdev->dev, > >> "Setting DCDC frequency for unsupported AXP variant\n"); > >> @@ -1220,6 +1260,10 @@ static int axp20x_regulator_probe(struct platform_device *pdev) > >> bool drivevbus = false; > >> > >> switch (axp20x->variant) { > >> + case AXP1530_ID: > >> + regulators = axp1530_regulators; > >> + nregulators = AXP1530_REG_ID_MAX; > >> + break; > >> case AXP202_ID: > >> case AXP209_ID: > >> regulators = axp20x_regulators; > >