On Tuesday 19 February 2013 11:46 PM, Stephen Warren wrote:
On 02/19/2013 06:38 AM, Laxman Dewangan wrote:
+ bits_per_word = t->bits_per_word ? t->bits_per_word :
+ spi->bits_per_word;
I thought I'd seen patches so this conditional wasn't needed any more;
isn't t->bit_per_word always set correctly by the SPI core now?
Certainly the existing spi-tegra20-slink.c doesn't seem to have any
conditional here.
Yes, core have changes. I will remove this check.
A similar comment applies in tegra_spi_read_rx_fifo_to_client_rxbuf()
and tegra_spi_copy_spi_rxbuf_to_client_rxbuf().
+ total_fifo_words = (max_len + 3)/4;
Need spaces around /. The same comment applies in some other places;
please search for them. Was checkpatch run? I'm not sure if catches this.
spi-tegra20-slink.c doesn't have that rounding; is just says:
total_fifo_words = max_len / 4;
Is that a bug in the old driver?
I will check and fix the either place.
+ if (tspi->cur_direction & DATA_DIR_TX) {
+ tegra_spi_copy_client_txbuf_to_spi_txbuf(tspi, t);
+ ret = tegra_spi_start_tx_dma(tspi, len);
In spi-tegra20-slink.c, there's a wmb() right between those last two
lines. Is it needed here?
I think wmb() is no require and hence not keeping here. Perhaps I got
the review feedback when I was working on serial tegra driver.
+static int tegra_spi_start_transfer_one(struct spi_device *spi,
+ struct spi_transfer *t, bool is_first_of_msg,
+ bool is_single_xfer)
...
+ /* possibly use the hw based chip select */
+ command1 |= SPI_CS_SW_HW;
+ if (spi->mode & SPI_CS_HIGH)
+ command1 |= SPI_CS_SS_VAL;
+ else
+ command1 &= ~SPI_CS_SS_VAL;
Why "possibly"; the code seems to always use HW chip select.
Yes, wrong comment. Remove the controller_data from driver but forgot to
remove this comment.
+ ret = pm_runtime_get_sync(tspi->dev);
+ if (ret < 0) {
+ dev_err(tspi->dev, "runtime PM get failed: %d\n", ret);
+ msg->status = ret;
+ spi_finalize_current_message(master);
+ return ret;
+ }
In the older Tegra SPI drivers, the PM runtime logic was was of
master->{un,}prepare_transfer. I'm curious why it's implemented
differently here.
The prepare is called in atomic context and in this we are calling
pm_runtime_get_sync() which is blocking and it can cause issue.
I have already bug reported by you that sometimes you saw locking in
tegra20 slink driver which we need to fix. When testing this, I ran into
similar case and hence now moving this out or prepare.
I will push the change for fixing this in tegra20_slink driver also.
+ prop = of_get_property(np, "spi-max-frequency", NULL);
+ if (prop)
+ tspi->spi_max_frequency = be32_to_cpup(prop);
The following might be better:
if (of_property_read_u32(np, "spi-max-frequency",
&tspi->spi_max_frequency))
tspi->spi_max_frequency = 25000000; /* 25MHz */
(and you can remove the check of !tspi->spi_max_frequency from probe()
then too)
Yes, Agree. Will do.
+static int tegra_spi_probe(struct platform_device *pdev)
...
+ if (!pdev->dev.of_node) {
+ dev_err(&pdev->dev, "Driver support DT registration only\n");
+ return -ENODEV;
+ }
I don't think there's much point checking that; see the Tegra20 SPI
cleanup patches I posted a couple days ago.
+ tspi->base = devm_request_and_ioremap(&pdev->dev, r);
+ if (!tspi->base) {
The existing Tegra20 driver checks if (IS_ERR(tspi->base)) here. Which
is wrong?
The tegra20 driver use the devm_ioremap_resource() which is new API get
added recently. I will change this driver to use this one.
+ tspi->clk = devm_clk_get(&pdev->dev, "spi");
Does this HW block use multiple clocks? If not, I think s/"spi"/NULL/
there, just like the Tegra20 driver.
No, spi controller uses the only one clock. I will change to NULL.
As an overall comment, this driver is textually perhaps 80-90% the same
as spi-tegra20-slink.c. Instead of creating a completely new driver, how
nasty would a unified driver look; one which contained some runtime
conditionals for the register layout and programming differences? It
might be worth looking at, although perhaps it would turn out to be a
crazy mess, so a separate driver really is appropriate.
We had created the similarly in past where common part is in same file
and controller specific is in the different file as hal file but the
driver becomes complex.
It was not easy to add any feature as the require api need to be added
in all places for hal. We were about to split the driver separately but
before then we moved to Linux.
So first look, it is great but adding feature/maintaining/enhancing is
difficult.
I like to go with separate driver until there is much pushback to make
it one.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
