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.1Hello 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
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 > >
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
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;
>> }
>
© 2016 - 2026 Red Hat, Inc.