[PATCH] Allowing Xilinx's AXI Quad widths different than 8 bits on userspace

[PATCH] Allowing Xilinx's AXI Quad widths different than 8 bits on userspace

[Alvaro Gamez Machado]

Hi,

We had a couple of days ago a nice discussion [1] about a patch that I sent
that was in need of clarification. I've taken into consideration all the
conversation and I believe this small series will manage my ultimate goal
(being able to use from user space a 32 bits wordwidth SPI slave with
Xilinx's AXI IP core) while being explained enough and through the proper
procedure.

I assume there may still need to be some discussion to go on with this, but
I thought it'd be clearer if we all had the code upfront in its current
state.

First patch documents the new device tree property, while the second one
implements it.

The third patch, that could be applied on its own regardless of the first
two, solves a bug that appears only in combination of spidev usage and a
master SPI device that does not support 8 bits as datawidth.

[1] I have not been able to find a link to the archives of linux-spi, sorry

Thanks,

Alvaro Gamez Machado (3):
  spi: xilinx: add description of new property xlnx,num-transfer-bits
  spi: xilinx: Add DT support for selecting transfer word width
  spi: set bits_per_word based on controller's bits_per_word_mask

 Documentation/devicetree/bindings/spi/spi-xilinx.txt | 4 +++-
 drivers/spi/spi-xilinx.c                             | 7 ++++++-
 drivers/spi/spi.c                                    | 2 ++
 3 files changed, 11 insertions(+), 2 deletions(-)

-- 
2.23.0

[PATCH] spi: xilinx: add description of new property xlnx,num-transfer-bits

[Alvaro Gamez Machado]

This property is used to set the number of bits per transfer (bits_per_word).

Xilinx' IP core allows either 8, 16 or 32, and is non changeable on runtime,
only when instantiating the core.

Signed-off-by: Alvaro Gamez Machado <alvaro.gamez@hazent.com>
---
 Documentation/devicetree/bindings/spi/spi-xilinx.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/spi/spi-xilinx.txt b/Documentation/devicetree/bindings/spi/spi-xilinx.txt
index dc924a5f71db..5f4ed3e5c994 100644
--- a/Documentation/devicetree/bindings/spi/spi-xilinx.txt
+++ b/Documentation/devicetree/bindings/spi/spi-xilinx.txt
@@ -8,7 +8,8 @@ Required properties:
 			  number.
 
 Optional properties:
-- xlnx,num-ss-bits	: Number of chip selects used.
+- xlnx,num-ss-bits	 : Number of chip selects used.
+- xlnx,num-transfer-bits : Number of bits per transfer. This will be 8 if not specified
 
 Example:
 	axi_quad_spi@41e00000 {
@@ -17,5 +18,6 @@ Example:
 			interrupts = <0 31 1>;
 			reg = <0x41e00000 0x10000>;
 			xlnx,num-ss-bits = <0x1>;
+			xlnx,num-transfer-bits = <32>;
 	};
 
-- 
2.23.0

[PATCH] spi: xilinx: Add DT support for selecting transfer word width

[Alvaro Gamez Machado]

This core supports either 8, 16 or 32 bits as word width. This value is only
settable on instantiation, and thus we need to support any of them by means
of the device tree.

Signed-off-by: Alvaro Gamez Machado <alvaro.gamez@hazent.com>
---
 drivers/spi/spi-xilinx.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
index 92ea22fedb33..46bb103ef30e 100644
--- a/drivers/spi/spi-xilinx.c
+++ b/drivers/spi/spi-xilinx.c
@@ -380,7 +380,7 @@ static int xilinx_spi_probe(struct platform_device *pdev)
 	struct xilinx_spi *xspi;
 	struct xspi_platform_data *pdata;
 	struct resource *res;
-	int ret, num_cs = 0, bits_per_word = 8;
+	int ret, num_cs = 0, bits_per_word;
 	struct spi_master *master;
 	u32 tmp;
 	u8 i;
@@ -392,6 +392,11 @@ static int xilinx_spi_probe(struct platform_device *pdev)
 	} else {
 		of_property_read_u32(pdev->dev.of_node, "xlnx,num-ss-bits",
 					  &num_cs);
+		ret = of_property_read_u32(pdev->dev.of_node,
+					   "xlnx,num-transfer-bits",
+					   &bits_per_word);
+		if (ret)
+			bits_per_word = 8;
 	}
 
 	if (!num_cs) {
-- 
2.23.0

[PATCH] spi: set bits_per_word based on controller's bits_per_word_mask

[Alvaro Gamez Machado]

By leaving this value unset, a default value of 8 was being set later on.

If it happens that the SPI master driver doesn't support this value of 8,
there will be an initial inconsistency between the SPI master and the device
itself. This isn't a problem for most devices because kernel drivers
associated to any device set the correct value themselves, but
for device drivers that do not change this value (such as spidev that
provides a userspace accesible device, which doesn't and can't know the
value it has to use), an error is raised:

xilinx_spi 44a10000.spi: can't setup spi1.0, status -22
spi_master spi1: spi_device register error /amba_pl/spi@44a10000/spidev@0
spi_master spi1: Failed to create SPI device for /amba_pl/spi@44a10000/spidev@0

When this happens, the expected /dev/spidevX.Y device isn't created,
and thus the user can't use the SPI_IOC_WR_BITS_PER_WORD ioctl to set the
desired value.

This change sets the initial value of bits_per to the smallest word that the
controller allows, so of_spi_parse_dt sets a value that is usable by the
controller.

Signed-off-by: Alvaro Gamez Machado <alvaro.gamez@hazent.com>
---
 drivers/spi/spi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index f9502dbbb5c1..794e20e54237 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1792,6 +1792,8 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
 	}
 	spi->max_speed_hz = value;
 
