[PATCH 0/2] spi: spidev: Introduce SPI_IOC_WR_DEFAULT_MAX_SPEED_HZ command

[PATCH 0/2] spi: spidev: Introduce SPI_IOC_WR_DEFAULT_MAX_SPEED_HZ command

[Seraphime Kirkovski]

Historically SPI_IOC_WR_MAX_SPEED_HZ was changing the ->max_speed_hz field
of the underlying struct spi_dev, which made the effects last way after
releasing one particular /dev/spidev* fd.

This changed in 9169051617df7 ("spi: spidev: Don't mangle max_speed_hz
in underlying spi device") or 7 years after the introduction of
SPI_IOC_WR_MAX_SPEED_HZ ! In the mean time there were userspace tools
developped with the assumptions that the effects of this particular
command are system-wide. [1] is a suite of two small programs for
reading, writing and configuring SPI interfaces. The `spi-config -s`
part was working good in our setup with old 3.x kernels. We discovered
this "regression" when we tried to port our workflow to newer kernels.

I think, this change is necessary, on the one hand, because there are still
a lot of longterm[2] supported kernels out there, whose users may be relying on
SPI_IOC_WR_MAX_SPEED being system-wide and, on the other hand, this
same command has been exhibiting a different behaviour for 3 years now,
so its users may break, if 9169051617df7 is reverted in one way or
another.

The semantics of the proposed command are the same as the old
SPI_IOC_WR_MAX_SPEED_HZ.

[1] https://github.com/cpb-/spi-tools
[2] A quick check on kernel.org shows that all 3.x with longterm support
    have not applied 9169051617df7

Seraphime Kirkovski (2):
  spi: spidev: introduce SPI_IOC_WR_DEFAULT_MAX_SPEED_HZ command
  spi: spidev: document SPI_IOC_WR_DEFAULT_MAX_SPEED_HZ

 Documentation/spi/spidev        | 7 +++++++
 drivers/spi/spidev.c            | 8 ++++++--
 include/uapi/linux/spi/spidev.h | 4 +++-
 3 files changed, 16 insertions(+), 3 deletions(-)

-- 
2.13.0.rc1

[PATCH 0/2] spi: spidev: Introduce SPI_IOC_WR_DEFAULT_MAX_SPEED_HZ command

[Seraphime Kirkovski]

Historically SPI_IOC_WR_MAX_SPEED_HZ was changing the ->max_speed_hz field
of the underlying struct spi_dev, which made the effects last way after
releasing one particular /dev/spidev* fd.

This changed in 9169051617df7 ("spi: spidev: Don't mangle max_speed_hz
in underlying spi device") or 7 years after the introduction of
SPI_IOC_WR_MAX_SPEED_HZ ! In the mean time there were userspace tools
developped with the assumptions that the effects of this particular
command are system-wide. [1] is a suite of two small programs for
reading, writing and configuring SPI interfaces. The `spi-config -s`
part was working good in our setup with old 3.x kernels. We discovered
this "regression" when we tried to port our workflow to newer kernels.

I think, this change is necessary, on the one hand, because there are still
a lot of longterm[2] supported kernels out there, whose users may be relying on
SPI_IOC_WR_MAX_SPEED being system-wide and, on the other hand, this
same command has been exhibiting a different behaviour for 3 years now,
so its users may break, if 9169051617df7 is reverted in one way or
another.

The semantics of the proposed command are the same as the old
SPI_IOC_WR_MAX_SPEED_HZ.

[1] https://github.com/cpb-/spi-tools
[2] A quick check on kernel.org shows that all 3.x with longterm support
    have not applied 9169051617df7

Seraphime Kirkovski (2):
  spi: spidev: introduce SPI_IOC_WR_DEFAULT_MAX_SPEED_HZ command
  spi: spidev: document SPI_IOC_WR_DEFAULT_MAX_SPEED_HZ

 Documentation/spi/spidev        | 7 +++++++
 drivers/spi/spidev.c            | 8 ++++++--
 include/uapi/linux/spi/spidev.h | 4 +++-
 3 files changed, 16 insertions(+), 3 deletions(-)

-- 
2.13.0.rc1

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

[PATCH 1/2] spi: spidev: introduce SPI_IOC_WR_DEFAULT_MAX_SPEED_HZ command

[Seraphime Kirkovski]

Historically SPI_IOC_WR_MAX_SPEED_HZ was changing the ->max_speed_hz field
of the underlying struct spi_dev, which made the effects last way after
releasing one particular /dev/spidev* fd.

This changed in 9169051617df7 ("spi: spidev: Don't mangle max_speed_hz
in underlying spi device") or 7 years after the introduction of
SPI_IOC_WR_MAX_SPEED_HZ ! In the mean time there were userspace tools
developped with the assumptions that the effects of this particular
command are system-wide. [1] is a suite of two small programs for
reading, writing and configuring SPI interfaces. The `spi-config -s`
part was working good in our setup with old 3.x kernels. We discovered
this "regression" when we tried to port our workflow to newer kernels.

I think, this change is necessary, on the one hand, because there are still
a lot of longterm[2] supported kernels out there, whose users may be relying on
SPI_IOC_WR_MAX_SPEED being system-wide and, on the other hand, this
same command has been exhibiting a different behaviour for 3 years now,
so its users may break, if 9169051617df7 is reverted in one way or
another.

The semantics of the proposed command are the same as the old
SPI_IOC_WR_MAX_SPEED_HZ.

[1] https://github.com/cpb-/spi-tools
[2] A quick check on kernel.org shows that all 3.x with longterm support
    have not applied 9169051617df7

Signed-off-by: Seraphime Kirkovski (Haapie) <kirkseraph@gmail.com>
---
 drivers/spi/spidev.c            | 8 ++++++--
 include/uapi/linux/spi/spidev.h | 4 +++-
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
index 9a2a79a871ba..18d310db90ab 100644
--- a/drivers/spi/spidev.c
+++ b/drivers/spi/spidev.c
@@ -474,6 +474,7 @@ spidev_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 				dev_dbg(&spi->dev, "%d bits per word\n", tmp);
 		}
 		break;
+	case SPI_IOC_WR_DEFAULT_MAX_SPEED_HZ:
 	case SPI_IOC_WR_MAX_SPEED_HZ:
 		retval = __get_user(tmp, (__u32 __user *)arg);
 		if (retval == 0) {
@@ -483,9 +484,12 @@ spidev_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 			retval = spi_setup(spi);
 			if (retval >= 0)
 				spidev->speed_hz = tmp;
-			else
+			else {
 				dev_dbg(&spi->dev, "%d Hz (max)\n", tmp);
-			spi->max_speed_hz = save;
+				spi->max_speed_hz = save;
+			}
+			if (cmd != SPI_IOC_WR_DEFAULT_MAX_SPEED_HZ)
+				spi->max_speed_hz = save;
 		}
 		break;
 
diff --git a/include/uapi/linux/spi/spidev.h b/include/uapi/linux/spi/spidev.h
index dd5f21e75805..91ed22154bc0 100644
--- a/include/uapi/linux/spi/spidev.h
+++ b/include/uapi/linux/spi/spidev.h
@@ -128,7 +128,7 @@ struct spi_ioc_transfer {
 #define SPI_IOC_RD_BITS_PER_WORD	_IOR(SPI_IOC_MAGIC, 3, __u8)
 #define SPI_IOC_WR_BITS_PER_WORD	_IOW(SPI_IOC_MAGIC, 3, __u8)
 
-/* Read / Write SPI device default max speed hz */
+/* Read / Write SPI device max speed hz */
 #define SPI_IOC_RD_MAX_SPEED_HZ		_IOR(SPI_IOC_MAGIC, 4, __u32)
 #define SPI_IOC_WR_MAX_SPEED_HZ		_IOW(SPI_IOC_MAGIC, 4, __u32)
 
@@ -136,6 +136,8 @@ struct spi_ioc_transfer {
 #define SPI_IOC_RD_MODE32		_IOR(SPI_IOC_MAGIC, 5, __u32)
 #define SPI_IOC_WR_MODE32		_IOW(SPI_IOC_MAGIC, 5, __u32)
 
+/* Write SPI device default max speed hz */
+#define SPI_IOC_WR_DEFAULT_MAX_SPEED_HZ	_IOW(SPI_IOC_MAGIC, 6, __u32)
 
 
 #endif /* SPIDEV_H */
-- 
2.13.0.rc1

Re: [PATCH 1/2] spi: spidev: introduce SPI_IOC_WR_DEFAULT_MAX_SPEED_HZ command

[Mark Brown]

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

On Tue, May 09, 2017 at 02:24:00PM +0200, Seraphime Kirkovski wrote:

> I think, this change is necessary, on the one hand, because there are still
> a lot of longterm[2] supported kernels out there, whose users may be relying on
> SPI_IOC_WR_MAX_SPEED being system-wide and, on the other hand, this
> same command has been exhibiting a different behaviour for 3 years now,
> so its users may break, if 9169051617df7 is reverted in one way or
> another.

Do we have any evidence that such users exist?  

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] spi: spidev: introduce SPI_IOC_WR_DEFAULT_MAX_SPEED_HZ command

[Mark Brown]

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

On Tue, May 09, 2017 at 02:24:00PM +0200, Seraphime Kirkovski wrote:

> I think, this change is necessary, on the one hand, because there are still
> a lot of longterm[2] supported kernels out there, whose users may be relying on
> SPI_IOC_WR_MAX_SPEED being system-wide and, on the other hand, this
> same command has been exhibiting a different behaviour for 3 years now,
> so its users may break, if 9169051617df7 is reverted in one way or
> another.

Do we have any evidence that such users exist?  

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 2/2] spi: spidev: document SPI_IOC_WR_DEFAULT_MAX_SPEED_HZ

[Seraphime Kirkovski]

This adds documentation of the new spidev ioctl.

Signed-off-by: Seraphime Kirkovski (Haapie) <kirkseraph@gmail.com>
---
 Documentation/spi/spidev | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/spi/spidev b/Documentation/spi/spidev
index 3d14035b1766..c67a00594dce 100644
--- a/Documentation/spi/spidev
+++ b/Documentation/spi/spidev
@@ -108,6 +108,13 @@ settings for data transfer parameters:
 	speed, in Hz.  The controller can't necessarily assign that specific
 	clock speed.
 
+    SPI_IOC_WR_DEFAULT_MAX_SPEED_HZ ... pass a pointer to a u32 which will
+	assign the maximum speed in Hz directly to the underlying device.
+	The controller can't necessarily assign that clock speed.
+	Any subsequent open() to the same /dev/spidevB.D will be configured
+	by default to use the maximum speed assigned by the last successful
+	call to SPI_IOC_WR_DEFAULT_MAX_SPEED_HZ.
+
 NOTES:
 
     - At this time there is no async I/O support; everything is purely
-- 
2.13.0.rc1

Re: [PATCH 2/2] spi: spidev: document SPI_IOC_WR_DEFAULT_MAX_SPEED_HZ

[Mark Brown]

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

On Tue, May 09, 2017 at 02:24:01PM +0200, Seraphime Kirkovski wrote:
> This adds documentation of the new spidev ioctl.

This should have been part of the first path adding the new ioctl() :(

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] spi: spidev: document SPI_IOC_WR_DEFAULT_MAX_SPEED_HZ

[Seraphime Kirkovski]

On Sun, May 14, 2017 at 06:27:43PM +0900, Mark Brown wrote:
> On Tue, May 09, 2017 at 02:24:01PM +0200, Seraphime Kirkovski wrote:
> > This adds documentation of the new spidev ioctl.
> 
> This should have been part of the first path adding the new ioctl() :(

Will change.

Re: [PATCH 0/2] spi: spidev: Introduce SPI_IOC_WR_DEFAULT_MAX_SPEED_HZ command

[Seraphime Kirkovski]

Any thoughts on this ?

Re: [PATCH 0/2] spi: spidev: Introduce SPI_IOC_WR_DEFAULT_MAX_SPEED_HZ command

[Seraphime Kirkovski]

Any thoughts on this ?
--
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 0/2] spi: spidev: Introduce SPI_IOC_WR_DEFAULT_MAX_SPEED_HZ command

[Mark Brown]

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

On Mon, May 15, 2017 at 09:34:25AM +0200, Seraphime Kirkovski wrote:
> Any thoughts on this ?

Please don't send content free pings and please allow a reasonable time
for review.  People get busy, go on holiday, attend conferences and so 
on so unless there is some reason for urgency (like critical bug fixes)
please allow at least a couple of weeks for review.  If there have been
review comments then people may be waiting for those to be addressed.

Sending content free pings adds to the mail volume (if they are seen at
all) which is often the problem and since they can't be reviewed
directly if something has gone wrong you'll have to resend the patches
anyway, though there are some other maintainers who like them - if in
doubt look at how patches for the subsystem are normally handled.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/2] spi: spidev: Introduce SPI_IOC_WR_DEFAULT_MAX_SPEED_HZ command

[Mark Brown]

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

On Mon, May 15, 2017 at 09:34:25AM +0200, Seraphime Kirkovski wrote:
> Any thoughts on this ?

Please don't send content free pings and please allow a reasonable time
for review.  People get busy, go on holiday, attend conferences and so 
on so unless there is some reason for urgency (like critical bug fixes)
please allow at least a couple of weeks for review.  If there have been
review comments then people may be waiting for those to be addressed.

Sending content free pings adds to the mail volume (if they are seen at
all) which is often the problem and since they can't be reviewed
directly if something has gone wrong you'll have to resend the patches
anyway, though there are some other maintainers who like them - if in
doubt look at how patches for the subsystem are normally handled.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 1/2] spi: spidev: introduce SPI_IOC_WR_DEFAULT_MAX_SPEED_HZ command

