lkml.org 
[lkml]   [2014]   [Nov]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH] spi: spidev: Don't mangle max_speed_hz in underlying spi device
Hi Mark,

On 11/08/2014 04:29 AM, Mark Brown wrote:
> Currently spidev allows callers to set the default speed by overriding the
> max_speed_hz in the underlying device. This achieves the immediate goal but
> is not what devices expect and can easily lead to userspace trying to set
> unsupported speeds and succeeding, apart from anything else drivers can't
> set a limit on the speed using max_speed_hz as they'd expect and any other
> devices on the bus will be affected.
>
> Instead store the default speed in the spidev struct and fill this in on
> each transfer.
>
> Signed-off-by: Mark Brown<broonie@kernel.org>
> ---
>
> I've not tested this at all yet.
>
> drivers/spi/spidev.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
> index e50039f..42239fd 100644
> --- a/drivers/spi/spidev.c
> +++ b/drivers/spi/spidev.c
> @@ -87,6 +87,7 @@ struct spidev_data {
> unsigned users;
> u8 *tx_buffer;
> u8 *rx_buffer;
> + u32 speed_hz;
> };
>
> static LIST_HEAD(device_list);
> @@ -274,6 +275,8 @@ static int spidev_message(struct spidev_data *spidev,
> k_tmp->bits_per_word = u_tmp->bits_per_word;
> k_tmp->delay_usecs = u_tmp->delay_usecs;
> k_tmp->speed_hz = u_tmp->speed_hz;
> + if (!k_tmp->speed_hz)
> + k_tmp->speed_hz = spidev->speed_hz;
> #ifdef VERBOSE
> dev_dbg(&spidev->spi->dev,
> " xfer len %zd %s%s%s%dbits %u usec %uHz\n",
> @@ -377,7 +380,7 @@ spidev_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> retval = __put_user(spi->bits_per_word, (__u8 __user *)arg);
> break;
> case SPI_IOC_RD_MAX_SPEED_HZ:
> - retval = __put_user(spi->max_speed_hz, (__u32 __user *)arg);
> + retval = __put_user(spidev->speed_hz, (__u32 __user *)arg);
> break;
>
> /* write requests */
> @@ -442,9 +445,10 @@ spidev_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> spi->max_speed_hz = tmp;
> retval = spi_setup(spi);
> if (retval < 0)
> - spi->max_speed_hz = save;
> + spidev->speed_hz = tmp;
> else
> dev_dbg(&spi->dev, "%d Hz (max)\n", tmp);
> + spi->max_speed_hz = save;

I think the test should be if (retval >= 0) otherwise the value is only
updated on an error. With this change, I was able to successfully change
the SPI speed without affecting the max speed. This was tested using
spidev_test.c on a Altera Cyclone V which includes the DesignWare SPI IP
(spi-dw.c).

> }
> break;
>
> @@ -570,6 +574,8 @@ static int spidev_release(struct inode *inode, struct file *filp)
> kfree(spidev->rx_buffer);
> spidev->rx_buffer = NULL;
>
> + spidev->speed_hz = spidev->spi->max_speed_hz;
> +
> /* ... after we unbound from the underlying device? */
> spin_lock_irq(&spidev->spi_lock);
> dofree = (spidev->spi == NULL);
> @@ -650,6 +656,8 @@ static int spidev_probe(struct spi_device *spi)
> }
> mutex_unlock(&device_list_lock);
>
> + spidev->speed_hz = spi->max_speed_hz;
> +
> if (status == 0)
> spi_set_drvdata(spi, spidev);
> else

The echo command calls spidev_write() directly. Although the speed can't
be specified in the echo command, it did prompt me to look at that path.

In that case I think we'd need to add the .speed_hz element in
spidev_sync_write() and spidev_sync_read().
struct spi_transfer t = {
.rx_buf = spidev->rx_buffer,
.len = len,
.speed_hz = spidev->speed_hz,
};

\
 
 \ /
  Last update: 2014-11-11 15:21    [W:1.498 / U:0.020 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site