| Message ID | 8b66102e44829c5dc445c17f7bfad1050840b9f0.1466696079.git.hramrach@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jun 24, 2016 at 04:20:32PM +0200, Michal Suchanek wrote: > 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.
On Thu, Jun 23, 2016 at 05:41:19PM -0000, Michal Suchanek wrote: > 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? > 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. of_match_device() compiles out when !OF. > +static const struct of_device_id spidev_check[] = { > + { .compatible = "spidev" }, > + {} > +}; The indentation here is completely non-standard. > - 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.
On 26 June 2016 at 03:09, Mark Brown <broonie@kernel.org> wrote: > On Fri, Jun 24, 2016 at 04:20:32PM +0200, Michal Suchanek wrote: > >> 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 26 June 2016 at 03:13, Mark Brown <broonie@kernel.org> wrote: > On Thu, Jun 23, 2016 at 05:41:19PM -0000, Michal Suchanek wrote: > >> 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. > >> 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. > > of_match_device() compiles out when !OF. > >> +static const struct of_device_id spidev_check[] = { >> + { .compatible = "spidev" }, >> + {} >> +}; > > The indentation here is completely non-standard. > >> - 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jun 26, 2016 at 04:12:10AM +0200, Michal Suchanek wrote: > On 26 June 2016 at 03:13, Mark Brown <broonie@kernel.org> wrote: > > On Thu, Jun 23, 2016 at 05:41:19PM -0000, Michal Suchanek wrote: > >> 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. 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.
diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c index e3c19f3..8045baf 100644 --- a/drivers/spi/spidev.c +++ b/drivers/spi/spidev.c @@ -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 */
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. Signed-off-by: Michal Suchanek <hramrach@gmail.com> --- drivers/spi/spidev.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)