[Seraphime Kirkovski]

christophe.blaess@gmail.com, fred@haapie.com
Bcc: Subject: Re: [PATCH 1/2] spi: spidev: introduce
 SPI_IOC_WR_DEFAULT_MAX_SPEED_HZ command
Reply-To: 
In-Reply-To: <20170514092716.kqx634djffgj32r6@sirena.org.uk>

On Sun, May 14, 2017 at 06:27:16PM +0900, Mark Brown wrote:
> On Tue, May 09, 2017 at 02:24:00PM +0200, Seraphime Kirkovski wrote:
> 
> > I think, this change is necessary, on the one hand, because there are still
> > a lot of longterm[2] supported kernels out there, whose users may be relying on
> > SPI_IOC_WR_MAX_SPEED being system-wide and, on the other hand, this
> > same command has been exhibiting a different behaviour for 3 years now,
> > so its users may break, if 9169051617df7 is reverted in one way or
> > another.
> 
> Do we have any evidence that such users exist?  

I can't guarantee for other such users, but this change did disturb our 
workflow. We were using this feature to prototype and test 
hardware/firmware at different speeds.

I was thinking this morning that maybe a sysfs interface will be better 
for setting global settings. Do you prefer ?

Re: [PATCH 1/2] spi: spidev: introduce SPI_IOC_WR_DEFAULT_MAX_SPEED_HZ command

