Re: [PATCH 1/2] spi: Add MXIC controller driver

[Boris Brezillon <boris.brezillon@xxxxxxxxxxx>]

Hi Mason,

On Fri, 3 Aug 2018 15:29:02 +0800
masonccyang@xxxxxxxxxxx wrote:

> 
> diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
> index b786327..89889b0 100644
> --- a/drivers/spi/spi-mxic.c
> +++ b/drivers/spi/spi-mxic.c
> @@ -1,4 +1,4 @@
> -// SPDX-License-Identifier: GPL-2.0
> +/* SPDX-License-Identifier: GPL-2.0 */
>  /*
>   * Copyright (C) 2018 Macronix International Co., Ltd.
>   *

Actually Mark asked you to do the opposite:

// SPDX-License-Identifier: GPL-2.0
//
// Copyright (C) 2018 Macronix International Co., Ltd.
// ...
// ...

> @@ -423,8 +423,13 @@ int mxic_spi_setup(struct spi_device *spi)
>         if (ret)
>                 return ret;
> 
> -       clk_prepare_enable(mxic->send_clk);
> +       ret = clk_prepare_enable(mxic->send_clk);
> +       if (ret)
> +               return ret;
> +
>         clk_prepare_enable(mxic->send_dly_clk);
> +       if (ret)
> +               return ret;

You should disable mxic->send_clk in case
prepare_enabled fails of mxic->send_dly_clk.

Anyway, that'

> 
>         return 0;
>  }
> 
> is it OK?
> 
> 
> >   
> > > +int mxic_spi_setup(struct spi_device *spi)
> > > +{
> > > +   struct mxic_spi *mxic = spi_master_get_devdata(spi->master);  
> >   
> > > +   clk_prepare_enable(mxic->send_clk);
> > > +   clk_prepare_enable(mxic->send_dly_clk);  
> > 
> > This will enable the clocks every time setup() is called (without
> > checking for errors!) but there's no corresponding disables so we're
> > leaking references to the clock enable.  What I'd recommend here is
> > moving the clock enables to runtime PM if you can, or just doing it in
> > probe if there's some problem with that.  Runtime PM should be slightly
> > better for power management but so slightly that it's not worth worrying
> > too much.
> >   
> 
> As I know that mxic_spi_setup() is called just one time in 
> spi_register_master()
> and the SCLK of our SPI Host is always in idle if there is no any 
> cmd/addr/data
> on bus.

That's not quite true. SPI device drivers can call spi_setup() on their
own, so this hook can called several times. I'd suggest moving that to
runtime PM hooks.

> 
> I patched it as follows:
> 
> @@ -504,6 +509,13 @@ static int mxic_spi_remove(struct platform_device 
> *pdev)
>         return 0;
>  }
> 
> +static void mxic_spi_shutdown(struct platform_device *pdev)
> +{
> +       struct mxic_spi *mxic = platform_get_drvdata(pdev);
> +
> +       clk_disable_unprepare(mxic->ps_clk);
> +}

I might be wrong, but I think you don't need special handling for
shutdown if you implement the runtime PM suspend/resume hooks.

Regards,

Boris
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html