Message-ID: <20100518083231.GA7939@emlix.com>
Date:	Tue, 18 May 2010 10:32:31 +0200
From:	"Oskar Schirmer" <os@...ix.com>
To:	H Hartley Sweeten <hartleys@...ionengravers.com>
Cc:	Mike Frysinger <vapier.adi@...il.com>,
	Oskar Schirmer <os@...ix.com>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Daniel Glöckner <dg@...ix.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Oliver Schneidewind <osw@...ix.com>,
	Michael Hennerich <michael.hennerich@...log.com>
Subject: Re: [PATCH] ad7877: fix spi word size to 16 bit

On Mon, May 17, 2010 at 19:18:28 -0500, H Hartley Sweeten wrote:
> On Monday, May 17, 2010 4:50 PM, Mike Frysinger wrote:
> > 2010/5/17 Oskar Schirmer:
> >> On Sun, May 16, 2010 at 15:25:34 -0400, Mike Frysinger wrote:
> >>> On Sat, May 15, 2010 at 14:15, Oskar Schirmer wrote:
> >>>> On Thu, May 13, 2010 at 00:53:35 -0700, Dmitry Torokhov wrote:
> >>>>> On Fri, May 07, 2010 at 02:23:07PM -0400, Mike Frysinger wrote:
> >>>>>> On Fri, May 7, 2010 at 05:41, Daniel Glöckner wrote:
> >>>>>>> On 05/06/2010 08:26 PM, Mike Frysinger wrote:
> >>>>>>>> i think it'd be a better idea to do something like:
> >>>>>>>>   if (spi->bits_per_word != 16) {
> >>>>>>>>     if (spi->bits_per_word) {
> >>>>>>>>       dev_err(&spi->dev, "Invalid SPI settings; bits_per_word must be 16\n");
> >>>>>>>>       return -EINVAL;
> >>>>>>>>     }
> >>>>>>>>     spi->bits_per_word = 16;
> >>>>>>>>     spi_setup(spi);
> >>>>>>>>   }
> >>>>>>>
> >>>>>>> There is no way to set bits_per_word using struct spi_board_info. The
> >>>>>>> description of that structure in spi.h explicitly lists the wordsize as
> >>>>>>> one of the parameters drivers should set themself in probe().
> >>>>>> >
> >>>>>>> Only struct bfin5xx_spi_chip allows to set this value in the board code.
> >>>>>>
> >>>>>> an obvious shortcoming in the SPI framework that should be fixed, but
> >>>>>> that doesnt make any difference to the above code now does it ?  it'll
> >>>>>> operate correctly regardless of the SPI bus master.
> >>>>>
> >>>>> So is the updated patch coming?
> >>>>
> >>>> The basic question I see is, whether it is in the
> >>>> responsibility of ad7877 to check a wrong setting
> >>>> possibly caused in board specific code. If so,
> >>>> then the proposal by Mike should be used, but if not
> >>>> so, it would introduce unneeded code.
> >>>>
> >>>> Remember: both versions end up in correctly setting
> >>>> bits_per_word, with the difference merely in feedback
> >>>> level.
> >>>
> >>> imo, unsupported board settings should always be detected & rejected.
> >>> all SPI master drivers do this (detect & reject unsupported SPI slave
> >>> settings).
> >>
> >> please note, that bits_per_word is not a board setting,
> >> it's a demand of the device. consequently, there is no one
> >> to set unsupported values and thus none to be detected.
> >>
> >> the only architecture setting bits_per_word thru spi_chip
> >> is blackfin, but I cannot see a good reason, why the board
> >> settings should engage with a fixed demand of the device?
> >
> > you're telling me that every single SPI device out there can only ever
> > operate in one specific bit length or lacks optional settings ?  i
> > find that hard to believe which means it does make sense to let the
> > boards pick a length when appropriate.

Hartley was so kind to give a description on how this
can be done, including an example. See below.

> > the board settings also function as default setup values so when using
> > generic things like the spidev driver, there is no kernel driver to
> > request the desired bitmode.
> >
> > the simple code i posted addresses your concerns as well as reject
> > invalid settings (wherever they may originate).  if you're not going
> > to post an updated patch, i'll take the simple one Michael committed
> > and post that.
> 
> Just my 0.02, I haven't been following this thread very closely...
> 
> I believe it's the spi "device", i.e. the protocol, drivers responsibility
> to supply the bits_per_word that it will operate with.  The spi master
> driver simply validates it if can support the requested size.  If the
> bits_per_word is left at the default (0) it indicates that the protocol
> words are eight bit bytes.
> 
> If a protocol driver can support multiple word sizes it should probably
> also support a "setup" callback during the driver probe so that the
> underlying board support can provide the desired word size.  This is what
> the libertas_spi driver does (drivers/net/wireless/libertas/if_spi.c).
> That driver does nothing with the bits_per_word value but the callbacks in
> arch/arm/mach-pxa/cm-x270.c and em-x270.c do.  They both set the bits_per_word
> to 16 and then call spi_setup.  They also probably should be checking the
> return value of spi_setup to make sure that the requested mode is supported
> by the master driver, but they probably don't since it is already known...

Right, for cases where both device driver and master driver
support multiple bits_per_word settings, and the intersection
does contain more than one possible setting, it may be desirable
to have some third party select the real value, unless it is
ok to just use the default value, which in most cases will be 8.

So the callback functions you mention solve the dilemma,
don't they?

> If the ad7877 "only" supports 16-bit word sizes it should be setting the
> bits_per_word to indicate this.  If the underlying master driver does not
> support that word size it will return an error when spi_setup is called.

Yes. So the original patch should be extended to
check the return value of spi_setup to see whether
16 bit are possible. Thank you for that point.

  Oskar
-- 
oskar schirmer, emlix gmbh, http://www.emlix.com
fon +49 551 30664-0, fax -11, bahnhofsallee 1b, 37081 göttingen, germany
sitz der gesellschaft: göttingen, amtsgericht göttingen hr b 3160
geschäftsführer: dr. uwe kracke, ust-idnr.: de 205 198 055

emlix - your embedded linux partner
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists