* [PATCH 1/2] spi: spidev: introduce SPI_IOC_WR_DEFAULT_MAX_SPEED_HZ command
@ 2017-05-15 8:17 Seraphime Kirkovski
2017-05-17 9:04 ` Mark Brown
0 siblings, 1 reply; 6+ messages in thread
From: Seraphime Kirkovski @ 2017-05-15 8:17 UTC (permalink / raw)
To: Mark Brown; +Cc: linux-kernel, linux-spi
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 ?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] spi: spidev: introduce SPI_IOC_WR_DEFAULT_MAX_SPEED_HZ command
2017-05-15 8:17 [PATCH 1/2] spi: spidev: introduce SPI_IOC_WR_DEFAULT_MAX_SPEED_HZ command Seraphime Kirkovski
@ 2017-05-17 9:04 ` Mark Brown
0 siblings, 0 replies; 6+ messages in thread
From: Mark Brown @ 2017-05-17 9:04 UTC (permalink / raw)
To: Seraphime Kirkovski; +Cc: linux-kernel, linux-spi
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] spi: spidev: introduce SPI_IOC_WR_DEFAULT_MAX_SPEED_HZ command
@ 2017-05-17 9:04 ` Mark Brown
0 siblings, 0 replies; 6+ messages in thread
From: Mark Brown @ 2017-05-17 9:04 UTC (permalink / raw)
To: Seraphime Kirkovski
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-spi-u79uwXL29TY76Z2rM5mHXA
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] spi: spidev: introduce SPI_IOC_WR_DEFAULT_MAX_SPEED_HZ command
@ 2017-05-14 9:27 ` Mark Brown
0 siblings, 0 replies; 6+ messages in thread
From: Mark Brown @ 2017-05-14 9:27 UTC (permalink / raw)
To: Seraphime Kirkovski; +Cc: linux-kernel, linux-spi
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] spi: spidev: introduce SPI_IOC_WR_DEFAULT_MAX_SPEED_HZ command
@ 2017-05-14 9:27 ` Mark Brown
0 siblings, 0 replies; 6+ messages in thread
From: Mark Brown @ 2017-05-14 9:27 UTC (permalink / raw)
To: Seraphime Kirkovski
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-spi-u79uwXL29TY76Z2rM5mHXA
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] spi: spidev: introduce SPI_IOC_WR_DEFAULT_MAX_SPEED_HZ command
2017-05-09 12:23 [PATCH 0/2] spi: spidev: Introduce " Seraphime Kirkovski
@ 2017-05-09 12:24 ` Seraphime Kirkovski
2017-05-14 9:27 ` Mark Brown
0 siblings, 1 reply; 6+ messages in thread
From: Seraphime Kirkovski @ 2017-05-09 12:24 UTC (permalink / raw)
To: linux-kernel; +Cc: linux-spi, broonie, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-05-17 9:04 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-15 8:17 [PATCH 1/2] spi: spidev: introduce SPI_IOC_WR_DEFAULT_MAX_SPEED_HZ command Seraphime Kirkovski
2017-05-17 9:04 ` Mark Brown
2017-05-17 9:04 ` Mark Brown
-- strict thread matches above, loose matches on Subject: below --
2017-05-09 12:23 [PATCH 0/2] spi: spidev: Introduce " Seraphime Kirkovski
2017-05-09 12:24 ` [PATCH 1/2] spi: spidev: introduce " Seraphime Kirkovski
2017-05-14 9:27 ` Mark Brown
2017-05-14 9:27 ` Mark Brown
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.