[PATCH v2] mtd: spi-nor: Fix the spi_nor_read() when config SPI_STACKED_PARALLEL is enabled

[Marek Vasut]

On 12/16/24 5:16 AM, Abbarapu, Venkatesh wrote:

[...]

>>> +++ b/drivers/mtd/spi/spi-nor-core.c
>>> @@ -1140,7 +1140,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct
>> erase_info *instr)
>>>    				nor->spi->flags &= ~SPI_XFER_U_PAGE;
>>>    		}
>>>    #ifdef CONFIG_SPI_FLASH_BAR
>>> -		ret = write_bar(nor, addr);
>>> +		ret = write_bar(nor, offset);
>>
>> This change is really inobvious, the code above likely needs to be compiled out if the
>> SPI_STACKED_PARALLEL stuff is disabled ?
> 
> In the spi_nor_erase()
> 	offset = addr;
> 	if(PARALLEL)
> 		offset/=2;
> 	so for parallel or single configuration we need to pass "offset" to write_bar()
> 	write_bar(nor, offset");

The code above likely needs to be compiled out if the 
SPI_STACKED_PARALLEL stuff is disabled ?

[...]

>> If I look at this change with 'git show -w' , the change looks like this:
>>
>> "
>>    #ifdef CONFIG_SPI_FLASH_BAR
>> +               u32 remain_len;
>> +
>>                   ret = write_bar(nor, offset);
>>                   if (ret < 0)
>>                           return log_ret(ret);
>> +               remain_len = (SZ_16M * (nor->bank_curr + 1)) - offset;
>> +               if (len < remain_len)
>> +                       read_len = len;
>> +               else
>> +                       read_len = remain_len;
>>    #endif
>> -
>> +               if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) {
>>                           if (len < rem_bank_len)
>>                                   read_len = len;
>>                           else
>>                                   read_len = rem_bank_len;
>> -
>> +               }
>>                   if (read_len == 0)
>>                           return -EIO;
>> "
>>
>> Why is there this part of code twice now, ifdeffed out differently in each case ?
>>
>> "
>>                           if (len < rem_bank_len)
>>                                   read_len = len;
>>                           else
>>                                   read_len = rem_bank_len; "
> 
> For parallel/stacked configuration and address width the "rem_bank_len" will vary and as we don't want to disturb the default read functionality added the ifdef separately.
What would happen if both SPI_FLASH_BAR and SPI_STACKED_PARALLEL are 
enabled on a system that only has one SPI NOR attached 
(non-stacked/parallel) ? I noticed the second "copy" of the code behaves 
slightly differently in the else branch, so does that mean this would 
break such setup ?