Skip to contentSkip to conversation

Allow specifying properties with spi_register_board_info

PATCH Superseded Dmitry Torokhov 5 participants 12 messages Feb 28, 2017 SPI

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Dmitry Torokhov

author
#0 lore raw

v2 cover Allow specifying properties with spi_register_board_info

I am trying to convert some input drivers to use only generic device properties, and get rid of legacy platform data. For that I need to be able to attach property lists to devices before they are registered, and quite a few boards use spi_register_board_info() to create their devices.

v2: change patch #2 from kmemdup to kmalloc + copy as they are different
data types.

v1: initial posting

Dmitry Torokhov (3):
spi: allow attaching device properties to SPI board info
spi: allocate spi_board_info entries one by one
spi: allow registering empty spi_board_info lists

 drivers/spi/spi.c       | 40 ++++++++++++++++++++++++++++------------
 include/linux/spi/spi.h |  4 ++++
 2 files changed, 32 insertions(+), 12 deletions(-)
Signature · 1 line
Dmitry
author
#1 lore raw

PATCH v2 1/3 spi: allow attaching device properties to SPI board info

Generic device properties support statically defined property sets. For them to be usable, we need to attach these property sets before devices are registered and probed. Allowing to attach property list to spi_board_info structure will allow non-ACPI non-DT boards switch to using generic properties and get rid of custom platform data.

Signed-off-by Dmitry Torokhov <[email protected]>
---
 drivers/spi/spi.c       | 27 +++++++++++++++++++++++----
 include/linux/spi/spi.h |  4 ++++
 2 files changed, 27 insertions(+), 4 deletions(-)
drivers/spi/spi.c +23 -4
index 656dd3e3220c..b948d8cdbace 100644
@@ -31,6 +31,7 @@
 #include <linux/of_gpio.h>
 #include <linux/pm_runtime.h>
 #include <linux/pm_domain.h>
+#include <linux/property.h>
 #include <linux/export.h>
 #include <linux/sched/rt.h>
 #include <linux/delay.h>

