Re: [PATCH 1/2] spi: dw: add reset control

[Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>]

On Thu, May 21, 2020 at 06:12:03PM +0100, Mark Brown wrote:
> On Thu, May 21, 2020 at 12:03:58PM -0500, Dinh Nguyen wrote:
> > Add mechanism to get the reset control and deassert it in order to bring
> > the IP out of reset.
> > 
> > Signed-off-by: Liang Jin J <liang.j.jin@xxxxxxxxxxxx>
> > Signed-off-by: Dinh Nguyen <dinguyen@xxxxxxxxxx>
> 
> Is the From: correct here?
> 
> Also adding everyone else who's got patches in flight for this driver
> right now.
> 
> > ---
> >  drivers/spi/spi-dw-mmio.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
> > index 384a3ab6dc2d..5c813e15ed89 100644
> > --- a/drivers/spi/spi-dw-mmio.c
> > +++ b/drivers/spi/spi-dw-mmio.c
> > @@ -20,6 +20,7 @@
> >  #include <linux/acpi.h>
> >  #include <linux/property.h>
> >  #include <linux/regmap.h>
> > +#include <linux/reset.h>
> >  
> >  #include "spi-dw.h"
> >  
> > @@ -30,6 +31,7 @@ struct dw_spi_mmio {
> >  	struct clk     *clk;
> >  	struct clk     *pclk;
> >  	void           *priv;
> > +	struct reset_control	*rstc;
> >  };
> >  
> >  #define MSCC_CPU_SYSTEM_CTRL_GENERAL_CTRL	0x24
> > @@ -145,6 +147,10 @@ static int dw_spi_mmio_probe(struct platform_device *pdev)
> >  	if (!dwsmmio)
> >  		return -ENOMEM;
> >  

> > +	dwsmmio->rstc = devm_reset_control_get_exclusive(&pdev->dev, "spi");

It would be better to use the optional-version of the reset getter:
devm_reset_control_get_optional_exclusive(), since for instance our DW APB SSI
doesn't have a dedicated reset line.

Note also that you don't need to check the rstc for being NULL. It's null if the
reset line is optional and isn't provided.

If we get to have this patch applied, then DT binding shall be also updated...
Though we haven't got it reviewed yet.(

-Sergey



> > +	if (!IS_ERR(dwsmmio->rstc))
> > +		reset_control_deassert(dwsmmio->rstc);
> > +
> >  	dws = &dwsmmio->dws;
> >  
> >  	/* Get basic io resource and map it */
> 
> Should we also undo the reset in error paths from probe()?  It's not the
> end of the world if we don't but...
> 
> > @@ -220,6 +226,9 @@ static int dw_spi_mmio_remove(struct platform_device *pdev)
> >  	clk_disable_unprepare(dwsmmio->pclk);
> >  	clk_disable_unprepare(dwsmmio->clk);
> >  
> > +	if (dwsmmio->rstc)
> > +		reset_control_assert(dwsmmio->rstc);
> > +
> >  	return 0;
> >  }
> >  
> > -- 
> > 2.17.1
> >