| Message ID | 42e65e5c714f079cb4b85761e59fb700d4550a26.1468880530.git.hramrach@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 19, 2016 at 12:35:41AM +0200, 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 used with spidev. > 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", Nothing has change since you last sent this patch which converts of_modailias_node() into something which looks up a driver so the patch description still fails to describe what the patch is doing. Please don't ignore review comments, people are generally making them for a reason and are likely to have the same concerns if issues remain unaddressed. Having to repeat the same comments can get repetitive and make people question the value of time spent reviewing. If you disagree with the review comments that's fine but you need to reply and discuss your concerns so that the reviewer can understand your decisions.
Hello, On 19 July 2016 at 01:02, Mark Brown <broonie@kernel.org> wrote: > On Tue, Jul 19, 2016 at 12:35:41AM +0200, 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 used with spidev. > >> 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", > > Nothing has change since you last sent this patch which converts > of_modailias_node() into something which looks up a driver so the > patch description still fails to describe what the patch is doing. > > Please don't ignore review comments, people are generally making them > for a reason and are likely to have the same concerns if issues remain > unaddressed. Having to repeat the same comments can get repetitive and > make people question the value of time spent reviewing. If you disagree > with the review comments that's fine but you need to reply and discuss > your concerns so that the reviewer can understand your decisions. I have split the other part of the patch. Regarding the commit message if you have suggestion for better wording please do share it. From my point of view the conceptual change described in the commit message is that whenever SPI slave node is encountered in devicetree you get either a device with active driver or a device with no driver whereas previously you either got a device with active driver or no device. So yes, it's about allowing SPI slave devices without a driver bound to them. The technical implementation detail that can be seen in the patch is ignoring the return value of of_modalias_node. 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 Tue, Jul 19, 2016 at 10:31:54AM +0200, Michal Suchanek wrote: > On 19 July 2016 at 01:02, Mark Brown <broonie@kernel.org> wrote: > > On Tue, Jul 19, 2016 at 12:35:41AM +0200, 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 used with spidev. > > Nothing has change since you last sent this patch which converts > > of_modailias_node() into something which looks up a driver so the > > patch description still fails to describe what the patch is doing. > I have split the other part of the patch. Regarding the commit message > if you have suggestion for better wording please do share it. As covered in SubmittingPatches your commit message should describe what the change does and what the intended effect is. If we were looking for a device driver the code would be looking up a struct device_driver or some other struct that contains one. > From my point of view the conceptual change described in the commit message > is that whenever SPI slave node is encountered in devicetree you get either > a device with active driver or a device with no driver whereas > previously you either > got a device with active driver or no device. So yes, it's about This is not the case, it is perfectly possible to have a device with no driver bound to it otherwise it would not be possible to use loadable modules for drivers.
On 19 July 2016 at 12:52, Mark Brown <broonie@kernel.org> wrote: > On Tue, Jul 19, 2016 at 10:31:54AM +0200, Michal Suchanek wrote: >> On 19 July 2016 at 01:02, Mark Brown <broonie@kernel.org> wrote: >> > On Tue, Jul 19, 2016 at 12:35:41AM +0200, 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 used with spidev. > >> > Nothing has change since you last sent this patch which converts >> > of_modailias_node() into something which looks up a driver so the >> > patch description still fails to describe what the patch is doing. > >> I have split the other part of the patch. Regarding the commit message >> if you have suggestion for better wording please do share it. > > As covered in SubmittingPatches your commit message should describe what > the change does and what the intended effect is. If we were looking for > a device driver the code would be looking up a struct device_driver or > some other struct that contains one. > >> From my point of view the conceptual change described in the commit message >> is that whenever SPI slave node is encountered in devicetree you get either >> a device with active driver or a device with no driver whereas >> previously you either >> got a device with active driver or no device. So yes, it's about > > This is not the case, it is perfectly possible to have a device with no > driver bound to it otherwise it would not be possible to use loadable > modules for drivers. Ok, I missed this part. That makes the commit message indeed broken. 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
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index f3ea768..f0de2ec 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1489,9 +1489,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 */
SPI slave devices are not created when looking up driver for the slave fails. Create a device anyway so it can be used with spidev. Signed-off-by: Michal Suchanek <hramrach@gmail.com> --- drivers/spi/spi.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)