Hi Grant,
Thanks a lot for the review.
On 01/21/2011 12:54 AM, Grant Likely wrote:
drivers/spi/oc_tiny_spi.c | 422 +++++++++++++++++++++++++++++++++++++++
Rename files to spi_oc_tiny.c
OK.
+#ifndef CONFIG_TINY_SPI_IDLE_VAL
+# define CONFIG_TINY_SPI_IDLE_VAL 0x00
+#endif
Do you really want this as a Kconfig symbol? Looks like something
that should be configured in pdata or the device tree.
I will drop this def, as linux use 00 as default.
+ if (t->len> 1) {
+ writeb(CONFIG_TINY_SPI_IDLE_VAL,
+ hw->base + TINY_SPI_TXDATA);
+ for (i = 2; i< t->len; i++) {
+ while (!(readb(hw->base + TINY_SPI_STATUS)&
+ TINY_SPI_STATUS_TXR))
+ cpu_relax();
+ writeb(CONFIG_TINY_SPI_IDLE_VAL,
+ hw->base + TINY_SPI_TXDATA);
+ }
+ }
+ while (!(readb(hw->base + TINY_SPI_STATUS)&
+ TINY_SPI_STATUS_TXE))
+ cpu_relax();
I see a bunch of while loops in this function with no exit condition
if something goes badly with the hardware. Also, this driver is going
to chew up *a lot* of cpu cycles when not using irqs. Consider
putting the polled work into a tasklet instead of blindly spinning.
It will actually make the driver simpler because the irq and non-irq
cases become very similar.
But bitbang driver already runs as a workqueue, do we still need to
create a tasklet?
bitbang->workqueue = create_singlethread_workqueue(
dev_name(bitbang->master->dev.parent));
+ hw->bitbang.master->dev.of_node = pdev->dev.of_node;
+ val = of_get_property(pdev->dev.of_node, "baud-width", NULL);
'baud-width'?
These properties need to be documented. See below.
This is the width of baud rate divider. I will document it. Do you have
suggestion on the naming?
+#ifdef CONFIG_OF
+static struct of_device_id oc_tiny_spi_match[] = {
+ {
+ .compatible = "opencores,oc_tiny_spi",
If this is a soft core, then there should be a version number of some
sort on the compatible value. Also, please use dash '-' instead of
underscore '_' in compatible values. Also, for all of these new
bindings, they need to be documented. Please add documentation to
Documentation/powerpc/dts-bindings (yes, I know, this is not
for powerpc, but that is the established directory. I'll move it to a
better location soon).
Finally, one nitpick. For conciseness, most of_device_id tables are
written in the form:
static struct of_device_id oc_tiny_spi_match[] = {
{ .compatible = "opencores,oc_tiny_spi", }, },
{},
}
Nice. I am working with Walter on the dts generator for our cores. We
will add them to the listing.
+static int __init tiny_spi_init(void)
+{
+ return platform_driver_probe(&tiny_spidrv, tiny_spi_probe);
+}
+module_init(tiny_spi_init);
Please use platform_driver_register, and put your probe function into
the platform_driver structure.
OK. I will do the same for spi_altera.
Best regards,
Thomas
_______________________________________________
devicetree-discuss mailing list
[email protected]
https://lists.ozlabs.org/listinfo/devicetree-discuss
