Re: [PATCH v2] spi: add OpenCores tiny SPI driver

[Thomas Chou]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/