Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Message-ID: <aea2b4fa-09e4-414d-87c0-c33bba5b2b2b@collabora.com>
Date:   Fri, 17 Nov 2023 16:32:34 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1 18/20] soc: mediatek: mtk-svs: Constify
 runtime-immutable members of svs_bank
Content-Language: en-US
To:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>, matthias.bgg@gmail.com
Cc:     krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
        robh+dt@kernel.org, p.zabel@pengutronix.de,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org, kernel@collabora.com,
        wenst@chromium.org
References: <20231117094228.40013-1-angelogioacchino.delregno@collabora.com>
 <20231117094228.40013-19-angelogioacchino.delregno@collabora.com>
From:   Eugen Hristev <eugen.hristev@collabora.com>
In-Reply-To: <20231117094228.40013-19-angelogioacchino.delregno@collabora.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


Hi Angelo,


On 11/17/23 11:42, AngeloGioacchino Del Regno wrote:
> Some members of struct svs_bank are not changed during runtime, so those
> are not variables but constants: move all of those to a new structure
> called svs_bank_pdata and refactor the code to make use of that.
> This effectively moves at least 50 bytes to the text segment.
> While at it, also uniform the thermal zone names across the banks.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>   drivers/soc/mediatek/mtk-svs.c | 1201 +++++++++++++++++---------------
>   1 file changed, 631 insertions(+), 570 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> index df39e7430ba9..aa50ae0cc1d1 100644
> --- a/drivers/soc/mediatek/mtk-svs.c
> +++ b/drivers/soc/mediatek/mtk-svs.c
> @@ -1,6 +1,8 @@
>   // SPDX-License-Identifier: GPL-2.0-only
>   /*
>    * Copyright (C) 2022 MediaTek Inc.
> + * Copyright (C) 2022 Collabora Ltd.
> + *                    AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>    */
>   
>   #include <linux/bitfield.h>
> @@ -118,7 +120,7 @@
>   #define SVSB_VOPS_FLD_VOP2_6		GENMASK(23, 16)
>   #define SVSB_VOPS_FLD_VOP3_7		GENMASK(31, 24)
>   
> -/* SVS Thermal Coefficients */
> +/* SVS Thermal */

'Coefficients' made sense for me. Without the word , does the comment 
many any sense anymore ?

