LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Thomas Chou <thomas@wytron.com.tw>
To: Grant Likely <grant.likely@secretlab.ca>
Cc: David Brownell <dbrownell@users.sourceforge.net>,
	linux-kernel@vger.kernel.org, nios2-dev@sopc.et.ntust.edu.tw,
	devicetree-discuss@lists.ozlabs.org,
	spi-devel-general@lists.sourceforge.net,
	Mike Frysinger <vapier@gentoo.org>
Subject: Re: [PATCH v2] spi: add OpenCores tiny SPI driver
Date: Fri, 21 Jan 2011 05:36:43 +0800	[thread overview]
Message-ID: <4D38AAEB.7020308@wytron.com.tw> (raw)
In-Reply-To: <20110120165430.GA24445@angua.secretlab.ca>

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

  reply	other threads:[~2011-01-20 21:36 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <1294645208-6322-1-git-send-email-thomas@wytron.com.tw>
2011-01-12  2:41 ` Thomas Chou
2011-01-17  7:32   ` Thomas Chou
2011-01-20 16:54     ` Grant Likely
2011-01-20 21:36       ` Thomas Chou [this message]
2011-01-21 12:27         ` Jonas Bonn
2011-01-24  1:19           ` Thomas Chou
2011-01-24 10:59             ` Jonas Bonn
2011-01-24 14:50               ` Thomas Chou
2011-01-24 15:50                 ` Grant Likely
2011-01-25  3:17               ` [PATCH v3] " Thomas Chou
2011-02-03 10:37                 ` [PATCH v4] " Thomas Chou
2011-02-03 11:03                   ` Wolfram Sang
2011-02-03 15:20                   ` Dirk Brandewie

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4D38AAEB.7020308@wytron.com.tw \
    --to=thomas@wytron.com.tw \
    --cc=dbrownell@users.sourceforge.net \
    --cc=devicetree-discuss@lists.ozlabs.org \
    --cc=grant.likely@secretlab.ca \
    --cc=linux-kernel@vger.kernel.org \
    --cc=nios2-dev@sopc.et.ntust.edu.tw \
    --cc=spi-devel-general@lists.sourceforge.net \
    --cc=vapier@gentoo.org \
    --subject='Re: [PATCH v2] spi: add OpenCores tiny SPI driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).