LKML Archive on
help / color / mirror / Atom feed
From: David Brownell <>
To: Nicolas Ferre <>
Cc: Haavard Skinnemoen <>,
	Patrice Vilchez <>,
	Linux Kernel list <>
Subject: Re: [PATCH] input/spi: add ads7843 support to ads7846 touchscreen driver
Date: Fri, 22 Dec 2006 12:05:55 -0800	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On Thursday 21 December 2006 6:40 am, Nicolas Ferre wrote:
> Nicolas Ferre a écrit :
> >>> As the SPI underlying code behaves quite differently from a 
> >>> controller driver to another whan not having a tx_buf filled,
> >>> I have add a zerroed buffer to give to the spi layer while
> >>> receiving data. 
> >>
> >> You must be working with a buggy controller driver then.  That part of
> >> this patch should never be needed.  It's expected that rx-only transfers
> >> will omit a tx buf; all controller drivers must handle that case.
> > 
> > I said that because it is true that most of spi controller drivers 
> > manage rx only transactions filling the tx buffer with zerros but the 
> > spi_s3c24xx.c driver seems to fill with ones (line 177 hw_txbyte())
> > 
> > Anyway, I will check in our controller driver to sort this out.
> I dug a bit into this.

I always like to see followup on such issues.  :)

> Well, in the atmel_spi driver code, we use previous rx buffer if we do 
> not provide a tx_buf (as it is said that in struct spi_transfer 
> comments,  "If the transmit buffer is null, undefined data will be 
> shifted out while filling rx_buf").

That seems like a reasonable approach.  If it's undefined, the only
reasons I can think of to not use the rx buffer are that:

 (a) the cachelines might not be managed correctly during DMA ...
     i.e. to defend against a bug in the controller driver; and
 (b) that writing such _defined_ data would be a "covert channel"
     in the security sense.

Now, (a) is just a bug to fix, and in most cases (b) won't be an
issue since even if the system with that driver is being evaluated
at a level where such channels matter, the hardware hooked up via
SPI will probably already be trusted.  (Unless the system allows
thing like MMC or SD cards...)  However, see below.

> So, the touchscreen controller sees sometimes a "start" condition (bit 7 
> of a control byte). It then takes the control byte and sets trash bits 
> as a configuration.

Actually, now that I look at it I see that the docs for both the
ads7846 and the ads7843 show that DIN/MOSI should be zero/low
after the command is given, while reading DOUT/MISO.

Which means that the real issue here is that the driver was wrong
in the first place to not explicitly write zeroes while it's reading
back that data.

The testing I've done with it involved two controller drivers which
do happen to interpret "undefined" as "write zeros":  omap_uwire,
which I'm guessing does so in hardware (MicroWire is half duplex),
and pxa2xx_spi, which explicitly writes zeros (null_writer).

> I ran into those troubles and add a zerroed buffer as tx.
> Do you think that shifting zerros out when a tx_buf is not specified is 
> the desired behavior ?

I'm leaning towards a "yes" there -- changing the spec for spi_transfer.

The alternative would seem to mean teaching the SPI interface about
two different kinds of "rx only" transfers, one with "must tx zero"
semantics and the other with "tx data doesn't matter" (and security
audits would define it anyway, to avoid covert channels).  And I can't
easily justify that.

For now, I'd suggest you update the atmel_spi driver so that it shifts
zeroes in that case; can't hurt (too much).  And I'll forward the issue
to the SPI list.  If nobody there objects, I'll send patches to update
the spec for spi_transfer, and the s3c driver.

- Dave

  reply	other threads:[~2006-12-22 20:06 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <>
2006-12-20 23:13 ` David Brownell
2006-12-21 13:08   ` Nicolas Ferre
2006-12-21 14:40     ` Nicolas Ferre
2006-12-22 20:05       ` David Brownell [this message]
2006-12-22 19:31     ` David Brownell
2006-12-22 20:14       ` Dmitry Torokhov
2006-12-22 19:25 [patch 2.6.20-rc1 6/6] input: ads7846 directly senses PENUP state David Brownell
2006-12-28 22:37 ` David Brownell
2006-12-29  6:22   ` Dmitry Torokhov
2006-12-29 20:26     ` David Brownell
2007-02-16 17:37       ` [PATCH] input/spi: add ads7843 support to ads7846 touchscreen driver Nicolas Ferre
2007-02-16 19:08         ` David Brownell
2007-02-19 12:48           ` Nicolas Ferre
2007-02-19 18:46             ` David Brownell
2007-02-20  9:19               ` Nicolas Ferre
2007-03-01  4:49                 ` Dmitry Torokhov
  -- strict thread matches above, loose matches on Subject: below --
2006-12-15 14:45 Nicolas FERRE
2006-12-20 22:03 ` Andrew Morton
2006-12-21  9:57   ` Nicolas Ferre

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:

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

  git send-email \ \ \ \ \ \ \
    --subject='Re: [PATCH] input/spi: add ads7843 support to ads7846 touchscreen driver' \

* 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).