Re: [PATCH RFC] spidev.c: add sysfs attributes for SPI configuration
On Sat, Dec 22, 2012 at 12:21:15PM +0100, Federico Vaga wrote: > > I'm cautious about adding operational interfaces to sysfs because it > > can be quite difficult to get the locking right. To begin with it > > splits up a single interface into multiple files, any of which can > > be held open by a process. Then there is the question of ordering > > of operations when there are multiple users. For instance, if there > > were two users, each of which using different transfer parameters, > > a sysfs interface doesn't provide any mechanism to group setting up > > the device with the transfer. > > > > These are lessons learned the hard way with the gpio sysfs abi. I > > don't want to get caught in the same trap for spi. > > > > g. > > I understand the problem, but I think that for very simple test on > devices, sysfs is easier. For example, it happens that a SPI device > does not work correctly with a driver, so I want to verify the SPI > traffic by writing directly the device commands and check with an > oscilloscope. I think that an easy way is to use sysfs like this: > > echo 123456 > speed_hz > [other options if needed] > echo -n -e "\x12\x34" > /dev/spidev1.1 > [oscilloscope] > hexdump -n 2 /dev/spidev1.1 > > This sysfs interface should be used only for testing/debugging, not to > develop an user space driver on it; moreover, the ioctl interface is > still there. Then move it to debugfs, don't put debugging code in sysfs, as once you do, you are stuck with it for life (hint, you forgot to add Documentation/ABI entries for your new sysfs files, which are required). If you move this to debugfs, you can do whatever you want, as it's obvious no one would ever put "real" interfaces in debugfs :) thanks, greg k-h -- LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial Remotely access PCs and mobile devices and provide instant support Improve your efficiency, and focus on delivering more value-add services Discover what IT Professionals Know. Rescue delivers http://p.sf.net/sfu/logmein_12329d2d ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH RFC] spidev.c: add sysfs attributes for SPI configuration
> I'm cautious about adding operational interfaces to sysfs because it > can be quite difficult to get the locking right. To begin with it > splits up a single interface into multiple files, any of which can > be held open by a process. Then there is the question of ordering > of operations when there are multiple users. For instance, if there > were two users, each of which using different transfer parameters, > a sysfs interface doesn't provide any mechanism to group setting up > the device with the transfer. > > These are lessons learned the hard way with the gpio sysfs abi. I > don't want to get caught in the same trap for spi. > > g. I understand the problem, but I think that for very simple test on devices, sysfs is easier. For example, it happens that a SPI device does not work correctly with a driver, so I want to verify the SPI traffic by writing directly the device commands and check with an oscilloscope. I think that an easy way is to use sysfs like this: echo 123456 > speed_hz [other options if needed] echo -n -e "\x12\x34" > /dev/spidev1.1 [oscilloscope] hexdump -n 2 /dev/spidev1.1 This sysfs interface should be used only for testing/debugging, not to develop an user space driver on it; moreover, the ioctl interface is still there. spidev_test and spidev_fdx does not allow me to customize tx buffer and I think (very personal opinion) that for debugging purpose is better sysfs with well known programs (echo, cat, hexdump, od) and oscilloscope. I know that I'm not so persuasive :) I can develop a simple program that can write custom tx buf with ioctl -- Federico Vaga -- LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial Remotely access PCs and mobile devices and provide instant support Improve your efficiency, and focus on delivering more value-add services Discover what IT Professionals Know. Rescue delivers http://p.sf.net/sfu/logmein_12329d2d ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH RFC] spidev.c: add sysfs attributes for SPI configuration
On Thu, 20 Dec 2012 16:30:36 +0100, Federico Vaga wrote: > On Wednesday 19 December 2012 15:09:25 Grant Likely wrote: > > Not a good idea. sysfs is not a good place for operational > > interfaces. Please use the spi character devices for direct > > manipulation of the SPI configuration. > > Hello, > > Can you explain why it is not a good idea? I do not understand; what > is the advantage of ioctl through char device? Or what it the issue > with sysfs? > > Thank you very much I'm cautious about adding operational interfaces to sysfs because it can be quite difficult to get the locking right. To begin with it splits up a single interface into multiple files, any of which can be held open by a process. Then there is the question of ordering of operations when there are multiple users. For instance, if there were two users, each of which using different transfer parameters, a sysfs interface doesn't provide any mechanism to group setting up the device with the transfer. These are lessons learned the hard way with the gpio sysfs abi. I don't want to get caught in the same trap for spi. g. -- LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial Remotely access PCs and mobile devices and provide instant support Improve your efficiency, and focus on delivering more value-add services Discover what IT Professionals Know. Rescue delivers http://p.sf.net/sfu/logmein_12329d2d ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH RFC] spidev.c: add sysfs attributes for SPI configuration
On Wednesday 19 December 2012 15:09:25 Grant Likely wrote: > Not a good idea. sysfs is not a good place for operational > interfaces. Please use the spi character devices for direct > manipulation of the SPI configuration. Hello, Can you explain why it is not a good idea? I do not understand; what is the advantage of ioctl through char device? Or what it the issue with sysfs? Thank you very much -- Federico Vaga -- LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial Remotely access PCs and mobile devices and provide instant support Improve your efficiency, and focus on delivering more value-add services Discover what IT Professionals Know. Rescue delivers http://p.sf.net/sfu/logmein_12329d2d ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH RFC] spidev.c: add sysfs attributes for SPI configuration
On Sat, 24 Nov 2012 18:20:08 +0100, Federico Vaga wrote: > This patch introduce the use of the sysfs attribute for the spidev > configuration. This avoid the user to have a specific program which does > ioctl() on spidev. The user can easily does cat (to read) and echo (to > write) on the sysfs file and configure SPI. > > The patch exports the following attributes: bits-per-word, lsb-first, > mode and speed-hz. > > Example: > # cat /sys/bus/spi/devices/spi1.0/speed-hz > 50 > # echo 45 > /sys/bus/spi/devices/spi1.0/speed-hz > # dmesg | tail -n 4 > spidev spi1.0: DEactivate 60, mr 000f0011 > spidev spi1.0: setup: 449447 Hz bpw 8 mode 0x0 -> csr0 dd02 > spidev spi1.0: setup mode 0, 8 bits/w, 45 Hz max --> 0 > spidev spi1.0: 45 Hz (max) > > Signed-off-by: Federico Vaga Not a good idea. sysfs is not a good place for operational interfaces. Please use the spi character devices for direct manipulation of the SPI configuration. g. -- LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial Remotely access PCs and mobile devices and provide instant support Improve your efficiency, and focus on delivering more value-add services Discover what IT Professionals Know. Rescue delivers http://p.sf.net/sfu/logmein_12329d2d ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
[PATCH RFC] spidev.c: add sysfs attributes for SPI configuration
This patch introduce the use of the sysfs attribute for the spidev
configuration. This avoid the user to have a specific program which does
ioctl() on spidev. The user can easily does cat (to read) and echo (to
write) on the sysfs file and configure SPI.
The patch exports the following attributes: bits-per-word, lsb-first,
mode and speed-hz.
Example:
# cat /sys/bus/spi/devices/spi1.0/speed-hz
50
# echo 45 > /sys/bus/spi/devices/spi1.0/speed-hz
# dmesg | tail -n 4
spidev spi1.0: DEactivate 60, mr 000f0011
spidev spi1.0: setup: 449447 Hz bpw 8 mode 0x0 -> csr0 dd02
spidev spi1.0: setup mode 0, 8 bits/w, 45 Hz max --> 0
spidev spi1.0: 45 Hz (max)
Signed-off-by: Federico Vaga
---
drivers/spi/spidev.c | 258 +--
1 file modificato, 208 inserzioni(+), 50 rimozioni(-)
diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
index 830adbe..4aa0832 100644
--- a/drivers/spi/spidev.c
+++ b/drivers/spi/spidev.c
@@ -31,6 +31,7 @@
#include
#include
#include
+#include
#include
#include
@@ -92,6 +93,201 @@ static unsigned bufsiz = 4096;
module_param(bufsiz, uint, S_IRUGO);
MODULE_PARM_DESC(bufsiz, "data bytes in biggest supported SPI message");
+
+/*-*/
+
+/* SYSFS */
+enum spidev_config_enum {
+ SPIDEV_SPEED_HZ,
+ SPIDEV_BIT_PER_WORD,
+ SPIDEV_LSB_FIRST,
+ SPIDEV_MODE,
+};
+struct spidev_config_attr {
+ struct device_attribute attr;
+ enum spidev_config_enum cmd;
+};
+#define to_spidev_attr(_attr) \
+ container_of(_attr, struct spidev_config_attr, attr)
+
+static int spidev_conf_mode(struct spi_device *spi, u32 tmp)
+{
+ u8 save = spi->mode;
+ int err = 0;
+
+ if (tmp & ~SPI_MODE_MASK)
+ return -EINVAL;
+
+ tmp |= spi->mode & ~SPI_MODE_MASK;
+ spi->mode = (u8)tmp;
+ err = spi_setup(spi);
+ if (err < 0)
+ spi->mode = save;
+ else
+ dev_dbg(&spi->dev, "spi mode %02x\n", tmp);
+
+ return err;
+}
+static int spidev_conf_lsb(struct spi_device *spi, u32 tmp)
+{
+ u8 save = spi->mode;
+ int err = 0;
+
+ if (tmp)
+ spi->mode |= SPI_LSB_FIRST;
+ else
+ spi->mode &= ~SPI_LSB_FIRST;
+ err = spi_setup(spi);
+ if (err < 0)
+ spi->mode = save;
+ else
+ dev_dbg(&spi->dev, "%csb first\n", (tmp ? 'l' : 'm'));
+
+ return err;
+}
+static int spidev_conf_bpw(struct spi_device *spi, u32 tmp)
+{
+ u8 save = spi->bits_per_word;
+ int err = 0;
+
+ spi->bits_per_word = tmp;
+ err = spi_setup(spi);
+ if (err < 0)
+ spi->bits_per_word = save;
+ else
+ dev_dbg(&spi->dev, "%d bits per word\n", tmp);
+
+ return err;
+}
+static int spidev_conf_speedhz(struct spi_device *spi, u32 tmp)
+{
+ u32 save = spi->max_speed_hz;
+ int err = 0;
+
+ spi->max_speed_hz = tmp;
+ err = spi_setup(spi);
+ if (err < 0)
+ spi->max_speed_hz = save;
+ else
+ dev_dbg(&spi->dev, "%d Hz (max)\n", tmp);
+
+ return err;
+}
+
+/* Return to user space the current SPI configuration */
+static ssize_t spidev_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct spidev_config_attr *sattr = to_spidev_attr(attr);
+ struct spidev_data *spidev;
+ struct spi_device *spi;
+ ssize_t count = 0;
+
+ spidev = spi_get_drvdata(to_spi_device(dev));
+
+ spin_lock_irq(&spidev->spi_lock);
+ spi = spi_dev_get(spidev->spi);
+ spin_unlock_irq(&spidev->spi_lock);
+
+ mutex_lock(&spidev->buf_lock);
+ switch (sattr->cmd) {
+ case SPIDEV_MODE:
+ count = sprintf(buf, "%d\n", (spi->mode & SPI_MODE_MASK));
+ break;
+ case SPIDEV_LSB_FIRST:
+ count = sprintf(buf, "%d\n",
+ ((spi->mode & SPI_LSB_FIRST) ? 1 : 0));
+ break;
+ case SPIDEV_BIT_PER_WORD:
+ count = sprintf(buf, "%d\n", spi->bits_per_word);
+ break;
+ case SPIDEV_SPEED_HZ:
+ count = sprintf(buf, "%d\n", spi->max_speed_hz);
+ break;
+ }
+ mutex_unlock(&spidev->buf_lock);
+ spi_dev_put(spi);
+
+ return count;
+}
+/* Configure the SPI from userspace */
+static ssize_t spidev_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct spidev_config_attr *sattr = to_spidev_attr(attr);
+ struct spidev_data *spidev;
+ struct spi_device *spi;
+ int err = 0;
+ u32 tmp;
+
+ spidev = spi_get_drvdata(to_spi_device(dev));
+
+ spin_lock_irq(&spidev->spi_lock);
+ spi = spi_dev_get(