>   #define SVSB_TS_COEFF_MT8195		250460
>   #define SVSB_TS_COEFF_MT8186		204650
>   
> @@ -391,7 +393,7 @@ struct svs_platform {
>   	size_t efuse_max;
>   	size_t tefuse_max;
>   	const u32 *regs;
> -	u32 bank_max;
> +	u16 bank_max;

does it make sense to order them by size ?

>   	u32 *efuse;
>   	u32 *tefuse;
>   	u32 ts_coeff;
> @@ -404,59 +406,92 @@ struct svs_platform_data {
>   	int (*probe)(struct svs_platform *svsp);
>   	const struct svs_fusemap *glb_fuse_map;
>   	const u32 *regs;
> -	u32 bank_max;
> +	u16 bank_max;
here as well.
>   	u32 ts_coeff;
>   };
>   
>   /**
> - * struct svs_bank - svs bank representation
> + * struct svs_bank_pdata - SVS Bank immutable config parameters
>    * @dev_fuse_map: Bank fuse map data
> + * @buck_name: Regulator name
> + * @tzone_name: Thermal zone name
> + * @age_config: Bank age configuration
> + * @ctl0: TS-x selection
> + * @dc_config: Bank dc configuration
> + * @int_st: Bank interrupt identification
> + * @turn_freq_base: Reference frequency for 2-line turn point
> + * @tzone_htemp: Thermal zone high temperature threshold
> + * @tzone_ltemp: Thermal zone low temperature threshold
> + * @volt_step: Bank voltage step
> + * @volt_base: Bank voltage base
> + * @tzone_htemp_voffset: Thermal zone high temperature voltage offset
> + * @tzone_ltemp_voffset: Thermal zone low temperature voltage offset
> + * @chk_shift: Bank chicken shift
> + * @cpu_id: CPU core ID for SVS CPU bank use only
> + * @opp_count: Bank opp count
> + * @vboot: Voltage request for bank init01 only
> + * @vco: Bank VCO value
> + * @sw_id: Bank software identification
> + * @type: SVS Bank Type (1 or 2-line) and Role (high/low)
> + * @set_freq_pct: function pointer to set bank frequency percent table
> + * @get_volts: function pointer to get bank voltages
> + */
> +struct svs_bank_pdata {
> +	const struct svs_fusemap *dev_fuse_map;
> +	char *buck_name;
> +	char *tzone_name;
> +	u32 age_config;
> +	u32 ctl0;
> +	u32 dc_config;
> +	u32 int_st;
> +	u32 turn_freq_base;
> +	u32 tzone_htemp;
> +	u32 tzone_ltemp;
> +	u32 volt_step;
> +	u32 volt_base;
> +	u16 tzone_htemp_voffset;
> +	u16 tzone_ltemp_voffset;
> +	u8 chk_shift;
> +	u8 cpu_id;
> +	u8 opp_count;
> +	u8 vboot;
> +	u8 vco;
> +	u8 sw_id;
> +	u8 type;
> +
> +	/* Callbacks */
> +	void (*set_freq_pct)(struct svs_platform *, struct svs_bank *svsb);
> +	void (*get_volts)(struct svs_platform *, struct svs_bank *svsb);

WARNING: function definition argument 'struct svs_platform *' should 
also have an identifier name

> +};
> +
> +/**
> + * struct svs_bank - svs bank representation
> + * @pdata: SVS Bank immutable config parameters
>    * @dev: bank device
>    * @opp_dev: device for opp table/buck control
>    * @init_completion: the timeout completion for bank init
>    * @buck: regulator used by opp_dev
>    * @tzd: thermal zone device for getting temperature
>    * @lock: mutex lock to protect voltage update process
> - * @set_freq_pct: function pointer to set bank frequency percent table
> - * @get_volts: function pointer to get bank voltages
>    * @name: bank name
> - * @buck_name: regulator name
> - * @tzone_name: thermal zone name
>    * @phase: bank current phase
>    * @volt_od: bank voltage overdrive
>    * @reg_data: bank register data in different phase for debug purpose
>    * @pm_runtime_enabled_count: bank pm runtime enabled count
> - * @mode_support: bank mode support.
> + * @mode_support: bank mode support
>    * @freq_base: reference frequency for bank init
> - * @turn_freq_base: refenrece frequency for 2-line turn point
> - * @vboot: voltage request for bank init01 only
>    * @opp_dfreq: default opp frequency table
>    * @opp_dvolt: default opp voltage table
>    * @freq_pct: frequency percent table for bank init
>    * @volt: bank voltage table
> - * @volt_step: bank voltage step
> - * @volt_base: bank voltage base
>    * @volt_flags: bank voltage flags
>    * @vmax: bank voltage maximum
>    * @vmin: bank voltage minimum
> - * @age_config: bank age configuration
>    * @age_voffset_in: bank age voltage offset
> - * @dc_config: bank dc configuration
>    * @dc_voffset_in: bank dc voltage offset
>    * @dvt_fixed: bank dvt fixed value
> - * @vco: bank VCO value
> - * @chk_shift: bank chicken shift
>    * @core_sel: bank selection
> - * @opp_count: bank opp count
> - * @int_st: bank interrupt identification
> - * @sw_id: bank software identification
> - * @cpu_id: cpu core id for SVS CPU bank use only
> - * @ctl0: TS-x selection
>    * @temp: bank temperature
> - * @tzone_htemp: thermal zone high temperature threshold
> - * @tzone_htemp_voffset: thermal zone high temperature voltage offset
> - * @tzone_ltemp: thermal zone low temperature threshold
> - * @tzone_ltemp_voffset: thermal zone low temperature voltage offset
>    * @bts: svs efuse data
>    * @mts: svs efuse data
>    * @bdes: svs efuse data
> @@ -466,7 +501,6 @@ struct svs_platform_data {
>    * @dcmdet: svs efuse data
>    * @turn_pt: 2-line turn point tells which opp_volt calculated by high/low bank
>    * @vbin_turn_pt: voltage bin turn point helps know which svsb_volt should be overridden
> - * @type: bank type to represent it is 2-line (high/low) bank or 1-line bank
>    *
>    * Svs bank will generate suitalbe voltages by below general math equation

can you also fix this typo 'suitalbe'

>    * and provide these voltages to opp voltage table.
> @@ -474,53 +508,34 @@ struct svs_platform_data {
>    * opp_volt[i] = (volt[i] * volt_step) + volt_base;
>    */
>   struct svs_bank {
> -	const struct svs_fusemap *dev_fuse_map;
> +	const struct svs_bank_pdata pdata;
>   	struct device *dev;
>   	struct device *opp_dev;
>   	struct completion init_completion;
>   	struct regulator *buck;
>   	struct thermal_zone_device *tzd;
> -	struct mutex lock;	/* lock to protect voltage update process */
> -	void (*set_freq_pct)(struct svs_platform *svsp, struct svs_bank *svsb);
> -	void (*get_volts)(struct svs_platform *svsp, struct svs_bank *svsb);
> +	struct mutex lock;
> +	int pm_runtime_enabled_count;
> +	short int volt_od;
>   	char *name;
> -	char *buck_name;
> -	char *tzone_name;
>   	enum svsb_phase phase;
> -	short int volt_od;
>   	u32 reg_data[SVSB_PHASE_MAX][SVS_REG_MAX];
> -	u32 pm_runtime_enabled_count;
>   	u8 mode_support;
> -	u32 freq_base;
> -	u32 turn_freq_base;
> -	u8 vboot;
>   	u32 opp_dfreq[MAX_OPP_ENTRIES];
>   	u32 opp_dvolt[MAX_OPP_ENTRIES];
>   	u32 freq_pct[MAX_OPP_ENTRIES];
>   	u32 volt[MAX_OPP_ENTRIES];
> -	u32 volt_step;
> -	u32 volt_base;
>   	u32 volt_flags;
> -	u8 vmax;
> -	u8 vmin;
> -	u32 age_config;
> +	u32 freq_base;
> +	u32 turn_pt;
> +	u32 vbin_turn_pt;
> +	u32 core_sel;
> +	u32 temp;
>   	u16 age_voffset_in;
> -	u32 dc_config;
>   	u16 dc_voffset_in;
>   	u8 dvt_fixed;
> -	u8 vco;
> -	u8 chk_shift;
> -	u32 core_sel;
> -	u8 opp_count;
> -	u32 int_st;
> -	u8 sw_id;
> -	u8 cpu_id;
> -	u32 ctl0;
> -	u32 temp;
> -	u32 tzone_htemp;
> -	u16 tzone_htemp_voffset;
> -	u32 tzone_ltemp;
> -	u16 tzone_ltemp_voffset;
> +	u8 vmax;
> +	u8 vmin;
>   	u16 bts;
>   	u16 mts;
>   	u16 bdes;
> @@ -528,9 +543,6 @@ struct svs_bank {
>   	u8 mtdes;
>   	u8 dcbdet;
>   	u8 dcmdet;
> -	u32 turn_pt;
> -	u32 vbin_turn_pt;
> -	u8 type;
>   };
>   
>   static u32 percent(u32 numerator, u32 denominator)
> @@ -572,10 +584,11 @@ static u32 svs_opp_volt_to_bank_volt(u32 opp_u_volt, u32 svsb_volt_step,
>   
>   static int svs_sync_bank_volts_from_opp(struct svs_bank *svsb)
>   {
> +	const struct svs_bank_pdata *bdata = &svsb->pdata;
>   	struct dev_pm_opp *opp;
>   	u32 i, opp_u_volt;
>   
> -	for (i = 0; i < svsb->opp_count; i++) {
> +	for (i = 0; i < bdata->opp_count; i++) {
>   		opp = dev_pm_opp_find_freq_exact(svsb->opp_dev,
>   						 svsb->opp_dfreq[i],
>   						 true);
> @@ -587,8 +600,8 @@ static int svs_sync_bank_volts_from_opp(struct svs_bank *svsb)
>   
>   		opp_u_volt = dev_pm_opp_get_voltage(opp);
>   		svsb->volt[i] = svs_opp_volt_to_bank_volt(opp_u_volt,
> -							  svsb->volt_step,
> -							  svsb->volt_base);
> +							  bdata->volt_step,
> +							  bdata->volt_base);
>   		dev_pm_opp_put(opp);
>   	}
>   
> @@ -598,6 +611,7 @@ static int svs_sync_bank_volts_from_opp(struct svs_bank *svsb)
>   static int svs_adjust_pm_opp_volts(struct svs_bank *svsb)
>   {
>   	int ret = -EPERM, tzone_temp = 0;
> +	const struct svs_bank_pdata *bdata = &svsb->pdata;
>   	u32 i, svsb_volt, opp_volt, temp_voffset = 0, opp_start, opp_stop;
>   
>   	mutex_lock(&svsb->lock);
> @@ -606,15 +620,15 @@ static int svs_adjust_pm_opp_volts(struct svs_bank *svsb)
>   	 * 2-line bank updates its corresponding opp volts.
>   	 * 1-line bank updates all opp volts.
>   	 */
> -	if (svsb->type == SVSB_TYPE_HIGH) {
> +	if (bdata->type == SVSB_TYPE_HIGH) {
>   		opp_start = 0;
>   		opp_stop = svsb->turn_pt;
> -	} else if (svsb->type == SVSB_TYPE_LOW) {
> +	} else if (bdata->type == SVSB_TYPE_LOW) {
>   		opp_start = svsb->turn_pt;
> -		opp_stop = svsb->opp_count;
> +		opp_stop = bdata->opp_count;
>   	} else {
>   		opp_start = 0;
> -		opp_stop = svsb->opp_count;
> +		opp_stop = bdata->opp_count;
>   	}
>   
>   	/* Get thermal effect */
> @@ -623,20 +637,20 @@ static int svs_adjust_pm_opp_volts(struct svs_bank *svsb)
>   		if (ret || (svsb->temp > SVSB_TEMP_UPPER_BOUND &&
>   			    svsb->temp < SVSB_TEMP_LOWER_BOUND)) {
>   			dev_err(svsb->dev, "%s: %d (0x%x), run default volts\n",
> -				svsb->tzone_name, ret, svsb->temp);
> +				bdata->tzone_name, ret, svsb->temp);
>   			svsb->phase = SVSB_PHASE_ERROR;
>   		}
>   
> -		if (tzone_temp >= svsb->tzone_htemp)
> -			temp_voffset += svsb->tzone_htemp_voffset;
> -		else if (tzone_temp <= svsb->tzone_ltemp)
> -			temp_voffset += svsb->tzone_ltemp_voffset;
> +		if (tzone_temp >= bdata->tzone_htemp)
> +			temp_voffset += bdata->tzone_htemp_voffset;
> +		else if (tzone_temp <= bdata->tzone_ltemp)
> +			temp_voffset += bdata->tzone_ltemp_voffset;
>   
>   		/* 2-line bank update all opp volts when running mon mode */
> -		if (svsb->phase == SVSB_PHASE_MON && (svsb->type == SVSB_TYPE_HIGH ||
> -						      svsb->type == SVSB_TYPE_LOW)) {
> +		if (svsb->phase == SVSB_PHASE_MON && (bdata->type == SVSB_TYPE_HIGH ||
> +						      bdata->type == SVSB_TYPE_LOW)) {
>   			opp_start = 0;
> -			opp_stop = svsb->opp_count;
> +			opp_stop = bdata->opp_count;
>   		}
>   	}
>   
> @@ -653,8 +667,8 @@ static int svs_adjust_pm_opp_volts(struct svs_bank *svsb)
>   		case SVSB_PHASE_MON:
>   			svsb_volt = max(svsb->volt[i] + temp_voffset, svsb->vmin);
>   			opp_volt = svs_bank_volt_to_opp_volt(svsb_volt,
> -							     svsb->volt_step,
> -							     svsb->volt_base);
> +							     bdata->volt_step,
> +							     bdata->volt_base);
>   			break;
>   		default:
>   			dev_err(svsb->dev, "unknown phase: %u\n", svsb->phase);
> @@ -816,7 +830,7 @@ static int svs_status_debug_show(struct seq_file *m, void *v)
>   			   svsb->name, tzone_temp, svsb->vbin_turn_pt,
>   			   svsb->turn_pt);
>   
> -	for (i = 0; i < svsb->opp_count; i++) {
> +	for (i = 0; i < svsb->pdata.opp_count; i++) {
>   		opp = dev_pm_opp_find_freq_exact(svsb->opp_dev,
>   						 svsb->opp_dfreq[i], true);
>   		if (IS_ERR(opp)) {
> @@ -923,9 +937,10 @@ static u32 interpolate(u32 f0, u32 f1, u32 v0, u32 v1, u32 fx)
>   
>   static void svs_get_bank_volts_v3(struct svs_platform *svsp, struct svs_bank *svsb)
>   {
> +	const struct svs_bank_pdata *bdata = &svsb->pdata;
>   	u32 i, j, *vop, vop74, vop30, turn_pt = svsb->turn_pt;
>   	u32 b_sft, shift_byte = 0, opp_start = 0, opp_stop = 0;
> -	u32 middle_index = (svsb->opp_count / 2);
> +	u32 middle_index = (bdata->opp_count / 2);
>   
>   	if (svsb->phase == SVSB_PHASE_MON &&
>   	    svsb->volt_flags & SVSB_MON_VOLT_IGNORE)
> @@ -936,7 +951,7 @@ static void svs_get_bank_volts_v3(struct svs_platform *svsp, struct svs_bank *sv
>   
>   	/* Target is to set svsb->volt[] by algorithm */
>   	if (turn_pt < middle_index) {
> -		if (svsb->type == SVSB_TYPE_HIGH) {
> +		if (bdata->type == SVSB_TYPE_HIGH) {
>   			/* volt[0] ~ volt[turn_pt - 1] */
>   			for (i = 0; i < turn_pt; i++) {
>   				b_sft = BITS8 * (shift_byte % REG_BYTES);
> @@ -945,12 +960,12 @@ static void svs_get_bank_volts_v3(struct svs_platform *svsp, struct svs_bank *sv
>   				svsb->volt[i] = (*vop >> b_sft) & GENMASK(7, 0);
>   				shift_byte++;
>   			}
> -		} else if (svsb->type == SVSB_TYPE_LOW) {
> +		} else if (bdata->type == SVSB_TYPE_LOW) {
>   			/* volt[turn_pt] + volt[j] ~ volt[opp_count - 1] */
> -			j = svsb->opp_count - 7;
> +			j = bdata->opp_count - 7;
>   			svsb->volt[turn_pt] = FIELD_GET(SVSB_VOPS_FLD_VOP0_4, vop30);
>   			shift_byte++;
> -			for (i = j; i < svsb->opp_count; i++) {
> +			for (i = j; i < bdata->opp_count; i++) {
>   				b_sft = BITS8 * (shift_byte % REG_BYTES);
>   				vop = (shift_byte < REG_BYTES) ? &vop30 :
>   								 &vop74;
> @@ -967,7 +982,7 @@ static void svs_get_bank_volts_v3(struct svs_platform *svsp, struct svs_bank *sv
>   							    svsb->freq_pct[i]);
>   		}
>   	} else {
> -		if (svsb->type == SVSB_TYPE_HIGH) {
> +		if (bdata->type == SVSB_TYPE_HIGH) {
>   			/* volt[0] + volt[j] ~ volt[turn_pt - 1] */
>   			j = turn_pt - 7;
>   			svsb->volt[0] = FIELD_GET(SVSB_VOPS_FLD_VOP0_4, vop30);
> @@ -987,9 +1002,9 @@ static void svs_get_bank_volts_v3(struct svs_platform *svsp, struct svs_bank *sv
>   							    svsb->volt[0],
>   							    svsb->volt[j],
>   							    svsb->freq_pct[i]);
> -		} else if (svsb->type == SVSB_TYPE_LOW) {
> +		} else if (bdata->type == SVSB_TYPE_LOW) {
>   			/* volt[turn_pt] ~ volt[opp_count - 1] */
> -			for (i = turn_pt; i < svsb->opp_count; i++) {
> +			for (i = turn_pt; i < bdata->opp_count; i++) {
>   				b_sft = BITS8 * (shift_byte % REG_BYTES);
>   				vop = (shift_byte < REG_BYTES) ? &vop30 :
>   								 &vop74;
> @@ -999,12 +1014,12 @@ static void svs_get_bank_volts_v3(struct svs_platform *svsp, struct svs_bank *sv
>   		}
>   	}
>   
> -	if (svsb->type == SVSB_TYPE_HIGH) {
> +	if (bdata->type == SVSB_TYPE_HIGH) {
>   		opp_start = 0;
>   		opp_stop = svsb->turn_pt;
> -	} else if (svsb->type == SVSB_TYPE_LOW) {
> +	} else if (bdata->type == SVSB_TYPE_LOW) {
>   		opp_start = svsb->turn_pt;
> -		opp_stop = svsb->opp_count;
> +		opp_stop = bdata->opp_count;
>   	}
>   
>   	for (i = opp_start; i < opp_stop; i++)
> @@ -1014,11 +1029,11 @@ static void svs_get_bank_volts_v3(struct svs_platform *svsp, struct svs_bank *sv
>   	/* For voltage bin support */
>   	if (svsb->opp_dfreq[0] > svsb->freq_base) {
>   		svsb->volt[0] = svs_opp_volt_to_bank_volt(svsb->opp_dvolt[0],
> -							  svsb->volt_step,
> -							  svsb->volt_base);
> +							  bdata->volt_step,
> +							  bdata->volt_base);
>   
>   		/* Find voltage bin turn point */
> -		for (i = 0; i < svsb->opp_count; i++) {
> +		for (i = 0; i < bdata->opp_count; i++) {
>   			if (svsb->opp_dfreq[i] <= svsb->freq_base) {
>   				svsb->vbin_turn_pt = i;
>   				break;
> @@ -1037,12 +1052,13 @@ static void svs_get_bank_volts_v3(struct svs_platform *svsp, struct svs_bank *sv
>   
>   static void svs_set_bank_freq_pct_v3(struct svs_platform *svsp, struct svs_bank *svsb)
>   {
> +	const struct svs_bank_pdata *bdata = &svsb->pdata;
>   	u32 i, j, *freq_pct, freq_pct74 = 0, freq_pct30 = 0;
>   	u32 b_sft, shift_byte = 0, turn_pt;
> -	u32 middle_index = (svsb->opp_count / 2);
> +	u32 middle_index = (bdata->opp_count / 2);
>   
> -	for (i = 0; i < svsb->opp_count; i++) {
> -		if (svsb->opp_dfreq[i] <= svsb->turn_freq_base) {
> +	for (i = 0; i < bdata->opp_count; i++) {
> +		if (svsb->opp_dfreq[i] <= bdata->turn_freq_base) {
>   			svsb->turn_pt = i;
>   			break;
>   		}
> @@ -1052,7 +1068,7 @@ static void svs_set_bank_freq_pct_v3(struct svs_platform *svsp, struct svs_bank
>   
>   	/* Target is to fill out freq_pct74 / freq_pct30 by algorithm */
>   	if (turn_pt < middle_index) {
> -		if (svsb->type == SVSB_TYPE_HIGH) {
> +		if (bdata->type == SVSB_TYPE_HIGH) {
>   			/*
>   			 * If we don't handle this situation,
>   			 * SVSB_TYPE_HIGH's FREQPCT74 / FREQPCT30 would keep "0"
> @@ -1069,15 +1085,15 @@ static void svs_set_bank_freq_pct_v3(struct svs_platform *svsp, struct svs_bank
>   				*freq_pct |= (svsb->freq_pct[i] << b_sft);
>   				shift_byte++;
>   			}
> -		} else if (svsb->type == SVSB_TYPE_LOW) {
> +		} else if (bdata->type == SVSB_TYPE_LOW) {
>   			/*
>   			 * freq_pct[turn_pt] +
>   			 * freq_pct[opp_count - 7] ~ freq_pct[opp_count -1]
>   			 */
>   			freq_pct30 = svsb->freq_pct[turn_pt];
>   			shift_byte++;
> -			j = svsb->opp_count - 7;
> -			for (i = j; i < svsb->opp_count; i++) {
> +			j = bdata->opp_count - 7;
> +			for (i = j; i < bdata->opp_count; i++) {
>   				b_sft = BITS8 * (shift_byte % REG_BYTES);
>   				freq_pct = (shift_byte < REG_BYTES) ?
>   					   &freq_pct30 : &freq_pct74;
> @@ -1086,7 +1102,7 @@ static void svs_set_bank_freq_pct_v3(struct svs_platform *svsp, struct svs_bank
>   			}
>   		}
>   	} else {
> -		if (svsb->type == SVSB_TYPE_HIGH) {
> +		if (bdata->type == SVSB_TYPE_HIGH) {
>   			/*
>   			 * freq_pct[0] +
>   			 * freq_pct[turn_pt - 7] ~ freq_pct[turn_pt - 1]
> @@ -1101,9 +1117,9 @@ static void svs_set_bank_freq_pct_v3(struct svs_platform *svsp, struct svs_bank
>   				*freq_pct |= (svsb->freq_pct[i] << b_sft);
>   				shift_byte++;
>   			}
> -		} else if (svsb->type == SVSB_TYPE_LOW) {
> +		} else if (bdata->type == SVSB_TYPE_LOW) {
>   			/* freq_pct[turn_pt] ~ freq_pct[opp_count - 1] */
> -			for (i = turn_pt; i < svsb->opp_count; i++) {
> +			for (i = turn_pt; i < bdata->opp_count; i++) {
>   				b_sft = BITS8 * (shift_byte % REG_BYTES);
>   				freq_pct = (shift_byte < REG_BYTES) ?
>   					   &freq_pct30 : &freq_pct74;
> @@ -1119,6 +1135,7 @@ static void svs_set_bank_freq_pct_v3(struct svs_platform *svsp, struct svs_bank
>   
>   static void svs_get_bank_volts_v2(struct svs_platform *svsp, struct svs_bank *svsb)
>   {
> +	const struct svs_bank_pdata *bdata = &svsb->pdata;
>   	u32 temp, i;
>   
>   	temp = svs_readl_relaxed(svsp, VOP74);
> @@ -1146,17 +1163,17 @@ static void svs_get_bank_volts_v2(struct svs_platform *svsp, struct svs_bank *sv
>   				     svsb->volt[14],
>   				     svsb->freq_pct[15]);
>   
> -	for (i = 0; i < svsb->opp_count; i++)
> +	for (i = 0; i < bdata->opp_count; i++)
>   		svsb->volt[i] += svsb->volt_od;
>   
>   	/* For voltage bin support */
>   	if (svsb->opp_dfreq[0] > svsb->freq_base) {
>   		svsb->volt[0] = svs_opp_volt_to_bank_volt(svsb->opp_dvolt[0],
> -							  svsb->volt_step,
> -							  svsb->volt_base);
> +							  bdata->volt_step,
> +							  bdata->volt_base);
>   
>   		/* Find voltage bin turn point */
> -		for (i = 0; i < svsb->opp_count; i++) {
> +		for (i = 0; i < bdata->opp_count; i++) {
>   			if (svsb->opp_dfreq[i] <= svsb->freq_base) {
>   				svsb->vbin_turn_pt = i;
>   				break;
> @@ -1196,6 +1213,7 @@ static void svs_set_bank_phase(struct svs_platform *svsp,
>   			       enum svsb_phase target_phase)
>   {
>   	struct svs_bank *svsb = &svsp->banks[bank_idx];
> +	const struct svs_bank_pdata *bdata = &svsb->pdata;
>   	u32 des_char, temp_char, det_char, limit_vals, init2vals, ts_calcs;
>   
>   	svs_switch_bank(svsp, svsb);
> @@ -1204,7 +1222,7 @@ static void svs_set_bank_phase(struct svs_platform *svsp,
>   		   FIELD_PREP(SVSB_DESCHAR_FLD_MDES, svsb->mdes);
>   	svs_writel_relaxed(svsp, des_char, DESCHAR);
>   
> -	temp_char = FIELD_PREP(SVSB_TEMPCHAR_FLD_VCO, svsb->vco) |
> +	temp_char = FIELD_PREP(SVSB_TEMPCHAR_FLD_VCO, bdata->vco) |
>   		    FIELD_PREP(SVSB_TEMPCHAR_FLD_MTDES, svsb->mtdes) |
>   		    FIELD_PREP(SVSB_TEMPCHAR_FLD_DVT_FIXED, svsb->dvt_fixed);
>   	svs_writel_relaxed(svsp, temp_char, TEMPCHAR);
> @@ -1213,11 +1231,11 @@ static void svs_set_bank_phase(struct svs_platform *svsp,
>   		   FIELD_PREP(SVSB_DETCHAR_FLD_DCMDET, svsb->dcmdet);
>   	svs_writel_relaxed(svsp, det_char, DETCHAR);
>   
> -	svs_writel_relaxed(svsp, svsb->dc_config, DCCONFIG);
> -	svs_writel_relaxed(svsp, svsb->age_config, AGECONFIG);
> +	svs_writel_relaxed(svsp, bdata->dc_config, DCCONFIG);
> +	svs_writel_relaxed(svsp, bdata->age_config, AGECONFIG);
>   	svs_writel_relaxed(svsp, SVSB_RUNCONFIG_DEFAULT, RUNCONFIG);
>   
> -	svsb->set_freq_pct(svsp, svsb);
> +	bdata->set_freq_pct(svsp, svsb);
>   
>   	limit_vals = FIELD_PREP(SVSB_LIMITVALS_FLD_DTLO, SVSB_VAL_DTLO) |
>   		     FIELD_PREP(SVSB_LIMITVALS_FLD_DTHI, SVSB_VAL_DTHI) |
> @@ -1227,13 +1245,13 @@ static void svs_set_bank_phase(struct svs_platform *svsp,
>   
>   	svs_writel_relaxed(svsp, SVSB_DET_WINDOW, DETWINDOW);
>   	svs_writel_relaxed(svsp, SVSB_DET_MAX, CONFIG);
> -	svs_writel_relaxed(svsp, svsb->chk_shift, CHKSHIFT);
> -	svs_writel_relaxed(svsp, svsb->ctl0, CTL0);
> +	svs_writel_relaxed(svsp, bdata->chk_shift, CHKSHIFT);
> +	svs_writel_relaxed(svsp, bdata->ctl0, CTL0);
>   	svs_writel_relaxed(svsp, SVSB_INTSTS_VAL_CLEAN, INTSTS);
>   
>   	switch (target_phase) {
>   	case SVSB_PHASE_INIT01:
> -		svs_writel_relaxed(svsp, svsb->vboot, VBOOT);
> +		svs_writel_relaxed(svsp, bdata->vboot, VBOOT);
>   		svs_writel_relaxed(svsp, SVSB_INTEN_INIT0x, INTEN);
>   		svs_writel_relaxed(svsp, SVSB_PTPEN_INIT01, SVSEN);
>   		break;
> @@ -1305,8 +1323,10 @@ static inline void svs_init01_isr_handler(struct svs_platform *svsp,
>   	svs_save_bank_register_data(svsp, bank_idx, SVSB_PHASE_INIT01);
>   
>   	svsb->phase = SVSB_PHASE_INIT01;
> +
>   	val = ~(svs_readl_relaxed(svsp, DCVALUES) & GENMASK(15, 0)) + 1;
>   	svsb->dc_voffset_in = val & GENMASK(15, 0);
> +
Do these new blank lines have any added value ?

>   	if (svsb->volt_flags & SVSB_INIT01_VOLT_IGNORE ||
>   	    (svsb->dc_voffset_in & SVSB_DC_SIGNED_BIT &&
>   	     svsb->volt_flags & SVSB_INIT01_VOLT_INC_ONLY))
> @@ -1324,6 +1344,8 @@ static inline void svs_init02_isr_handler(struct svs_platform *svsp,
>   					  unsigned short bank_idx)
>   {
>   	struct svs_bank *svsb = &svsp->banks[bank_idx];
> +	const struct svs_bank_pdata *bdata = &svsb->pdata;
> +
>   
I guess here is a bogus new line

>   	dev_info(svsb->dev, "%s: VOP74~30:0x%08x~0x%08x, DC:0x%08x\n",
>   		 __func__, svs_readl_relaxed(svsp, VOP74),
> @@ -1333,7 +1355,7 @@ static inline void svs_init02_isr_handler(struct svs_platform *svsp,
>   	svs_save_bank_register_data(svsp, bank_idx, SVSB_PHASE_INIT02);
>   
>   	svsb->phase = SVSB_PHASE_INIT02;
> -	svsb->get_volts(svsp, svsb);
> +	bdata->get_volts(svsp, svsb);
>   
>   	svs_writel_relaxed(svsp, SVSB_PTPEN_OFF, SVSEN);
>   	svs_writel_relaxed(svsp, SVSB_INTSTS_F0_COMPLETE, INTSTS);
> @@ -1343,11 +1365,12 @@ static inline void svs_mon_mode_isr_handler(struct svs_platform *svsp,
>   					    unsigned short bank_idx)
>   {
>   	struct svs_bank *svsb = &svsp->banks[bank_idx];
> +	const struct svs_bank_pdata *bdata = &svsb->pdata;
>   
>   	svs_save_bank_register_data(svsp, bank_idx, SVSB_PHASE_MON);
>   
>   	svsb->phase = SVSB_PHASE_MON;
> -	svsb->get_volts(svsp, svsb);
> +	bdata->get_volts(svsp, svsb);
>   
>   	svsb->temp = svs_readl_relaxed(svsp, TEMP) & GENMASK(7, 0);
>   	svs_writel_relaxed(svsp, SVSB_INTSTS_FLD_MONVOP, INTSTS);
> @@ -1356,18 +1379,20 @@ static inline void svs_mon_mode_isr_handler(struct svs_platform *svsp,
>   static irqreturn_t svs_isr(int irq, void *data)
>   {
>   	struct svs_platform *svsp = data;
> +	const struct svs_bank_pdata *bdata = NULL;

Is this init to NULL redundant ?

>   	struct svs_bank *svsb = NULL;
>   	unsigned long flags;
>   	u32 idx, int_sts, svs_en;
>   
>   	for (idx = 0; idx < svsp->bank_max; idx++) {
>   		svsb = &svsp->banks[idx];
> +		bdata = &svsb->pdata;
>   		WARN(!svsb, "%s: svsb(%s) is null", __func__, svsb->name);
>   
>   		spin_lock_irqsave(&svs_lock, flags);
>   
>   		/* Find out which svs bank fires interrupt */
> -		if (svsb->int_st & svs_readl_relaxed(svsp, INTST)) {
> +		if (bdata->int_st & svs_readl_relaxed(svsp, INTST)) {
>   			spin_unlock_irqrestore(&svs_lock, flags);
>   			continue;
>   		}
> @@ -1412,6 +1437,7 @@ static bool svs_mode_available(struct svs_platform *svsp, u8 mode)
>   
>   static int svs_init01(struct svs_platform *svsp)
>   {
> +	const struct svs_bank_pdata *bdata;
>   	struct svs_bank *svsb;
>   	unsigned long flags, time_left;
>   	bool search_done;
> @@ -1427,6 +1453,7 @@ static int svs_init01(struct svs_platform *svsp)
>   	 /* Svs bank init01 preparation - power enable */
>   	for (idx = 0; idx < svsp->bank_max; idx++) {
>   		svsb = &svsp->banks[idx];
> +		bdata = &svsb->pdata;
>   
>   		if (!(svsb->mode_support & SVSB_MODE_INIT01))
>   			continue;
> @@ -1434,7 +1461,7 @@ static int svs_init01(struct svs_platform *svsp)
>   		ret = regulator_enable(svsb->buck);
>   		if (ret) {
>   			dev_err(svsb->dev, "%s enable fail: %d\n",
> -				svsb->buck_name, ret);
> +				bdata->buck_name, ret);
>   			goto svs_init01_resume_cpuidle;
>   		}
>   
> @@ -1464,6 +1491,7 @@ static int svs_init01(struct svs_platform *svsp)
>   	 */
>   	for (idx = 0; idx < svsp->bank_max; idx++) {
>   		svsb = &svsp->banks[idx];
> +		bdata = &svsb->pdata;
>   
>   		if (!(svsb->mode_support & SVSB_MODE_INIT01))
>   			continue;
> @@ -1473,11 +1501,11 @@ static int svs_init01(struct svs_platform *svsp)
>   		 * fix to that freq until svs_init01 is done.
>   		 */
>   		search_done = false;
> -		opp_vboot = svs_bank_volt_to_opp_volt(svsb->vboot,
> -						      svsb->volt_step,
> -						      svsb->volt_base);
> +		opp_vboot = svs_bank_volt_to_opp_volt(bdata->vboot,
> +						      bdata->volt_step,
> +						      bdata->volt_base);
>   
> -		for (i = 0; i < svsb->opp_count; i++) {
> +		for (i = 0; i < bdata->opp_count; i++) {
>   			opp_freq = svsb->opp_dfreq[i];
>   			if (!search_done && svsb->opp_dvolt[i] <= opp_vboot) {
>   				ret = dev_pm_opp_adjust_voltage(svsb->opp_dev,
> @@ -1509,13 +1537,14 @@ static int svs_init01(struct svs_platform *svsp)
>   	/* Svs bank init01 begins */
>   	for (idx = 0; idx < svsp->bank_max; idx++) {
>   		svsb = &svsp->banks[idx];
> +		bdata = &svsb->pdata;
>   
>   		if (!(svsb->mode_support & SVSB_MODE_INIT01))
>   			continue;
>   
> -		opp_vboot = svs_bank_volt_to_opp_volt(svsb->vboot,
> -						      svsb->volt_step,
> -						      svsb->volt_base);
> +		opp_vboot = svs_bank_volt_to_opp_volt(bdata->vboot,
> +						      bdata->volt_step,
> +						      bdata->volt_base);
>   
>   		buck_volt = regulator_get_voltage(svsb->buck);
>   		if (buck_volt != opp_vboot) {
> @@ -1542,11 +1571,12 @@ static int svs_init01(struct svs_platform *svsp)
>   svs_init01_finish:
>   	for (idx = 0; idx < svsp->bank_max; idx++) {
>   		svsb = &svsp->banks[idx];
> +		bdata = &svsb->pdata;
>   
>   		if (!(svsb->mode_support & SVSB_MODE_INIT01))
>   			continue;
>   
> -		for (i = 0; i < svsb->opp_count; i++) {
> +		for (i = 0; i < bdata->opp_count; i++) {
>   			r = dev_pm_opp_enable(svsb->opp_dev,
>   					      svsb->opp_dfreq[i]);
>   			if (r)
> @@ -1572,7 +1602,7 @@ static int svs_init01(struct svs_platform *svsp)
>   		r = regulator_disable(svsb->buck);
>   		if (r)
>   			dev_err(svsb->dev, "%s disable fail: %d\n",
> -				svsb->buck_name, r);
> +				bdata->buck_name, r);
>   	}
>   
>   svs_init01_resume_cpuidle:
> @@ -1583,6 +1613,7 @@ static int svs_init01(struct svs_platform *svsp)
>   
>   static int svs_init02(struct svs_platform *svsp)
>   {
> +	const struct svs_bank_pdata *bdata;
>   	struct svs_bank *svsb;
>   	unsigned long flags, time_left;
>   	int ret;
> @@ -1618,11 +1649,12 @@ static int svs_init02(struct svs_platform *svsp)
>   	 */
>   	for (idx = 0; idx < svsp->bank_max; idx++) {
>   		svsb = &svsp->banks[idx];
> +		bdata = &svsb->pdata;
>   
>   		if (!(svsb->mode_support & SVSB_MODE_INIT02))
>   			continue;
>   
> -		if (svsb->type == SVSB_TYPE_HIGH || svsb->type == SVSB_TYPE_LOW) {
> +		if (bdata->type == SVSB_TYPE_HIGH || bdata->type == SVSB_TYPE_LOW) {
>   			if (svs_sync_bank_volts_from_opp(svsb)) {
>   				dev_err(svsb->dev, "sync volt fail\n");
>   				ret = -EPERM;
> @@ -1680,12 +1712,12 @@ static int svs_start(struct svs_platform *svsp)
>   static int svs_suspend(struct device *dev)
>   {
>   	struct svs_platform *svsp = dev_get_drvdata(dev);
> -	struct svs_bank *svsb;
>   	int ret;
>   	u32 idx;
>   
>   	for (idx = 0; idx < svsp->bank_max; idx++) {
> -		svsb = &svsp->banks[idx];
> +		struct svs_bank *svsb = &svsp->banks[idx];
> +
>   		svs_bank_disable_and_restore_default_volts(svsp, svsb);
>   	}
>   
> @@ -1736,6 +1768,7 @@ static int svs_resume(struct device *dev)
>   
>   static int svs_bank_resource_setup(struct svs_platform *svsp)
>   {
> +	const struct svs_bank_pdata *bdata;
>   	struct svs_bank *svsb;
>   	struct dev_pm_opp *opp;
>   	unsigned long freq;
> @@ -1746,8 +1779,9 @@ static int svs_bank_resource_setup(struct svs_platform *svsp)
>   
>   	for (idx = 0; idx < svsp->bank_max; idx++) {
>   		svsb = &svsp->banks[idx];
> +		bdata = &svsb->pdata;
>   
> -		if (svsb->sw_id >= SVSB_SWID_MAX || svsb->type >= SVSB_TYPE_MAX) {
> +		if (bdata->sw_id >= SVSB_SWID_MAX || bdata->type >= SVSB_TYPE_MAX) {
>   			dev_err(svsb->dev, "unknown bank sw_id or type\n");
>   			return -EINVAL;
>   		}
> @@ -1757,8 +1791,8 @@ static int svs_bank_resource_setup(struct svs_platform *svsp)
>   			return -ENOMEM;
>   
>   		svsb->name = devm_kasprintf(svsp->dev, GFP_KERNEL, "%s%s",
> -					    svs_swid_names[svsb->sw_id],
> -					    svs_type_names[svsb->type]);
> +					    svs_swid_names[bdata->sw_id],
> +					    svs_type_names[bdata->type]);
>   		if (!svsb->name)
>   			return -ENOMEM;
>   
> @@ -1779,10 +1813,10 @@ static int svs_bank_resource_setup(struct svs_platform *svsp)
>   
>   		if (svsb->mode_support & SVSB_MODE_INIT01) {
>   			svsb->buck = devm_regulator_get_optional(svsb->opp_dev,
> -								 svsb->buck_name);
> +								 bdata->buck_name);
>   			if (IS_ERR(svsb->buck)) {
>   				dev_err(svsb->dev, "cannot get \"%s-supply\"\n",
> -					svsb->buck_name);
> +					bdata->buck_name);
>   				return PTR_ERR(svsb->buck);
>   			}
>   		}
> @@ -1793,18 +1827,17 @@ static int svs_bank_resource_setup(struct svs_platform *svsp)
>   				dev_err(svsb->dev, "cannot get \"%s\" thermal zone\n",
>   					svsb->tzone_name);
>   				return PTR_ERR(svsb->tzd);
> -			}

This deleted line above is strange. Have you removed an open bracket and 
I missed it ?

>   		}
>   
>   		count = dev_pm_opp_get_opp_count(svsb->opp_dev);
> -		if (svsb->opp_count != count) {
> +		if (bdata->opp_count != count) {
>   			dev_err(svsb->dev,
>   				"opp_count not \"%u\" but get \"%d\"?\n",
> -				svsb->opp_count, count);
> +				bdata->opp_count, count);
>   			return count;
>   		}
>   
> -		for (i = 0, freq = ULONG_MAX; i < svsb->opp_count; i++, freq--) {
> +		for (i = 0, freq = ULONG_MAX; i < bdata->opp_count; i++, freq--) {
>   			opp = dev_pm_opp_find_freq_floor(svsb->opp_dev, &freq);
>   			if (IS_ERR(opp)) {
>   				dev_err(svsb->dev, "cannot find freq = %ld\n",
> @@ -1901,7 +1934,8 @@ static bool svs_common_parse_efuse(struct svs_platform *svsp,
>   
>   	for (i = 0; i < svsp->bank_max; i++) {
>   		struct svs_bank *svsb = &svsp->banks[i];
> -		const struct svs_fusemap *dfmap = svsb->dev_fuse_map;
> +		const struct svs_bank_pdata *bdata = &svsb->pdata;
> +		const struct svs_fusemap *dfmap = bdata->dev_fuse_map;
>   
>   		if (vmin == 1)
>   			svsb->vmin = 0x1e;
> @@ -1927,11 +1961,11 @@ static bool svs_mt8183_efuse_parsing(struct svs_platform *svsp,
>   				     const struct svs_platform_data *pdata)
>   {
>   	struct svs_bank *svsb;
> +	const struct svs_bank_pdata *bdata;
>   	int format[6], x_roomt[6], o_vtsmcu[5], o_vtsabb, tb_roomt = 0;
>   	int adc_ge_t, adc_oe_t, ge, oe, gain, degc_cali, adc_cali_en_t;
>   	int o_slope, o_slope_sign, ts_id;
>   	u32 idx, i, ft_pgm, mts, temp0, temp1, temp2;
> -	int ret;
>   
>   	for (i = 0; i < svsp->efuse_max; i++)
>   		if (svsp->efuse[i])
> @@ -1948,7 +1982,8 @@ static bool svs_mt8183_efuse_parsing(struct svs_platform *svsp,
>   
>   	for (idx = 0; idx < svsp->bank_max; idx++) {
>   		svsb = &svsp->banks[idx];
> -		const struct svs_fusemap *dfmap = svsb->dev_fuse_map;
> +		bdata = &svsb->pdata;
> +		const struct svs_fusemap *dfmap = bdata->dev_fuse_map;
>   
>   		if (ft_pgm <= 1)
>   			svsb->volt_flags |= SVSB_INIT01_VOLT_IGNORE;
> @@ -1959,7 +1994,7 @@ static bool svs_mt8183_efuse_parsing(struct svs_platform *svsp,
>   		svsb->dcbdet = svs_get_fuse_val(svsp->efuse, &dfmap[BDEV_DCBDET], 8);
>   		svsb->dcmdet = svs_get_fuse_val(svsp->efuse, &dfmap[BDEV_DCMDET], 8);
>   
> -		switch (svsb->sw_id) {
> +		switch (bdata->sw_id) {
>   		case SVSB_SWID_CPU_LITTLE:
>   		case SVSB_SWID_CCI:
>   			if (ft_pgm <= 3)
> @@ -1985,6 +2020,7 @@ static bool svs_mt8183_efuse_parsing(struct svs_platform *svsp,
>   		}
>   	}
>   
> +

This line appears to be bogus

>   	/* Thermal efuse parsing */
>   	adc_ge_t = (svsp->tefuse[1] >> 22) & GENMASK(9, 0);
>   	adc_oe_t = (svsp->tefuse[1] >> 12) & GENMASK(9, 0);


[snip]