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

[Mark Brown]

On Fri, Sep 28, 2018 at 03:35:11PM +0800, masonccyang@xxxxxxxxxxx wrote:

This looks mostly good, a couple of fairly small things:

> +static int mxic_spi_clk_enable(struct mxic_spi *mxic)
> +{
> +	int ret;
> +
> +	ret = clk_prepare_enable(mxic->send_clk);
> +	if (ret)
> +		goto err_send_clk;
> +
> +	ret = clk_prepare_enable(mxic->send_dly_clk);
> +	if (ret)
> +		goto err_send_dly_clk;
> +
> +	return ret;
> +
> +err_send_dly_clk:
> +	clk_disable_unprepare(mxic->send_dly_clk);
> +err_send_clk:
> +	clk_disable_unprepare(mxic->send_clk);

You should only undo operations that succeeded in the error path, if
something failed you don't need to revert it.

> +static int mxic_spi_setup(struct spi_device *spi)
> +{
> +	struct mxic_spi *mxic = spi_master_get_devdata(spi->master);
> +	unsigned long freq = spi->max_speed_hz;
> +	int ret;
> +
> +	ret = clk_set_rate(mxic->send_clk, freq);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_set_rate(mxic->send_dly_clk, freq);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * A constant delay range from 0x0 ~ 0x1F for input delay,
> +	 * the unit is 78 ps, the max input delay is 2.418 ns.
> +	 */
> +	mxic_spi_set_input_delay_dqs(mxic, 0xf);

This is changing the hardware state for all slaves, not just the device
being configured - setup() isn't supposed to do that (yes, it's a
confusing API but unfortunately a legacy one that'll be a pain to clean
up).  prepare_transfer_hardware() is usually a better place to do this.

Attachment: signature.asc
Description: PGP signature