Re: [PATCH v6 2/2] scsi: ufs: spacemit: k3: Add UFS Host Controller driver

[Yixun Lan <dlan@kernel.org>]

Hi Troy,

On 17:25 Tue 22 Sep     , Troy Mitchell wrote:
> On Mon Sep 21, 2026 at 8:36 PM +08, Yixun Lan wrote:
> > [...]
> >
> > +static int ufs_spacemit_link_startup_pre_change(struct ufs_hba *hba)
> > +{
> > +	u32 value, sys1clk_1us;
> > +	int err;
> > +
> > +	err = ufs_spacemit_mphy_init(hba);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	err = ufs_spacemit_uniprov1p6_init(hba);
> > +	if (err < 0)
> > +		return err;
> > +
> 
> Should this be `if (err)`? The call chain is:
> 
>   ufs_spacemit_uniprov1p6_init()
>     -> ufs_spacemit_dme_set()
>       -> ufshcd_dme_set()
>         -> ufshcd_dme_set_attr()
> 
> ufshcd_dme_set_attr() returns 0 on success and non-zero on failure,
the comment of ufshcd_dme_set_attr() says so,

> including positive UIC error codes. The current check misses those.
> 
but as I checked the code, there is no positive err code, so I don't
think it's necessary to change

> Also, ufshcd_link_startup() ignores the PRE_CHANGE return value in this
> series' base, so returning an error here alone will not abort startup.
> 
I think it's totally fine for it to ignore, as there are follow-up
checks, as I see, it's function of link_startup_notify() that returning
the error code

> > [...]
> >
> > +static int ufs_spacemit_apply_dev_quirks(struct ufs_hba *hba)
> > +{
> > [...]
> > +	err = ufs_spacemit_dme_set(hba, setup_attrs, ARRAY_SIZE(setup_attrs));
> > +	if (err < 0)
> > +		return err;
> The same `if (err)` check is needed here for positive DME errors.
> 
ditto
> > +
> > +	return ufs_spacemit_wait_mphy_pll_lock(hba);
> > +}
> > +
> > [...]
> >
> > +static int ufs_spacemit_init(struct ufs_hba *hba)
> > +{
> > [...]
> > +	hba->caps |= UFSHCD_CAP_RPM_AUTOSUSPEND;
> > +
> > +	return 0;
> > +}
> > +
> 

-- 
Yixun Lan (dlan)