| Message ID | 1424772249-9676-1-git-send-email-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 24, 2015 at 12:04:09PM +0200, Andy Shevchenko wrote: > Currently it's unclear from the documentation if an actual driver may or may > not override the message->status field. It seems there is no other way to > indicate an error when transfer is failed, e.g. when gets overrun or underrun > interrupt. The patch states this clearly in the comments near to field > definition. > + > + /* The actual driver may set this field to indicate an error, e.g. -EIO > + * for failed transfer. */ > int status; Let's take a step back here: what documentation are you looking at which says anything else about status? I'd also expect this to go with the other kerneldoc for the field.
On Tue, 2015-02-24 at 22:24 +0900, Mark Brown wrote: > On Tue, Feb 24, 2015 at 12:04:09PM +0200, Andy Shevchenko wrote: > > Currently it's unclear from the documentation if an actual driver may or may > > not override the message->status field. It seems there is no other way to > > indicate an error when transfer is failed, e.g. when gets overrun or underrun > > interrupt. The patch states this clearly in the comments near to field > > definition. > > > + > > + /* The actual driver may set this field to indicate an error, e.g. -EIO > > + * for failed transfer. */ > > int status; > > Let's take a step back here: what documentation are you looking at which > says anything else about status? The description of this field above, namely " * @status: zero for success, else negative errno". > I'd also expect this to go with the other kerneldoc for the field. Any place that suits better.
On Tue, Feb 24, 2015 at 03:33:27PM +0200, Andy Shevchenko wrote: > On Tue, 2015-02-24 at 22:24 +0900, Mark Brown wrote: > > > + /* The actual driver may set this field to indicate an error, e.g. -EIO > > > + * for failed transfer. */ > > > int status; > > Let's take a step back here: what documentation are you looking at which > > says anything else about status? > The description of this field above, namely " * @status: zero for > success, else negative errno". ...which suggests that anything detecting an error should report it via status, no? > > I'd also expect this to go with the other kerneldoc for the field. > Any place that suits better. Well, see above...
On Tue, 2015-02-24 at 23:17 +0900, Mark Brown wrote: > On Tue, Feb 24, 2015 at 03:33:27PM +0200, Andy Shevchenko wrote: > > On Tue, 2015-02-24 at 22:24 +0900, Mark Brown wrote: > > > > > + /* The actual driver may set this field to indicate an error, e.g. -EIO > > > > + * for failed transfer. */ > > > > int status; > > > > Let's take a step back here: what documentation are you looking at which > > > says anything else about status? > > > The description of this field above, namely " * @status: zero for > > success, else negative errno". > > ...which suggests that anything detecting an error should report it via > status, no? Unclear. I dived into spi.c to understand if I could override or not. There are two fields status and state, the latter is dedicated for an actual driver usage, and that is clear. Many (old) drivers are using state as a storage for custom status. Newer drivers, that are using SPI core, mostly don't care about error handling at all (at least I didn't find an existing example of the message->status usage). > > > I'd also expect this to go with the other kerneldoc for the field. > > > Any place that suits better. > > Well, see above... Regarding my above comment can we extend the description of the field then?
On Tue, Feb 24, 2015 at 04:35:27PM +0200, Andy Shevchenko wrote: > On Tue, 2015-02-24 at 23:17 +0900, Mark Brown wrote: > > ...which suggests that anything detecting an error should report it via > > status, no? > Unclear. I dived into spi.c to understand if I could override or not. > There are two fields status and state, the latter is dedicated for an > actual driver usage, and that is clear. Many (old) drivers are using > state as a storage for custom status. Newer drivers, that are using SPI > core, mostly don't care about error handling at all (at least I didn't > find an existing example of the message->status usage). This isn't something specific to modern drivers, the status field has been there since forever and is still widely used. Almost all error detection for SPI controllers (especially that's likely to trigger and therefore have a practical effect) is timeouts and that's been factored out into the core for the more modern drivers. > > > > I'd also expect this to go with the other kerneldoc for the field. > > > Any place that suits better. > > Well, see above... > Regarding my above comment can we extend the description of the field > then? Let's see what a patch looks like. I am wary of documentation that's too verbose since it can cause people to glaze over when reading it.
On Thu, 2015-02-26 at 11:48 +0900, Mark Brown wrote: > On Tue, Feb 24, 2015 at 04:35:27PM +0200, Andy Shevchenko wrote: > > On Tue, 2015-02-24 at 23:17 +0900, Mark Brown wrote: > > > > ...which suggests that anything detecting an error should report it via > > > status, no? … > > Regarding my above comment can we extend the description of the field > > then? > > Let's see what a patch looks like. I am wary of documentation that's > too verbose since it can cause people to glaze over when reading it. I asked Mika's opinion and he wasn't confused by current description. Hence, let's discontinuing this effort.
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index ed9489d..92cff9a 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -680,6 +680,9 @@ struct spi_message { void *context; unsigned frame_length; unsigned actual_length; + + /* The actual driver may set this field to indicate an error, e.g. -EIO + * for failed transfer. */ int status; /* for optional use by whatever driver currently owns the
Currently it's unclear from the documentation if an actual driver may or may not override the message->status field. It seems there is no other way to indicate an error when transfer is failed, e.g. when gets overrun or underrun interrupt. The patch states this clearly in the comments near to field definition. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- include/linux/spi/spi.h | 3 +++ 1 file changed, 3 insertions(+)