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

Hi Philipp,
 Thanks for your review

On 09:53 Thu 02 Jul     , Philipp Zabel wrote:
> On Do, 2026-07-02 at 02:31 +0000, Yixun Lan wrote:
> > SpacemiT K3 SoC consist of UFS (Universal Flash Storage) Host Controller
> > which has features compatible with JEDEC UFS 2.2, MIPI UniPro v1.61 and
> > M-PHY v3.0 standard.
> > 
> > Signed-off-by: Yixun Lan <[email protected]>
> > ---
> >  drivers/ufs/host/Kconfig        |  12 +
> >  drivers/ufs/host/Makefile       |   1 +
> >  drivers/ufs/host/ufs-spacemit.c | 931 ++++++++++++++++++++++++++++++++++++++++
> >  drivers/ufs/host/ufs-spacemit.h |  90 ++++
> >  4 files changed, 1034 insertions(+)
> > 
> [...]
> > --- /dev/null
> > +++ b/drivers/ufs/host/ufs-spacemit.c
> > @@ -0,0 +1,931 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2026 SpacemiT (Hangzhou) Technology Co. Ltd
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/delay.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> 
> Missing #include <linux/reset.h> for
> devm_reset_control_get_optional_exclusive_deasserted() below.
> Don't rely on indirect includes.
> 
Will add in next version

> [...]
> > +/**
> > + * ufs_spacemit_init - init phy and prepare clk
> > + * @hba: host controller instance
> > + */
> > +static int ufs_spacemit_init(struct ufs_hba *hba)
> > +{
> > +	int err = 0;
> > +	struct device *dev = hba->dev;
> > +	struct ufs_spacemit_host *host;
> > +
> > +	host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
> > +	if (!host)
> > +		return -ENOMEM;
> > +
> > +	host->rst = devm_reset_control_get_optional_exclusive_deasserted(dev, NULL);
> 
> Why is this stored in struct ufs_spacemit_host at all? As far as I can
> see it is never used again, so this could be a local variable.
> 
Ok, will make it a local variable

> [...]
> > diff --git a/drivers/ufs/host/ufs-spacemit.h b/drivers/ufs/host/ufs-spacemit.h
> > new file mode 100644
> > index 000000000000..6ae3c263a360
> > --- /dev/null
> > +++ b/drivers/ufs/host/ufs-spacemit.h
> > @@ -0,0 +1,90 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * SpacemiT UFS Host Controller driver
> > + *
> > + * Copyright (c) 2026 SpacemiT (Hangzhou) Technology Co. Ltd
> > + */
> > +
> > +#ifndef _UFS_SPACEMIT_H_
> > +#define _UFS_SPACEMIT_H_
> > +
> > +#include <linux/reset-controller.h>
> 
> Drop this, we are not implementing a reset controller driver here.
> 
Ok

> > +#include <linux/reset.h>
> 
> You could replace this with a struct reset_control forward declaration.
> Or drop it ...
> 
Will drop it

> [...]
> > +struct ufs_spacemit_host {
> > +	struct ufs_hba *hba;
> > +	struct ufs_pa_layer_attr dev_req_params;
> > +	struct reset_control *rst;
> 
> ... if you end up removing the rst field entirely.
> 
Yes, I will remove it

-- 
Yixun Lan (dlan)