[PATCH 0/3] Another stab at making spidev usable with devicetree

[PATCH 0/3] Another stab at making spidev usable with devicetree

[Michal Suchanek]

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

-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[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.

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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 385 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Michal Suchanek]

On 26 June 2016 at 03:09, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1150 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Michal Suchanek]

On 26 June 2016 at 03:13, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1207 bytes --]

On Sun, Jun 26, 2016 at 04:12:10AM +0200, Michal Suchanek wrote:
> On 26 June 2016 at 03:13, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH 2/3] spi: of: allow instantiating slaves without a driver

[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.

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

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);
-- 
2.8.1

Re: [PATCH 2/3] spi: of: allow instantiating slaves without a driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 913 bytes --]

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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/3] spi: of: allow instantiating slaves without a driver

[Michal Suchanek]

On 26 June 2016 at 03:15, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] spi: of: allow instantiating slaves without a driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1563 bytes --]

On Sun, Jun 26, 2016 at 04:23:41AM +0200, Michal Suchanek wrote:
> On 26 June 2016 at 03:15, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/3] spi: of: allow instantiating slaves without a driver

[Michal Suchanek]

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

Re: [PATCH 2/3] spi: of: allow instantiating slaves without a driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1093 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/3] spi: of: allow instantiating slaves without a driver

[Michal Suchanek]

On 26 June 2016 at 13:58, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] spi: of: allow instantiating slaves without a driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 428 bytes --]

On Sun, Jun 26, 2016 at 02:39:20PM +0200, Michal Suchanek wrote:
> On 26 June 2016 at 13:58, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/3] spi: of: allow instantiating slaves without a driver

[Michal Suchanek]

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

Re: [PATCH 2/3] spi: of: allow instantiating slaves without a driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 635 bytes --]

On Sun, Jun 26, 2016 at 02:53:31PM +0200, Michal Suchanek wrote:
> On 26 June 2016 at 14:45, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/3] spi: of: allow instantiating slaves without a driver

[Michal Suchanek]

On 26 June 2016 at 14:57, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] spi: of: allow instantiating slaves without a driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1150 bytes --]

On Sun, Jun 26, 2016 at 05:19:54PM +0200, Michal Suchanek wrote:
> On 26 June 2016 at 14:57, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH 3/3] drivers core: allow id match override when manually binding driver

[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 <hramrach@gmail.com>
---
 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

diff --git a/drivers/base/Kconfig.debug b/drivers/base/Kconfig.debug
new file mode 100644
index 0000000..e21d3cc
--- /dev/null
+++ b/drivers/base/Kconfig.debug
@@ -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
diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 6470eb8..752c2a0 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -199,6 +199,73 @@ static ssize_t unbind_store(struct device_driver *drv, const char *buf,
 }
 static DRIVER_ATTR_WO(unbind);
 
+#ifdef CONFIG_DRIVER_MATCH_OVERRIDE_BUSES
+
+/* nul separated "" terminated strings */
+static const char *driver_override_buses;
+
+static inline void init_overrides(void)
+{
+	const char *buses_str = CONFIG_DRIVER_MATCH_OVERRIDE_BUSES;
+	char *transcript;
+	int i, len = strlen(buses_str);
+
+	if (!len)
+		return;
+
+	transcript = kzalloc(len + 1, GFP_KERNEL);
+	if (!transcript)
+		return;
+	driver_override_buses = transcript;
+
+	for (i = 0; i < len; i++) {
+
+		while (buses_str[i] == ' ')
+			i++;
+
+		if (buses_str[i]) {
+			const char *name_start = buses_str + i;
+			const char *name_end = strchrnul(name_start, ' ');
+			int name_len = name_end - name_start;
+
+			strncpy(transcript, name_start, name_len);
+			i += name_len;
+			transcript += name_len;
+			transcript++;
+		}
+	}
+}
+
+static inline bool driver_match_override(struct device_driver *drv,
+					 struct device *dev)
+{
+	struct bus_type *bus = bus_get(drv->bus);
+	const char *cmp_name = driver_override_buses;
+
+	while (cmp_name && *cmp_name) {
+		if (!strcmp(bus->name, cmp_name)) {
+			pr_notice("Overriding id match on manual driver binding:\n bus: %s  driver: %s  device: %s\n",
+				  bus->name, drv->name, dev_name(dev));
+			return true;
+		}
+		cmp_name += strlen(cmp_name);
+		cmp_name++;
+	}
+
+	return false;
+}
+
+#else /*CONFIG_DRIVER_MATCH_OVERRIDE_BUSES*/
+
+static inline void init_overrides(void)
+{}
+
+static inline bool driver_match_override(struct device_driver *drv,
+					 struct device *dev)
+{ return false; }
+
+#endif
+
 /*
  * Manually attach a device to a driver.
  * Note: the driver must want to bind to the device,
@@ -212,7 +279,8 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf,
 	int err = -ENODEV;
 
 	dev = bus_find_device_by_name(bus, NULL, buf);
-	if (dev && dev->driver == NULL && driver_match_device(drv, dev)) {
+	if (dev && dev->driver == NULL && (driver_match_device(drv, dev)
+	    || driver_match_override(drv, dev))) {
 		if (dev->parent)	/* Needed for USB */
 			device_lock(dev->parent);
 		device_lock(dev);
@@ -1280,5 +1348,7 @@ int __init buses_init(void)
 	if (!system_kset)
 		return -ENOMEM;
 
+	init_overrides();
+
 	return 0;
 }
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 1e9a607..f388212 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -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
-- 
2.8.1

