[PATCH v5 12/17] mtd: rawnand: tegra: convert driver to nand_scan()

[stefan@xxxxxxxx (Stefan Agner)]

On 27.07.2018 10:06, Miquel Raynal wrote:
> Hi Stefan,
> 
> Stefan Agner <stefan at agner.ch> wrote on Fri, 27 Jul 2018 09:13:09 +0200:
> 
>> On 26.07.2018 20:01, Boris Brezillon wrote:
>> > On Thu, 26 Jul 2018 18:29:36 +0200
>> > Stefan Agner <stefan at agner.ch> wrote:
>> >
>> >> On 25.07.2018 15:31, Miquel Raynal wrote:
>> >> > Two helpers have been added to the core to do all kind of controller
>> >> > side configuration/initialization between the detection phase and the
>> >> > final NAND scan. Implement these hooks so that we can convert the driver
>> >> > to just use nand_scan() instead of the nand_scan_ident() +
>> >> > nand_scan_tail() pair.
>> >> >
>> >>
>> >> While the patch looks technically correct, I wonder whether the driver
>> >> now does what we expect it from attach logically...
>> >>
>> >> E.g. shouldn't we get the wp_gpio in attach?
>> >
>> > Well, this series does things mechanically to avoid breaking drivers (we
>> > just move all the code between ident and tail into the attach hook),
>> > but any resource that is not needed for the identification phase and is
>> > tied to the NAND chip could/should be requested in the attach hook (the
>> > WP pin is such a resource).
>>
>> Ok, that makes completely sense and I agree with that approach! However,
>> it is not obvious when looking at the series.
>>
>> Can we mention that fact in the commit log, e.g. something like:
>>
>> "To avoid breaking this patch converts the driver mechanically by just
>> moving all the code between ...ident and ..tail into the attach hook.
>> Ideally driver should request all resources tied to the NAND chip in the
>> attach hook."
> 
> As Boris said, the whole change was somehow mechanic, I already had a
> hard time understanding what each driver wanted to achieve between
> those two functions, I did not dig any further as it was already very
> time consuming. A cleanup of many drivers would be appreciated though.

Yeah sure, and your work is very much appreciated!

Until closer look and Boris' confirmation it really wasn't clear to me
whether things are on purpose the way they are or because this patch is
seen as a first (mechanical) step...

> 
> As this would apply to the 30 patches of the series and because I
> already merged all of them, I think I'll pass for this one, even if I
> completely agree with the request.

Ok, at least it is documented on the ML now :-)

--
Stefan