Re: [PATCH v7 16/23] scsi: ufs: mediatek: Clean up logging prints

["Peter Wang (王信友)" <peter.wang@mediatek.com>]

On Mon, 2026-02-16 at 14:37 +0100, Nicolas Frattaroli wrote:
>  drivers/ufs/host/ufs-mediatek.c | 99 ++++++++++++++++++-------------
> ----------
>  1 file changed, 43 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-
> mediatek.c
> index ecf16e82a326..2b1f26b55782 100644
> --- a/drivers/ufs/host/ufs-mediatek.c
> +++ b/drivers/ufs/host/ufs-mediatek.c
> @@ -192,8 +192,8 @@ static void ufs_mtk_crypto_enable(struct ufs_hba
> *hba)
>  
>  	ufs_mtk_crypto_ctrl(res, 1);
>  	if (res.a0) {
> -		dev_info(hba->dev, "%s: crypto enable failed, err:
> %lu\n",
> -			 __func__, res.a0);
> +		dev_err(hba->dev, "%s: crypto enable failed with
> error %lu, disabling\n",
> +			__func__, res.a0);
>  		hba->caps &= ~UFSHCD_CAP_CRYPTO;
>  	}
>  }
> @@ -542,40 +542,38 @@ static void ufs_mtk_boost_crypt(struct ufs_hba
> *hba, bool boost)
>  
>  	ret = clk_prepare_enable(cfg->clk_crypt_mux);
>  	if (ret) {
> -		dev_info(hba->dev, "clk_prepare_enable(): %d\n",
> -			 ret);
> +		dev_err(hba->dev, "%s: Failed to enable
> clk_crypt_mux: %pe\n",
> +			__func__, ERR_PTR(ret));
>  		return;
>  	}
>  
>  	if (boost) {
>  		ret = regulator_set_voltage(reg, volt, INT_MAX);
>  		if (ret) {
> -			dev_info(hba->dev,
> -				 "failed to set vcore to %d\n",
> volt);
> +			dev_err(hba->dev, "%s: Failed to set vcore
> to %d: %pe\n",
> +				__func__, volt, ERR_PTR(ret));
>  			goto out;
>  		}
>  
> -		ret = clk_set_parent(cfg->clk_crypt_mux,
> -				     cfg->clk_crypt_perf);
> +		ret = clk_set_parent(cfg->clk_crypt_mux, cfg-
> >clk_crypt_perf);
>  		if (ret) {
> -			dev_info(hba->dev,
> -				 "failed to set clk_crypt_perf\n");
> +			dev_err(hba->dev, "%s: Failed to reparent
> clk_crypt_perf: %pe\n",
> +				__func__, ERR_PTR(ret));
>  			regulator_set_voltage(reg, 0, INT_MAX);
>  			goto out;
>  		}
>  	} else {
> -		ret = clk_set_parent(cfg->clk_crypt_mux,
> -				     cfg->clk_crypt_lp);
> +		ret = clk_set_parent(cfg->clk_crypt_mux, cfg-
> >clk_crypt_lp);
>  		if (ret) {
> -			dev_info(hba->dev,
> -				 "failed to set clk_crypt_lp\n");
> +			dev_err(hba->dev, "%s: Failed to reparent
> clk_crypt_lp: %pe\n",
> +				__func__, ERR_PTR(ret));
>  			goto out;
>  		}
>  
>  		ret = regulator_set_voltage(reg, 0, INT_MAX);
>  		if (ret) {
> -			dev_info(hba->dev,
> -				 "failed to set vcore to MIN\n");
> +			dev_err(hba->dev, "%s: Failed to set vcore
> to minimum: %pe\n",
> +				__func__, ERR_PTR(ret));
>  		}
>  	}
>  out:
> @@ -763,10 +761,8 @@ static int ufs_mtk_setup_clocks(struct ufs_hba
> *hba, bool on,
>  		if (clk_pwr_off) {
>  			ufs_mtk_pwr_ctrl(hba, false);
>  		} else {
> -			dev_warn(hba->dev, "Clock is not turned off,
> hba->ahit = 0x%x, AHIT = 0x%x\n",
> -				hba->ahit,
> -				ufshcd_readl(hba,
> -
> 					REG_AUTO_HIBERNATE_IDLE_TIMER));
> +			dev_warn(hba->dev, "Clock isn't off, hba-
> >ahit = 0x%x, AHIT = 0x%x\n",
> +				 hba->ahit, ufshcd_readl(hba,
> REG_AUTO_HIBERNATE_IDLE_TIMER));
>  		}
>  		ufs_mtk_mcq_disable_irq(hba);
>  	} else if (on && status == POST_CHANGE) {
> @@ -810,11 +806,11 @@ static void ufs_mtk_mcq_set_irq_affinity(struct
> ufs_hba *hba, unsigned int cpu)
>  	_cpu = (cpu == 0) ? 3 : cpu;
>  	ret = irq_set_affinity(irq, cpumask_of(_cpu));
>  	if (ret) {
> -		dev_err(hba->dev, "set irq %d affinity to CPU %d
> failed\n",
> +		dev_err(hba->dev, "setting irq %d affinity to CPU %d
> failed\n",
>  			irq, _cpu);
>  		return;
>  	}
> -	dev_info(hba->dev, "set irq %d affinity to CPU: %d\n", irq,
> _cpu);
> +	dev_dbg(hba->dev, "set irq %d affinity to CPU %d\n", irq,
> _cpu);
> 

Is it more appropriate to use dev_info for state changes or for setting
changes?

>  }
>  
>  static bool ufs_mtk_is_legacy_chipset(struct ufs_hba *hba, u32
> hw_ip_ver)
> @@ -830,7 +826,8 @@ static bool ufs_mtk_is_legacy_chipset(struct
> ufs_hba *hba, u32 hw_ip_ver)
>  	default:
>  		break;
>  	}
> -	dev_info(hba->dev, "legacy IP version - 0x%x, is legacy :
> %d", hw_ip_ver, is_legacy);
> +	dev_dbg(hba->dev, "IP version 0x%x, legacy = %s", hw_ip_ver,
> +		str_true_false(is_legacy));
>  
>  	return is_legacy;
>  }
> @@ -935,15 +932,12 @@ static void ufs_mtk_init_clocks(struct ufs_hba
> *hba)
>  		}
>  	}
>  
> -	list_for_each_entry(clki, head, list) {
> -		dev_info(hba->dev, "clk \"%s\" present", clki-
> >name);
> -	}
> +	list_for_each_entry(clki, head, list)
> +		dev_dbg(hba->dev, "clk \"%s\" present", clki->name);
>  
>  	if (!ufs_mtk_is_clk_scale_ready(hba)) {
>  		hba->caps &= ~UFSHCD_CAP_CLK_SCALING;
> -		dev_info(hba->dev,
> -			 "%s: Clk-scaling not ready. Feature
> disabled.",
> -			 __func__);
> +		dev_info(hba->dev, "%s: Clock scaling unavailable",
> __func__);
>  		return;
>  	}
>  
> @@ -953,8 +947,8 @@ static void ufs_mtk_init_clocks(struct ufs_hba
> *hba)
>  	 */
>  	reg = devm_regulator_get_optional(dev, "dvfsrc-vcore");
>  	if (IS_ERR(reg)) {
> -		dev_info(dev, "failed to get dvfsrc-vcore: %ld",
> -			 PTR_ERR(reg));
> +		if (PTR_ERR(reg) != -ENODEV)
> +			dev_err(dev, "Failed to get dvfsrc-vcore:
> %pe\n", reg);
>  		return;
>  	}
>  
> @@ -968,12 +962,9 @@ static void ufs_mtk_init_clocks(struct ufs_hba
> *hba)
>  	host->mclk.vcore_volt = volt;
>  
>  	/* If default boot is max gear, request vcore */
> -	if (reg && volt && host->clk_scale_up) {
> -		if (regulator_set_voltage(reg, volt, INT_MAX)) {
> -			dev_info(hba->dev,
> -				"Failed to set vcore to %d\n",
> volt);
> -		}
> -	}
> +	if (reg && volt && host->clk_scale_up)
> +		if (regulator_set_voltage(reg, volt, INT_MAX))
> +			dev_err(hba->dev, "Failed to set vcore to
> %d\n", volt);
>  }
>  
>  static void ufs_mtk_setup_clk_gating(struct ufs_hba *hba)
> @@ -1060,7 +1051,7 @@ static void ufs_mtk_init_mcq_irq(struct ufs_hba
> *hba)
>  		}
>  		host->mcq_intr_info[i].hba = hba;
>  		host->mcq_intr_info[i].irq = irq;
> -		dev_info(hba->dev, "get platform mcq irq: %d, %d\n",
> i, irq);
> +		dev_dbg(hba->dev, "get platform mcq irq: %d, %d\n",
> i, irq);
>  	}
>  
>  	return;
> @@ -1307,10 +1298,8 @@ static int ufs_mtk_pre_pwr_change(struct
> ufs_hba *hba,
>  		host_params.desired_working_mode = UFS_PWM_MODE;
>  
>  	ret = ufshcd_negotiate_pwr_params(&host_params,
> dev_max_params, dev_req_params);
> -	if (ret) {
> -		pr_info("%s: failed to determine capabilities\n",
> -			__func__);
> -	}
> +	if (ret)
> +		dev_warn(hba->dev, "%s: failed to determine
> capabilities\n", __func__);
>  
>  	if (ufs_mtk_pmc_via_fastauto(hba, dev_req_params)) {
>  		ufs_mtk_adjust_sync_length(hba);
> @@ -1356,10 +1345,9 @@ static int ufs_mtk_pre_pwr_change(struct
> ufs_hba *hba,
>  		ret = ufshcd_uic_change_pwr_mode(hba,
>  					FASTAUTO_MODE << 4 |
> FASTAUTO_MODE);
>  
> -		if (ret) {
> -			dev_err(hba->dev, "%s: HSG1B FASTAUTO failed
> ret=%d\n",
> -				__func__, ret);
> -		}
> +		if (ret)
> +			dev_err(hba->dev, "%s: HSG1B FASTAUTO
> failed: %pe\n",
> +				__func__, ERR_PTR(ret));
>  	}
>  
>  	/* if already configured to the requested pwr_mode, skip
> adapt */
> @@ -1409,7 +1397,7 @@ static int ufs_mtk_auto_hibern8_disable(struct
> ufs_hba *hba)
>  
>  out:
>  	if (ret) {
> -		dev_warn(hba->dev, "exit h8 state fail, ret=%d\n",
> ret);
> +		dev_err(hba->dev, "Failed to exit h8 state: %pe\n",
> ERR_PTR(ret));
>  
>  		ufshcd_force_error_recovery(hba);
>  
> @@ -1571,7 +1559,7 @@ static int ufs_mtk_device_reset(struct ufs_hba
> *hba)
>  	/* Some devices may need time to respond to rst_n */
>  	usleep_range(10000, 15000);
>  
> -	dev_info(hba->dev, "device reset done\n");
> +	dev_dbg(hba->dev, "device reset done\n");
>  

Is it more appropriate to use dev_info for state changes or for setting
changes?


Thanks
Peter