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