Re: [PATCH 2/3] spi: meson: Add support for Amlogic Meson SPIFC

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2647 bytes --]

On Sun, Nov 09, 2014 at 10:25:12AM +0100, Beniamino Galvani wrote:

> +static int meson_spifc_wait_ready(struct meson_spifc *spifc)
> +{
> +	unsigned long deadline = jiffies + msecs_to_jiffies(1000);
> +	u32 data;
> +
> +	do {
> +		regmap_read(spifc->regmap, REG_SLAVE, &data);
> +		if (data & SLAVE_TRST_DONE)
> +			return 0;
> +		cond_resched();
> +	} while (time_before(jiffies, deadline));

This will busy wait for up to a second, that seems like a long time to
busy wait.  We also appear to be using this for the entire duration of
the transfer which could be a fairly long time even during normal
operation if doing a large transfer such as a firmware download, or if
the bus speed is low.

> +	meson_spifc_setup_speed(spifc, xfer->speed_hz ? xfer->speed_hz :
> +				spi->max_speed_hz);
> +

Please avoid the ternery operator, it does nothing for legibility and in
this case it's not needed as the core will always ensure that there is a
per-transfer speed set.

> +	while (done < xfer->len && !ret) {
> +		len = min_t(int, xfer->len - done, SPIFC_BUFFER_SIZE);
> +		ret = meson_spifc_txrx(spifc, xfer, done, len,
> +				       last_xfer, done + len >= xfer->len);
> +		done += len;
> +	}

I noticed that the handling of /CS was done in the spifc_txrx() function
- will this do the right thing if the transfer needs to be split for the
buffer size?

> +	if (!ret && xfer->delay_usecs)
> +		udelay(xfer->delay_usecs);

The core will do this for you if you implement this as transfer_one().

> +static int meson_spifc_transfer_one_message(struct spi_master *master,
> +					    struct spi_message *msg)

This appears to do nothing that the core won't do - just implement
transfer_one() and remove this.

> +	spifc = spi_master_get_devdata(master);
> +	memset(spifc, 0, sizeof(struct meson_spifc));

There should be no need for this memset.

> +	spifc_regmap_config.max_register = resource_size(res) - 4;
> +	spifc_regmap_config.name = "amlogic,meson-spifc";

If you're dynamically initializing the structure you need to work with a
copy of it rather than directly since there may be multiple instances.
I'm not seeing a reason to override the regmap name here, this is only
really intended for devices with multiple register maps.

> +	ret = clk_prepare_enable(spifc->clk);
> +	if (ret) {
> +		dev_err(spifc->dev, "can't prepare clock\n");
> +		goto out_err;
> +	}

You should really implement runtime PM operations to disable this when
not in use and use auto_runtime_pm to make sure this happens.

> +	master->bus_num = pdev->id;

Leave this blank for DT only devices (and for non-DT devices this won't
work if you get two different buses).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]