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

[Boris Brezillon <boris.brezillon@xxxxxxxxxxx>]

On Thu, 2 Aug 2018 11:07:41 +0100
Mark Brown <broonie@xxxxxxxxxx> wrote:

> 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.

Sure, we will.

> 
> > +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.

I think we'll go for the runtime PM approach.

Thanks for your review.

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