Re: [PATCH v3 05/17] mtd: spinand: Define ctrl_ops for non-page read/write op templates

[Apurva Nandan <a-nandan@ti.com>]

On 10/03/22 14:10, Boris Brezillon wrote:
> On Thu, 10 Mar 2022 13:27:06 +0530
> Apurva Nandan <a-nandan@ti.com> wrote:
>
>>>>> This way, you can easily pick the right set of operations based
>>>>> on the protocol/mode you're in:
>>>>>
>>>>> #define spinand_get_op_template(spinand, opname) \
>>>>> 	((spinand)->op_templates[(spinand)->protocol]->opname)
>>>>>
>>>>> static int spinand_read_reg_op(struct spinand_device *spinand, u8 reg, u8 *val)
>>>>> {
>>>>> 	struct spi_mem_op op = *spinand_get_op_template(spinand, get_feature);
>>>>> 	int ret;
>>>>>
>>>>> 	...
>>>>> }
>>>> I find a couple of issues with this  method,
>>>>
>>>> 1. read_cache, write_cache, update_cache op templates don't fit well
>>>> with the other non-data ops, as
>>>> these data ops are used to create a dirmap, and that can be done only
>>>> once at probe time. Hence, there
>>>> is a different mechanism of selecting of data ops and non-data ops.
>>> Not sure I see why this is a problem. You can populate data-ops for all
>>> modes, and pick the one that provides the best perfs when you create
>>> the dirmap (which should really be at the end of the probe, if it's not
>>> already).
>>>   
>>>> Hence, this division in the op templates
>>>> struct as data_ops and ctrl_ops is required. Currently, the core only
>>>> supports using a single protocol for
>>>> data ops, chosen at the time of probing.
>>> Again, I don't see why you need to differentiate the control and data
>>> ops when populating this table. Those are just operations the NAND
>>> supports, and the data operations is just a subset.
>>>   
>>>> 2. If we use this single op_templates struct, I can't think of any good
>>>> way to initialize these in the
>>>> manufacturers driver (winbond.c), refer to 17th patch in this series.
>>>> Could you please suggest a macro
>>>> implementation also for winbond.c with the suggested op_templates struct.
>>> First replace the op_variants field by something more generic:
>>>
>>> struct spinand_info {
>>> ...
>>> 	const struct spinand_op_variants **ops_variants;
>>> ...
>>> };
>>>
>>> #define SPINAND_OP_VARIANTS(_id, ...) \
>>> 	[SPI_NAND_OP_ ## _id] = { __VA_ARGS__ }
>>>
>>> #define SPINAND_OPS_VARIANTS(name, ...)
>>> 	const struct spinand_op_variants name[]{
>>> 		__VA_ARGS__,
>>> 	};
>>>
>>> #define SPINAND_INFO_OPS_VARIANTS(defs)
>>> 	.ops_variants = defs
>> If we modify these macros, it would require other spinand vendor drivers
>> to change (toshiba, micron, etc).
>> The older macros suit them well, should we go about changing them to
>> this new macro (will require re-testing all of them),
>> or can we keep them unchanged and have new set of macros with different
>> name (please give suggestion for it) for op variants.
> I'd rather have everything converted to the new approach (we don't want
> 2 ways of describing the same thing), and I'm not sure you can make the
> old macros map to the new solution, so I fear you'll have to patch all
> vendors. This being said, I'm fine providing simple wrappers if that
> helps, but I don't see how they'd make the description simpler/more
> compact to be honest.
Okay, I will convert all of the vendor drivers, but please note I don't 
have any way to test the changes on all the flashes.