[PATCH net] qca_spi: Fix clock speed for multiple QCA7000

[PATCH net] qca_spi: Fix clock speed for multiple QCA7000

[Stefan Wahren]

Storing the maximum clock speed in module parameter qcaspi_clkspeed
has the unintended side effect that the first probed instance
defines the value for all other instances. Fix this issue by storing
it in max_speed_hz of the relevant SPI device.

This fix keeps the priority of the speed parameter (module parameter,
device tree property, driver default).

Fixes: 291ab06ecf67 ("net: qualcomm: new Ethernet over SPI driver for QCA7000")
Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 drivers/net/ethernet/qualcomm/qca_spi.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/qca_spi.c b/drivers/net/ethernet/qualcomm/qca_spi.c
index ef9c02b000e4..a79fd2d66534 100644
--- a/drivers/net/ethernet/qualcomm/qca_spi.c
+++ b/drivers/net/ethernet/qualcomm/qca_spi.c
@@ -909,17 +909,15 @@ qca_spi_probe(struct spi_device *spi)
 	legacy_mode = of_property_read_bool(spi->dev.of_node,
 					    "qca,legacy-mode");

-	if (qcaspi_clkspeed == 0) {
-		if (spi->max_speed_hz)
-			qcaspi_clkspeed = spi->max_speed_hz;
-		else
-			qcaspi_clkspeed = QCASPI_CLK_SPEED;
-	}
-
-	if ((qcaspi_clkspeed < QCASPI_CLK_SPEED_MIN) ||
-	    (qcaspi_clkspeed > QCASPI_CLK_SPEED_MAX)) {
-		dev_err(&spi->dev, "Invalid clkspeed: %d\n",
-			qcaspi_clkspeed);
+	if (qcaspi_clkspeed)
+		spi->max_speed_hz = qcaspi_clkspeed;
+	else if (!spi->max_speed_hz)
+		spi->max_speed_hz = QCASPI_CLK_SPEED;
+
+	if (spi->max_speed_hz < QCASPI_CLK_SPEED_MIN ||
+	    spi->max_speed_hz > QCASPI_CLK_SPEED_MAX) {
+		dev_err(&spi->dev, "Invalid clkspeed: %u\n",
+			spi->max_speed_hz);
 		return -EINVAL;
 	}

@@ -944,14 +942,13 @@ qca_spi_probe(struct spi_device *spi)
 		return -EINVAL;
 	}

-	dev_info(&spi->dev, "ver=%s, clkspeed=%d, burst_len=%d, pluggable=%d\n",
+	dev_info(&spi->dev, "ver=%s, clkspeed=%u, burst_len=%d, pluggable=%d\n",
 		 QCASPI_DRV_VERSION,
-		 qcaspi_clkspeed,
+		 spi->max_speed_hz,
 		 qcaspi_burst_len,
 		 qcaspi_pluggable);

 	spi->mode = SPI_MODE_3;
-	spi->max_speed_hz = qcaspi_clkspeed;
 	if (spi_setup(spi) < 0) {
 		dev_err(&spi->dev, "Unable to setup SPI device\n");
 		return -EFAULT;
--
2.34.1

Re: [PATCH net] qca_spi: Fix clock speed for multiple QCA7000

[Jakub Kicinski]

On Mon,  2 Dec 2024 16:58:53 +0100 Stefan Wahren wrote:
> Storing the maximum clock speed in module parameter qcaspi_clkspeed
> has the unintended side effect that the first probed instance
> defines the value for all other instances. Fix this issue by storing
> it in max_speed_hz of the relevant SPI device.
> 
> This fix keeps the priority of the speed parameter (module parameter,
> device tree property, driver default).

I think we should also delete the (seemingly unused?) qca->clkspeed 
in this change. Otherwise it looks surprising that we still assign
the module param to it?
-- 
pw-bot: cr

Re: [PATCH net] qca_spi: Fix clock speed for multiple QCA7000

[Stefan Wahren]

Hi Jakub,

Am 05.12.24 um 03:48 schrieb Jakub Kicinski:
> On Mon,  2 Dec 2024 16:58:53 +0100 Stefan Wahren wrote:
>> Storing the maximum clock speed in module parameter qcaspi_clkspeed
>> has the unintended side effect that the first probed instance
>> defines the value for all other instances. Fix this issue by storing
>> it in max_speed_hz of the relevant SPI device.
>>
>> This fix keeps the priority of the speed parameter (module parameter,
>> device tree property, driver default).
> I think we should also delete the (seemingly unused?) qca->clkspeed
> in this change. Otherwise it looks surprising that we still assign
> the module param to it?
Good catch! But shouldn't this be a separate clean-up, because the
clkspeed was already unused before?

Re: [PATCH net] qca_spi: Fix clock speed for multiple QCA7000

[Jakub Kicinski]

On Thu, 5 Dec 2024 12:07:21 +0100 Stefan Wahren wrote:
> > I think we should also delete the (seemingly unused?) qca->clkspeed
> > in this change. Otherwise it looks surprising that we still assign
> > the module param to it?  
> Good catch! But shouldn't this be a separate clean-up, because the
> clkspeed was already unused before?

I'd put it in the same change for the sake of backporters.
Otherwise they may worry there was an intermediate patch,
or perhaps there even is one, I haven't checked.
If we delete the field and the assignment - if the backport
compiles its probably correct.