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;
Message-ID: <775471c6-7a68-938d-3641-6eb2a0f055f2@gmail.com>
Date:   Thu, 8 Jun 2023 11:30:32 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Content-Language: en-US, en-GB
To:     Chen-Yu Tsai <wenst@chromium.org>, Mark Brown <broonie@kernel.org>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Manivannan Sadhasivam <mani@kernel.org>,
        Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
Cc:     linux-actions@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org
References: <20230608075651.3214540-1-wenst@chromium.org>
From:   Matti Vaittinen <mazziesaccount@gmail.com>
Subject: Re: [PATCH] regulator: Use bitfield values for range selectors
In-Reply-To: <20230608075651.3214540-1-wenst@chromium.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 6/8/23 10:56, Chen-Yu Tsai wrote:
> Right now the regulator helpers expect raw register values for the range
> selectors. This is different from the voltage selectors, which are
> normalized as bitfield values. This leads to a bit of confusion. Also,
> raw values are harder to copy from datasheets or match up with them,
> as datasheets will typically have bitfield values.
> 
> Make the helpers expect bitfield values, and convert existing users.
> Include bitops.h explicitly for ffs(), and reorder the header include
> statements. While at it, also replace module.h with export.h, since the
> only use is EXPORT_SYMBOL_GPL.
> 
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
>   drivers/regulator/atc260x-regulator.c  | 2 +-
>   drivers/regulator/bd718x7-regulator.c  | 8 ++++----
>   drivers/regulator/helpers.c            | 9 ++++++---
>   drivers/regulator/tps6287x-regulator.c | 2 +-
>   4 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/regulator/atc260x-regulator.c b/drivers/regulator/atc260x-regulator.c
> index 87e237d740bc..0bba33955a1a 100644
> --- a/drivers/regulator/atc260x-regulator.c
> +++ b/drivers/regulator/atc260x-regulator.c
> @@ -37,7 +37,7 @@ static const struct linear_range atc2609a_ldo_voltage_ranges1[] = {
>   };
>   
>   static const unsigned int atc260x_ldo_voltage_range_sel[] = {
> -	0x0, 0x20,
> +	0x0, 0x1,
>   };
>   
>   static int atc260x_dcdc_set_voltage_time_sel(struct regulator_dev *rdev,
> diff --git a/drivers/regulator/bd718x7-regulator.c b/drivers/regulator/bd718x7-regulator.c
> index b0b9938c20a1..da1eea1207e5 100644
> --- a/drivers/regulator/bd718x7-regulator.c
> +++ b/drivers/regulator/bd718x7-regulator.c
> @@ -289,7 +289,7 @@ static const struct linear_range bd71837_buck5_volts[] = {
>    * and 0x1 for last 3 ranges.
>    */
>   static const unsigned int bd71837_buck5_volt_range_sel[] = {
> -	0x0, 0x0, 0x0, 0x80, 0x80, 0x80
> +	0x0, 0x0, 0x0, 0x1, 0x1, 0x1
>   };
>   
>   /*
> @@ -309,7 +309,7 @@ static const struct linear_range bd71847_buck3_volts[] = {
>   };
>   
>   static const unsigned int bd71847_buck3_volt_range_sel[] = {
> -	0x0, 0x0, 0x0, 0x40, 0x80, 0x80, 0x80
> +	0x0, 0x0, 0x0, 0x1, 0x2, 0x2, 0x2
>   };
>   
>   static const struct linear_range bd71847_buck4_volts[] = {
> @@ -360,7 +360,7 @@ static const struct linear_range bd718xx_ldo1_volts[] = {
>   	REGULATOR_LINEAR_RANGE(1600000, 0x00, 0x03, 100000),
>   };

Shouldn't the
static const unsigned int bd71847_buck4_volt_range_sel[] = { 0x0, 0x40 };

be also converted to { 0x0, 0x1 }? The range mask seems to be:
#define BD71847_BUCK4_RANGE_MASK        0x40

Other than that - the helpers + bd718x7 look good to me.

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~