+	spi->bits_per_word = ffs(ctlr->bits_per_word_mask);
+
 	return 0;
 }
 
-- 
2.23.0

Re: [PATCH] spi: set bits_per_word based on controller's bits_per_word_mask

[Mark Brown]

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

On Thu, Oct 24, 2019 at 01:07:57PM +0200, Alvaro Gamez Machado wrote:
> By leaving this value unset, a default value of 8 was being set later on.
> 
> If it happens that the SPI master driver doesn't support this value of 8,
> there will be an initial inconsistency between the SPI master and the device
> itself. This isn't a problem for most devices because kernel drivers

This will break things, client devices are working on the basis that the
default transfer width is 8 bits.  As I've repeatedly said if we have
different parts of the system with different ideas about the word size
we're going to end up with data corruption.  Please take this feedback
on board.

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

Re: [PATCH] spi: xilinx: add description of new property xlnx,num-transfer-bits

[Mark Brown]

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

On Thu, Oct 24, 2019 at 01:07:55PM +0200, Alvaro Gamez Machado wrote:
> This property is used to set the number of bits per transfer (bits_per_word).
> 
> Xilinx' IP core allows either 8, 16 or 32, and is non changeable on runtime,
> only when instantiating the core.

When sending a patch series you should number the patches within the
series - if you use git format-patch to generate the series it'll do
that for you.  Instead of [PATCH] it should say [PATCH x/y].

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

Applied "spi: xilinx: add description of new property xlnx,num-transfer-bits" to the spi tree

[Mark Brown]

The patch

   spi: xilinx: add description of new property xlnx,num-transfer-bits

has been applied to the spi tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-5.5

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From e3354b17b4ac10ad2c23e244444ab38927a69ee9 Mon Sep 17 00:00:00 2001
From: Alvaro Gamez Machado <alvaro.gamez@hazent.com>
Date: Thu, 24 Oct 2019 13:07:55 +0200
Subject: [PATCH] spi: xilinx: add description of new property
 xlnx,num-transfer-bits

This property is used to set the number of bits per transfer (bits_per_word).

Xilinx' IP core allows either 8, 16 or 32, and is non changeable on runtime,
only when instantiating the core.

