| Message ID | ebe2a48124441819adab773b28cd12425ba5bda4.1466696079.git.hramrach@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 23, 2016 at 05:41:20PM -0000, Michal Suchanek wrote: > 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. > @@ -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; > 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?
On 26 June 2016 at 03:15, Mark Brown <broonie@kernel.org> wrote: > On Thu, Jun 23, 2016 at 05:41:20PM -0000, Michal Suchanek wrote: >> 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. > >> @@ -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; >> > > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jun 26, 2016 at 04:23:41AM +0200, Michal Suchanek wrote: > On 26 June 2016 at 03:15, Mark Brown <broonie@kernel.org> wrote: > > 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. > 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. > 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.
On 26 June 2016 at 13:21, Mark Brown <broonie@kernel.org> wrote: > On Sun, Jun 26, 2016 at 04:23:41AM +0200, Michal Suchanek wrote: >> On 26 June 2016 at 03:15, Mark Brown <broonie@kernel.org> wrote: > >> > 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. > >> 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. > (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. > 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. > >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jun 26, 2016 at 01:35:46PM +0200, Michal Suchanek wrote: > On 26 June 2016 at 13:21, Mark Brown <broonie@kernel.org> wrote: > > 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... > > 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.
On 26 June 2016 at 13:58, Mark Brown <broonie@kernel.org> wrote: > On Sun, Jun 26, 2016 at 01:35:46PM +0200, Michal Suchanek wrote: >> On 26 June 2016 at 13:21, Mark Brown <broonie@kernel.org> wrote: > >> > 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... 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. > >> > 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. 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jun 26, 2016 at 02:39:20PM +0200, Michal Suchanek wrote: > On 26 June 2016 at 13:58, Mark Brown <broonie@kernel.org> wrote: > > 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.
On 26 June 2016 at 14:45, Mark Brown <broonie@kernel.org> wrote: > On Sun, Jun 26, 2016 at 02:39:20PM +0200, Michal Suchanek wrote: >> On 26 June 2016 at 13:58, Mark Brown <broonie@kernel.org> wrote: > >> > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jun 26, 2016 at 02:53:31PM +0200, Michal Suchanek wrote: > On 26 June 2016 at 14:45, Mark Brown <broonie@kernel.org> wrote: > > 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.
On 26 June 2016 at 14:57, Mark Brown <broonie@kernel.org> wrote: > On Sun, Jun 26, 2016 at 02:53:31PM +0200, Michal Suchanek wrote: >> On 26 June 2016 at 14:45, Mark Brown <broonie@kernel.org> wrote: > >> > 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. 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jun 26, 2016 at 05:19:54PM +0200, Michal Suchanek wrote: > On 26 June 2016 at 14:57, Mark Brown <broonie@kernel.org> wrote: > > 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. 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.
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 0239b45..73b1125 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -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);
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. Signed-off-by: Michal Suchanek <hramrach@gmail.com> --- drivers/spi/spi.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)