Re: [PATCH v9 16/18] mtd: spinand: negotiate optimal controller operating point before dirmap creation

[Miquel Raynal]

On 04/09/2026 at 18:23:36 +02, Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:

> Hello Santhosh,
>
>> +static bool spinand_try_ranked_variant(struct spinand_device *spinand,
>> +				       struct spi_mem *mem,
>> +				       enum spinand_bus_interface iface,
>> +				       u32 *tried_mask)
>> +{
>> +	const struct spinand_op_variants *variants = spinand->all_read_variants;
>> +	struct spinand_mem_ops *templates = iface == ODTR ?
>> +		&spinand->odtr_op_templates : &spinand->ssdr_op_templates;
>> +	const struct spi_mem_op *best;
>> +	int ret;
>> +
>> +	if (!variants)
>> +		return false;
>> +
>> +	while ((best = spinand_op_find_best_variant(spinand, variants, iface,
>> +						    *tried_mask, NULL))) {
>> +		*tried_mask |= BIT(best - variants->ops);
>> +		spinand->max_read_op = *best;
>> +		spinand->max_read_op.max_freq = 0;
>> +		spinand->max_write_op.max_freq = 0;
>> +		ret = spi_mem_execute_tuning(mem, &spinand->max_read_op,
>> +					     &spinand->max_write_op);
>> +		if (ret && ret != -EOPNOTSUPP)
>> +			dev_dbg(&mem->spi->dev, "%s optimization failed: %d\n",
>> +				iface == ODTR ? "ODTR" : "SSDR", ret);
>> +		if (!ret && spinand->max_read_op.max_freq) {
>> +			templates->read_cache = best;
>> +			if (spinand->all_cont_read_variants) {
>> +				templates->cont_read_cache =
>> +					spinand_find_cont_read_variant(spinand, best, iface);
>> +				spinand->cont_read_possible =
>> +					!!templates->cont_read_cache;
>
> I believe you almost addressed the concerns raised in v7 about
> cont_read_possible, except what you do here is still a bit dangerous
> since you could re-enable continuous reads on no_cs_assertion capable
> controllers. I believe you should clear the flag only, never risk to set
> it, something like:

cf. Sashiko's: [High] Unconditional overwrite of `spinand->cont_read_possible`
bypasses system capability checks, risking unsupported continuous reads.

>
> if (!templates->cont_read_cache)
>     spinand->cont_read_possible = false;
>
> With this fixed,
>
> Reviewed-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
>
> I believe I will take this patch only after Mark takes the batch before,
> which looks mostly good for my non-spi-expert eyes. Either Mark shares
> an immutable tag, or I wait one cycle.
>
> Thanks,
> Miquèl