Export SPI and OF module aliases in missing drivers

The SPI core always reports the MODALIAS uevent as "spi:<modalias>" regardless of the mechanism that was used to register the device (i.e: OF or board code) and the table that is used later to match the driver with the device (i.e: SPI id table or OF match table).

So drivers needs to export the SPI id table and this be built into the module or udev won't have the necessary information to autoload the needed driver module when the device is added.

Signed-off-by Javier Martinez Canillas <[email protected]>

Quoted 5 lines · Javier Martinez Canillas

The SPI core always reports the MODALIAS uevent as "spi:<modalias>" regardless of the mechanism that was used to register the device (i.e: OF or board code) and the table that is used later to match the driver with the device (i.e: SPI id table or OF match table).

Quoted 5 lines

So drivers needs to export the SPI id table and this be built into the module or udev won't have the necessary information to autoload the needed driver module when the device is added.

Signed-off-by Javier Martinez Canillas <[email protected]>

Acked-by Lars-Peter Clausen <[email protected]>

Thanks.

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

The SPI core always reports the MODALIAS uevent as "spi:<modalias>" regardless of the mechanism that was used to register the device (i.e: OF or board code) and the table that is used later to match the driver with the device (i.e: SPI id table or OF match table).

So drivers needs to export the SPI id table and this be built into the module or udev won't have the necessary information to autoload the needed driver module when the device is added.

Signed-off-by Javier Martinez Canillas <[email protected]>

---

 drivers/staging/iio/magnetometer/hmc5843_spi.c | 1 +
 1 file changed, 1 insertion(+)

drivers/staging/iio/magnetometer/hmc5843_spi.c +1 -0

index 8e658f736e1f..4dfb372d2fec 100644
@@ -81,6 +81,7 @@ static const struct spi_device_id hmc5843_id[] = {
 	{ "hmc5983", HMC5983_ID },
 	{ }
 };
