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;
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Wed, 06 Apr 2022 12:31:22 +0200
From:   Michael Walle <michael@walle.cc>
To:     Pratyush Yadav <p.yadav@ti.com>
Cc:     Tudor.Ambarus@microchip.com, miquel.raynal@bootlin.com,
        richard@nod.at, vigneshr@ti.com, linux-mtd@lists.infradead.org,
        linux-kernel@vger.kernel.org, Nicolas.Ferre@microchip.com
Subject: Re: [PATCH v2 4/4] mtd: spi-nor: sfdp: Keep SFDP definitions private
In-Reply-To: <20220405193111.pnekaivbsj7hronp@ti.com>
References: <20220309144215.179449-1-tudor.ambarus@microchip.com>
 <20220309144215.179449-5-tudor.ambarus@microchip.com>
 <20220401200133.gyyvoe7xdbsww7cv@ti.com>
 <ed093fa5-b514-b0a6-166f-ecf117582b0b@microchip.com>
 <20220405193111.pnekaivbsj7hronp@ti.com>
User-Agent: Roundcube Webmail/1.4.13
Message-ID: <1fa079c4a0341ffe6ad7bdebd3cb8958@walle.cc>
Precedence: bulk

Hi,

I didn't follow the series closely, so I might miss something.

Am 2022-04-05 21:31, schrieb Pratyush Yadav:
> On 04/04/22 06:19AM, Tudor.Ambarus@microchip.com wrote:
>> On 4/1/22 23:01, Pratyush Yadav wrote:
>> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>> >
>> > On 09/03/22 04:42PM, Tudor Ambarus wrote:
>> >> Keep the SFDP definitions private and expose just the definitions that are
>> >> required by the core and manufacturer drivers.
>> >
>> > I am not so sure about this. Since the post_bfpt hook passes in the bfpt
>> > table to flash drivers, they might end up wanting to use these for some
>> > checks like issi.c does for DWORD 1. They would have to move them back
>> > to sfdp.h for that, which just causes extra churn, and also puts some
>> > BFPT related defines in sfdp.h and some in sfdp.c.
>> >

It's not the extra churn, but also git blame doesn't really work. In
fact, it is already really hard to follow all the commits in spi-nor/
with all the code moving around. So what is the actual adavantage of
having this stuff private? One clear disadvantage is that you need to
move it from sfdp.c to sfdp.h once you need it. And potentially, you
might need all the defines.

Also, if you pass the SFDP data to any fixups, the drivers are expected
to use them.

>> That's correct, but I think exposing just the public defines in sfdp.h 
>> is
>> the path to follow. We should keep private all the definitions that we 
>> can
>> private in sfdp.c and expose publicly in sfdp.h just the ones that are 
>> shared.
>> Flash collisions, and implicitly the need of public SFDP definitions, 
>> should be
>> an exception, so I expect sfdp.h to be short in size.
> 
> I disagree. I think we should keep everything in the same place. And
> since we need to expose this to manufacturer drivers, that place is
> sfdp.h. Who is going to cast the tiebreaking vote here? ;-)

I'm leaning towards Pratyush opinion unless there is a clear advantage
to move the defines.

-michael