David Hendricks posted comments on this change.

View Change

Patch set 4:

Thanks for the quick (and thorough) review as always.

I ended up re-writing a significant portion of this patch. Assuming the changes look good, what's the "right" thing to do to get things merged? Merge everything in this patch, or split my updates into a follow-up patch?

(5 comments)

• Commit Message:

  • Patch Set #3, Line 9:

    Read max buffer size from sysfs if available.
    
    

    That's not true, and I can say that from this patch: If we

    I guess the original author meant that it's a compile-time constant in the kernel? In any case, I went ahead and simplified this further to remove any possible confusion.

• File linux_spi.c:

  • Patch Set #3, Line 134: fp = fopen(BUF_SIZE_FROM_SYSFS, "r");

    Isn't there a way to query this over the device interface?

    Not sure... I don't see any other obvious way of getting at it. There's no ioctl for this AFAICT by looking thru the spidev documentation. The only clue seems to be:

    • There's a limit on the number of bytes each I/O request can transfer to the SPI device. It defaults to one page, but that can be changed using a module parameter.

    So either read the module parameter via sysfs or default to using getpagesize() which the code currently does.

  • Patch Set #3, Line 135: if (!fp) {

    What if read() returns a short byte count? What if it gets inter-

    Done.

  • Patch Set #3, Line 199: The implementation currently does not support requests that

    Why? rx/tx buffers seem to be handled separately.

    Re "why": As to not rely on the driver to split large transactions? I'm not if this is a big deal for spidev, but I have had issues with MTD in the past (https://chromium-review.googlesource.com/505409).

    Re rx/tx being handled separately: Are you referring to the current (old) code? That may be an error, I think Keno was correct to handle read and write buffers the same and account for command size.

• File linux_spi.c:

  • Patch Set #4, Line 222: *

    oops, i'll fix this...

To view, visit change 22344. To unsubscribe, visit settings.