Signed-off-by: Alvaro Gamez Machado <alvaro.gamez@hazent.com>
Link: https://lore.kernel.org/r/20191024110757.25820-2-alvaro.gamez@hazent.com
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 Documentation/devicetree/bindings/spi/spi-xilinx.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/spi/spi-xilinx.txt b/Documentation/devicetree/bindings/spi/spi-xilinx.txt
index dc924a5f71db..5f4ed3e5c994 100644
--- a/Documentation/devicetree/bindings/spi/spi-xilinx.txt
+++ b/Documentation/devicetree/bindings/spi/spi-xilinx.txt
@@ -8,7 +8,8 @@ Required properties:
 			  number.
 
 Optional properties:
-- xlnx,num-ss-bits	: Number of chip selects used.
+- xlnx,num-ss-bits	 : Number of chip selects used.
+- xlnx,num-transfer-bits : Number of bits per transfer. This will be 8 if not specified
 
 Example:
 	axi_quad_spi@41e00000 {
@@ -17,5 +18,6 @@ Example:
 			interrupts = <0 31 1>;
 			reg = <0x41e00000 0x10000>;
 			xlnx,num-ss-bits = <0x1>;
+			xlnx,num-transfer-bits = <32>;
 	};
 
-- 
2.20.1

Applied "spi: xilinx: Add DT support for selecting transfer word width" to the spi tree

[Mark Brown]

The patch

   spi: xilinx: Add DT support for selecting transfer word width

has been applied to the spi tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-5.5

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From e58f7d15e6beb255b3907054a0536db77c979a31 Mon Sep 17 00:00:00 2001
From: Alvaro Gamez Machado <alvaro.gamez@hazent.com>
Date: Thu, 24 Oct 2019 13:07:56 +0200
Subject: [PATCH] spi: xilinx: Add DT support for selecting transfer word width

This core supports either 8, 16 or 32 bits as word width. This value is only
settable on instantiation, and thus we need to support any of them by means
of the device tree.

Signed-off-by: Alvaro Gamez Machado <alvaro.gamez@hazent.com>
Link: https://lore.kernel.org/r/20191024110757.25820-3-alvaro.gamez@hazent.com
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/spi/spi-xilinx.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
index d5f9d5fbb3e8..8dd2bb99cb4d 100644
--- a/drivers/spi/spi-xilinx.c
+++ b/drivers/spi/spi-xilinx.c
@@ -391,7 +391,7 @@ static int xilinx_spi_probe(struct platform_device *pdev)
 	struct xilinx_spi *xspi;
 	struct xspi_platform_data *pdata;
 	struct resource *res;
-	int ret, num_cs = 0, bits_per_word = 8;
+	int ret, num_cs = 0, bits_per_word;
 	struct spi_master *master;
 	u32 tmp;
 	u8 i;
@@ -403,6 +403,11 @@ static int xilinx_spi_probe(struct platform_device *pdev)
 	} else {
 		of_property_read_u32(pdev->dev.of_node, "xlnx,num-ss-bits",
 					  &num_cs);
+		ret = of_property_read_u32(pdev->dev.of_node,
+					   "xlnx,num-transfer-bits",
+					   &bits_per_word);
+		if (ret)
+			bits_per_word = 8;
 	}
 
 	if (!num_cs) {
-- 
2.20.1

Re: [PATCH] spi: set bits_per_word based on controller's bits_per_word_mask

[Alvaro Gamez Machado]

On Thu, Oct 24, 2019 at 12:13:00PM +0100, Mark Brown wrote:
> On Thu, Oct 24, 2019 at 01:07:57PM +0200, Alvaro Gamez Machado wrote:
> > By leaving this value unset, a default value of 8 was being set later on.
> > 
> > If it happens that the SPI master driver doesn't support this value of 8,
> > there will be an initial inconsistency between the SPI master and the device
> > itself. This isn't a problem for most devices because kernel drivers
> 
> This will break things, client devices are working on the basis that the
> default transfer width is 8 bits.  As I've repeatedly said if we have
> different parts of the system with different ideas about the word size
> we're going to end up with data corruption.  Please take this feedback
> on board.

Oh, ok. I didn't understand this cleary from previous mails, now I see what
you mean.

I think then the only way this would be feasible is to check if 8 bits is an
acceptable number for the master and, if it isn't, apply the lowest
available data width. I believe this cannot break anything, as it leaves 8
as the default unless the master can't work with that number, in which case
it really doesn't matter what client device wants because the hardware can't
provide it.

Thanks!

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 794e20e54237..4e26ac79e133 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -3079,8 +3079,12 @@ int spi_setup(struct spi_device *spi)
                return -EINVAL;
        }
 
