Messages in this thread | | | Date | Thu, 12 Jun 2008 10:00:43 +0200 (CEST) | From | Guennadi Liakhovetski <> | Subject | Re: [PATCH] gpio: gpio driver for max7301 SPI GPIO expander |
| |
On Thu, 12 Jun 2008, David Brownell wrote:
> Some comments about one routine; presumably more to follow:
Thanks, David, I'll pass your comments on to the author and try to get them acted upon.
Thanks Guennadi
> > > On Friday 06 June 2008, Guennadi Liakhovetski wrote: > > +/** > > + * max7301_read - Read back register content > > + * @spi: The SPI device > > + * @reg: Register offset > > + * > > + * A read from the MAX7301 means one message with two transfers > > That's not what you implement though ... what it needs is: > > - transfer #1: > * select > * write 16 bit command, discard rx data > * deselect > - transfer #2 > * select > * write 16 bit command, saving rx data > * deselect > > That's from the data sheet. Now, that *could* be implemented > with a single message ... or, as you do, with two messages. > > You should at least correct the comment, if you don't want to > switch to a slightly faster single-message implementation. :) > > > > + * > > + * Returns positive 8 bit value from device if successfull or a > > + * negative value on error > > Actually it returns a 16 bit value... also, "successful" has one "l". > You seem to have forgotten to mask off the high byte ... > > > > + */ > > +static int max7301_read(struct spi_device *spi, unsigned int reg) > > +{ > > + int ret; > > + u16 word; > > + > > + word = 0x8000 | (reg << 8); > > + if ((ret = spi_write(spi, (const u8*)&word, siz > > > eof(word))) != 0) > > + return ret; > > + /* > > + * FIXME: This read should write 0x0000 (=NOOP at MAX7301 side) > > Take out this FIXME, and just replace it with a note that this relies > on the fact that TX always shifts out zeroes if there's no data specified. > > > + */ > > + if ((ret = spi_read(spi, (u8*)&word, sizeof(word)))) > > + return ret; > > + return word; > > +} > > + > > > Oh, and in the probe(): > > > + spi->master->setup(spi); > > should be > > ret = spi_setup(spi); > if (ret < 0) > return ret; > > It's quite possible that something get misconfigured, so you end up trying > to talk through a spi_master controller that doesn't support 16-bit words. > > - Dave >
--- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |