[PATCH] spi: Add a sysfs interface to instantiate devices

[PATCH] spi: Add a sysfs interface to instantiate devices

[Kyle Roeschley]

Add a sysfs interface to instantiate and delete SPI devices using the
spidev driver. This can be used when developing a driver on a
self-soldered board which doesn't yet have proper SPI device declaration
at the platform level, and presumably for various debugging situations.

Inspired by 99cd8e25875a ("i2c: Add a sysfs interface to instantiate
devices").

Signed-off-by: Kyle Roeschley <kyle.roeschley-acOepvfBmUk@public.gmane.org>
---
 Documentation/spi/spi-summary | 14 ++++++++
 drivers/spi/spi.c             | 78 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/spi/spi.h       |  3 ++
 3 files changed, 95 insertions(+)

diff --git a/Documentation/spi/spi-summary b/Documentation/spi/spi-summary
index 1721c1b570c3..51d9747c4426 100644
--- a/Documentation/spi/spi-summary
+++ b/Documentation/spi/spi-summary
@@ -339,6 +339,20 @@ up the spi bus master, and will likely need spi_new_device() to provide the
 board info based on the board that was hotplugged.  Of course, you'd later
 call at least spi_unregister_device() when that board is removed.
 
+Alternatively, a sysfs interface was added to let the user create devices which
+using the spidev driver. This interface is made of 2 attribute files which are
+created in every SPI master directory: new_device and delete_device. Both files
+are write only and you must write the decimal SPI chip select number to them in
+order to properly instantiate or delete a SPI device. As no two devices can be
+attached to the same master with the same chip select line, the chip select
+number is sufficient to uniquely identify the device to be deleted.
+
+Example:
+# echo 1 > /sys/class/spi_master/spi0/new_device
+
+In general, this interface should only be used when in-kernel device
+declaration can't be done.
+
 When Linux includes support for MMC/SD/SDIO/DataFlash cards through SPI, those
 configurations will also be dynamic.  Fortunately, such devices all support
 basic device identification probes, so they should hotplug normally.
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index b33a727a0158..648ccdf359f9 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -242,8 +242,85 @@ static const struct attribute_group spi_controller_statistics_group = {
 	.attrs  = spi_controller_statistics_attrs,
 };
 
+static ssize_t
+new_device_store(struct device *dev, struct device_attribute *attr,
+		 const char *buf, size_t count)
+{
+	struct spi_controller *ctlr = container_of(dev, struct spi_controller,
+						   dev);
+	struct spi_device *spi;
+	struct spi_board_info bi = {
+		.modalias = "spidev",
+		.max_speed_hz = ctlr->max_speed_hz,
+	};
+
+	if (kstrtou16(buf, 0, &bi.chip_select) < 0)
+		return -EINVAL;
+
+	spi = spi_new_device(ctlr, &bi);
+	if (!spi) {
+		dev_err(dev, "can't create new device\n");
+		return -ENXIO;
+	}
+
+	mutex_lock(&ctlr->bus_lock_mutex);
+	list_add_tail(&spi->userspace_device, &ctlr->userspace_devices);
+	mutex_unlock(&ctlr->bus_lock_mutex);
+
+	dev_info(dev, "created spidev device %s\n", dev_name(&spi->dev));
+
+	return count;
+}
+static DEVICE_ATTR_WO(new_device);
+
+static ssize_t
+delete_device_store(struct device *dev, struct device_attribute *attr,
+		    const char *buf, size_t count)
+{
+	struct spi_controller *ctlr = container_of(dev, struct spi_controller,
+						   dev);
+	struct spi_device *spi, *next;
+	int ret = -ENXIO;
+	u16 cs;
+
+	if (kstrtou16(buf, 0, &cs) < 0)
+		return -EINVAL;
+
+	mutex_lock(&ctlr->bus_lock_mutex);
+	list_for_each_entry_safe(spi, next, &ctlr->userspace_devices,
+				 userspace_device) {
+		if (spi->chip_select != cs)
+			continue;
+
+		dev_info(dev, "deleting spidev device %s\n",
+			 dev_name(&spi->dev));
+		list_del(&spi->userspace_device);
+		spi_unregister_device(spi);
+		ret = count;
+		break;
+	}
+	mutex_unlock(&ctlr->bus_lock_mutex);
+
+	if (ret == -ENXIO)
+		dev_err(dev, "can't find spidev device %u in list\n", cs);
+
+	return ret;
+}
+static DEVICE_ATTR_WO(delete_device);
+
+static struct attribute *spi_controller_userspace_attrs[] = {
+	&dev_attr_new_device.attr,
+	&dev_attr_delete_device.attr,
+	NULL,
+};
+
+static const struct attribute_group spi_controller_userspace_group = {
+	.attrs  = spi_controller_userspace_attrs,
+};
+
 static const struct attribute_group *spi_master_groups[] = {
 	&spi_controller_statistics_group,
+	&spi_controller_userspace_group,
 	NULL,
 };
 
@@ -2129,6 +2206,7 @@ int spi_register_controller(struct spi_controller *ctlr)
 			return id;
 		ctlr->bus_num = id;
 	}