+MODULE_DEVICE_TABLE(spi, hmc5843_id);
 
 static struct spi_driver hmc5843_driver = {
 		.driver = {

Quoted 9 lines · Javier Martinez Canillas

The SPI core always reports the MODALIAS uevent as "spi:<modalias>" regardless of the mechanism that was used to register the device (i.e: OF or board code) and the table that is used later to match the driver with the device (i.e: SPI id table or OF match table).

So drivers needs to export the SPI id table and this be built into the module or udev won't have the necessary information to autoload the needed driver module when the device is added.

Quoted 1 line

Signed-off-by Javier Martinez Canillas <[email protected]>

Applied to the togreg branch of iio.git.

This is too late for the upcoming merge window so it will be queued up for the next one.

Thanks,

Jonathan

Hello Jonathan,

Quoted 13 lines · Jonathan Cameron

Quoted 10 lines · Javier Martinez Canillas

The SPI core always reports the MODALIAS uevent as "spi:<modalias>" regardless of the mechanism that was used to register the device (i.e: OF or board code) and the table that is used later to match the driver with the device (i.e: SPI id table or OF match table).

So drivers needs to export the SPI id table and this be built into the module or udev won't have the necessary information to autoload the needed driver module when the device is added.

Signed-off-by Javier Martinez Canillas <[email protected]>

Applied to the togreg branch of iio.git.

Quoted 3 lines

This is too late for the upcoming merge window so it will be queued up for the next one.

IMHO this patch and "[PATCH 01/18] iio: Export SPI module alias information in missing drivers" [0] are fixing broken module autoloading which are bugs so are material for the 4.3 -rc cycle.

Quoted 4 lines

Thanks,

Jonathan

[0]: https://lkml.org/lkml/2015/8/20/111

Best regards,

Quoted 16 lines · Javier Martinez Canillas

Hello Jonathan,

Quoted 13 lines · Jonathan Cameron

Quoted 10 lines · Javier Martinez Canillas

The SPI core always reports the MODALIAS uevent as "spi:<modalias>" regardless of the mechanism that was used to register the device (i.e: OF or board code) and the table that is used later to match the driver with the device (i.e: SPI id table or OF match table).

So drivers needs to export the SPI id table and this be built into the module or udev won't have the necessary information to autoload the needed driver module when the device is added.

Signed-off-by Javier Martinez Canillas <[email protected]>

Applied to the togreg branch of iio.git.

Quoted 7 lines

Quoted 3 lines

This is too late for the upcoming merge window so it will be queued up for the next one.

IMHO this patch and "[PATCH 01/18] iio: Export SPI module alias information in missing drivers" [0] are fixing broken module autoloading which are bugs so are material for the 4.3 -rc cycle.

It's a corner case. Could also be argued that this isn't a bug but rather a case of a feature (autoprobing) being added that wasn't supported before. It's not obligatory to support autoloading (even if we would normally aim to do so).

I'm happy enough for a request to apply these to stable occurs after they hit Linus' tree (as a trivial backport) but I don't think they really deserve being sent on as fixes.

Of course, I might be missing something that means something is actually broken, as opposed to not present.

Jonathan

The SPI core always reports the MODALIAS uevent as "spi:<modalias>" regardless of the mechanism that was used to register the device (i.e: OF or board code) and the table that is used later to match the driver with the device (i.e: SPI id table or OF match table).

So drivers needs to export the SPI id table and this be built into the module or udev won't have the necessary information to autoload the needed driver module when the device is added.

But this means that OF-only drivers needs to have both OF and SPI id tables that have to be kept in sync and also the dev node compatible manufacturer prefix is stripped when reporting the MODALIAS. Which can lead to issues if two vendors use the same SPI device name for example.

To avoid the above, the SPI core behavior may be changed in the future to not require an SPI device table for OF-only drivers and report the OF module alias. So, it's better to also export the OF table even when is unused now to prevent breaking module loading when the core changes.

Signed-off-by Javier Martinez Canillas <[email protected]>

---

 drivers/mtd/devices/mtd_dataflash.c | 1 +
 1 file changed, 1 insertion(+)

drivers/mtd/devices/mtd_dataflash.c +0 -0

index 0099aba72a8b..df6f61137376 100644

Quoted 14 lines · Javier Martinez Canillas

The SPI core always reports the MODALIAS uevent as "spi:<modalias>" regardless of the mechanism that was used to register the device (i.e: OF or board code) and the table that is used later to match the driver with the device (i.e: SPI id table or OF match table).

So drivers needs to export the SPI id table and this be built into the module or udev won't have the necessary information to autoload the needed driver module when the device is added.

But this means that OF-only drivers needs to have both OF and SPI id tables that have to be kept in sync and also the dev node compatible manufacturer prefix is stripped when reporting the MODALIAS. Which can lead to issues if two vendors use the same SPI device name for example.

Quoted 6 lines

To avoid the above, the SPI core behavior may be changed in the future to not require an SPI device table for OF-only drivers and report the OF module alias. So, it's better to also export the OF table even when is unused now to prevent breaking module loading when the core changes.

Signed-off-by Javier Martinez Canillas <[email protected]>

I appreciate the context for the whole problem here, but this commit subject mostly doesn't apply to this driver, and it deson't seem to match the problem this is solving. As I see it, the patch description for this patch should be more like:

"The OF ID table is used for auto-probing the device, but it is not exported to the module device table. That means this driver won't autoload when built as a module.

Export the OF ID table to fix that.

(insert some reference to the larger issue here)"

Do you want to rewrite the message, shall I just prepend my modifications, or am I off-base?

So this patch really is a no-op right now and is not fixing anything. As long as the driver has a MODULE_DEVICE_TABLE(spi,...), things will

work without a MODULE_DEVICE_TABLE(of,...).

Quoted 2 lines · Javier Martinez Canillas

So this patch really is a no-op right now and is not fixing anything. As long as the driver has a MODULE_DEVICE_TABLE(spi,...), things will

This driver does not have a MODULE_DEVICE_TABLE() at all, nor does it use spi_device_id. So, it is currently broken for the module use case.

Quoted 1 line

work without a MODULE_DEVICE_TABLE(of,...).

Brian

Quoted 4 lines · Brian Norris

Quoted 2 lines · Javier Martinez Canillas

So this patch really is a no-op right now and is not fixing anything. As long as the driver has a MODULE_DEVICE_TABLE(spi,...), things will

Quoted 3 lines

This driver does not have a MODULE_DEVICE_TABLE() at all, nor does it use spi_device_id. So, it is currently broken for the module use case.

That is correct but also that does not mean that this patch will fix module autoloading right now. It will though once the SPI core is changed to report a OF type module alias.

So what about something along this lines?

mtd: dataflash: Export OF module alias information

The SPI core currently reports the MODALIAS uevent as "spi:<modalias>" even for SPI devices that were registered by OF.

That means the OF module alias exported by MODULE_OF_TABLE(of,...) is currently not used and user-space has no way to autoload this module.

But is still a good practice to add the OF module alias information

into the kernel module even when currently is unused so once the SPI

core is changed to report a correct OF modalias uevent, module autoloading will be working for this driver.

Hi Javier,

Quoted 9 lines · Javier Martinez Canillas

Quoted 7 lines · Brian Norris

Quoted 2 lines · Javier Martinez Canillas

So this patch really is a no-op right now and is not fixing anything. As long as the driver has a MODULE_DEVICE_TABLE(spi,...), things will

This driver does not have a MODULE_DEVICE_TABLE() at all, nor does it use spi_device_id. So, it is currently broken for the module use case.

Quoted 3 lines

That is correct but also that does not mean that this patch will fix module autoloading right now. It will though once the SPI core is changed to report a OF type module alias.

Ah, thanks for the patience. I missed the point that it will still be broken.

Quoted 11 lines

So what about something along this lines?

mtd: dataflash: Export OF module alias information

The SPI core currently reports the MODALIAS uevent as "spi:<modalias>" even for SPI devices that were registered by OF.

That means the OF module alias exported by MODULE_OF_TABLE(of,...) is currently not used and user-space has no way to autoload this module.

But is still a good practice to add the OF module alias information

nit: s/is/it is/

Quoted 1 line

into the kernel module even when currently is unused so once the SPI

ditto

Quoted 2 lines

core is changed to report a correct OF modalias uevent, module autoloading will be working for this driver.

Otherwise, looks good. I'll either patch in this commit message, or await v2. Your call.

Regards,
Brian

Quoted 2 lines · Brian Norris

Otherwise, looks good. I'll either patch in this commit message, or await v2. Your call.

If you don't mind fixing it up yourself when applying, then that would be really helpful. Otherwise I can post a v2 on Monday.

Quoted 6 lines · Javier Martinez Canillas

Quoted 2 lines · Brian Norris

Otherwise, looks good. I'll either patch in this commit message, or await v2. Your call.

If you don't mind fixing it up yourself when applying, then that would be really helpful. Otherwise I can post a v2 on Monday.

Queued to l2-mtd.git/next (for 4.4), with the edited commit message.

The SPI core always reports the MODALIAS uevent as "spi:<modalias>" regardless of the mechanism that was used to register the device (i.e: OF or board code) and the table that is used later to match the driver with the device (i.e: SPI id table or OF match table).

So drivers needs to export the SPI id table and this be built into the module or udev won't have the necessary information to autoload the needed driver module when the device is added.

But this means that OF-only drivers needs to have both OF and SPI id tables that have to be kept in sync and also the dev node compatible manufacturer prefix is stripped when reporting the MODALIAS. Which can lead to issues if two vendors use the same SPI device name for example.

To avoid the above, the SPI core behavior may be changed in the future to not require an SPI device table for OF-only drivers and report the OF module alias. So, it's better to also export the OF table even when is unused now to prevent breaking module loading when the core changes.

Signed-off-by Javier Martinez Canillas <[email protected]>

---

 drivers/media/i2c/s5c73m3/s5c73m3-spi.c | 1 +
 1 file changed, 1 insertion(+)

drivers/media/i2c/s5c73m3/s5c73m3-spi.c +0 -0

index fa4a5ebda6b2..9983635ec253 100644

Hello,

Quoted 35 lines · Javier Martinez Canillas

The SPI core always reports the MODALIAS uevent as "spi:<modalias>" regardless of the mechanism that was used to register the device (i.e: OF or board code) and the table that is used later to match the driver with the device (i.e: SPI id table or OF match table).

So drivers needs to export the SPI id table and this be built into the module or udev won't have the necessary information to autoload the needed driver module when the device is added.

But this means that OF-only drivers needs to have both OF and SPI id tables that have to be kept in sync and also the dev node compatible manufacturer prefix is stripped when reporting the MODALIAS. Which can lead to issues if two vendors use the same SPI device name for example.

To avoid the above, the SPI core behavior may be changed in the future to not require an SPI device table for OF-only drivers and report the OF module alias. So, it's better to also export the OF table even when is unused now to prevent breaking module loading when the core changes.

Signed-off-by Javier Martinez Canillas <[email protected]>

---

 drivers/media/i2c/s5c73m3/s5c73m3-spi.c | 1 +
 1 file changed, 1 insertion(+)

drivers/media/i2c/s5c73m3/s5c73m3-spi.c +1 -0

index fa4a5ebda6b2..9983635ec253 100644
@@ -31,6 +31,7 @@ static const struct of_device_id s5c73m3_spi_ids[] = {
 	{ .compatible = "samsung,s5c73m3" },
 	{ }
 };
+MODULE_DEVICE_TABLE(of, s5c73m3_spi_ids;);
 

drivers/media/i2c/s5c73m3/s5c73m3-spi.c static const struct of_device_id s5c73m3_spi_ids[] = { · line 37

Quoted 3 lines

drivers/media/i2c/s5c73m3/s5c73m3-spi.c +0 -0

 enum spi_direction {
 	SPI_DIR_RX,

Any comments about this patch?

Best regards,

Hello,

Quoted 25 lines · Javier Martinez Canillas

The SPI core always reports the MODALIAS uevent as "spi:<modalias>" regardless of the mechanism that was used to register the device (i.e: OF or board code) and the table that is used later to match the driver with the device (i.e: SPI id table or OF match table).

So drivers needs to export the SPI id table and this be built into the module or udev won't have the necessary information to autoload the needed driver module when the device is added.

But this means that OF-only drivers needs to have both OF and SPI id tables that have to be kept in sync and also the dev node compatible manufacturer prefix is stripped when reporting the MODALIAS. Which can lead to issues if two vendors use the same SPI device name for example.

To avoid the above, the SPI core behavior may be changed in the future to not require an SPI device table for OF-only drivers and report the OF module alias. So, it's better to also export the OF table even when is unused now to prevent breaking module loading when the core changes.

Signed-off-by Javier Martinez Canillas <[email protected]>

---

 drivers/media/i2c/s5c73m3/s5c73m3-spi.c | 1 +
 1 file changed, 1 insertion(+)

drivers/media/i2c/s5c73m3/s5c73m3-spi.c static const struct of_device_id s5c73m3_spi_ids[] = { · line 34

Quoted 9 lines

drivers/media/i2c/s5c73m3/s5c73m3-spi.c +1 -0

index fa4a5ebda6b2..9983635ec253 100644
@@ -31,6 +31,7 @@ static const struct of_device_id s5c73m3_spi_ids[] = {
 	{ .compatible = "samsung,s5c73m3" },
 	{ }
 };
+MODULE_DEVICE_TABLE(of, s5c73m3_spi_ids;);

drivers/media/i2c/s5c73m3/s5c73m3-spi.c static const struct of_device_id s5c73m3_spi_ids[] = {

Quoted 4 lines

drivers/media/i2c/s5c73m3/s5c73m3-spi.c +0 -0

 enum spi_direction {
 	SPI_DIR_RX,
 

Any comments about this patch?

Best regards,

Quoted 28 lines · Javier Martinez Canillas

Hello,

Quoted 25 lines · Javier Martinez Canillas

The SPI core always reports the MODALIAS uevent as "spi:<modalias>" regardless of the mechanism that was used to register the device (i.e: OF or board code) and the table that is used later to match the driver with the device (i.e: SPI id table or OF match table).

So drivers needs to export the SPI id table and this be built into the module or udev won't have the necessary information to autoload the needed driver module when the device is added.

But this means that OF-only drivers needs to have both OF and SPI id tables that have to be kept in sync and also the dev node compatible manufacturer prefix is stripped when reporting the MODALIAS. Which can lead to issues if two vendors use the same SPI device name for example.

To avoid the above, the SPI core behavior may be changed in the future to not require an SPI device table for OF-only drivers and report the OF module alias. So, it's better to also export the OF table even when is unused now to prevent breaking module loading when the core changes.

Signed-off-by Javier Martinez Canillas <[email protected]>

---

 drivers/media/i2c/s5c73m3/s5c73m3-spi.c | 1 +
 1 file changed, 1 insertion(+)

Quoted 9 lines

drivers/media/i2c/s5c73m3/s5c73m3-spi.c static const struct of_device_id s5c73m3_spi_ids[] = { · line 34

Quoted 9 lines

drivers/media/i2c/s5c73m3/s5c73m3-spi.c +1 -0

index fa4a5ebda6b2..9983635ec253 100644
@@ -31,6 +31,7 @@ static const struct of_device_id s5c73m3_spi_ids[] = {
 	{ .compatible = "samsung,s5c73m3" },
 	{ }
 };
+MODULE_DEVICE_TABLE(of, s5c73m3_spi_ids;);

Unnecessary semicolon inside parenthesis, I guess it wont compile :)

Regards
Andrzej

The Documentation/devicetree/bindings/mfd/cros-ec.txt DT binding doc lists "google,cros-ec-spi" as a compatible string but the corresponding driver does not have an OF match table. Add the table to the driver so the SPI core can do an OF style match.

Signed-off-by Javier Martinez Canillas <[email protected]>

---

 drivers/mfd/cros_ec_spi.c | 7 +++++++
 1 file changed, 7 insertions(+)

drivers/mfd/cros_ec_spi.c +7 -0

index 16f228dc243f..30a296b4e748 100644
@@ -701,6 +701,12 @@ static int cros_ec_spi_resume(struct device *dev)
 static SIMPLE_DEV_PM_OPS(cros_ec_spi_pm_ops, cros_ec_spi_suspend,
 			 cros_ec_spi_resume);
 
+static const struct of_device_id cros_ec_spi_of_match[] = {
+	{ .compatible = "google,cros-ec-spi", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, cros_ec_spi_of_match);
+
 static const struct spi_device_id cros_ec_spi_id[] = {
 	{ "cros-ec-spi", 0 },
 	{ }

@@ -710,6 +716,7 @@ MODULE_DEVICE_TABLE(spi, cros_ec_spi_id);
 static struct spi_driver cros_ec_driver_spi = {
 	.driver	= {
 		.name	= "cros-ec-spi",
+		.of_match_table = of_match_ptr(cros_ec_spi_of_match),
 		.owner	= THIS_MODULE,
 		.pm	= &cros_ec_spi_pm_ops,
 	},

The Documentation/devicetree/bindings/iio/dac/ad7303.txt DT binding doc lists "adi,ad7303" as a compatible string but the corresponding driver does not have an OF match table. Add the table to the driver so the SPI core can do an OF style match.

Signed-off-by Javier Martinez Canillas <[email protected]>

Quoted 6 lines · Javier Martinez Canillas

The Documentation/devicetree/bindings/iio/dac/ad7303.txt DT binding doc lists "adi,ad7303" as a compatible string but the corresponding driver does not have an OF match table. Add the table to the driver so the SPI core can do an OF style match.

Signed-off-by Javier Martinez Canillas <[email protected]>

Acked-by Lars-Peter Clausen <[email protected]>

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

The driver has an OF id table but the .of_match_table is not set so the SPI core can't do an OF style match and the table was unused.

Signed-off-by Javier Martinez Canillas <[email protected]>

---

 drivers/iio/adc/max1027.c | 1 +
 1 file changed, 1 insertion(+)

drivers/iio/adc/max1027.c +1 -0

index 44bf815adb6c..54a8302aaace 100644
@@ -508,6 +508,7 @@ static int max1027_remove(struct spi_device *spi)
 static struct spi_driver max1027_driver = {
 	.driver = {
 		.name	= "max1027",
+		.of_match_table = of_match_ptr(max1027_adc_dt_ids),
 		.owner	= THIS_MODULE,
 	},
 	.probe		= max1027_probe,

The Documentation/devicetree/bindings/mfd/stmpe.txt DT binding doc lists "st,stmpe[610|801|811|1601|2401|2403]" as valid compatible strings but the corresponding driver does not have an OF match table. Add the table to the driver so the SPI core can do an OF style match.

Signed-off-by Javier Martinez Canillas <[email protected]>

---

 drivers/mfd/stmpe-spi.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

drivers/mfd/stmpe-spi.c +13 -0

index 6fdb30e84a2b..618ba244d98a 100644
@@ -11,6 +11,7 @@
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/types.h>
 #include "stmpe.h"
 

@@ -108,6 +109,17 @@ static int stmpe_spi_remove(struct spi_device *spi)
 	return stmpe_remove(stmpe);
 }
 
+static const struct of_device_id stmpe_spi_of_match[] = {
+	{ .compatible = "st,stmpe610", },
+	{ .compatible = "st,stmpe801", },
+	{ .compatible = "st,stmpe811", },
+	{ .compatible = "st,stmpe1601", },
+	{ .compatible = "st,stmpe2401", },
+	{ .compatible = "st,stmpe2403", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, stmpe_spi_of_match);
+
 static const struct spi_device_id stmpe_spi_id[] = {
 	{ "stmpe610", STMPE610 },
 	{ "stmpe801", STMPE801 },

@@ -122,6 +134,7 @@ MODULE_DEVICE_TABLE(spi, stmpe_id);
 static struct spi_driver stmpe_spi_driver = {
 	.driver = {
 		.name	= "stmpe-spi",
+		.of_match_table = of_match_ptr(stmpe_spi_of_match),
 		.owner	= THIS_MODULE,
 #ifdef CONFIG_PM
 		.pm	= &stmpe_dev_pm_ops,

The driver has an OF id table but the .of_match_table is not set so the SPI core can't do an OF style match and the table was unused.

Signed-off-by Javier Martinez Canillas <[email protected]>

---

 drivers/iio/adc/mcp320x.c | 1 +
 1 file changed, 1 insertion(+)

drivers/iio/adc/mcp320x.c +1 -0

index b19e4f9d16e0..41a21e986c1a 100644
@@ -404,6 +404,7 @@ MODULE_DEVICE_TABLE(spi, mcp320x_id);
 static struct spi_driver mcp320x_driver = {
 	.driver = {
 		.name = "mcp320x",
+		.of_match_table = of_match_ptr(mcp320x_dt_ids),
 		.owner = THIS_MODULE,
 	},
 	.probe = mcp320x_probe,

Signature · 1 line

2.4.3

Quoted 3 lines · Javier Martinez Canillas

The driver has an OF id table but the .of_match_table is not set so the SPI core can't do an OF style match and the table was unused.

Is an OF style match necessary?

I have been using devicetree and it matches based on the .id_table.

Couldn't we just remove the mcp320x_dt_ids table instead?

Hello Michael,

Quoted 5 lines · Michael Welling

Quoted 3 lines · Javier Martinez Canillas

The driver has an OF id table but the .of_match_table is not set so the SPI core can't do an OF style match and the table was unused.

Quoted 2 lines

Is an OF style match necessary?

Did you read the cover letter [0] on which I explain why is needed to avoid breaking module autoloading in the future? Once the SPI core is changed by RFC patch 18/18? (you were cc'ed in the cover letter BTW).

Quoted 3 lines

I have been using devicetree and it matches based on the .id_table.

Yes it fallbacks to the .id_table or the driver name but the correct thing to do for devices registered by OF, is to match using the compatible string.

Quoted 2 lines

Couldn't we just remove the mcp320x_dt_ids table instead?

No, that is the wrong thing to do IMHO since the compatible string contains both vendor and device name whle the .id_table only contains a device name.

So it makes sense to match using the compatible string and also report the OF modalias information to user-space.

Otherwise what's the point of the vendor in the compatible string for SPI devices? You can just use "bar" instead of "foo,bar" as a string.

[0]: https://lkml.org/lkml/2015/8/20/109

Best regards,

Signature · 3 lines

Javier Martinez Canillas
Open Source Group
Samsung Research America

Quoted 8 lines · Javier Martinez Canillas

Hello Michael,

Quoted 5 lines · Michael Welling

Quoted 3 lines · Javier Martinez Canillas

The driver has an OF id table but the .of_match_table is not set so the SPI core can't do an OF style match and the table was unused.

Quoted 6 lines

Quoted 2 lines

Is an OF style match necessary?

Did you read the cover letter [0] on which I explain why is needed to avoid breaking module autoloading in the future? Once the SPI core is changed by RFC patch 18/18? (you were cc'ed in the cover letter BTW).

Well I have read it now. :)

Quoted 18 lines

Quoted 3 lines

I have been using devicetree and it matches based on the .id_table.

Yes it fallbacks to the .id_table or the driver name but the correct thing to do for devices registered by OF, is to match using the compatible string.

Quoted 2 lines

Couldn't we just remove the mcp320x_dt_ids table instead?

No, that is the wrong thing to do IMHO since the compatible string contains both vendor and device name whle the .id_table only contains a device name.

So it makes sense to match using the compatible string and also report the OF modalias information to user-space.

Quoted 3 lines

Otherwise what's the point of the vendor in the compatible string for SPI devices? You can just use "bar" instead of "foo,bar" as a string.

Well then shouldn't the patch include adding the vendor to the compatible string?

Hello Michael,

Quoted 9 lines · Michael Welling

Quoted 8 lines · Javier Martinez Canillas

Hello Michael,

Quoted 5 lines · Michael Welling

Quoted 3 lines · Javier Martinez Canillas

The driver has an OF id table but the .of_match_table is not set so the SPI core can't do an OF style match and the table was unused.

Quoted 9 lines

Quoted 6 lines

Quoted 2 lines

Is an OF style match necessary?

Did you read the cover letter [0] on which I explain why is needed to avoid breaking module autoloading in the future? Once the SPI core is changed by RFC patch 18/18? (you were cc'ed in the cover letter BTW).

Well I have read it now. :)

Great :)

Quoted 18 lines

Quoted 18 lines

Quoted 3 lines

I have been using devicetree and it matches based on the .id_table.

Yes it fallbacks to the .id_table or the driver name but the correct thing to do for devices registered by OF, is to match using the compatible string.

Quoted 2 lines

Couldn't we just remove the mcp320x_dt_ids table instead?

No, that is the wrong thing to do IMHO since the compatible string contains both vendor and device name whle the .id_table only contains a device name.

So it makes sense to match using the compatible string and also report the OF modalias information to user-space.

Quoted 7 lines

Quoted 3 lines

Otherwise what's the point of the vendor in the compatible string for SPI devices? You can just use "bar" instead of "foo,bar" as a string.

Well then shouldn't the patch include adding the vendor to the compatible string?

Well, I was talking in general. You are right that this specific driver does not have a vendor prefix for the compatible strings. This is incorrect according to the ePAPR document [0].

However, these compatible strings are already documented as a DT binding doc in Documentation/devicetree/bindings/iio/adc/mcp320x.txt so I don't know what is the correct thing to do in this situation.

Changing the compatible string will break old DTB with newer kernels and that is a no go. But that doesn't invalidate what I said since once the SPI core is changed and report OF modalias, you will need a MODULE_DEVICE_TABLE(of,...) regardless if the compatible string has a vendor prefix or not.

[0]: https://www.power.org/documentation/epapr-version-1-1/

Best regards,

Signature · 3 lines

Javier Martinez Canillas
Open Source Group
Samsung Research America

Quoted 54 lines · Javier Martinez Canillas

Hello Michael,

Quoted 9 lines · Michael Welling

Quoted 8 lines · Javier Martinez Canillas

Hello Michael,

Quoted 5 lines · Michael Welling

Quoted 3 lines · Javier Martinez Canillas

The driver has an OF id table but the .of_match_table is not set so the SPI core can't do an OF style match and the table was unused.

Quoted 9 lines

Quoted 6 lines

Quoted 2 lines

Is an OF style match necessary?

Did you read the cover letter [0] on which I explain why is needed to avoid breaking module autoloading in the future? Once the SPI core is changed by RFC patch 18/18? (you were cc'ed in the cover letter BTW).

Well I have read it now. :)

Great :)

Quoted 18 lines

Quoted 18 lines

Quoted 3 lines

I have been using devicetree and it matches based on the .id_table.

Yes it fallbacks to the .id_table or the driver name but the correct thing to do for devices registered by OF, is to match using the compatible string.

Quoted 2 lines

Couldn't we just remove the mcp320x_dt_ids table instead?

No, that is the wrong thing to do IMHO since the compatible string contains both vendor and device name whle the .id_table only contains a device name.

So it makes sense to match using the compatible string and also report the OF modalias information to user-space.

Quoted 7 lines

Quoted 3 lines

Otherwise what's the point of the vendor in the compatible string for SPI devices? You can just use "bar" instead of "foo,bar" as a string.

Well then shouldn't the patch include adding the vendor to the compatible string?

Well, I was talking in general. You are right that this specific driver does not have a vendor prefix for the compatible strings. This is incorrect according to the ePAPR document [0].

Quoted 9 lines

However, these compatible strings are already documented as a DT binding doc in Documentation/devicetree/bindings/iio/adc/mcp320x.txt so I don't know what is the correct thing to do in this situation.

Changing the compatible string will break old DTB with newer kernels and that is a no go. But that doesn't invalidate what I said since once the SPI core is changed and report OF modalias, you will need a MODULE_DEVICE_TABLE(of,...) regardless if the compatible string has a vendor prefix or not.

Okay.

Acked-by Michael Welling <[email protected]>

Quoted 8 lines

[0]: https://www.power.org/documentation/epapr-version-1-1/

Best regards,

Signature · 3 lines

Javier Martinez Canillas
Open Source Group
Samsung Research America

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hello Michael,

Quoted 9 lines · Michael Welling

Quoted 8 lines · Javier Martinez Canillas

Hello Michael,

Quoted 5 lines · Michael Welling

Quoted 3 lines · Javier Martinez Canillas

The driver has an OF id table but the .of_match_table is not set so the SPI core can't do an OF style match and the table was unused.

Quoted 9 lines

Quoted 6 lines

Quoted 2 lines

Is an OF style match necessary?

Did you read the cover letter [0] on which I explain why is needed to avoid breaking module autoloading in the future? Once the SPI core is changed by RFC patch 18/18? (you were cc'ed in the cover letter BTW).

Well I have read it now. :)

Great :)

Quoted 18 lines

Quoted 18 lines

Quoted 3 lines

I have been using devicetree and it matches based on the .id_table.

Yes it fallbacks to the .id_table or the driver name but the correct thing to do for devices registered by OF, is to match using the compatible string.

Quoted 2 lines

Couldn't we just remove the mcp320x_dt_ids table instead?

No, that is the wrong thing to do IMHO since the compatible string contains both vendor and device name whle the .id_table only contains a device name.

So it makes sense to match using the compatible string and also report the OF modalias information to user-space.

Quoted 7 lines

Quoted 3 lines

Otherwise what's the point of the vendor in the compatible string for SPI devices? You can just use "bar" instead of "foo,bar" as a string.

Well then shouldn't the patch include adding the vendor to the compatible string?

Well, I was talking in general. You are right that this specific driver does not have a vendor prefix for the compatible strings. This is incorrect according to the ePAPR document [0].

However, these compatible strings are already documented as a DT binding doc in Documentation/devicetree/bindings/iio/adc/mcp320x.txt so I don't know what is the correct thing to do in this situation.

Changing the compatible string will break old DTB with newer kernels and that is a no go. But that doesn't invalidate what I said since once the SPI core is changed and report OF modalias, you will need a MODULE_DEVICE_TABLE(of,...) regardless if the compatible string has a vendor prefix or not.

[0]: https://www.power.org/documentation/epapr-version-1-1/

Best regards,

Quoted 54 lines · Javier Martinez Canillas

Hello Michael,

Quoted 9 lines · Michael Welling

Quoted 8 lines · Javier Martinez Canillas

Hello Michael,

Quoted 5 lines · Michael Welling

Quoted 3 lines · Javier Martinez Canillas

The driver has an OF id table but the .of_match_table is not set so the SPI core can't do an OF style match and the table was unused.

Quoted 9 lines

Quoted 6 lines

Quoted 2 lines

Is an OF style match necessary?

Did you read the cover letter [0] on which I explain why is needed to avoid breaking module autoloading in the future? Once the SPI core is changed by RFC patch 18/18? (you were cc'ed in the cover letter BTW).

Well I have read it now. :)

Great :)

Quoted 18 lines

Quoted 18 lines

Quoted 3 lines

I have been using devicetree and it matches based on the .id_table.

Yes it fallbacks to the .id_table or the driver name but the correct thing to do for devices registered by OF, is to match using the compatible string.

Quoted 2 lines

Couldn't we just remove the mcp320x_dt_ids table instead?

No, that is the wrong thing to do IMHO since the compatible string contains both vendor and device name whle the .id_table only contains a device name.

So it makes sense to match using the compatible string and also report the OF modalias information to user-space.

Quoted 7 lines

Quoted 3 lines

Otherwise what's the point of the vendor in the compatible string for SPI devices? You can just use "bar" instead of "foo,bar" as a string.

Well then shouldn't the patch include adding the vendor to the compatible string?

Well, I was talking in general. You are right that this specific driver does not have a vendor prefix for the compatible strings. This is incorrect according to the ePAPR document [0].

Quoted 3 lines

However, these compatible strings are already documented as a DT binding doc in Documentation/devicetree/bindings/iio/adc/mcp320x.txt so I don't know what is the correct thing to do in this situation.

Take the view the old version is wrong but needs to be supported and add also the corrected strings + document them.

cc'd The device tree list for any more comments on this.

Quoted 58 lines · Jonathan Cameron

Quoted 57 lines · Javier Martinez Canillas

Hello Michael,

Quoted 9 lines · Michael Welling

Quoted 8 lines · Javier Martinez Canillas

Hello Michael,

Quoted 5 lines · Michael Welling

Quoted 3 lines · Javier Martinez Canillas

The driver has an OF id table but the .of_match_table is not set so the SPI core can't do an OF style match and the table was unused.

Quoted 9 lines

Quoted 6 lines

Quoted 2 lines

Is an OF style match necessary?

Did you read the cover letter [0] on which I explain why is needed to avoid breaking module autoloading in the future? Once the SPI core is changed by RFC patch 18/18? (you were cc'ed in the cover letter BTW).

Well I have read it now. :)

Great :)

Quoted 18 lines

Quoted 18 lines

Quoted 3 lines

I have been using devicetree and it matches based on the .id_table.

Yes it fallbacks to the .id_table or the driver name but the correct thing to do for devices registered by OF, is to match using the compatible string.

Quoted 2 lines

Couldn't we just remove the mcp320x_dt_ids table instead?

No, that is the wrong thing to do IMHO since the compatible string contains both vendor and device name whle the .id_table only contains a device name.

So it makes sense to match using the compatible string and also report the OF modalias information to user-space.

Quoted 7 lines

Quoted 3 lines

Otherwise what's the point of the vendor in the compatible string for SPI devices? You can just use "bar" instead of "foo,bar" as a string.

Well then shouldn't the patch include adding the vendor to the compatible string?

Well, I was talking in general. You are right that this specific driver does not have a vendor prefix for the compatible strings. This is incorrect according to the ePAPR document [0].

However, these compatible strings are already documented as a DT binding doc in Documentation/devicetree/bindings/iio/adc/mcp320x.txt so I don't know what is the correct thing to do in this situation.

Quoted 4 lines

Take the view the old version is wrong but needs to be supported and add also the corrected strings + document them.

cc'd The device tree list for any more comments on this.

Agreed. Document both and mark the old one deprecated.

Rob

The Documentation/devicetree/bindings/iio/proximity/as3935.txt DT binding doc lists "ams,as3935" as a compatible string but the corresponding driver does not have an OF match table. Add the table to the driver so the SPI core can do an OF style match.

Signed-off-by Javier Martinez Canillas <[email protected]>

---

 drivers/iio/proximity/as3935.c | 7 +++++++
 1 file changed, 7 insertions(+)

drivers/iio/proximity/as3935.c +7 -0

index bc0d68efd455..e95035136889 100644
@@ -434,6 +434,12 @@ static int as3935_remove(struct spi_device *spi)
 	return 0;
 }
 
+static const struct of_device_id as3935_of_match[] = {
+	{ .compatible = "ams,as3935", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, as3935_of_match);
+
 static const struct spi_device_id as3935_id[] = {
 	{"as3935", 0},
 	{},

@@ -443,6 +449,7 @@ MODULE_DEVICE_TABLE(spi, as3935_id);
 static struct spi_driver as3935_driver = {
 	.driver = {
 		.name	= "as3935",
+		.of_match_table = of_match_ptr(as3935_of_match),
 		.owner	= THIS_MODULE,
 		.pm	= AS3935_PM_OPS,
 	},

The Documentation/devicetree/bindings/iio/adc/ti-adc128s052.txt DT binding doc lists "ti,adc128s052" or "ti,adc122s021" as compatible strings but the corresponding driver does not have an OF match table. Add the table to the driver so the SPI core can do an OF style match.

Signed-off-by Javier Martinez Canillas <[email protected]>

---

 drivers/iio/adc/ti-adc128s052.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/iio/adc/ti-adc128s052.c +8 -0

index 915be6b60097..98c0d2b444bf 100644
@@ -174,6 +174,13 @@ static int adc128_remove(struct spi_device *spi)
 	return 0;
 }
 
+static const struct of_device_id adc128_of_match[] = {
+	{ .compatible = "ti,adc128s052", },
+	{ .compatible = "ti,adc122s021", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, adc128_of_match);
+
 static const struct spi_device_id adc128_id[] = {
 	{ "adc128s052", 0},	/* index into adc128_config */
 	{ "adc122s021",	1},

@@ -184,6 +191,7 @@ MODULE_DEVICE_TABLE(spi, adc128_id);
 static struct spi_driver adc128_driver = {
 	.driver = {
 		.name = "adc128s052",
+		.of_match_table = of_match_ptr(adc128_of_match),
 		.owner = THIS_MODULE,
 	},
 	.probe = adc128_probe,

The Documentation/devicetree/bindings/iio/frequency/adf4350.txt DT binding doc lists "adi,adf4350" or "adi,adf4351" as compatible strings but the corresponding driver does not have an OF match table. Add the table to the driver so the SPI core can do an OF style match.

Signed-off-by Javier Martinez Canillas <[email protected]>

Quoted 6 lines · Javier Martinez Canillas

The Documentation/devicetree/bindings/iio/frequency/adf4350.txt DT binding doc lists "adi,adf4350" or "adi,adf4351" as compatible strings but the corresponding driver does not have an OF match table. Add the table to the driver so the SPI core can do an OF style match.

Signed-off-by Javier Martinez Canillas <[email protected]>

Acked-by Lars-Peter Clausen <[email protected]>

Thanks.

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

From: Javier Martinez Canillas <javier.martinez-ZGY8ohtN/[email protected]>

An SPI driver that supports both OF and legacy platforms, will have both an OF and SPI ID table. This means that when built as a module, the aliases will be filled from both tables, e.g:

Quoted 5 lines · Javier Martinez Canillas

From: Javier Martinez Canillas <javier.martinez-ZGY8ohtN/[email protected]>

An SPI driver that supports both OF and legacy platforms, will have both an OF and SPI ID table. This means that when built as a module, the aliases will be filled from both tables, e.g:

This is tagged as something that can't be applied but you've not explained why it can't be applied or what comments might be useful :(

Quoted 9 lines · Mark Brown

Quoted 5 lines · Javier Martinez Canillas

From: Javier Martinez Canillas <javier.martinez-ZGY8ohtN/[email protected]>

An SPI driver that supports both OF and legacy platforms, will have both an OF and SPI ID table. This means that when built as a module, the aliases will be filled from both tables, e.g:

This is tagged as something that can't be applied but you've not explained why it can't be applied or what comments might be useful :(

I believe that's mostly addressed in the cover letter [1].

Patch #18 changes the logic of spi_uevent() to report an OF modalias if the device was registered using OF. But this patch is included in the series only as an RFC for illustration purposes since changing that without first applying all the other patches in this series, will break module autoloading for the drivers of devices registered using OF but that lacks an of_match_table. I'll repost patch #18 once all the patches in this series have landed.

IOW, it's labeled as such mostly for safety, since it has quite a few distributed dependencies.

Brian

[1] https://lkml.org/lkml/2015/8/20/109

Quoted 2 lines · Brian Norris

Quoted 2 lines

This is tagged as something that can't be applied but you've not explained why it can't be applied or what comments might be useful :(

As Brian pointed out it was in the cover letter so I thought it would just be duplicated information. But you are right, I should had added a brief note as well just to make the patch self contained.

Quoted 9 lines

I believe that's mostly addressed in the cover letter [1].

Patch #18 changes the logic of spi_uevent() to report an OF modalias if the device was registered using OF. But this patch is included in the series only as an RFC for illustration purposes since changing that without first applying all the other patches in this series, will break module autoloading for the drivers of devices registered using OF but that lacks an of_match_table. I'll repost patch #18 once all the patches in this series have landed.

Quoted 2 lines

IOW, it's labeled as such mostly for safety, since it has quite a few distributed dependencies.

Quoted 4 lines · Javier Martinez Canillas

Quoted 2 lines · Brian Norris

Quoted 2 lines

This is tagged as something that can't be applied but you've not explained why it can't be applied or what comments might be useful :(

Quoted 3 lines

As Brian pointed out it was in the cover letter so I thought it would just be duplicated information. But you are right, I should had added a brief note as well just to make the patch self contained.

Right, a big part of what I was looking for was something about why this is an incompatible change in the changelog so that once it gets applied someone with out of tree code which gets broken can see what happens.

Plus...

Quoted 9 lines

Quoted 9 lines

I believe that's mostly addressed in the cover letter [1].

Patch #18 changes the logic of spi_uevent() to report an OF modalias if the device was registered using OF. But this patch is included in the series only as an RFC for illustration purposes since changing that without first applying all the other patches in this series, will break module autoloading for the drivers of devices registered using OF but that lacks an of_match_table. I'll repost patch #18 once all the patches in this series have landed.

Quoted 2 lines

Quoted 2 lines

IOW, it's labeled as such mostly for safety, since it has quite a few distributed dependencies.

Are there really only 17 drivers that are missing an explict of_table? That seems like a low number.

Quoted 7 lines · Mark Brown

Quoted 2 lines · Javier Martinez Canillas

Quoted 2 lines

IOW, it's labeled as such mostly for safety, since it has quite a few distributed dependencies.

Are there really only 17 drivers that are missing an explict of_table? That seems like a low number.

In fact the 17 patches are the combination of the SPI drivers that:

a) Have a .id_table but not a MODULE_DEVICE_TABLE(spi,...)
b) Have a .of_match_table but no a MODULE_DEVICE_TABLE(of,...)
c) Don't have a .of_match_table but have a DT binding document

Maybe there are more SPI drivers out there that only have a .id_table and don't have a .of_match_table nor a DT binding doc. But in that case there isn't too much I can do since I've no information that these are drivers are actually used in systems booted with OF.

Quoted 11 lines · Javier Martinez Canillas

Quoted 7 lines · Mark Brown

Quoted 2 lines · Javier Martinez Canillas

Quoted 2 lines

IOW, it's labeled as such mostly for safety, since it has quite a few distributed dependencies.

Are there really only 17 drivers that are missing an explict of_table? That seems like a low number.

In fact the 17 patches are the combination of the SPI drivers that:

Quoted 8 lines

a) Have a .id_table but not a MODULE_DEVICE_TABLE(spi,...)
b) Have a .of_match_table but no a MODULE_DEVICE_TABLE(of,...)
c) Don't have a .of_match_table but have a DT binding document

Maybe there are more SPI drivers out there that only have a .id_table and don't have a .of_match_table nor a DT binding doc. But in that case there isn't too much I can do since I've no information that these are drivers are actually used in systems booted with OF.

We could at the very least scan through the in tree DTs. There does seem to be a substantial overlap between systems that often don't use modular kernels but could and systems where people are using the I2C and SPI ID mapping shims.

Patches #1 and #2 solves a), patches #3 to #8 solves b) and patches

Patch #18 changes the logic of spi_uevent() to report an OF modalias if the device was registered using OF. But this patch is included in the series only as an RFC for illustration purposes since changing that without first applying all the other patches in this series, will break module autoloading for the drivers of devices registered using OF but that lacks an of_match_table. I'll repost patch #18 once all the patches in this series have landed.

Quoted 1 line · Javier Martinez Canillas

Patches #1 and #2 solves a), patches #3 to #8 solves b) and patches

^^^ I'm dying to know how this sentence ends :)

Quoted 7 lines

Patch #18 changes the logic of spi_uevent() to report an OF modalias if the device was registered using OF. But this patch is included in the series only as an RFC for illustration purposes since changing that without first applying all the other patches in this series, will break module autoloading for the drivers of devices registered using OF but that lacks an of_match_table. I'll repost patch #18 once all the patches in this series have landed.

On a more productive note, the patches I've looked at look good to me. The missing aliases are a problem enough that should be fixed (i.e., part (b)). I'll leave the SPI framework changes to others to comment on.

Brian