Re: [PATCH] ad7877: fix spi word size to 16 bit

[Oskar Schirmer]

On Mon, May 17, 2010 at 09:14:39 +0100, Hennerich, Michael wrote:
> Oskar Schirmer wrote on 2010-05-17:
> > 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?
> >
> >   Oskar
> 
> 100% agreed.
> 
> Two ways to address the issue:
> 1) Forcing spi->bits_per_word = 16 like this patch does.
> 2) Or going with the default 8-bit transfers and using be16_to_cpu().
> 
> Personally I prefer 1) unless someone tells me that not all SPI bus drivers
> support 16-bit transfers.

yes, I prefer (1) too.

if (2) is for the bus driver that didn't support
16 bit up to now, though the peripheral unit does
support it, that bus driver needs a fix.

but then, if (2) is for, e.g., a 16 bit only device wired
to an 8 bit only bus driver, which, I'ld say, is a
board design fault. sure one may want to implement an
ugly software work around for it, just to rescue some
high budget project and managers calculated that messing
up the software will cost less then having another
board design cycle, but I'ld better propose to give the
hardware engineer the whip.

  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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/