Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 29 May 2021 01:06:26 +0300
From:   Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
To:     Axel Lin <axel.lin@ingics.com>
Cc:     Mark Brown <broonie@kernel.org>,
        Manivannan Sadhasivam <mani@kernel.org>,
        linux-actions@lists.infradead.org,
        Liam Girdwood <lgirdwood@gmail.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH RFT] regulator: atc260x: Fix n_voltages and min_sel for
 pickable linear ranges
Message-ID: <20210528220626.GA222540@BV030612LT>
References: <20210528123826.350771-1-axel.lin@ingics.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210528123826.350771-1-axel.lin@ingics.com>
Precedence: bulk

Hi Axel,

On Fri, May 28, 2021 at 08:38:26PM +0800, Axel Lin wrote:
> The .n_voltages was missed for pickable linear ranges, fix it.

Nice spot! I'm not quite sure if this is really mandatory to be manually
specified or gets computed internally based on the provided linear
ranges, but let's be on the safe side.

> The min_sel for each pickable range should be starting from 0.

Right, although in this particular case the driver behaves correctly
because the overflow bit of every item in the 2nd range is exactly the
range selector (bit 5 - LDO<N>_VOL_SEL in datasheet). Additionally, this
bit is not cleared internally as the selector mask (vsel_mask) is not
applied directly, but in conjunction with the range mask, according to
the implementation of "regulator_set_voltage_sel_pickable_regmap()":

	[...]
		ret = regmap_update_bits(rdev->regmap,
					 rdev->desc->vsel_reg,
					 rdev->desc->vsel_range_mask |
					 rdev->desc->vsel_mask, sel | range);
	[...]

As a matter of fact, using 0-based ranges will break the driver because
the 2nd item in the range sel array "atc260x_ldo_voltage_range_sel" is
not correct, it should be 0x20 instead of 0x1 (0x1<<5, since it is not
shifted automatically as in the case of the voltage selectors).

> Fixes: 3b15ccac161a ("regulator: Add regulator driver for ATC260x PMICs")
> Signed-off-by: Axel Lin <axel.lin@ingics.com>
> ---
> Hi Cristian,
> I don't have this h/w to test, please review and test this patch.

Unfortunately I can only test the ATC2603C variant, I haven't been able
to get a 2609A based board yet.

Thanks,
Cristi

> Thanks,
> Axel
>  drivers/regulator/atc260x-regulator.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/regulator/atc260x-regulator.c b/drivers/regulator/atc260x-regulator.c
> index d8b429955d33..5dee02394cd9 100644
> --- a/drivers/regulator/atc260x-regulator.c
> +++ b/drivers/regulator/atc260x-regulator.c
> @@ -28,12 +28,12 @@ static const struct linear_range atc2609a_dcdc_voltage_ranges[] = {
>  
>  static const struct linear_range atc2609a_ldo_voltage_ranges0[] = {
>  	REGULATOR_LINEAR_RANGE(700000, 0, 15, 100000),
> -	REGULATOR_LINEAR_RANGE(2100000, 16, 28, 100000),
> +	REGULATOR_LINEAR_RANGE(2100000, 0, 12, 100000),
>  };
>  
>  static const struct linear_range atc2609a_ldo_voltage_ranges1[] = {
>  	REGULATOR_LINEAR_RANGE(850000, 0, 15, 100000),
> -	REGULATOR_LINEAR_RANGE(2100000, 16, 27, 100000),
> +	REGULATOR_LINEAR_RANGE(2100000, 0, 11, 100000),
>  };
>  
>  static const unsigned int atc260x_ldo_voltage_range_sel[] = {
> @@ -411,7 +411,7 @@ enum atc2609a_reg_ids {
>  	.owner = THIS_MODULE, \
>  }
>  
> -#define atc2609a_reg_desc_ldo_range_pick(num, n_range) { \
> +#define atc2609a_reg_desc_ldo_range_pick(num, n_range, n_volt) { \
>  	.name = "LDO"#num, \
>  	.supply_name = "ldo"#num, \
>  	.of_match = of_match_ptr("ldo"#num), \
> @@ -421,6 +421,7 @@ enum atc2609a_reg_ids {
>  	.type = REGULATOR_VOLTAGE, \
>  	.linear_ranges = atc2609a_ldo_voltage_ranges##n_range, \
>  	.n_linear_ranges = ARRAY_SIZE(atc2609a_ldo_voltage_ranges##n_range), \
> +	.n_voltages = n_volt, \
>  	.vsel_reg = ATC2609A_PMU_LDO##num##_CTL0, \
>  	.vsel_mask = GENMASK(4, 1), \
>  	.vsel_range_reg = ATC2609A_PMU_LDO##num##_CTL0, \
> @@ -458,12 +459,12 @@ static const struct regulator_desc atc2609a_reg[] = {
>  	atc2609a_reg_desc_ldo_bypass(0),
>  	atc2609a_reg_desc_ldo_bypass(1),
>  	atc2609a_reg_desc_ldo_bypass(2),
> -	atc2609a_reg_desc_ldo_range_pick(3, 0),
> -	atc2609a_reg_desc_ldo_range_pick(4, 0),
> +	atc2609a_reg_desc_ldo_range_pick(3, 0, 29),
> +	atc2609a_reg_desc_ldo_range_pick(4, 0, 29),
>  	atc2609a_reg_desc_ldo(5),
> -	atc2609a_reg_desc_ldo_range_pick(6, 1),
> -	atc2609a_reg_desc_ldo_range_pick(7, 0),
> -	atc2609a_reg_desc_ldo_range_pick(8, 0),
> +	atc2609a_reg_desc_ldo_range_pick(6, 1, 28),
> +	atc2609a_reg_desc_ldo_range_pick(7, 0, 29),
> +	atc2609a_reg_desc_ldo_range_pick(8, 0, 29),
>  	atc2609a_reg_desc_ldo_fixed(9),
>  };
>  
> -- 
> 2.25.1
>