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:
signature.asc
Description: Digital signature