@@ -599,13 +600,25 @@ struct spi_device *spi_new_device(struct spi_master *master,
 	proxy->controller_data = chip->controller_data;
 	proxy->controller_state = NULL;
 
-	status = spi_add_device(proxy);
-	if (status < 0) {
-		spi_dev_put(proxy);
-		return NULL;
+	if (chip->properties) {
+		status = device_add_properties(&proxy->dev, chip->properties);
+		if (status) {
+			dev_err(&master->dev,
+				"failed to add properties to '%s': %d\n",
+				chip->modalias, status);
+			goto err_out;
+		}
 	}
 
+	status = spi_add_device(proxy);
+	if (status < 0)
+		goto err_out;
+
 	return proxy;
+
+err_out:
+	spi_dev_put(proxy);
+	return NULL;
 }
 EXPORT_SYMBOL_GPL(spi_new_device);
 

@@ -661,6 +674,7 @@ static void spi_match_master_to_boardinfo(struct spi_master *master,
  *
  * The board info passed can safely be __initdata ... but be careful of
  * any embedded pointers (platform_data, etc), they're copied as-is.
+ * Device properties are deep-copied though.
  *
  * Return: zero on success, else a negative error code.
  */

@@ -680,6 +694,11 @@ int spi_register_board_info(struct spi_board_info const *info, unsigned n)
 		struct spi_master *master;
 
 		memcpy(&bi->board_info, info, sizeof(*info));
+		bi->board_info.properties =
+			property_entries_dup(info->properties);
+		if (IS_ERR(bi->board_info.properties))
+			return PTR_ERR(bi->board_info.properties);
+
 		mutex_lock(&board_lock);
 		list_add_tail(&bi->list, &board_list);
 		list_for_each_entry(master, &spi_master_list, list)
include/linux/spi/spi.h +4 -0
index 75c6bd0ac605..5a8c4b24f2dc 100644
@@ -23,6 +23,7 @@
 #include <linux/scatterlist.h>
 
 struct dma_chan;
+struct property_entry;
 struct spi_master;
 struct spi_transfer;
 struct spi_flash_read_message;

@@ -1209,6 +1210,7 @@ int spi_flash_read(struct spi_device *spi,
  * @modalias: Initializes spi_device.modalias; identifies the driver.
  * @platform_data: Initializes spi_device.platform_data; the particular
  *	data stored there is driver-specific.
+ * @properties: Additional device properties for the device.
  * @controller_data: Initializes spi_device.controller_data; some
  *	controllers need hints about hardware setup, e.g. for DMA.
  * @irq: Initializes spi_device.irq; depends on how the board is wired.

@@ -1241,10 +1243,12 @@ struct spi_board_info {
 	 *
 	 * platform_data goes to spi_device.dev.platform_data,
 	 * controller_data goes to spi_device.controller_data,
+	 * device properties are copied and attached to spi_device,
 	 * irq is copied too
 	 */
 	char		modalias[SPI_NAME_SIZE];
 	const void	*platform_data;
+	const struct property_entry *properties;
 	void		*controller_data;
 	int		irq;
Signature · 1 line
2.11.0.483.g087da7b7c-goog
author
#2 lore raw

PATCH v2 2/3 spi: allocate spi_board_info entries one by one

In reply to Dmitry Torokhov

Lists of spi_board_info entries registered with spi_register_board_info() can be quite long; instead of forcing memory allocator find contagious chunk of memory, let;s allocate them one-by-one, so they can be packed as needed.

Signed-off-by Dmitry Torokhov <[email protected]>
---
 drivers/spi/spi.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/spi/spi.c +6 -6
index b948d8cdbace..1b66e0497327 100644
@@ -686,14 +686,14 @@ int spi_register_board_info(struct spi_board_info const *info, unsigned n)
 	if (!n)
 		return -EINVAL;
 
-	bi = kzalloc(n * sizeof(*bi), GFP_KERNEL);
-	if (!bi)
-		return -ENOMEM;
-
-	for (i = 0; i < n; i++, bi++, info++) {
+	for (i = 0; i < n; i++, info++) {
 		struct spi_master *master;
 
-		memcpy(&bi->board_info, info, sizeof(*info));
+		bi = kmalloc(sizeof(*bi), GFP_KERNEL);
+		if (!bi)
+			return -ENOMEM;
+
+		bi->board_info = *info;
 		bi->board_info.properties =
 			property_entries_dup(info->properties);
 		if (IS_ERR(bi->board_info.properties))
Signature · 1 line
2.11.0.483.g087da7b7c-goog
maintainer
#3 lore raw

Re: [PATCH v2 2/3] spi: allocate spi_board_info entries one by one

Quoted 2 lines · Dmitry Torokhov

Lists of spi_board_info entries registered with spi_register_board_info() can be quite long; instead of forcing memory allocator find contagious

Do you have numbers on that?

author
#4 lore raw

Re: [PATCH v2 2/3] spi: allocate spi_board_info entries one by one

Quoted 5 lines · Mark Brown
Quoted 2 lines · Dmitry Torokhov

Lists of spi_board_info entries registered with spi_register_board_info() can be quite long; instead of forcing memory allocator find contagious

Do you have numbers on that?

Hm, so the largest array seems to be in arch/blackfin/mach-bf537/boards/stamp.c at max of 43 entries. The new board info is ether 60 or 72 bytes, so we get 2 or 3K table. Not above page, but still could be packed I think.

If we decide that we want to keep single chunk I'll just change the allocation to kcalloc. Let me know.

We should probably redo patch #1 to avoid allocating empty property sets anyway.

Thanks.

Signature · 5 lines
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
maintainer
#5 lore raw

Re: [PATCH v2 2/3] spi: allocate spi_board_info entries one by one

Quoted 11 lines · Dmitry Torokhov
Quoted 5 lines · Mark Brown
Quoted 2 lines · Dmitry Torokhov

Lists of spi_board_info entries registered with spi_register_board_info() can be quite long; instead of forcing memory allocator find contagious

Do you have numbers on that?

Hm, so the largest array seems to be in arch/blackfin/mach-bf537/boards/stamp.c at max of 43 entries. The new board info is ether 60 or 72 bytes, so we get 2 or 3K table. Not above page, but still could be packed I think.

Oh wow, that's impressively large. Still not sure the optimization is particularly worth it though, it's small change in the grand scheme of things. OTOH it's a small change.

Quoted 2 lines

If we decide that we want to keep single chunk I'll just change the allocation to kcalloc. Let me know.

I'd be inclined to do that because it requires less thinking about the value of what should be a very small optimization either way but whatever :)

Quoted 2 lines

We should probably redo patch #1 to avoid allocating empty property sets anyway.

Yeah.

#6 lore raw

Re: [PATCH v2 2/3] spi: allocate spi_board_info entries one by one

Quoted 8 lines · Mark Brown
Quoted 6 lines · Dmitry Torokhov
Quoted 5 lines · Mark Brown
Quoted 2 lines · Dmitry Torokhov

Lists of spi_board_info entries registered with spi_register_board_info() can be quite long; instead of forcing memory allocator find contagious

Do you have numbers on that?

Quoted 8 lines
Quoted 4 lines

Hm, so the largest array seems to be in arch/blackfin/mach-bf537/boards/stamp.c at max of 43 entries. The new board info is ether 60 or 72 bytes, so we get 2 or 3K table. Not above page, but still could be packed I think.

Oh wow, that's impressively large. Still not sure the optimization is particularly worth it though, it's small change in the grand scheme of things. OTOH it's a small change.

Given this is done during early boot, what's the probability of not having sufficient contiguous memory?

Quoted 6 lines
Quoted 2 lines

If we decide that we want to keep single chunk I'll just change the allocation to kcalloc. Let me know.

I'd be inclined to do that because it requires less thinking about the value of what should be a very small optimization either way but whatever :)

Tada...

commit f9bdb7fdd2cac17bdc9c344b6036e6939fa087cd

Author: Markus Elfring <[email protected]>
Date:   Fri Jan 13 12:28:04 2017 +0100

spi: Use kcalloc() in spi_register_board_info()

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#7 lore raw

Re: [PATCH v2 2/3] spi: allocate spi_board_info entries one by one

Quoted 28 lines · Geert Uytterhoeven
Quoted 8 lines · Mark Brown
Quoted 6 lines · Dmitry Torokhov
Quoted 5 lines · Mark Brown
Quoted 2 lines · Dmitry Torokhov

Lists of spi_board_info entries registered with spi_register_board_info() can be quite long; instead of forcing memory allocator find contagious

Do you have numbers on that?

Quoted 8 lines
Quoted 4 lines

Hm, so the largest array seems to be in arch/blackfin/mach-bf537/boards/stamp.c at max of 43 entries. The new board info is ether 60 or 72 bytes, so we get 2 or 3K table. Not above page, but still could be packed I think.

Oh wow, that's impressively large. Still not sure the optimization is particularly worth it though, it's small change in the grand scheme of things. OTOH it's a small change.

Given this is done during early boot, what's the probability of not having sufficient contiguous memory?

Quoted 6 lines
Quoted 2 lines

If we decide that we want to keep single chunk I'll just change the allocation to kcalloc. Let me know.

I'd be inclined to do that because it requires less thinking about the value of what should be a very small optimization either way but whatever :)

Quoted 7 lines

Tada...

commit f9bdb7fdd2cac17bdc9c344b6036e6939fa087cd

Author: Markus Elfring <[email protected]>
Date:   Fri Jan 13 12:28:04 2017 +0100

spi: Use kcalloc() in spi_register_board_info()

Or even kmalloc_array() as zeroing out memory is not needed. But I'll let Mark sort it out and drop my patch #2.

Thanks.

Signature · 1 line
Dmitry
#8 lore raw

Re: [PATCH v2 2/3] spi: allocate spi_board_info entries one by one

In reply to Mark Brown
Quoted 8 lines · Mark Brown
Quoted 6 lines · Dmitry Torokhov
Quoted 5 lines · Mark Brown
Quoted 2 lines · Dmitry Torokhov

Lists of spi_board_info entries registered with spi_register_board_info() can be quite long; instead of forcing memory allocator find contagious

Do you have numbers on that?

Quoted 8 lines
Quoted 4 lines

Hm, so the largest array seems to be in arch/blackfin/mach-bf537/boards/stamp.c at max of 43 entries. The new board info is ether 60 or 72 bytes, so we get 2 or 3K table. Not above page, but still could be packed I think.

Oh wow, that's impressively large. Still not sure the optimization is particularly worth it though, it's small change in the grand scheme of things. OTOH it's a small change.

The Blackfin machine files shouldn't be used as valid example. While in theory it is possible to build a kernel with that many entries nobody is ever going to do that. What the Blackfin machine files do is basically a poor man's overlays from the days before overlays existed. Most of the entries in the table use the same chip-select pins, it wouldn't be possible to use use a setup where more than two or three of the entries is enabled at the same time.

author
#9 lore raw

PATCH v2 3/3 spi: allow registering empty spi_board_info lists

In reply to Dmitry Torokhov

Many boards form list of spi_board_info entries depending on config, and it is possible to end up with empty list. Do not report error in such cases.

Signed-off-by Dmitry Torokhov <[email protected]>
---
 drivers/spi/spi.c | 3 ---
 1 file changed, 3 deletions(-)
drivers/spi/spi.c +0 -3
index 1b66e0497327..dbc4f00dc2fc 100644
@@ -683,9 +683,6 @@ int spi_register_board_info(struct spi_board_info const *info, unsigned n)
 	struct boardinfo *bi;
 	int i;
 
-	if (!n)
-		return -EINVAL;
-
 	for (i = 0; i < n; i++, info++) {
 		struct spi_master *master;
Signature · 1 line
2.11.0.483.g087da7b7c-goog
Reviewed-by on v2 3/3
#10 lore raw

Re: [PATCH v2 3/3] spi: allow registering empty spi_board_info lists

Quoted 3 lines · Dmitry Torokhov

Many boards form list of spi_board_info entries depending on config, and it is possible to end up with empty list. Do not report error in such cases.

If there is an actual case already it would be better to make the patch first in the series with Cc: stable@.

FWIW:

Reviewed-by Andy Shevchenko <[email protected]>
Signature · 6 lines
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#11 lore raw

Re: [PATCH v2 3/3] spi: allow registering empty spi_board_info lists

Quoted 8 lines · Andy Shevchenko
Quoted 3 lines · Dmitry Torokhov

Many boards form list of spi_board_info entries depending on config, and it is possible to end up with empty list. Do not report error in such cases.

If there is an actual case already it would be better to make the patch first in the series with Cc: stable@.

arch/blackfin/mach-bf533/boards/ezkit.c: you can get config convoluted enough so that you'll end up with empty array. But nobody checks the result of the call, so the failure is not visible. There is no need to cc stable@ for this.

Quoted 4 lines

FWIW:

Reviewed-by Andy Shevchenko <[email protected]>

Thanks.

Signature · 1 line
Dmitry