Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1430697 > unrolled thread

[PATCH 1/3] spi: spidev: fix the check for spidev in dt

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/3] spi: spidev: fix the check for spidev in dt Michal Suchanek <hramrach@gmail.com> - 2016-06-24 16:30 +0200
    Re: [PATCH 1/3] spi: spidev: fix the check for spidev in dt Mark Brown <broonie@kernel.org> - 2016-06-26 03:20 +0200
      Re: [PATCH 1/3] spi: spidev: fix the check for spidev in dt Michal Suchanek <hramrach@gmail.com> - 2016-06-26 04:20 +0200
        Re: [PATCH 1/3] spi: spidev: fix the check for spidev in dt Mark Brown <broonie@kernel.org> - 2016-06-26 13:10 +0200
    Re: [PATCH 1/3] spi: spidev: fix the check for spidev in dt Mark Brown <broonie@kernel.org> - 2016-06-26 03:20 +0200
      Re: [PATCH 1/3] spi: spidev: fix the check for spidev in dt Michal Suchanek <hramrach@gmail.com> - 2016-06-26 04:00 +0200

#1430697 — [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.

Signed-off-by: Michal Suchanek <hramrach@gmail.com>
---
 drivers/spi/spidev.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

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 */
-- 
2.8.1

[toc] | [next] | [standalone]

#1431276

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

#1431285

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

[toc] | [prev] | [next] | [standalone]

#1431434

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

#1431280

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

#1431284

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web