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

[Apurva Nandan]

On 10/03/22 14:10, Boris Brezillon wrote:
> 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.
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.