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

[Mike Frysinger]

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.

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.
-mike
--
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/