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
------------------------------------------------------------------------------
Protect Your Site and Customers from Malware Attacks
Learn about various malware tactics and how to avoid them. Understand
malware threats, the impact they can have on your business, and how you
can protect your company and customers by using code signing.
http://p.sf.net/sfu/oracle-sfdevnl
_______________________________________________
spi-devel-general mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
