1. Patchew
  2. linux
  3. [PATCH 0/4] Miscellaneous fixes and clean-ups
  4. View patch

[PATCH 3/4] spi: cadence-quadspi: Fix cqspi_setup_flash()

Santhosh Kumar K posted 4 patches 4 weeks ago
There is a newer version of this series
[PATCH 3/4] spi: cadence-quadspi: Fix cqspi_setup_flash()
Posted by Santhosh Kumar K 4 weeks ago 
The 'max_cs' stores the largest chip select number. It should only
be updated when the current 'cs' is greater than existing 'max_cs'. So,
fix the condition accordingly.

Fixes: 0f3841a5e115 ("spi: cadence-qspi: report correct number of chip-select")
Signed-off-by: Santhosh Kumar K <s-k6@ti.com>
---
 drivers/spi/spi-cadence-quadspi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index 447a32a08a93..da3ec15abb3e 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -1722,7 +1722,7 @@ static const struct spi_controller_mem_caps cqspi_mem_caps = {
 
 static int cqspi_setup_flash(struct cqspi_st *cqspi)
 {
-	unsigned int max_cs = cqspi->num_chipselect - 1;
+	unsigned int max_cs = 0;
 	struct platform_device *pdev = cqspi->pdev;
 	struct device *dev = &pdev->dev;
 	struct cqspi_flash_pdata *f_pdata;
@@ -1740,7 +1740,7 @@ static int cqspi_setup_flash(struct cqspi_st *cqspi)
 		if (cs >= cqspi->num_chipselect) {
 			dev_err(dev, "Chip select %d out of range.\n", cs);
 			return -EINVAL;
-		} else if (cs < max_cs) {
+		} else if (cs > max_cs) {
 			max_cs = cs;
 		}
 
-- 
2.34.1
Re: [PATCH 3/4] spi: cadence-quadspi: Fix cqspi_setup_flash()
Posted by Théo Lebrun 4 weeks ago 
Hello Santhosh,

On Thu Sep 4, 2025 at 3:31 PM CEST, Santhosh Kumar K wrote:
> The 'max_cs' stores the largest chip select number. It should only
> be updated when the current 'cs' is greater than existing 'max_cs'. So,
> fix the condition accordingly.

Good catch. Current code can only work with one chip-select.

Reviewed-by: Théo Lebrun <theo.lebrun@bootlin.com>

Maybe we should error out if we don't enter the loop, ie if we have no
flash declared?
 - Before your patch, cqspi->num_chipselect was set to num-cs DT prop or
   CQSPI_MAX_CHIPSELECT as fallback.
 - After your patch, cqspi->num_chipselect is set to one.

In neither case do we get an error if no flash is defined in DT.

We could either return some error code or set cqspi->num_chipselect=0
which will lead to spi_register_controller() to fail [0].

[0]: https://elixir.bootlin.com/linux/v6.16.4/source/drivers/spi/spi.c#L3322-L3329

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Re: [PATCH 3/4] spi: cadence-quadspi: Fix cqspi_setup_flash()
Posted by Santhosh Kumar K 3 weeks, 6 days ago 
Hello Theo,

On 04/09/25 21:02, Théo Lebrun wrote:
> Hello Santhosh,
> 
> On Thu Sep 4, 2025 at 3:31 PM CEST, Santhosh Kumar K wrote:
>> The 'max_cs' stores the largest chip select number. It should only
>> be updated when the current 'cs' is greater than existing 'max_cs'. So,
>> fix the condition accordingly.
> 
> Good catch. Current code can only work with one chip-select.
> 
> Reviewed-by: Théo Lebrun <theo.lebrun@bootlin.com>
> 
> Maybe we should error out if we don't enter the loop, ie if we have no
> flash declared?
>   - Before your patch, cqspi->num_chipselect was set to num-cs DT prop or
>     CQSPI_MAX_CHIPSELECT as fallback.
>   - After your patch, cqspi->num_chipselect is set to one.
> 
> In neither case do we get an error if no flash is defined in DT.
> 
> We could either return some error code or set cqspi->num_chipselect=0
> which will lead to spi_register_controller() to fail [0].

Yeah, we can use max_cs for this. Initiate max_cs with -1, check if it's 
still negative after the loop part - if yes, then it didn't enter the 
loop as there was no flash declared - return failure. I'll add this 
logic in v2. Thanks!

Regards,
Santhosh.

> 
> [0]: https://elixir.bootlin.com/linux/v6.16.4/source/drivers/spi/spi.c#L3322-L3329
> 
> Thanks,
> 
> --
> Théo Lebrun, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 
>
Re: [PATCH 3/4] spi: cadence-quadspi: Fix cqspi_setup_flash()
Posted by Pratyush Yadav 4 weeks ago 
On Thu, Sep 04 2025, Santhosh Kumar K wrote:

> The 'max_cs' stores the largest chip select number. It should only
> be updated when the current 'cs' is greater than existing 'max_cs'. So,
> fix the condition accordingly.
>
> Fixes: 0f3841a5e115 ("spi: cadence-qspi: report correct number of chip-select")
> Signed-off-by: Santhosh Kumar K <s-k6@ti.com>
> ---
>  drivers/spi/spi-cadence-quadspi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index 447a32a08a93..da3ec15abb3e 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
> @@ -1722,7 +1722,7 @@ static const struct spi_controller_mem_caps cqspi_mem_caps = {
>  
>  static int cqspi_setup_flash(struct cqspi_st *cqspi)
>  {
> -	unsigned int max_cs = cqspi->num_chipselect - 1;
> +	unsigned int max_cs = 0;
>  	struct platform_device *pdev = cqspi->pdev;
>  	struct device *dev = &pdev->dev;
>  	struct cqspi_flash_pdata *f_pdata;
> @@ -1740,7 +1740,7 @@ static int cqspi_setup_flash(struct cqspi_st *cqspi)
>  		if (cs >= cqspi->num_chipselect) {
>  			dev_err(dev, "Chip select %d out of range.\n", cs);
>  			return -EINVAL;
> -		} else if (cs < max_cs) {
> +		} else if (cs > max_cs) {

Makes sense. Out of curiosity, are you using multiple CS in a real use
case or is this only theoretical?

Also nit: this could be simplified to:

		if (cs >= cqspi->num_chipselect) {
			dev_err(dev, "Chip select %d out of range.\n", cs);
			return -EINVAL;
		}

		max_cs = max_t(unsigned int, cs, max_cs);

but I think it is fine either way.

Reviewed-by: Pratyush Yadav <pratyush@kernel.org>

>  			max_cs = cs;
>  		}

-- 
Regards,
Pratyush Yadav
Re: [PATCH 3/4] spi: cadence-quadspi: Fix cqspi_setup_flash()
Posted by Santhosh Kumar K 3 weeks, 6 days ago 
Hello,

On 04/09/25 20:11, Pratyush Yadav wrote:
> On Thu, Sep 04 2025, Santhosh Kumar K wrote:
> 
>> The 'max_cs' stores the largest chip select number. It should only
>> be updated when the current 'cs' is greater than existing 'max_cs'. So,
>> fix the condition accordingly.
>>
>> Fixes: 0f3841a5e115 ("spi: cadence-qspi: report correct number of chip-select")
>> Signed-off-by: Santhosh Kumar K <s-k6@ti.com>
>> ---
>>   drivers/spi/spi-cadence-quadspi.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
>> index 447a32a08a93..da3ec15abb3e 100644
>> --- a/drivers/spi/spi-cadence-quadspi.c
>> +++ b/drivers/spi/spi-cadence-quadspi.c
>> @@ -1722,7 +1722,7 @@ static const struct spi_controller_mem_caps cqspi_mem_caps = {
>>   
>>   static int cqspi_setup_flash(struct cqspi_st *cqspi)
>>   {
>> -	unsigned int max_cs = cqspi->num_chipselect - 1;
>> +	unsigned int max_cs = 0;
>>   	struct platform_device *pdev = cqspi->pdev;
>>   	struct device *dev = &pdev->dev;
>>   	struct cqspi_flash_pdata *f_pdata;
>> @@ -1740,7 +1740,7 @@ static int cqspi_setup_flash(struct cqspi_st *cqspi)
>>   		if (cs >= cqspi->num_chipselect) {
>>   			dev_err(dev, "Chip select %d out of range.\n", cs);
>>   			return -EINVAL;
>> -		} else if (cs < max_cs) {
>> +		} else if (cs > max_cs) {
> 
> Makes sense. Out of curiosity, are you using multiple CS in a real use
> case or is this only theoretical?

Real use case,  Pratyush - we have both OSPI NOR and QSPI NAND in our 
new AM62Lx EVM - CS0 and CS3 respectively.

> 
> Also nit: this could be simplified to:
> 
> 		if (cs >= cqspi->num_chipselect) {
> 			dev_err(dev, "Chip select %d out of range.\n", cs);
> 			return -EINVAL;
> 		}
> 
> 		max_cs = max_t(unsigned int, cs, max_cs);
> 
> but I think it is fine either way.

Yeah, this one's simpler, I'll go with this. Thanks!

Regards,
Santhosh.

> 
> Reviewed-by: Pratyush Yadav <pratyush@kernel.org>
> 
>>   			max_cs = cs;
>>   		}
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.