[PATCH v1 1/1] spi: clarify what driver can do with message->status

[PATCH v1 1/1] spi: clarify what driver can do with message->status

[Andy Shevchenko]

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-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 include/linux/spi/spi.h | 3 +++
 1 file changed, 3 insertions(+)

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
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 1/1] spi: clarify what driver can do with message->status

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 692 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v1 1/1] spi: clarify what driver can do with message->status

[Andy Shevchenko]

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.

-- 
Andy Shevchenko <andriy.shevchenko-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Intel Finland Oy

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 1/1] spi: clarify what driver can do with message->status

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 684 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v1 1/1] spi: clarify what driver can do with message->status

[Andy Shevchenko]

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?

-- 
Andy Shevchenko <andriy.shevchenko-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Intel Finland Oy

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 1/1] spi: clarify what driver can do with message->status

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1321 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v1 1/1] spi: clarify what driver can do with message->status

[Andy Shevchenko]

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.

-- 
Andy Shevchenko <andriy.shevchenko-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Intel Finland Oy

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html