[RFC 0/2] sysfs new_id file support for spidev

[RFC 0/2] sysfs new_id file support for spidev

[Bhuvanchandra DV]

This patchset adds support for loading spidev via sysfs new_id file.
Any comments/suggestions welcome!

Bhuvanchandra DV (2):
  spi: spidev: Use 'new_id' sysfs file for enabling spidev
  spi core: Add new sysfs 'num_chipselect' file

 Documentation/ABI/testing/sysfs-bus-spi-spidev |  8 +++
 Documentation/spi/spi-summary                  |  3 ++
 drivers/spi/spi.c                              | 23 ++++++++
 drivers/spi/spidev.c                           | 75 +++++++++++++++++++++++---
 4 files changed, 103 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-spi-spidev

-- 
2.7.3

[RFC 1/2] spi: spidev: Use 'new_id' sysfs file for enabling spidev

[Bhuvanchandra DV]

Use sysfs new_id file to enable spidev at runtime. new_id file accepts bus
number and chip select in 'B.C' format.

With reference to the last ML discussion here[1] regarding the solution for
spidev with device trees. This patch adds support to load spidevB.C via sysfs
new_id file as suggested by Greg.

[1] http://thread.gmane.org/gmane.linux.kernel.spi.devel/20331/focus=20369

Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com>
Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
---
 Documentation/ABI/testing/sysfs-bus-spi-spidev |  8 +++
 drivers/spi/spidev.c                           | 75 +++++++++++++++++++++++---
 2 files changed, 77 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-spi-spidev

diff --git a/Documentation/ABI/testing/sysfs-bus-spi-spidev b/Documentation/ABI/testing/sysfs-bus-spi-spidev
new file mode 100644
index 0000000..3f6e092
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-spi-spidev
@@ -0,0 +1,8 @@
+What:		/sys/bus/spi/drivers/spidev/new_id
+Date:		March 2016
+Description:
+		This allows to load spidev at runtime. new_id file accepts bus
+		number and chip select in 'B.C' format.
+		e.g.
+		To load spidev1.1 at runtime:
+		$ echo 1.1 > /sys/bus/spi/drivers/spidev/new_id
diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
index e3c19f3..99e9842 100644
--- a/drivers/spi/spidev.c
+++ b/drivers/spi/spidev.c
@@ -667,6 +667,57 @@ static int spidev_release(struct inode *inode, struct file *filp)
 	return 0;
 }
 