+	INIT_LIST_HEAD(&ctlr->userspace_devices);
 	INIT_LIST_HEAD(&ctlr->queue);
 	spin_lock_init(&ctlr->queue_lock);
 	spin_lock_init(&ctlr->bus_lock_spinlock);
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index bc6bb325d1bf..f7255745326d 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -172,6 +172,8 @@ struct spi_device {
 	/* the statistics */
 	struct spi_statistics	statistics;
 
+	struct list_head	userspace_device;
+
 	/*
 	 * likely need more hooks for more protocol options affecting how
 	 * the controller talks to each chip, like:
@@ -410,6 +412,7 @@ struct spi_controller {
 	struct device	dev;
 
 	struct list_head list;
+	struct list_head userspace_devices;
 
 	/* other than negative (== assign one dynamically), bus_num is fully
 	 * board-specific.  usually that simplifies to being SOC-specific.
-- 
2.15.1

--
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] spi: Add a sysfs interface to instantiate devices

[Randy Dunlap]

On 12/21/2017 12:03 PM, Kyle Roeschley wrote:
> Add a sysfs interface to instantiate and delete SPI devices using the
> spidev driver. This can be used when developing a driver on a
> self-soldered board which doesn't yet have proper SPI device declaration
> at the platform level, and presumably for various debugging situations.
> 
> Inspired by 99cd8e25875a ("i2c: Add a sysfs interface to instantiate
> devices").
> 
> Signed-off-by: Kyle Roeschley <kyle.roeschley-acOepvfBmUk@public.gmane.org>
> ---
>  Documentation/spi/spi-summary | 14 ++++++++
>  drivers/spi/spi.c             | 78 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/spi/spi.h       |  3 ++
>  3 files changed, 95 insertions(+)
> 
> diff --git a/Documentation/spi/spi-summary b/Documentation/spi/spi-summary
> index 1721c1b570c3..51d9747c4426 100644
> --- a/Documentation/spi/spi-summary
> +++ b/Documentation/spi/spi-summary
> @@ -339,6 +339,20 @@ up the spi bus master, and will likely need spi_new_device() to provide the
>  board info based on the board that was hotplugged.  Of course, you'd later
>  call at least spi_unregister_device() when that board is removed.
>  
> +Alternatively, a sysfs interface was added to let the user create devices which

                                                                             when

> +using the spidev driver. This interface is made of 2 attribute files which are
> +created in every SPI master directory: new_device and delete_device. Both files
> +are write only and you must write the decimal SPI chip select number to them in

       write-only

> +order to properly instantiate or delete a SPI device. As no two devices can be
> +attached to the same master with the same chip select line, the chip select
> +number is sufficient to uniquely identify the device to be deleted.
> +
> +Example:
> +# echo 1 > /sys/class/spi_master/spi0/new_device
> +
> +In general, this interface should only be used when in-kernel device
> +declaration can't be done.
> +
>  When Linux includes support for MMC/SD/SDIO/DataFlash cards through SPI, those
>  configurations will also be dynamic.  Fortunately, such devices all support
>  basic device identification probes, so they should hotplug normally.

thanks.
-- 
~Randy
--
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] spi: Add a sysfs interface to instantiate devices

[Trent Piepho]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 6174 bytes --]

On Thu, 2017-12-21 at 14:03 -0600, Kyle Roeschley wrote:
> Add a sysfs interface to instantiate and delete SPI devices using the
> spidev driver. This can be used when developing a driver on a
> self-soldered board which doesn't yet have proper SPI device declaration
> at the platform level, and presumably for various debugging situations.
> 
> Inspired by 99cd8e25875a ("i2c: Add a sysfs interface to instantiate
> devices").

The i2c interface allows one to specify the type of device to create. 
Why must this interface be linked to spidev and only capable of
creating spidev devices?  

> 
> Signed-off-by: Kyle Roeschley <kyle.roeschley@ni.com>
> ---
>  Documentation/spi/spi-summary | 14 ++++++++
>  drivers/spi/spi.c             | 78 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/spi/spi.h       |  3 ++
>  3 files changed, 95 insertions(+)
> 
> diff --git a/Documentation/spi/spi-summary b/Documentation/spi/spi-summary
> index 1721c1b570c3..51d9747c4426 100644
> --- a/Documentation/spi/spi-summary
> +++ b/Documentation/spi/spi-summary
> @@ -339,6 +339,20 @@ up the spi bus master, and will likely need spi_new_device() to provide the
>  board info based on the board that was hotplugged.  Of course, you'd later
>  call at least spi_unregister_device() when that board is removed.
>  
> +Alternatively, a sysfs interface was added to let the user create devices which
> +using the spidev driver. This interface is made of 2 attribute files which are
> +created in every SPI master directory: new_device and delete_device. Both files
> +are write only and you must write the decimal SPI chip select number to them in
> +order to properly instantiate or delete a SPI device. As no two devices can be
> +attached to the same master with the same chip select line, the chip select
> +number is sufficient to uniquely identify the device to be deleted.
> +
> +Example:
> +# echo 1 > /sys/class/spi_master/spi0/new_device
> +
> +In general, this interface should only be used when in-kernel device
> +declaration can't be done.
> +
>  When Linux includes support for MMC/SD/SDIO/DataFlash cards through SPI, those
>  configurations will also be dynamic.  Fortunately, such devices all support
>  basic device identification probes, so they should hotplug normally.
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index b33a727a0158..648ccdf359f9 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -242,8 +242,85 @@ static const struct attribute_group spi_controller_statistics_group = {
>  	.attrs  = spi_controller_statistics_attrs,
>  };
>  
> +static ssize_t
> +new_device_store(struct device *dev, struct device_attribute *attr,
> +		 const char *buf, size_t count)
> +{
> +	struct spi_controller *ctlr = container_of(dev, struct spi_controller,
> +						   dev);
> +	struct spi_device *spi;
> +	struct spi_board_info bi = {
> +		.modalias = "spidev",
> +		.max_speed_hz = ctlr->max_speed_hz,
> +	};
> +
> +	if (kstrtou16(buf, 0, &bi.chip_select) < 0)
> +		return -EINVAL;
> +
> +	spi = spi_new_device(ctlr, &bi);
> +	if (!spi) {
> +		dev_err(dev, "can't create new device\n");
> +		return -ENXIO;

I2C returns -EINVAL

> +	}
> +
> +	mutex_lock(&ctlr->bus_lock_mutex);
> +	list_add_tail(&spi->userspace_device, &ctlr->userspace_devices);
> +	mutex_unlock(&ctlr->bus_lock_mutex);
> +
> +	dev_info(dev, "created spidev device %s\n", dev_name(&spi->dev));
> +
> +	return count;
> +}
> +static DEVICE_ATTR_WO(new_device);
> +
> +static ssize_t
> +delete_device_store(struct device *dev, struct device_attribute *attr,
> +		    const char *buf, size_t count)
> +{
> +	struct spi_controller *ctlr = container_of(dev, struct spi_controller,
> +						   dev);
> +	struct spi_device *spi, *next;
> +	int ret = -ENXIO;
> +	u16 cs;
> +
> +	if (kstrtou16(buf, 0, &cs) < 0)
> +		return -EINVAL;
> +
> +	mutex_lock(&ctlr->bus_lock_mutex);
> +	list_for_each_entry_safe(spi, next, &ctlr->userspace_devices,
> +				 userspace_device) {
> +		if (spi->chip_select != cs)
> +			continue;
> +
> +		dev_info(dev, "deleting spidev device %s\n",
> +			 dev_name(&spi->dev));
> +		list_del(&spi->userspace_device);
> +		spi_unregister_device(spi);
> +		ret = count;
> +		break;
> +	}
> +	mutex_unlock(&ctlr->bus_lock_mutex);
> +
> +	if (ret == -ENXIO)
> +		dev_err(dev, "can't find spidev device %u in list\n", cs);
> +
> +	return ret;
> +}
> +static DEVICE_ATTR_WO(delete_device);
> +
> +static struct attribute *spi_controller_userspace_attrs[] = {
> +	&dev_attr_new_device.attr,
> +	&dev_attr_delete_device.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group spi_controller_userspace_group = {
> +	.attrs  = spi_controller_userspace_attrs,
> +};
> +
>  static const struct attribute_group *spi_master_groups[] = {
>  	&spi_controller_statistics_group,
> +	&spi_controller_userspace_group,
>  	NULL,
>  };
>  
> @@ -2129,6 +2206,7 @@ int spi_register_controller(struct spi_controller *ctlr)
>  			return id;
>  		ctlr->bus_num = id;
>  	}
> +	INIT_LIST_HEAD(&ctlr->userspace_devices);
>  	INIT_LIST_HEAD(&ctlr->queue);
>  	spin_lock_init(&ctlr->queue_lock);
>  	spin_lock_init(&ctlr->bus_lock_spinlock);
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index bc6bb325d1bf..f7255745326d 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -172,6 +172,8 @@ struct spi_device {
>  	/* the statistics */
>  	struct spi_statistics	statistics;
>  
> +	struct list_head	userspace_device;
> +
>  	/*
>  	 * likely need more hooks for more protocol options affecting how
>  	 * the controller talks to each chip, like:
> @@ -410,6 +412,7 @@ struct spi_controller {
>  	struct device	dev;
>  
>  	struct list_head list;
> +	struct list_head userspace_devices;
>  
>  	/* other than negative (== assign one dynamically), bus_num is fully
>  	 * board-specific.  usually that simplifies to being SOC-specific.N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±²˜¢žØ^n‡r¡ö¦zË\x1aëh™¨è­Ú&¢îý»\x05ËÛÔØï¦v¬Îf\x1dp)¹¹br	šê+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹\x1e®w¥¢¸?™¨è­Ú&¢)ߢ^[f

Re: [PATCH] spi: Add a sysfs interface to instantiate devices

[Mark Brown]

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

On Thu, Dec 21, 2017 at 09:05:43PM +0000, Trent Piepho wrote:
> On Thu, 2017-12-21 at 14:03 -0600, Kyle Roeschley wrote:
> > Add a sysfs interface to instantiate and delete SPI devices using the
> > spidev driver. This can be used when developing a driver on a
> > self-soldered board which doesn't yet have proper SPI device declaration
> > at the platform level, and presumably for various debugging situations.

> > Inspired by 99cd8e25875a ("i2c: Add a sysfs interface to instantiate
> > devices").

> The i2c interface allows one to specify the type of device to create. 
> Why must this interface be linked to spidev and only capable of
> creating spidev devices?  

Right, that doesn't seem good.  I also can't see anything in the actual
code which suggests that this is tied to spidev except the log messages.

> > +						   dev);
> > +	struct spi_device *spi, *next;
> > +	int ret = -ENXIO;
> > +	u16 cs;

Please delete unneeded context from mails when replying.  Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.

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

Re: [PATCH] spi: Add a sysfs interface to instantiate devices

[Kyle Roeschley]

On Fri, Dec 22, 2017 at 03:56:03PM +0000, Mark Brown wrote:
> On Thu, Dec 21, 2017 at 09:05:43PM +0000, Trent Piepho wrote:
> > On Thu, 2017-12-21 at 14:03 -0600, Kyle Roeschley wrote:
> > > Add a sysfs interface to instantiate and delete SPI devices using the
> > > spidev driver. This can be used when developing a driver on a
> > > self-soldered board which doesn't yet have proper SPI device declaration
> > > at the platform level, and presumably for various debugging situations.
> 
> > > Inspired by 99cd8e25875a ("i2c: Add a sysfs interface to instantiate
> > > devices").
> 
> > The i2c interface allows one to specify the type of device to create.
> > Why must this interface be linked to spidev and only capable of
> > creating spidev devices?
> 
> Right, that doesn't seem good.  I also can't see anything in the actual
> code which suggests that this is tied to spidev except the log messages.
> 

Quoting Geert's email [1] on the subject:

> To me, the above sounds a bit contradictive: either you have
>   1. a simple (trivial) description, which can be handled by spidev and
>      userspace, and thus by just writing "<unit-addr> spidev" to a new_device
>      sysfs node, or
>   2. a complex description, for which you need a specialized in-kernel driver,
>      so you're gonna need a real DT node (and overlays?) to describe it.
> 
> I don't think writing a complex description to a new_device sysfs node makes
> sense.

And regarding not being linked to spidev, see modalias in new_device_store:

> > > +	struct spi_board_info bi = {
> > > +		.modalias = "spidev",
> > > +		.max_speed_hz = ctlr->max_speed_hz,
> > > +	};

[1] https://marc.info/?l=linux-spi&m=151199390921251&w=2

Happy holidays,

-- 
Kyle Roeschley
Software Engineer
National Instruments
--
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] spi: Add a sysfs interface to instantiate devices

[Geert Uytterhoeven]

Hi Kyle,

On Fri, Dec 22, 2017 at 6:11 PM, Kyle Roeschley <kyle.roeschley-acOepvfBmUk@public.gmane.org> wrote:
> On Fri, Dec 22, 2017 at 03:56:03PM +0000, Mark Brown wrote:
>> On Thu, Dec 21, 2017 at 09:05:43PM +0000, Trent Piepho wrote:
>> > On Thu, 2017-12-21 at 14:03 -0600, Kyle Roeschley wrote:
>> > > Add a sysfs interface to instantiate and delete SPI devices using the
>> > > spidev driver. This can be used when developing a driver on a
>> > > self-soldered board which doesn't yet have proper SPI device declaration
>> > > at the platform level, and presumably for various debugging situations.
>>
>> > > Inspired by 99cd8e25875a ("i2c: Add a sysfs interface to instantiate
>> > > devices").
>>
>> > The i2c interface allows one to specify the type of device to create.
>> > Why must this interface be linked to spidev and only capable of
>> > creating spidev devices?
>>
>> Right, that doesn't seem good.  I also can't see anything in the actual
>> code which suggests that this is tied to spidev except the log messages.
>
> Quoting Geert's email [1] on the subject:
>
>> To me, the above sounds a bit contradictive: either you have
>>   1. a simple (trivial) description, which can be handled by spidev and
>>      userspace, and thus by just writing "<unit-addr> spidev" to a new_device

Note the "spidev" in the string written...

>>      sysfs node, or
>>   2. a complex description, for which you need a specialized in-kernel driver,
>>      so you're gonna need a real DT node (and overlays?) to describe it.
>>
>> I don't think writing a complex description to a new_device sysfs node makes
>> sense.
>
> And regarding not being linked to spidev, see modalias in new_device_store:
>
>> > > + struct spi_board_info bi = {
>> > > +         .modalias = "spidev",

I would make it a little bit more generic and extract the modalias from the
string written.

>> > > +         .max_speed_hz = ctlr->max_speed_hz,
>> > > + };
>
> [1] https://marc.info/?l=linux-spi&m=151199390921251&w=2

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
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] spi: Add a sysfs interface to instantiate devices

[Mark Brown]

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

On Sat, Dec 23, 2017 at 09:58:51AM +0100, Geert Uytterhoeven wrote:

> >> > > + struct spi_board_info bi = {
> >> > > +         .modalias = "spidev",

> I would make it a little bit more generic and extract the modalias from the
> string written.

Right, that'd be much better.

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