Another stab at making spidev usable with devicetree
Subsystems (3)
cover Another stab at making spidev usable with devicetree
Hello,
This small patchset makes it possible to bind spidev manually to any SPI slave without much hassle.
Thanks
Michal
Michal Suchanek (3):
spi: spidev: fix the check for spidev in dt
spi: of: allow instantiating slaves without a driver drivers core: allow id match override when manually binding driver
drivers/base/Kconfig.debug | 14 +++++++++ drivers/base/bus.c | 72 +++++++++++++++++++++++++++++++++++++++++++++- drivers/spi/spi.c | 10 +++---- drivers/spi/spidev.c | 9 ++++-- lib/Kconfig.debug | 2 ++ 5 files changed, 98 insertions(+), 9 deletions(-) create mode 100644 drivers/base/Kconfig.debug
Signature · 6 lines
2.8.1 -- 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
PATCH 1/3 spi: spidev: fix the check for spidev in dt
The check is supposed to warn about spidev specified directly in devicetree as compatible. This just does not work. I have a devicetree with no compatible whatsoever and hacked my kernel so I can manually bind spidev. This still triggers.
Also I have no idea how this could have build with ! CONFIG_OF since the id table which the code checks is not compiled then.
--- drivers/spi/spidev.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
drivers/spi/spidev.c +7 -2
@@ -700,6 +700,11 @@ static const struct of_device_id spidev_dt_ids[] = {MODULE_DEVICE_TABLE (of, spidev_dt_ids);#endif +static const struct of_device_id spidev_check[] = { + { .compatible ="spidev" }, + {} +}; +/*-------------------------------------------------------------------------*/ static int spidev_probe (struct spi_device *spi)
@@ -713,10 +718,10 @@ static int spidev_probe(struct spi_device *spi)* compatible string, it is a Linux implementation thing * rather than a description of the hardware. */ -if (spi->dev.of_node && !of_match_device (spidev_dt_ids, &spi->dev)) { +if (spi->dev.of_node &&of_match_device (spidev_check, &spi->dev)) {dev_err (&spi->dev,"buggy DT: spidev listed directly in DT\n" );WARN_ON (spi->dev.of_node && - !of_match_device (spidev_dt_ids, &spi->dev)); +of_match_device (spidev_check, &spi->dev)); }/* Allocate driver data */
Signature · 1 line
2.8.1
Mark Brown
maintainerRe: [PATCH 1/3] spi: spidev: fix the check for spidev in dt
Quoted 4 lines · Michal Suchanek
The check is supposed to warn about spidev specified directly in devicetree as compatible. This just does not work. I have a devicetree with no compatible whatsoever and hacked my kernel so I can manually bind spidev. This still triggers.
This is the third copy of this I've got in two days, please calm down.
Re: [PATCH 1/3] spi: spidev: fix the check for spidev in dt
Quoted 8 lines · Mark Brown
Quoted 4 lines · Michal Suchanek
The check is supposed to warn about spidev specified directly in devicetree as compatible. This just does not work. I have a devicetree with no compatible whatsoever and hacked my kernel so I can manually bind spidev. This still triggers.
This is the third copy of this I've got in two days, please calm down.
Sorry about that.
This is the first copy I have seen on the ML. The first two were rejected for some reason and the ML software was not so kind as to send a rejection message stating the reason.
Thanks
Michal
--
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
Mark Brown
maintainerRe: [PATCH 1/3] spi: spidev: fix the check for spidev in dt
In reply to Michal SuchanekQuoted 4 lines · Michal Suchanek
The check is supposed to warn about spidev specified directly in devicetree as compatible. This just does not work. I have a devicetree with no compatible whatsoever and hacked my kernel so I can manually bind spidev. This still triggers.
Well, a DT device won't instantiate without a compatible string... could you please explain exactly what makes you say this won't work?
Quoted 2 lines
Also I have no idea how this could have build with !
CONFIG_OFsince the id table which the code checks is not compiled then.
of_match_device() compiles out when !OF.
drivers/spi/spidev.c static const struct of_device_id spidev_dt_ids[] = { · line 706
Quoted 4 lines
drivers/spi/spidev.c +4
-0+static const struct of_device_id spidev_check[] = { + { .compatible ="spidev" }, + {} +};
The indentation here is completely non-standard.
drivers/spi/spidev.c static int spidev_probe(struct spi_device *spi) · line 721
Quoted 2 lines
drivers/spi/spidev.c +1
-1-if (spi->dev.of_node && !of_match_device (spidev_dt_ids, &spi->dev)) { +if (spi->dev.of_node &&of_match_device (spidev_check, &spi->dev)) {
I think what you intend to say in the commit message is that you want to change from a whitelist to a blacklist since that is what the code says, but like I say we also need an explanation of the logic behind such a change.
Re: [PATCH 1/3] spi: spidev: fix the check for spidev in dt
Quoted 9 lines · Mark Brown
Quoted 4 lines · Michal Suchanek
The check is supposed to warn about spidev specified directly in devicetree as compatible. This just does not work. I have a devicetree with no compatible whatsoever and hacked my kernel so I can manually bind spidev. This still triggers.
Well, a DT device won't instantiate without a compatible string... could you please explain exactly what makes you say this won't work?
That's because the whitelist concept for this check is completely broken.
Without any patches whatsoever I should be able to specify m25p80 binding in the DT, let the kernel create the device, unbind the driver, and bind spidev.
Then I have the jedec,spi-nor compatible which is not on the whitelist.
Quoted 13 lines
Quoted 3 lines
Also I have no idea how this could have build with !
CONFIG_OFsince the id table which the code checks is not compiled then.
of_match_device()compiles out when !OF.drivers/spi/spidev.c static const struct of_device_id spidev_dt_ids[] = { · line 706
Quoted 4 lines
drivers/spi/spidev.c +4
-0+static const struct of_device_id spidev_check[] = { + { .compatible ="spidev" }, + {} +};The indentation here is completely non-standard.
Quoted 7 lines
drivers/spi/spidev.c static int spidev_probe(struct spi_device *spi) · line 721
Quoted 2 lines
drivers/spi/spidev.c +1
-1-if (spi->dev.of_node && !of_match_device (spidev_dt_ids, &spi->dev)) { +if (spi->dev.of_node &&of_match_device (spidev_check, &spi->dev)) {I think what you intend to say in the commit message is that you want to change from a whitelist to a blacklist since that is what the code says, but like I say we also need an explanation of the logic behind such a change.
It's because the check kernel log message says it's a blacklist and it's incorrectly implemented as a whitelist.
The change is to correct that.
Thanks
Michal
--
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
Mark Brown
maintainerRe: [PATCH 1/3] spi: spidev: fix the check for spidev in dt
Quoted 11 lines · Michal Suchanek
Quoted 9 lines · Mark Brown
Quoted 4 lines · Michal Suchanek
The check is supposed to warn about spidev specified directly in devicetree as compatible. This just does not work. I have a devicetree with no compatible whatsoever and hacked my kernel so I can manually bind spidev. This still triggers.
Well, a DT device won't instantiate without a compatible string... could you please explain exactly what makes you say this won't work?
Quoted 7 lines
That's because the whitelist concept for this check is completely broken.
Without any patches whatsoever I should be able to specify m25p80 binding in the DT, let the kernel create the device, unbind the driver, and bind spidev.
Then I have the jedec,spi-nor compatible which is not on the whitelist.
So, none of that is in the changelog where it needs to be and it only makes sense if we adopt the very specific solution you are proposing. You need to describe this change properly and you need to put it at the end of the patch series were it makes sense.
PATCH 2/3 spi: of: allow instantiating slaves without a driver
In reply to Michal SuchanekSPI slave devices are not created when looking up driver for the slave fails. Create a device anyway so it can be manually bound to a driver.
--- drivers/spi/spi.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
drivers/spi/spi.c +4 -6
@@ -1477,9 +1477,8 @@ of_register_spi_device(struct spi_master *master, struct device_node *nc) rc =of_modalias_node (nc, spi->modalias,sizeof (spi->modalias));if (rc <0 ) { -dev_err (&master->dev,"cannot find modalias for %s\n" , +dev_warn (&master->dev,"cannot find modalias for %s\n" , nc->full_name); - goto err_out; }/* Device address */
@@ -1543,11 +1542,10 @@ of_register_spi_device(struct spi_master *master, struct device_node *nc)/* Device speed */ rc =of_property_read_u32 (nc,"spi-max-frequency" , &value);if (rc) { -dev_err (&master->dev,"%s has no valid 'spi-max-frequency' property (%d)\n" , +dev_warn (&master->dev,"%s has no valid 'spi-max-frequency' property (%d)\n" , nc->full_name, rc); - goto err_out; - } - spi->max_speed_hz = value; + }else + spi->max_speed_hz = value;/* Store a pointer to the node in the device structure */ of_node_get (nc);
Signature · 1 line
2.8.1
Mark Brown
maintainerRe: [PATCH 2/3] spi: of: allow instantiating slaves without a driver
Quoted 6 lines · Michal Suchanek
SPI slave devices are not created when looking up driver for the slave fails. Create a device anyway so it can be manually bound to a driver.
drivers/spi/spi.c +0
-0@@ -1543,11 +1542,10 @@ of_register_spi_device(struct spi_master *master, struct device_node *nc)/* Device speed */ rc =of_property_read_u32 (nc,"spi-max-frequency" , &value);
drivers/spi/spi.c of_register_spi_device(struct spi_master *master, struct device_node *nc) · line 1549
Quoted 10 lines
drivers/spi/spi.c +3
-4if (rc) { -dev_err (&master->dev,"%s has no valid 'spi-max-frequency' property (%d)\n" , +dev_warn (&master->dev,"%s has no valid 'spi-max-frequency' property (%d)\n" , nc->full_name, rc); - goto err_out; - } - spi->max_speed_hz = value; + }else + spi->max_speed_hz = value;
I can't relate this hunk to the changelog and there's a coding style problem, if there's { } on one side of an if statement it should be on both sides. Why are we making this change?
Re: [PATCH 2/3] spi: of: allow instantiating slaves without a driver
Quoted 13 lines · Mark Brown
Quoted 12 lines · Michal Suchanek
SPI slave devices are not created when looking up driver for the slave fails. Create a device anyway so it can be manually bound to a driver.
drivers/spi/spi.c +1
-3@@ -1543,11 +1542,10 @@ of_register_spi_device(struct spi_master *master, struct device_node *nc)/* Device speed */ rc =of_property_read_u32 (nc,"spi-max-frequency" , &value);if (rc) { -dev_err (&master->dev,"%s has no valid 'spi-max-frequency' property (%d)\n" , +dev_warn (&master->dev,"%s has no valid 'spi-max-frequency' property (%d)\n" , nc->full_name, rc); - goto err_out; - }
Quoted 8 lines
drivers/spi/spi.c of_register_spi_device(struct spi_master *master, struct device_node *nc) · line 1549
Quoted 4 lines
drivers/spi/spi.c +2
-1- spi->max_speed_hz = value; + }else + spi->max_speed_hz = value;I can't relate this hunk to the changelog and there's a coding style problem, if there's { } on one side of an if statement it should be on both sides. Why are we making this change?
The intention is that you can specify that your SPI master controller has a CS available without setting up the slave
&spi2 {
pinctrl-names = "default";
pinctrl-0 = <&spi2_pins_a>,
<&spi2_cs0_pins_a>;
status = "okay";
uext_spi: spi@uext {
reg = <0>;
};
};Then you can amend the slave node with an overlay or bind a driver that can deal with the node having no configuration.
The check here isn't very effective anyway since slaves with zero speed somehow creep into the kernel. I have seen people reporting division by zero in SPI master driver as a result. The proper way to fix this is to specify the master minimum and maximum speed limit so the SPI core can reject transfers with speed outside of the allowed range.
Thanks
Michal
--
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
Mark Brown
maintainerRe: [PATCH 2/3] spi: of: allow instantiating slaves without a driver
Quoted 8 lines · Michal Suchanek
Quoted 3 lines · Mark Brown
I can't relate this hunk to the changelog and there's a coding style problem, if there's { } on one side of an if statement it should be on both sides. Why are we making this change?
The intention is that you can specify that your SPI master controller has a CS available without setting up the slave
This is completely unrelated to the rest of the change and needs describing in the changelog.
Quoted 2 lines
Then you can amend the slave node with an overlay or bind a driver that can deal with the node having no configuration.
You can just add the entire slave node in the overlay, it's not clear that this buys us anything useful (and typically you'd want to as the maximum speed here is more a function of the slave device than the master), and this needs to be joined up with the work going on to allow expansion connector overlays to be loaded in the first place.
Quoted 6 lines
The check here isn't very effective anyway since slaves with zero speed somehow creep into the kernel. I have seen people reporting division by zero in SPI master driver as a result. The proper way to fix this is to specify the master minimum and maximum speed limit so the SPI core can reject transfers with speed outside of the allowed range.
That's a separate argument which is again unrelated to the changelog. If the check isn't working it would be better to have an analysis of why it's not working.
Re: [PATCH 2/3] spi: of: allow instantiating slaves without a driver
Quoted 10 lines · Mark Brown
Quoted 8 lines · Michal Suchanek
Quoted 3 lines · Mark Brown
I can't relate this hunk to the changelog and there's a coding style problem, if there's { } on one side of an if statement it should be on both sides. Why are we making this change?
The intention is that you can specify that your SPI master controller has a CS available without setting up the slave
Quoted 8 lines
This is completely unrelated to the rest of the change and needs describing in the changelog.
Quoted 2 lines
Then you can amend the slave node with an overlay or bind a driver that can deal with the node having no configuration.
You can just add the entire slave node in the overlay, it's not clear that this buys us anything useful
You have to target the master node and specify the CS in the overlay if you create the whole node. If you target the slave node you have the CS specified in the board devicetree and the overlay can apply to multiple boards without change.
Also you have to create an overlay for drivers which don't really need it and could be just manually bound.
Quoted 3 lines
(and typically you'd want to as the
maximum speed here is more a function of the slave device than the master),
Speed is the property of the slave device and if you don't specify the device it does not make sense to specify speed.
Quoted 2 lines
and this needs to be joined up with the work going on to allow expansion connector overlays to be loaded in the first place.
The overlays work equally well before and after this patch. I don't see any problem with them other than part of the infrastructure missing in mainline kernel.
Quoted 11 lines
Quoted 7 lines
The check here isn't very effective anyway since slaves with zero speed somehow creep into the kernel. I have seen people reporting division by zero in SPI master driver as a result. The proper way to fix this is to specify the master minimum and maximum speed limit so the SPI core can reject transfers with speed outside of the allowed range.
That's a separate argument which is again unrelated to the changelog. If the check isn't working it would be better to have an analysis of why it's not working.
My handwawing analysis is that it's probably due to the ability of spidev and other drivers to change the speed later on after this check is performed.
Anyway, it's not terribly useful so a warning suffices imho.
Thanks
Michal
Mark Brown
maintainerRe: [PATCH 2/3] spi: of: allow instantiating slaves without a driver
Quoted 9 lines · Michal Suchanek
Quoted 2 lines · Mark Brown
You can just add the entire slave node in the overlay, it's not clear that this buys us anything useful
You have to target the master node and specify the CS in the overlay if you create the whole node. If you target the slave node you have the CS specified in the board devicetree and the overlay can apply to multiple boards without change.
No, there's a lot more to having overlays to board-neutral connectors...
Quoted 6 lines
Quoted 2 lines
and this needs to be joined up with the work going on to allow expansion connector overlays to be loaded in the first place.
The overlays work equally well before and after this patch. I don't see any problem with them other than part of the infrastructure missing in mainline kernel.
...you're missing all this stuff here - currently the active work on this is being done by Patelis. It's not at all clear to me that referring to a slave rather than a master would be any easier in a board neutral overlay.
Re: [PATCH 2/3] spi: of: allow instantiating slaves without a driver
Quoted 6 lines · Mark Brown
Quoted 4 lines · Michal Suchanek
Quoted 2 lines · Mark Brown
You can just add the entire slave node in the overlay, it's not clear that this buys us anything useful
Quoted 6 lines
Quoted 4 lines
You have to target the master node and specify the CS in the overlay if you create the whole node. If you target the slave node you have the CS specified in the board devicetree and the overlay can apply to multiple boards without change.
No, there's a lot more to having overlays to board-neutral connectors...
If you are fine with using only the SPI pins then this is all it takes.
If you want to know that i2c2 pins happen to be next to the SPI pins on the connector and use them as GPIO pins for your SPI device reset and irq lines then you will need a lot more infrastructure for that to happen.
Quoted 4 lines
Quoted 3 lines
Quoted 3 lines
and this needs to be joined up with the work going on to allow expansion connector overlays to be loaded in the first place.
Quoted 8 lines
Quoted 3 lines
The overlays work equally well before and after this patch. I don't see any problem with them other than part of the infrastructure missing in mainline kernel.
...you're missing all this stuff here - currently the active work on this is being done by Patelis. It's not at all clear to me that referring to a slave rather than a master would be any easier in a board neutral overlay.
I cherry-picked one of his branches to try overlay loading. Anyhow, most of the kernel support is already there. The missing part which blocks any progress for a very long time already is importing DTC with overlay support into the kernel and building the devicetree blobs with overlay support.
Thanks
Michal
--
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
Mark Brown
maintainerRe: [PATCH 2/3] spi: of: allow instantiating slaves without a driver
Quoted 5 lines · Michal Suchanek
Quoted 1 line · Mark Brown
No, there's a lot more to having overlays to board-neutral connectors...
If you are fine with using only the SPI pins then this is all it takes.
No, there's other things like figuring out which controller to bind to that need to be taken into consideration.
Re: [PATCH 2/3] spi: of: allow instantiating slaves without a driver
Quoted 9 lines · Mark Brown
Quoted 5 lines · Michal Suchanek
Quoted 1 line · Mark Brown
No, there's a lot more to having overlays to board-neutral connectors...
If you are fine with using only the SPI pins then this is all it takes.
No, there's other things like figuring out which controller to bind to that need to be taken into consideration.
Why do you care? So long as you name the CS that is available on connector with the same pinout same on all boards you can target the slave regardless of the controller it's attached to.
Thanks
Michal
Mark Brown
maintainerRe: [PATCH 2/3] spi: of: allow instantiating slaves without a driver
Quoted 8 lines · Michal Suchanek
Quoted 2 lines · Mark Brown
No, there's other things like figuring out which controller to bind to that need to be taken into consideration.
Why do you care? So long as you name the CS that is available on connector with the same pinout same on all boards you can target the slave regardless of the controller it's attached to.
That would be a binding for the connector which is the big missing bit here - it's not clear that such a limited connector description would be a good idea.
Re: [PATCH 2/3] spi: of: allow instantiating slaves without a driver
Quoted 6 lines · Mark Brown
Quoted 4 lines · Michal Suchanek
Quoted 2 lines · Mark Brown
No, there's other things like figuring out which controller to bind to that need to be taken into consideration.
Quoted 7 lines
Quoted 3 lines
Why do you care? So long as you name the CS that is available on connector with the same pinout same on all boards you can target the slave regardless of the controller it's attached to.
That would be a binding for the connector which is the big missing bit here - it's not clear that such a limited connector description would be a good idea.
It would work for a limited number of devices. Anyway, connectors are supposed to be transparent so if binding devices has issues now it will supposedly have same issues once connectors allow renaming several devices at once from board-specific name to connector-specific name rather than one at a time as this limited connector binding allows.
Thanks
Michal
--
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
Mark Brown
maintainerRe: [PATCH 2/3] spi: of: allow instantiating slaves without a driver
Quoted 6 lines · Michal Suchanek
Quoted 3 lines · Mark Brown
That would be a binding for the connector which is the big missing bit here - it's not clear that such a limited connector description would be a good idea.
Quoted 6 lines
It would work for a limited number of devices. Anyway, connectors are supposed to be transparent so if binding devices has issues now it will supposedly have same issues once connectors allow renaming several devices at once from board-specific name to connector-specific name rather than one at a time as this limited connector binding allows.
It's not entirely clear to me that connectors are going to end up transparent, at least to the host system - there's things like pinmuxing in there. They're a definite thing and some work needs to go into hiding them from the plugin modules, work which might mean that these dummy nodes don't need to be created.
In any case this series needs a bunch of restructuring, some of it needs replacing and the whole thing needs to be presented a lot more clearly.
PATCH 3/3 drivers core: allow id match override when manually binding driver
In reply to Michal SuchanekThis allows binding spidev on any slave device by hand using sysfs without adding superfluous compatibles or any other needless complication.
Note that any slave driver that requires configuration will fail to probe anyway. Only a driver that binds to anything can be bound successfully.
--- drivers/base/Kconfig.debug | 14 +++++++++ drivers/base/bus.c | 72 +++++++++++++++++++++++++++++++++++++++++++++- lib/Kconfig.debug | 2 ++ 3 files changed, 87 insertions(+), 1 deletion(-) create mode 100644 drivers/base/Kconfig.debug
drivers/base/Kconfig.debug new file +14 -0
@@ -0,0 +1,14 @@ +menuconfig DRIVER_MATCH_OVERRIDE + bool "Allow manual driver binding to override id match (DANGEROUS)" + default n + help + When binding a driver manually bypass the check of driver id table + against device id in driver core. This can be useful for development + or on buses that don't provide reliable device identification. + +config DRIVER_MATCH_OVERRIDE_BUSES + string "Specify buses for which id matching will be overridden" + default "spi" + depends on DRIVER_MATCH_OVERRIDE + help + space separated bus names
drivers/base/bus.c +71 -1
89 lines not on this page · raw message
lib/Kconfig.debug +2 -0
@@ -2002,3 +2002,5 @@ config IO_STRICT_DEVMEM if the driver using a given range cannot be disabled. If in doubt, say Y. + +source drivers/base/Kconfig.debug
Signature · 1 line
2.8.1
Mark Brown
maintainerRe: [PATCH 3/3] drivers core: allow id match override when manually binding driver
Quoted 3 lines · Michal Suchanek
This allows binding spidev on any slave device by hand using sysfs without adding superfluous compatibles or any other needless complication.
This says "spidev" but it's a change to the driver core, not something that is specific to spidev. It needs to be explained in terms ofn the driver core, especially given that other runtime ID setting is done in a particular bus.
Quoted 4 lines
drivers/base/Kconfig.debug new file +4
-0+menuconfig DRIVER_MATCH_OVERRIDE + bool "Allow manual driver binding to override id match (DANGEROUS)" + default n + help
drivers/base/Kconfig.debug line 14
Quoted 10 lines
drivers/base/Kconfig.debug new file +10
-0+ When binding a driver manually bypass the check of driver id table + against device id in driver core. This can be useful for development + or on buses that don't provide reliable device identification. + +config DRIVER_MATCH_OVERRIDE_BUSES + string "Specify buses for which id matching will be overridden" + default "spi" + depends on DRIVER_MATCH_OVERRIDE + help + space separated bus names
This doesn't seem at all idomatic for how I'd expect bus features to be implemented, or Linux configuration in general. If we're going to allow a feature like this at the core level we should just allow it, if we're going to opt buses into any such feature I'd expect it to be done unconditionally.
Re: [PATCH 3/3] drivers core: allow id match override when manually binding driver
In reply to Michal SuchanekQuoted 8 lines · Michal Suchanek
This allows binding spidev on any slave device by hand using sysfs without adding superfluous compatibles or any other needless complication.
Note that any slave driver that requires configuration will fail to probe anyway. Only a driver that binds to anything can be bound successfully.
Quoted 8 lines
Signed-off-by Michal Suchanek <[email protected]>--- drivers/base/Kconfig.debug | 14 +++++++++ drivers/base/bus.c | 72 +++++++++++++++++++++++++++++++++++++++++++++- lib/Kconfig.debug | 2 ++ 3 files changed, 87 insertions(+), 1 deletion(-) create mode 100644 drivers/base/Kconfig.debug
Why change the driver core? The matching policy can be changed with the "match" operation of a custom "struct bus_type".
Re: [PATCH 3/3] drivers core: allow id match override when manually binding driver
Hello,
Quoted 18 lines · Dan Williams
Quoted 8 lines · Michal Suchanek
This allows binding spidev on any slave device by hand using sysfs without adding superfluous compatibles or any other needless complication.
Note that any slave driver that requires configuration will fail to probe anyway. Only a driver that binds to anything can be bound successfully.
Quoted 8 lines
Signed-off-by Michal Suchanek <[email protected]>--- drivers/base/Kconfig.debug | 14 +++++++++ drivers/base/bus.c | 72 +++++++++++++++++++++++++++++++++++++++++++++- lib/Kconfig.debug | 2 ++ 3 files changed, 87 insertions(+), 1 deletion(-) create mode 100644 drivers/base/Kconfig.debug
Quoted 2 lines
Why change the driver core? The matching policy can be changed with the "match" operation of a custom "struct
bus_type".
It cannot. Only driver core knows that the match is result of writing the bind file in sysfs. This is information is discarded at this very point.
Quoted 23 lines · Mark Brown
Quoted 3 lines · Michal Suchanek
This allows binding spidev on any slave device by hand using sysfs without adding superfluous compatibles or any other needless complication.
This says "spidev" but it's a change to the driver core, not something that is specific to spidev. It needs to be explained in terms ofn the driver core, especially given that other runtime ID setting is done in a particular bus.
Quoted 12 lines
drivers/base/Kconfig.debug new file +12
-0+menuconfig DRIVER_MATCH_OVERRIDE + bool "Allow manual driver binding to override id match (DANGEROUS)" + default n + help + When binding a driver manually bypass the check of driver id table + against device id in driver core. This can be useful for development + or on buses that don't provide reliable device identification. + +config DRIVER_MATCH_OVERRIDE_BUSES + string "Specify buses for which id matching will be overridden" + default "spi" + depends on DRIVER_MATCH_OVERRIDE
Quoted 8 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ help + space separated bus namesThis doesn't seem at all idomatic for how I'd expect bus features to be implemented, or Linux configuration in general. If we're going to allow a feature like this at the core level we should just allow it, if we're going to opt buses into any such feature I'd expect it to be done unconditionally.
Allowing some control over enabling or disabling this may be more generally acceptable. I was even considering a sysfs file that would enable control of this feature at runtime.
The main use case for this is spidev. I am not familiar with all kernel subsystems so there might be other uses.
The problem with spidev is this:
The SPI bus feature-wise is closest to serial UARTs form the other buses available in kernel. Serial uart does not even have a bus driver.
The main hardware difference is that part of SPI specification is CS which makes it proper bus whereas serial UARTs have no CS which makes them point to point. You can use gpios to multiplex serial devices too if you wanted - it's just non-standard setup.
The main kernel difference is that for serial UART a character device is created which a userspace program can use to send and receive data in the hope that whatever device is expected on the other side is actually there. This is the traditional use of serial lines. In addition there are serial drivers that expect to be driving a device that is on the mainboard and just happens to be connected by a serial line to the CPU. These might need additional GPIOs and can probably be configured by devicetree (iirc IR and Bluetooth devices of this type exist). For SPI the latter is the traditional use and recently boards with SPI connectors started to be common and people want to use the SPI bus the same way serial ports are used - connect some random device to the pins and try to talk to it from userspace.
Spidev is the driver which allows the use of SPI similar to how serial ports are used. The boards typically use devicetree for hardware description. And spidev is not configured anywhere in the devicetree. The response was to generate random compatible for the SPI port, add that compatible to spidev, and load it as the driver. Actually, you were supposed to generate a new compatible for every kind of device. That does not make any sense. It's like asking people to generate a new kernel config and reboot just to connect a different kind of modem to their serial port.
With devicetree overlays you can load an overlay for complicated devices that need GPIOs or extra settings and happen to have kernel driver (eg fbtft). Once loading the overlays is possible in mainline.
With less complicated drivers like m25p80 you can get full features with overlays and limited features with this patch.
And with spidev you cannot use overlays because 'it is not supposed to be specified in devicetree because it is linux implementation detail and not hardware'. Another option to do this outside of core is to generate a compatible for generic SPI external header, add that to spidev, and be done with it. No-go again.
An alternative approach that has been tried was to probe the SPI slave and if no driver is found bind spidev automatically in the kernel. This has two problems. If you cannot specify the external connector in DT you cannot know which CS to probe and which to leave alone. The concern is that triggering unused CS could disturb other hardware. The other problem is you never know if a driver is going to be loaded later and will be blocked from binding by spidev. It would also mean that if you unbind spidev you cannot re-bind it because it does not match.
Just leaving the device without a driver and let userspace decide to bind it or not seems like more general approach which can be potentially reused for other cases.
The last option I see is to get rid of separate spidev driver and just create a character device for every spi slave and bind the spidev IOCTLs to it when spidev support is enabled. It is specific to spidev and needs no core changes. It may cause mild conflict with spi slaves creating their own character devices.
Thanks
Michal
--
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
Re: [PATCH 3/3] drivers core: allow id match override when manually binding driver
Hi Michal,
Quoted 3 lines · Michal Suchanek
The SPI bus feature-wise is closest to serial UARTs form the other buses available in kernel. Serial uart does not even have a bus driver.
That's being worked on, as currently you can't describe the other side of the UART pins (e.g. a connected Bluetooth device).
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
Greg Kroah-Hartman
maintainerRe: [PATCH 3/3] drivers core: allow id match override when manually binding driver
In reply to Michal SuchanekQuoted 22 lines · Michal Suchanek
This allows binding spidev on any slave device by hand using sysfs without adding superfluous compatibles or any other needless complication.
Note that any slave driver that requires configuration will fail to probe anyway. Only a driver that binds to anything can be bound successfully.
Signed-off-by Michal Suchanek <[email protected]>--- drivers/base/Kconfig.debug | 14 +++++++++ drivers/base/bus.c | 72 +++++++++++++++++++++++++++++++++++++++++++++- lib/Kconfig.debug | 2 ++ 3 files changed, 87 insertions(+), 1 deletion(-) create mode 100644 drivers/base/Kconfig.debugdrivers/base/Kconfig.debug new file +0
-0
drivers/base/Kconfig.debug line 7
Quoted 7 lines
drivers/base/Kconfig.debug new file +7
-0@@ -0,0 +1,14 @@ +menuconfig DRIVER_MATCH_OVERRIDE + bool "Allow manual driver binding to override id match (DANGEROUS)" + default n + help + When binding a driver manually bypass the check of driver id table + against device id in driver core. This can be useful for development + or on buses that don't provide reliable device identification.
Ick, no no no. Why would you ever want to let this happen? If you really want to override the check, just write things to the 'bind' file in sysfs, that will skip the driver id check entirely, right?
drivers/base/Kconfig.debug line 14
Quoted 7 lines
drivers/base/Kconfig.debug new file +7
-0+ +config DRIVER_MATCH_OVERRIDE_BUSES + string "Specify buses for which id matching will be overridden" + default "spi" + depends on DRIVER_MATCH_OVERRIDE + help + space separated bus names
Gotta love parsers of config items :(
Again, please no.
greg k-h
--
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
Re: [PATCH 3/3] drivers core: allow id match override when manually binding driver
Quoted 26 lines · Greg Kroah-Hartman
Quoted 25 lines · Michal Suchanek
This allows binding spidev on any slave device by hand using sysfs without adding superfluous compatibles or any other needless complication.
Note that any slave driver that requires configuration will fail to probe anyway. Only a driver that binds to anything can be bound successfully.
Signed-off-by Michal Suchanek <[email protected]>--- drivers/base/Kconfig.debug | 14 +++++++++ drivers/base/bus.c | 72 +++++++++++++++++++++++++++++++++++++++++++++- lib/Kconfig.debug | 2 ++ 3 files changed, 87 insertions(+), 1 deletion(-) create mode 100644 drivers/base/Kconfig.debugdrivers/base/Kconfig.debug new file +3
-0@@ -0,0 +1,14 @@ +menuconfig DRIVER_MATCH_OVERRIDE + bool "Allow manual driver binding to override id match (DANGEROUS)" + default n
Quoted 8 lines
drivers/base/Kconfig.debug line 7
Quoted 4 lines
drivers/base/Kconfig.debug new file +4
-0+ help + When binding a driver manually bypass the check of driver id table + against device id in driver core. This can be useful for development + or on buses that don't provide reliable device identification.Ick, no no no. Why would you ever want to let this happen? If you really want to override the check, just write things to the 'bind' file in sysfs, that will skip the driver id check entirely, right?
Well, it does not. Hence this patch which enables skipping the check. It's the whole point of it.
Thanks
Michal
--
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
Greg Kroah-Hartman
maintainerRe: [PATCH 3/3] drivers core: allow id match override when manually binding driver
Quoted 36 lines · Michal Suchanek
Quoted 26 lines · Greg Kroah-Hartman
Quoted 25 lines · Michal Suchanek
This allows binding spidev on any slave device by hand using sysfs without adding superfluous compatibles or any other needless complication.
Note that any slave driver that requires configuration will fail to probe anyway. Only a driver that binds to anything can be bound successfully.
Signed-off-by Michal Suchanek <[email protected]>--- drivers/base/Kconfig.debug | 14 +++++++++ drivers/base/bus.c | 72 +++++++++++++++++++++++++++++++++++++++++++++- lib/Kconfig.debug | 2 ++ 3 files changed, 87 insertions(+), 1 deletion(-) create mode 100644 drivers/base/Kconfig.debugdrivers/base/Kconfig.debug new file +3
-0@@ -0,0 +1,14 @@ +menuconfig DRIVER_MATCH_OVERRIDE + bool "Allow manual driver binding to override id match (DANGEROUS)" + default nQuoted 8 lines
drivers/base/Kconfig.debug line 7
Quoted 4 lines
drivers/base/Kconfig.debug new file +4
-0+ help + When binding a driver manually bypass the check of driver id table + against device id in driver core. This can be useful for development + or on buses that don't provide reliable device identification.Ick, no no no. Why would you ever want to let this happen? If you really want to override the check, just write things to the 'bind' file in sysfs, that will skip the driver id check entirely, right?
Quoted 2 lines
Well, it does not. Hence this patch which enables skipping the check. It's the whole point of it.
Then write the id you wish to use for that driver to the new_id file for that driver. Doesn't that work?
thanks,
greg k-h
--
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