Re: [PATCH v3 04/10] scsi: ufs-mediatek: Fix the timing of configuring device regulators

[Stanley Chu]

On Tue, 2022-06-14 at 09:33 -0700, Bart Van Assche wrote:
> On 6/14/22 07:16, Stanley Chu wrote:
> > +int ufs_mtk_system_suspend(struct device *dev)
> > +{
> > +	int ret = 0;
> > +	struct ufs_hba *hba = dev_get_drvdata(dev);
> > +
> > +	ret = ufshcd_system_suspend(dev);
> > +
> > +	if (!ret)
> > +		ufs_mtk_vreg_set_lpm(hba, true);
> > +
> > +	return ret;
> > +}
> 
> 
> Please use the traditional kernel coding style and return early in
> case 
> of an error. For the above code, that means to rewrite it as follows:
> 
> 	struct ufs_hba *hba = dev_get_drvdata(dev);
> 	int ret;
> 
> 	ret = ufshcd_system_suspend(dev);
> 	if (ret)
> 		return ret;
> 
> 	ufs_mtk_vreg_set_lpm(hba, true);
> 
> 	return 0;

OK! Fixed in v4.

> 
> > +int ufs_mtk_system_resume(struct device *dev)
> > +{
> > +	int ret = 0;
> > +	struct ufs_hba *hba = dev_get_drvdata(dev);
> > +
> > +	ufs_mtk_vreg_set_lpm(hba, false);
> > +
> > +	ret = ufshcd_system_resume(dev);
> > +
> > +	return ret;
> > +}
> 
> Please remove the variable 'ret' from the above function.

OK! Fixed in v4.
> 
> > +int ufs_mtk_runtime_suspend(struct device *dev)
> > +{
> > +	struct ufs_hba *hba = dev_get_drvdata(dev);
> > +	int ret = 0;
> > +
> > +	ret = ufshcd_runtime_suspend(dev);
> > +
> > +	if (!ret)
> > +		ufs_mtk_vreg_set_lpm(hba, true);
> > +
> > +	return ret;
> > +}
> 
> Please use the "early return" style.

OK! Fixed in v4.
> 
> > +int ufs_mtk_runtime_resume(struct device *dev)
> > +{
> > +	struct ufs_hba *hba = dev_get_drvdata(dev);
> > +	int ret = 0;
> > +
> > +	ufs_mtk_vreg_set_lpm(hba, false);
> > +
> > +	ret = ufshcd_runtime_resume(dev);
> > +
> > +	return ret;
> > +}
> 
> Please remove the variable 'ret' from the above function.

OK! Fixed in v4.
> 
> Thanks,
> 
> Bart.