From: Vitaly Wool <vwool@ru.mvista.com>
To: David Brownell <david-b@pacbell.net>
Cc: dpervushin@gmail.com, dpervushin@ru.mvista.com,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] SPI
Date: Fri, 30 Sep 2005 22:30:45 +0400 [thread overview]
Message-ID: <433D8455.4030601@ru.mvista.com> (raw)
In-Reply-To: <20050930175923.F3C89E9E25@adsl-69-107-32-110.dsl.pltn13.pacbell.net>
Greetings,
<snip>
>> drivers/spi/Kconfig | 33 +++
>> drivers/spi/Makefile | 14 +
>> include/linux/spi.h | 232 ++++++++++++++++++++++
>>
>>
>
>Looks familiar. :) But another notion for the headers would be
>
> <linux/spi/spi.h> ... main header
> <linux/spi/CHIP.h> ... platform_data, for CHIP.c driver
>
>Not all chips would need them, but it might be nice to have some place
>other than <linux/CHIP.h> for such things. The platform_data would have
>various important data that can't be ... chip variants, initialization
>data, and similar stuff that differs between boards is knowable only by
>board-specific init code, yet is needed by board-agnostic driver code.
>
>
What about SPI busses that are common for different boards?
<snip>
>>+static int spi_thread(void *context);
>>
>>
>
>You're imposing the same implementation strategy Mark Underwood was.
>I believe I persuaded him not to want that, pointing out three other
>implementation strategies that can be just as reasonable:
>
> http://marc.theaimsgroup.com/?l=linux-kernel&m=112684135722116&w=2
>
>It'd be fine if for example your PNX controller driver worked that way
>internally. But other drivers shouldn't be forced to allocate kernel
>threads when they don't need them.
>
>
Hm, so does that imply that the whole -rt patches from
Ingo/Sven/Daniel/etc. are implementing wrong strategy (interrupts in
threads)?
How will your strategy work with that BTW?
<snip>
>>+ /*
>>+ * all messages for current selected_device
>>+ * are processed.
>>+ * let's switch to another device
>>+ */
>>
>>
>
>Why are you hard-wiring such an unfair scheduling policy ... and
>preventing use of better ones? I'd use FIFO rather than something
>as unfair as that; and FIFO is much simpler to code, too.
>
>
Sounds reasonable to me.
>>--- /dev/null
>>+++ linux-2.6.10/drivers/spi/spi-dev.c
>>@@ -0,0 +1,219 @@
>>+/*
>>+ spi-dev.c - spi driver, char device interface
>>+
>>
>>
>
>Do you have userspace drivers that work with this? I can see how to use
>it with read-only sensors (each read generates a 12bit sample, etc) and
>certain write-only devices, I guess.
>
>But it doesn't look like this character device can handle RPC-ish things
>like "give me an ADC conversion for line 3" (which commonly maps to a
>"write 8 bits, then start reading 12 data bits one half clock after
>the last bit is written" ... hard to make that work with separate
>read and write transactions, given the half clock rule).
>
>
Hm. I thought half-clock cases were to be programmed kernel-wise.
<snip>
>
>
>>+ +--------------+ +---------+
>>+ | platform_bus | | spi_bus |
>>+ +--------------+ +---------+
>>+ |..| |
>>+ |..|--------+ +---------------+
>>+ +------------+| is parent to | SPI devices |
>>+ | SPI busses |+-------------> | |
>>+ +------------+ +---------------+
>>+ | |
>>+ +----------------+ +----------------------+
>>+ | SPI bus driver | | SPI device driver |
>>+ +----------------+ +----------------------+
>>
>>
>
>That seems wierd even if I assume "platform_bus" is just an example.
>For example there are two rather different "spi bus" notions there,
>and it looks like neither one is the physical parent of any SPI device ...
>
>
>
Not sure if I understand you :(
>
>
>>+3.2 How do the SPI devices gets discovered and probed ?
>>
>>
>
>Better IMO to have tables that get consulted when the SPI master controller
>drivers register the parent ... tables that are initialized by the board
>specific __init section code, early on. (Or maybe by __setup commandline
>parameters.)
>
>Doing it the way you are prevents you from declaring all the SPI devices in
>a single out-of-the-way location like the arch/.../board-specific.c file,
>which is normally responsible for declaring devices that are hard-wired on
>a given board and can't be probed.
>
>
By what means does it prevent that?
>>+static inline struct spi_msg *spimsg_alloc(struct spi_device *device,
>>+ unsigned flags,
>>+ unsigned short len,
>>+ void (*status) (struct spi_msg *,
>>+ int code))
>>+{
>>+ ... 30+ lines including...
>>+
>>+ msg = kmalloc(sizeof(struct spi_msg), GFP_KERNEL);
>>+ memset(msg, 0, sizeof(struct spi_msg));
>>
>>
>
>If these things aren't going to be refcounted, then it'd be easier
>to just let them be stack-allocated; they ARE that small. And if
>they've _got_ to be on the heap, then there's a new "kzalloc()" call
>you should be looking at ...
>
>
>
>
>>+ msg->devbuf_rd = drv->alloc ?
>>+ drv->alloc(len, GFP_KERNEL) : kmalloc(len, GFP_KERNEL);
>>+ msg->databuf_rd = drv->get_buffer ?
>>+ drv->get_buffer(device, msg->devbuf_rd) : msg->devbuf_rd;
>>
>>
>
>Oy. More dynamic allocation. (Repeated for write buffers too ...)
>See above; don't force such costs on all drivers, few will ever need it.
>
>
I guess that won't necessarily be actual allocation, it's a matter of
drv callbacks.
<snip>
>>+#define SPI_MAJOR 153
>>+
>>+...
>>+
>>+#define SPI_DEV_CHAR "spi-char"
>>
>>
I thought 153 was the official SPI device number.
Best regards,
Vitaly
next prev parent reply other threads:[~2005-09-30 18:30 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2005-09-30 17:59 [PATCH] SPI David Brownell
2005-09-30 18:30 ` Vitaly Wool [this message]
2005-09-30 19:20 ` dpervushin
-- strict thread matches above, loose matches on Subject: below --
2005-10-03 16:26 David Brownell
2005-10-03 5:01 David Brownell
2005-10-03 6:20 ` Vitaly Wool
2005-10-03 4:56 David Brownell
2005-09-26 11:12 SPI dmitry pervushin
2005-09-27 12:43 ` SPI Greg KH
2005-09-27 14:27 ` [spi-devel-general] SPI dmitry pervushin
2005-09-27 14:35 ` Greg KH
2005-09-27 14:49 ` dmitry pervushin
2005-09-27 14:54 ` Greg KH
2005-09-28 13:14 ` [PATCH] SPI dmitry pervushin
2005-08-08 23:07 [PATCH] spi david-b
2005-08-09 9:38 ` Mark Underwood
2005-07-10 20:01 [PATCH 3/3] kconfig: linux.pot for all arch Egry Gábor
2005-08-08 9:12 ` [PATCH] spi dmitry pervushin
2005-08-08 10:41 ` Jiri Slaby
2005-08-08 13:16 ` Mark Underwood
2005-08-08 16:41 ` dmitry pervushin
2005-08-08 18:51 ` Mark Underwood
2005-08-08 14:55 ` Greg KH
2005-08-08 17:35 ` Marcel Holtmann
2005-08-08 17:47 ` Marc Singer
2005-08-09 17:54 ` Andy Isaacson
2005-08-09 19:05 ` Marc Singer
2005-08-09 19:29 ` Andy Isaacson
2005-08-15 7:51 ` Denis Vlasenko
2005-08-08 22:58 ` Andrew Morton
2005-08-10 13:10 ` Pavel Machek
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=433D8455.4030601@ru.mvista.com \
--to=vwool@ru.mvista.com \
--cc=david-b@pacbell.net \
--cc=dpervushin@gmail.com \
--cc=dpervushin@ru.mvista.com \
--cc=linux-kernel@vger.kernel.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).