[Mark Brown]

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

On Mon, May 15, 2017 at 10:17:11AM +0200, Seraphime Kirkovski wrote:
> On Sun, May 14, 2017 at 06:27:16PM +0900, Mark Brown wrote:

> > Do we have any evidence that such users exist?  

> I can't guarantee for other such users, but this change did disturb our 
> workflow. We were using this feature to prototype and test 
> hardware/firmware at different speeds.

> I was thinking this morning that maybe a sysfs interface will be better 
> for setting global settings. Do you prefer ?

I just don't see this as something that should be being varied at
runtime; it is supposed to be the maximum speed that the bus can be run
at, that's not something that should vary at runtime.  Possibly a sysfs
thing but honestly if you're prototyping via spidev I'd just set the
speed in your application each time it starts.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] spi: spidev: introduce SPI_IOC_WR_DEFAULT_MAX_SPEED_HZ command

[Mark Brown]

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

On Mon, May 15, 2017 at 10:17:11AM +0200, Seraphime Kirkovski wrote:
> On Sun, May 14, 2017 at 06:27:16PM +0900, Mark Brown wrote:

> > Do we have any evidence that such users exist?  

> I can't guarantee for other such users, but this change did disturb our 
> workflow. We were using this feature to prototype and test 
> hardware/firmware at different speeds.

> I was thinking this morning that maybe a sysfs interface will be better 
> for setting global settings. Do you prefer ?

I just don't see this as something that should be being varied at
runtime; it is supposed to be the maximum speed that the bus can be run
at, that's not something that should vary at runtime.  Possibly a sysfs
thing but honestly if you're prototyping via spidev I'd just set the
speed in your application each time it starts.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]