-       if (!spi->bits_per_word)
-               spi->bits_per_word = 8;
+       if (!spi->bits_per_word) {
+               if (spi->controller->bits_per_word_mask & SPI_BPW_MASK(8))
+                       spi->bits_per_word = 8;
+               else
+                       spi->bits_per_word = ffs(spi->controller->bits_per_word_mask);
+       }
 
        status = __spi_validate_bits_per_word(spi->controller,
                                              spi->bits_per_word);





-- 
Alvaro G. M.

Re: [PATCH] spi: set bits_per_word based on controller's bits_per_word_mask

[Mark Brown]

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

On Thu, Oct 24, 2019 at 02:54:37PM +0200, Alvaro Gamez Machado wrote:

> I think then the only way this would be feasible is to check if 8 bits is an
> acceptable number for the master and, if it isn't, apply the lowest
> available data width. I believe this cannot break anything, as it leaves 8
> as the default unless the master can't work with that number, in which case
> it really doesn't matter what client device wants because the hardware can't
> provide it.

No, that still leaves the slave driver thinking it's sending 8 bits when
really it's sending something else - the default is just 8 bits, if the
controller can't do it then the transfer can't happen and there's an
error.  It's not a good idea to carry on if we're likely to introduce
data corruption.

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

Re: [PATCH] spi: set bits_per_word based on controller's bits_per_word_mask

[Alvaro Gamez Machado]

On Thu, Oct 24, 2019 at 02:11:29PM +0100, Mark Brown wrote:
> On Thu, Oct 24, 2019 at 02:54:37PM +0200, Alvaro Gamez Machado wrote:
> 
> > I think then the only way this would be feasible is to check if 8 bits is an
> > acceptable number for the master and, if it isn't, apply the lowest
> > available data width. I believe this cannot break anything, as it leaves 8
> > as the default unless the master can't work with that number, in which case
> > it really doesn't matter what client device wants because the hardware can't
> > provide it.
> 
> No, that still leaves the slave driver thinking it's sending 8 bits when
> really it's sending something else - the default is just 8 bits, if the
> controller can't do it then the transfer can't happen and there's an
> error.  It's not a good idea to carry on if we're likely to introduce
> data corruption.

Well, yes. But I don't think that's a software issue but a hardware one.

If you have a board that has a SPI master that cannot talk to an 8 bits
device and you expect to communicate with anything that accepts 8 bits
you're not going to be able to. Either the kernel raises an error or it
shuts up and tries its best. I understand the first option is better, but I
also think that's not a software issue, that hardware simply cannot work as
is regardless of what we do in software. The hardware devices simply can't
talk to each other.


-- 
Alvaro G. M.

Re: [PATCH] spi: set bits_per_word based on controller's bits_per_word_mask

[Mark Brown]

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

On Thu, Oct 24, 2019 at 03:18:57PM +0200, Alvaro Gamez Machado wrote:
> On Thu, Oct 24, 2019 at 02:11:29PM +0100, Mark Brown wrote:

> > No, that still leaves the slave driver thinking it's sending 8 bits when
> > really it's sending something else - the default is just 8 bits, if the
> > controller can't do it then the transfer can't happen and there's an
> > error.  It's not a good idea to carry on if we're likely to introduce
> > data corruption.

> Well, yes. But I don't think that's a software issue but a hardware one.

> If you have a board that has a SPI master that cannot talk to an 8 bits
> device and you expect to communicate with anything that accepts 8 bits
> you're not going to be able to. Either the kernel raises an error or it
> shuts up and tries its best. I understand the first option is better, but I
> also think that's not a software issue, that hardware simply cannot work as
> is regardless of what we do in software. The hardware devices simply can't
> talk to each other.

Sure, but then it's going to be easier to diagnose problems if the
software says that it's identified a data format problem than it is to
try to figure out the results of data corruption.  There is also the
possibility that if the formats the hardware needs to use can be made to
align through rewriting software can persuade things to interoperate
(eg, if all the transfers are multiples of 32 bits then a device can
probably work with a controller that only supports 32 bit words even if
the device expects a byte stream) but that requires explicit code rather
than just silently accepting the data and hoping for the best.

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

