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
