Re: [PATCH 4/8] scsi: ufs: qcom: Implement the freq_to_gear_speed() vops

[Manivannan Sadhasivam <mani@kernel.org>]

On Mon, Jan 20, 2025 at 08:07:07PM +0800, Ziqi Chen wrote:
> Hi Mani，
> 
> Thanks for your comments~
> 
> On 1/19/2025 3:30 PM, Manivannan Sadhasivam wrote:
> > On Thu, Jan 16, 2025 at 05:11:45PM +0800, Ziqi Chen wrote:
> > > From: Can Guo <quic_cang@quicinc.com>
> > > 
> > > Implement the freq_to_gear_speed() vops to map the unipro core clock
> > > frequency to the corresponding maximum supported gear speed.
> > > 
> > > Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> > > Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> > > Signed-off-by: Can Guo <quic_cang@quicinc.com>
> > > ---
> > >   drivers/ufs/host/ufs-qcom.c | 32 ++++++++++++++++++++++++++++++++
> > >   1 file changed, 32 insertions(+)
> > > 
> > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> > > index 1e8a23eb8c13..64263fa884f5 100644
> > > --- a/drivers/ufs/host/ufs-qcom.c
> > > +++ b/drivers/ufs/host/ufs-qcom.c
> > > @@ -1803,6 +1803,37 @@ static int ufs_qcom_config_esi(struct ufs_hba *hba)
> > >   	return ret;
> > >   }
> > > +static int ufs_qcom_freq_to_gear_speed(struct ufs_hba *hba, unsigned long freq, u32 *gear)
> > > +{
> > > +	int ret = 0 >
> > Please do not initialize ret with 0. Return the actual value directly.
> > 
> 
> If we don't initialize ret here, for the cases of freq matched in the table,
> it will return an unknown ret value. It is not make sense, right?
> 
> Or you may want to say we don't need “ret” , just need to return gear value?
> But we need this "ret" to check whether the freq is invalid.
> 

I meant to say that you can just return 0 directly in success case and -EINVAL
in the case of error.

> > > +
> > > +	switch (freq) {
> > > +	case 403000000:
> > > +		*gear = UFS_HS_G5;
> > > +		break;
> > > +	case 300000000:
> > > +		*gear = UFS_HS_G4;
> > > +		break;
> > > +	case 201500000:
> > > +		*gear = UFS_HS_G3;
> > > +		break;
> > > +	case 150000000:
> > > +	case 100000000:
> > > +		*gear = UFS_HS_G2;
> > > +		break;
> > > +	case 75000000:
> > > +	case 37500000:
> > > +		*gear = UFS_HS_G1;
> > > +		break;
> > > +	default:
> > > +		ret = -EINVAL;
> > > +		dev_err(hba->dev, "Unsupported clock freq\n");
> > 
> > Print the freq.
> 
> Ok, thank for your suggestion, we can print freq with dev_dbg() in next
> version.
> 

No, use dev_err() with the freq.

- Mani 

-- 
மணிவண்ணன் சதாசிவம்