Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <1dfd6a3d-452a-1e71-ca26-1fffe6db1636@ti.com>
Date:   Mon, 14 Mar 2022 17:17:10 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH v3 05/17] mtd: spinand: Define ctrl_ops for non-page
 read/write op templates
Content-Language: en-US
To:     Boris Brezillon <boris.brezillon@collabora.com>
CC:     Miquel Raynal <miquel.raynal@bootlin.com>,
        Richard Weinberger <richard@nod.at>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        Mark Brown <broonie@kernel.org>,
        Patrice Chotard <patrice.chotard@foss.st.com>,
        Christophe Kerello <christophe.kerello@foss.st.com>,
        Daniel Palmer <daniel@0x0f.com>,
        Alexander Lobakin <alobakin@pm.me>,
        <linux-mtd@lists.infradead.org>, <linux-kernel@vger.kernel.org>,
        <linux-spi@vger.kernel.org>, <p.yadav@ti.com>
References: <20220101074250.14443-1-a-nandan@ti.com>
 <20220101074250.14443-6-a-nandan@ti.com>
 <20220103110107.45594e78@collabora.com>
 <bf3ea909-e0a5-eeac-12e9-c8a809685f48@ti.com>
 <20220215183705.574df0c1@collabora.com>
 <12c7a6a6-8b49-1c3e-087f-79c77388b091@ti.com>
 <20220310094053.1d86d13a@collabora.com>
From:   Apurva Nandan <a-nandan@ti.com>
In-Reply-To: <20220310094053.1d86d13a@collabora.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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.