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

[masonccyang@xxxxxxxxxxx]

Hi Mark,
Thanks for your comments.


Mark Brown <broonie@xxxxxxxxxx> 已在 2018/08/02
下午 06:07:41 上寫入: 

> Mark Brown <broonie@xxxxxxxxxx> 
> 2018/08/02 下午 06:07
> 
> To
> 
> masonccyang@xxxxxxxxxxx, 
> 
> cc
> 
> linux-spi@xxxxxxxxxxxxxxx, Boris Brezillon 
> <boris.brezillon@xxxxxxxxxxx>, juliensu@xxxxxxxxxxx, zhengxunli@xxxxxxxxxxx
> 
> Subject
> 
> Re: [PATCH 1/2] spi: Add MXIC controller driver
> 
> On Thu, Aug 02, 2018 at 10:07:49AM +0800, masonccyang@xxxxxxxxxxx
wrote:
> 
> This looks mostly good, one small technical thing and a style nit
though:
> 
> > +++ b/drivers/spi/spi-mxic.c
> > @@ -0,0 +1,526 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2018 Macronix International Co., Ltd.
> 
> Please make the entire comment a C++ one, it makes things look more
> intentional.

I patched it as follows:

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.
  *
@@ -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;

        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.

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);
+}
+
 static const struct of_device_id mxic_spi_of_ids[]
= {
        { .compatible = "mxicy,mx25f0a-spi",
},
        { /* sentinel */ }
@@ -513,6 +525,7 @@ static int mxic_spi_remove(struct
platform_device *pdev)
 static struct platform_driver mxic_spi_driver
= {
        .probe = mxic_spi_probe,
        .remove = mxic_spi_remove,
+       .shutdown = mxic_spi_shutdown,
        .driver = {
               
.name = "mxic-spi",
               
.of_match_table = mxic_spi_of_ids,




If you think it's OK and I will send a new patch file
for your review.

thanks & best regards,
Mason 

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or
personal data, which is protected by applicable laws. Please be reminded
that duplication, disclosure, distribution, or use of this e-mail (and/or
its attachments) or any part thereof is prohibited. If you receive this
e-mail in error, please notify us immediately and delete this mail as well
as its attachment(s) from your system. In addition, please be informed
that collection, processing, and/or use of personal data is prohibited
unless expressly permitted by personal data protection laws. Thank you
for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================