Re: [PATCH] spi: set bits_per_word based on controller's bits_per_word_mask

[Alvaro Gamez Machado]

On Thu, Oct 24, 2019 at 02:41:16PM +0100, Mark Brown wrote:
> On Thu, Oct 24, 2019 at 03:18:57PM +0200, Alvaro Gamez Machado wrote:
> > On Thu, Oct 24, 2019 at 02:11:29PM +0100, Mark Brown wrote:
> 
> > > No, that still leaves the slave driver thinking it's sending 8 bits when
> > > really it's sending something else - the default is just 8 bits, if the
> > > controller can't do it then the transfer can't happen and there's an
> > > error.  It's not a good idea to carry on if we're likely to introduce
> > > data corruption.
> 
> > Well, yes. But I don't think that's a software issue but a hardware one.
> 
> > If you have a board that has a SPI master that cannot talk to an 8 bits
> > device and you expect to communicate with anything that accepts 8 bits
> > you're not going to be able to. Either the kernel raises an error or it
> > shuts up and tries its best. I understand the first option is better, but I
> > also think that's not a software issue, that hardware simply cannot work as
> > is regardless of what we do in software. The hardware devices simply can't
> > talk to each other.
> 
> Sure, but then it's going to be easier to diagnose problems if the
> software says that it's identified a data format problem than it is to
> try to figure out the results of data corruption.  There is also the
> possibility that if the formats the hardware needs to use can be made to
> align through rewriting software can persuade things to interoperate
> (eg, if all the transfers are multiples of 32 bits then a device can
> probably work with a controller that only supports 32 bit words even if
> the device expects a byte stream) but that requires explicit code rather
> than just silently accepting the data and hoping for the best.

I guess there could be some workarounds to help in that situation. But I see
that as an hypothetical occurrence whereas I have with me a physical board
that needs 32 bits in both master and slave that I want to access using
spidev and cannot. Lots of I's in that sentence, I know :)

Anyhow, if this is not acceptable, the only other alternative I see right
now is adding a new DT property, as in my emails yesterday.


diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index f9502dbbb5c1..06424b7b0d73 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1792,6 +1792,10 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
 	}
 	spi->max_speed_hz = value;
 
+	/* Bits per word */
+	if (!of_property_read_u32(nc, "spi-bits-per-word", &value))
+		spi->bits_per_word = value;
+
 	return 0;
 }

What do you think about this? This requires the user to explicitly select a
different value rather than the default, so it can't break anything and
would allow with the diagnostic of such broken hardware.

I still think I like more changing the default to something the master is
able to do. Otherwise we're going to keep trying to send 8 bits using a
master that simply cannot do that, but this solution would work fine as
well.

Regards,

-- 
Alvaro G. M.

Re: [PATCH] spi: set bits_per_word based on controller's bits_per_word_mask

[Mark Brown]

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

On Thu, Oct 24, 2019 at 04:07:32PM +0200, Alvaro Gamez Machado wrote:

> I guess there could be some workarounds to help in that situation. But I see
> that as an hypothetical occurrence whereas I have with me a physical board
> that needs 32 bits in both master and slave that I want to access using
> spidev and cannot. Lots of I's in that sentence, I know :)

If you want to access this using spidev then add support for changing
the word size to spidev and use that as I think Geert already suggested.

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

Re: [PATCH] spi: set bits_per_word based on controller's bits_per_word_mask

[Alvaro Gamez Machado]

On Thu, Oct 24, 2019 at 06:40:33PM +0100, Mark Brown wrote:
> On Thu, Oct 24, 2019 at 04:07:32PM +0200, Alvaro Gamez Machado wrote:
> 
> > I guess there could be some workarounds to help in that situation. But I see
> > that as an hypothetical occurrence whereas I have with me a physical board
> > that needs 32 bits in both master and slave that I want to access using
> > spidev and cannot. Lots of I's in that sentence, I know :)
> 
> If you want to access this using spidev then add support for changing
> the word size to spidev and use that as I think Geert already suggested.

