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.
> +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.
Attachment:
signature.asc
Description: PGP signature