Re: [PATCH 3/3] drivers core: allow id match override when manually binding driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1302 bytes --]

On Thu, Jun 23, 2016 at 05:41:20PM -0000, Michal Suchanek wrote:

> 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.

> +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

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 3/3] drivers core: allow id match override when manually binding driver

[Greg Kroah-Hartman]

On Thu, Jun 23, 2016 at 05:41:20PM -0000, Michal Suchanek wrote:
> 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 <hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  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
> 
> diff --git a/drivers/base/Kconfig.debug b/drivers/base/Kconfig.debug
> new file mode 100644
> index 0000000..e21d3cc
> --- /dev/null
> +++ b/drivers/base/Kconfig.debug
> @@ -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?

> +
> +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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
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

[Michal Suchanek]

On 26 June 2016 at 20:28, Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> On Thu, Jun 23, 2016 at 05:41:20PM -0000, Michal Suchanek wrote:
>> 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 <hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  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
>>
>> diff --git a/drivers/base/Kconfig.debug b/drivers/base/Kconfig.debug
>> new file mode 100644
>> index 0000000..e21d3cc
>> --- /dev/null
>> +++ b/drivers/base/Kconfig.debug
>> @@ -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?

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
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

[Greg Kroah-Hartman]

On Sun, Jun 26, 2016 at 09:07:08PM +0200, Michal Suchanek wrote:
> On 26 June 2016 at 20:28, Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> > On Thu, Jun 23, 2016 at 05:41:20PM -0000, Michal Suchanek wrote:
> >> 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 <hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> ---
> >>  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
> >>
> >> diff --git a/drivers/base/Kconfig.debug b/drivers/base/Kconfig.debug
> >> new file mode 100644
> >> index 0000000..e21d3cc
> >> --- /dev/null
> >> +++ b/drivers/base/Kconfig.debug
> >> @@ -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?
> 
> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
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

[Dan Williams]

On Thu, Jun 23, 2016 at 10:41 AM, Michal Suchanek <hramrach@gmail.com> wrote:
> 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 <hramrach@gmail.com>
> ---
>  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

[Michal Suchanek]

Hello,

On 26 June 2016 at 06:14, Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> On Thu, Jun 23, 2016 at 10:41 AM, Michal Suchanek <hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> 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 <hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  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".

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.

On 26 June 2016 at 03:17, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Thu, Jun 23, 2016 at 05:41:20PM -0000, Michal Suchanek wrote:
>
>> 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.
>
>> +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
>
> 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.

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
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

[Geert Uytterhoeven]

Hi Michal,

On Sun, Jun 26, 2016 at 8:44 AM, Michal Suchanek <hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 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 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html