I've been trying to do so for a couple hours and I've reached a conclusion.

I've been too focused on my personal use case (too many I's indeed) and
thought that the problem was in fact in spidev as Geert indicated, but now
I think it isn't, so I must present my excuses for mistakenly drive the
conversation in that direction. Geert also thought this could be an SPI core
bug, and he was right on that, I think.

In fact, not a single line of spidev is being executed when the error
message is printed:

xilinx_spi 44a00000.spi: at 0x44A00000 mapped to 0x(ptrval), irq=3
xilinx_spi 44a10000.spi: can't setup spi1.0, status -22
spi_master spi1: spi_device register error /amba_pl/spi@44a10000/spidev@0
spi_master spi1: Failed to create SPI device for /amba_pl/spi@44a10000/spidev@0

This does not come from spidev but directly from spi. What is happening is
that when SPI slaves are defined via DT, their bits_per_word value is always
unset (as 0), which turns later on in a default value of 8.

Inside spi_setup function immediately after setting this default value to 8,
__spi_validate_bits_per_word is called, that checks whether bits_per_word for
the slave matches the controller available bitwidths:

	if (!spi->bits_per_word)
		spi->bits_per_word = 8;

	status = __spi_validate_bits_per_word(spi->controller,
					      spi->bits_per_word);

	if (status)
		return status;


This means that it doesn't really matter which is the driver that is going
to claim the specific SPI slave. It may be spidev as in my use case, or it
may really be any other driver. But its probe() function is never going to
be called because the error is not raised inside the driver, but immediately
after forcibly setting the default value to 8 in spi.c

I can't modify spidev because spidev doesn't even know this is happening.

I was completely wrong in my blaming of spidev, but now I'm reassured that
my previous patches were going to core of the issue, regardless of my
mistaken initial diagnostic.

Thanks,

-- 
Alvaro G. M.

Re: [PATCH] spi: set bits_per_word based on controller's bits_per_word_mask

[Mark Brown]

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

On Fri, Oct 25, 2019 at 08:39:48AM +0200, Alvaro Gamez Machado wrote:

> to claim the specific SPI slave. It may be spidev as in my use case, or it
> may really be any other driver. But its probe() function is never going to
> be called because the error is not raised inside the driver, but immediately
> after forcibly setting the default value to 8 in spi.c

Then you need to extend the validation the core is doing here to
skip this parameter when registering the device and only enforce
it after a driver is bound, we don't have a driver at the time we
initially register the device so we can't enforce this.

> I can't modify spidev because spidev doesn't even know this is happening.

You are, at some point, going to need to set your spidev to 32
bits per word (spidev does already support this).

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

Re: [PATCH] spi: set bits_per_word based on controller's bits_per_word_mask

[Alvaro Gamez Machado]

On Fri, Oct 25, 2019 at 12:56:55PM +0100, Mark Brown wrote:
> On Fri, Oct 25, 2019 at 08:39:48AM +0200, Alvaro Gamez Machado wrote:
> 
> > to claim the specific SPI slave. It may be spidev as in my use case, or it
> > may really be any other driver. But its probe() function is never going to
> > be called because the error is not raised inside the driver, but immediately
> > after forcibly setting the default value to 8 in spi.c
> 
> Then you need to extend the validation the core is doing here to
> skip this parameter when registering the device and only enforce
> it after a driver is bound, we don't have a driver at the time we
> initially register the device so we can't enforce this.

Ok, so I can remove the call to __spi_validate_bits_per_word() from
spi_setup(), and instead place it on spi_drv_probe after calling
spi_drv_probe() and detach the device if it returns with an error. The
behaviour would be basically the same as it is now, but the slave driver
will have an opportunity to choose a different transfer width if it wants to
and can match the capabilities of the master.

> 
> > I can't modify spidev because spidev doesn't even know this is happening.
> 
> You are, at some point, going to need to set your spidev to 32
> bits per word (spidev does already support this).

If I do the former, spidev will then need to load the initial bits per word
value from DT before returning from probe(), otherwise it will end up detached
when the check above is performed.

Should I go for this?

Thanks

-- 
Alvaro G. M.