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

[Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>]

On Thu, 10 Mar 2022 13:27:06 +0530
Apurva Nandan <a-nandan@xxxxxx> 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.