Re: [PATCH v2] mtd: spinand: Do not re-enable continuous reads vetoed during variant selection

[Frieder Schrempf]

Hi Miquèl,

On 12.10.26 18:56, Miquel Raynal wrote:
> Hi Frieder,
> 
>> Fixes: 6eb7c193e751 ("mtd: spinand: Use secondary ops for continuous reads")
>> Assisted-by: LLM
>> Signed-off-by: Frieder Schrempf <frieder.schrempf@xxxxxxxxxx>
>> ---
>> I was using an LLM to check sync patches for U-Boot and the LLM reported
>> a bug in the upstream kernel code. To me it looks like this is indeed a
>> bug and the fix also looks correct to me, but I would like to mention, that
>> this is purely LLM-generated and theoretical. I didn't verify the bug and
>> the fix on actual hardware.
>>
>> @Miquel: Do you have a hardware setup that would be affected by this?
> 
> I do not.
> 
>> Do you think there is a better fix?
> 
> I agree with your bug analysis and with the proposal, just a few nits below.
> 
>> --- a/include/linux/mtd/spinand.h
>> +++ b/include/linux/mtd/spinand.h
>> @@ -772,6 +772,9 @@ struct spinand_mem_ops {
>>   *		suitable to use or not in general with this chip/configuration.
>>   *		A per-transfer check must of course be done to ensure it is
>>   *		actually relevant to enable this feature.
>> + * @has_cont_read_variants: The chip needs dedicated continuous read from
>> + *			    cache operations, which must then be available for
>> + *			    the bus interface in use
> 
> "The chip needs dedicated continuous read from cache variants"

As below you are proposing to rename the field to needs_cont_read_op,
which contains "op", I will leave the description as is to match this.

> 
>>   * @set_cont_read: Enable/disable the continuous read feature
>>   * @priv: manufacturer private data
>>   * @fact_otp: SPI NAND factory OTP info.
>> @@ -810,6 +813,7 @@ struct spinand_device {
>>  	int (*configure_chip)(struct spinand_device *spinand,
>>  			      enum spinand_bus_interface iface);
>>  	bool cont_read_possible;
>> +	bool has_cont_read_variants;
> 
> Can we name it needs_cont_read_op?

Sure, will do in v3.

> 
>>  	int (*set_cont_read)(struct spinand_device *spinand,
>>  			     bool enable);
>>  	int (*set_randomizer)(struct spinand_device *spinand,
>>
> 
> The rest is good to me.

Thanks for the review!

Best regards
Frieder