+/**
+ * new_id_store - add a new spidevB.C instance
+ * @driver: target device driver
+ * @buf: buffer for scanning bus number and chip select
+ * @count: input size
+ *
+ * Adds a new dynamic spidev instance based on the requested bus number
+ * and chip select.
+ */
+static ssize_t new_id_store(struct device_driver *drv, const char *buf,
+		size_t count)
+{
+	int ret;
+	u32 bus, cs;
+	struct spi_device *spi;
+	struct spi_master *master;
+
+	ret = sscanf(buf, "%u.%u", &bus, &cs);
+
+	if (ret < 2)
+		return -EINVAL;
+
+	master = spi_busnum_to_master(bus);
+	if (!master)
+		return -ENODEV;
+
+	if (cs >= master->num_chipselect)
+		return -ENODEV;
+
+	spi = spi_alloc_device(master);
+	if (!spi) {
+		dev_err(&master->dev, "Couldn't allocate spidev device\n");
+		return -ENOMEM;;
+	}
+
+	spi->chip_select = cs;
+	master->bus_num = bus;
+
+	strlcpy(spi->modalias, "spidev", sizeof(spi->modalias));
+
+	ret = spi_add_device(spi);
+	if (ret) {
+		dev_err(&master->dev, "Couldn't add spidev device\n");
+		spi_dev_put(spi);
+		return ret;
+	}
+
+	return count;
+}
+static DRIVER_ATTR_WO(new_id);
+
 static const struct file_operations spidev_fops = {
 	.owner =	THIS_MODULE,
 	/* REVISIT switch to aio primitives, so that userspace
@@ -817,21 +868,33 @@ static int __init spidev_init(void)
 
 	spidev_class = class_create(THIS_MODULE, "spidev");
 	if (IS_ERR(spidev_class)) {
-		unregister_chrdev(SPIDEV_MAJOR, spidev_spi_driver.driver.name);
-		return PTR_ERR(spidev_class);
+		status = PTR_ERR(spidev_class);
+		goto err_unregister_chrdev;
 	}
 
 	status = spi_register_driver(&spidev_spi_driver);
-	if (status < 0) {
-		class_destroy(spidev_class);
-		unregister_chrdev(SPIDEV_MAJOR, spidev_spi_driver.driver.name);
-	}
+	if (status < 0)
+		goto err_destroy_class;
+
+	status = driver_create_file(&spidev_spi_driver.driver, &driver_attr_new_id);
+	if (status < 0)
+		goto err_unregister_driver;
+
+	return status;
+
+err_unregister_driver:
+	spi_unregister_driver(&spidev_spi_driver);
+err_destroy_class:
+	class_destroy(spidev_class);
+err_unregister_chrdev:
+	unregister_chrdev(SPIDEV_MAJOR, spidev_spi_driver.driver.name);
 	return status;
 }
 module_init(spidev_init);
 
 static void __exit spidev_exit(void)
 {
+	driver_remove_file(&spidev_spi_driver.driver, &driver_attr_new_id);
 	spi_unregister_driver(&spidev_spi_driver);
 	class_destroy(spidev_class);
 	unregister_chrdev(SPIDEV_MAJOR, spidev_spi_driver.driver.name);
-- 
2.7.3

Re: [RFC 1/2] spi: spidev: Use 'new_id' sysfs file for enabling spidev

[Mark Brown]

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

On Mon, Apr 18, 2016 at 04:48:27PM +0530, Bhuvanchandra DV wrote:

> +++ b/Documentation/ABI/testing/sysfs-bus-spi-spidev
> @@ -0,0 +1,8 @@
> +What:		/sys/bus/spi/drivers/spidev/new_id
> +Date:		March 2016
> +Description:
> +		This allows to load spidev at runtime. new_id file accepts bus
> +		number and chip select in 'B.C' format.
> +		e.g.
> +		To load spidev1.1 at runtime:
> +		$ echo 1.1 > /sys/bus/spi/drivers/spidev/new_id

I would expect a new_id file to allow a new device identifier to be
added to a driver at runtime but this isn't doing that, it's specifying
by bus and chip select instead which is totally different to how we
normally bind SPI devices.  That seems likely to cause confusion down
the line.  I'd expect to be able to supply a DT compatible string or a
Linux SPI ID.

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

Re: [RFC 1/2] spi: spidev: Use 'new_id' sysfs file for enabling spidev

[Bhuvanchandra DV]

On 04/18/2016 06:31 PM, Mark Brown wrote:
> On Mon, Apr 18, 2016 at 04:48:27PM +0530, Bhuvanchandra DV wrote:
>
>> +++ b/Documentation/ABI/testing/sysfs-bus-spi-spidev
>> @@ -0,0 +1,8 @@
>> +What:		/sys/bus/spi/drivers/spidev/new_id
>> +Date:		March 2016
>> +Description:
>> +		This allows to load spidev at runtime. new_id file accepts bus
>> +		number and chip select in 'B.C' format.
>> +		e.g.
>> +		To load spidev1.1 at runtime:
>> +		$ echo 1.1 > /sys/bus/spi/drivers/spidev/new_id
>
> I would expect a new_id file to allow a new device identifier to be
> added to a driver at runtime but this isn't doing that, it's specifying
> by bus and chip select instead which is totally different to how we
> normally bind SPI devices.  That seems likely to cause confusion down
> the line.  I'd expect to be able to supply a DT compatible string or a
> Linux SPI ID.

SPI drivers bind to the device as configured in DT, but in case of 
spidev(non DT approach) the device is not available in hand to bind the 
driver. So tried this approach of creating the device and then bind 
spidev. I agree this is not a standard way we bind SPI devices. With out 
a device available, seems it is not possible to bind spidev in non DT 
approach. Please correct me if i'm wrong.

>

-- 
Best regards,
Bhuvan

Re: [RFC 1/2] spi: spidev: Use 'new_id' sysfs file for enabling spidev

[Mark Brown]

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

On Wed, Apr 20, 2016 at 05:43:32PM +0530, Bhuvanchandra DV wrote:

> SPI drivers bind to the device as configured in DT, but in case of
> spidev(non DT approach) the device is not available in hand to bind the
> driver. So tried this approach of creating the device and then bind spidev.
> I agree this is not a standard way we bind SPI devices. With out a device
> available, seems it is not possible to bind spidev in non DT approach.
> Please correct me if i'm wrong.

The DT should describe the hardware so the DT should describe whatever
device is connected to the spidev, what you're trying to do is have
spidev bind to these new compatible strings.

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

Re: [RFC 1/2] spi: spidev: Use 'new_id' sysfs file for enabling spidev

[Bhuvanchandra DV]

On 04/20/2016 05:47 PM, Mark Brown wrote:
> On Wed, Apr 20, 2016 at 05:43:32PM +0530, Bhuvanchandra DV wrote:
>
>> SPI drivers bind to the device as configured in DT, but in case of
>> spidev(non DT approach) the device is not available in hand to bind the
>> driver. So tried this approach of creating the device and then bind spidev.
>> I agree this is not a standard way we bind SPI devices. With out a device
>> available, seems it is not possible to bind spidev in non DT approach.
>> Please correct me if i'm wrong.
>
> The DT should describe the hardware so the DT should describe whatever
> device is connected to the spidev, what you're trying to do is have
> spidev bind to these new compatible strings.

The device which i was pointing earlier is the SPI master device(spiB.C) 
not the external h/w device interfaced to it. Yes, DT will describe the 
h/w interfaced on SPI master device(spiB.C). In case of spidev we are 
not supposed to describe in DT as it is not a real h/w. The intention of 
this patchset is to completely avoid spidev in DT and load it via sysfs 
new_id file for selected SPI master device(spiB.C).

>

-- 
Best regards,
Bhuvan

Re: [RFC 1/2] spi: spidev: Use 'new_id' sysfs file for enabling spidev

[Mark Brown]

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

On Wed, Apr 20, 2016 at 06:41:11PM +0530, Bhuvanchandra DV wrote:
> On 04/20/2016 05:47 PM, Mark Brown wrote:

> >The DT should describe the hardware so the DT should describe whatever
> >device is connected to the spidev, what you're trying to do is have
> >spidev bind to these new compatible strings.

> The device which i was pointing earlier is the SPI master device(spiB.C) not
> the external h/w device interfaced to it. Yes, DT will describe the h/w
> interfaced on SPI master device(spiB.C). In case of spidev we are not
> supposed to describe in DT as it is not a real h/w. The intention of this
> patchset is to completely avoid spidev in DT and load it via sysfs new_id
> file for selected SPI master device(spiB.C).

This is the problem, you've not understood what spidev is.  spidev is
providing access to a slave device so it binds to the slave device.

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

[RFC 2/2] spi core: Add new sysfs 'num_chipselect' file

[Bhuvanchandra DV]

Add new sysfs 'num_chipselect' file to expose the maximum number
of chipselects a SPI master can support.
This allows to create a script in user space which automatically
creates a new spidev instance for every chipselect on a bus.

Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com>
Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
---
 Documentation/spi/spi-summary |  3 +++
 drivers/spi/spi.c             | 23 +++++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/Documentation/spi/spi-summary b/Documentation/spi/spi-summary
index d1824b3..b4d15e4 100644
--- a/Documentation/spi/spi-summary
+++ b/Documentation/spi/spi-summary
@@ -181,6 +181,9 @@ shows up in sysfs in several locations:
 	controller managing bus "B".  All spiB.* devices share one
 	physical SPI bus segment, with SCLK, MOSI, and MISO.
 
+   /sys/class/spi_master/spiB/num_chipselect ... exposes the maximum
+	number of chipselects a SPI master can support.
+
 Note that the actual location of the controller's class state depends
 on whether you enabled CONFIG_SYSFS_DEPRECATED or not.  At this time,
 the only class-specific state is the bus number ("B" in "spiB"), so
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index de2f2f9..c3eb29f 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -67,6 +67,19 @@ modalias_show(struct device *dev, struct device_attribute *a, char *buf)
 }
 static DEVICE_ATTR_RO(modalias);
 
+static ssize_t
+num_chipselect_show(struct device *dev, struct device_attribute *a, char *buf)
+{
+	struct spi_master *master = container_of(dev,
+						 struct spi_master, dev);
+
+	return sprintf(buf, "%d\n", master->num_chipselect);
+}
+static struct device_attribute dev_attr_num_chipselect = {
+	.attr = { .name = "num_chipselect", .mode = S_IRUGO },
+	.show = num_chipselect_show,
+};
+
 #define SPI_STATISTICS_ATTRS(field, file)				\
 static ssize_t spi_master_##field##_show(struct device *dev,		\
 					 struct device_attribute *attr,	\
@@ -155,6 +168,15 @@ static const struct attribute_group spi_dev_group = {
 	.attrs  = spi_dev_attrs,
 };
 
+static struct attribute *spi_master_attrs[] = {
+	&dev_attr_num_chipselect.attr,
+	NULL,
+};
+
+static const struct attribute_group spi_master_group = {
+	.attrs  = spi_master_attrs,
+};
+
 static struct attribute *spi_device_statistics_attrs[] = {
 	&dev_attr_spi_device_messages.attr,
 	&dev_attr_spi_device_transfers.attr,
@@ -236,6 +258,7 @@ static const struct attribute_group spi_master_statistics_group = {
 };
 
 static const struct attribute_group *spi_master_groups[] = {
+	&spi_master_group,
 	&spi_master_statistics_group,
 	NULL,
 };
-- 
2.7.3

Re: [RFC 2/2] spi core: Add new sysfs 'num_chipselect' file

[Mark Brown]

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

On Mon, Apr 18, 2016 at 04:48:28PM +0530, Bhuvanchandra DV wrote:
> Add new sysfs 'num_chipselect' file to expose the maximum number
> of chipselects a SPI master can support.
> This allows to create a script in user space which automatically
> creates a new spidev instance for every chipselect on a bus.

This does not seem like a good idea, even if a chip select is supported
in the IP that doesn't mean it's brought out safely on a board and
pinmuxing may mean that the set of chip selects that can be used is
sparse.

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