lkml.org 
[lkml]   [2011]   [Jan]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2] spi: add OpenCores tiny SPI driver
    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


    \
     
     \ /
      Last update: 2011-01-20 22:39    [W:0.026 / U:0.256 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site