Re: [PATCH v2 3/3] mtd: nand: tmio_nand.c: prefer sharpslpart MTD partition parser

[Brian Norris]

On Thu, May 25, 2017 at 10:47:37PM +0200, Andrea Adami wrote:
> On Thu, May 25, 2017 at 9:25 PM, Brian Norris
> <computersforpeace@xxxxxxxxx> wrote:
> > On Sat, Apr 22, 2017 at 01:20:13PM +0200, Andrea Adami wrote:
> >> This is the specific parser for Sharp SL Series (Zaurus)
> >>
> >> Signed-off-by: Andrea Adami <andrea.adami@xxxxxxxxx>
> >> ---
> >>  drivers/mtd/nand/tmio_nand.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/mtd/nand/tmio_nand.c b/drivers/mtd/nand/tmio_nand.c
> >> index fc5e773..f3612ac 100644
> >> --- a/drivers/mtd/nand/tmio_nand.c
> >> +++ b/drivers/mtd/nand/tmio_nand.c
> >> @@ -357,6 +357,8 @@ static void tmio_hw_stop(struct platform_device *dev, struct tmio_nand *tmio)
> >>               cell->disable(dev);
> >>  }
> >>
> >> +static const char * const probes[] = { "sharpslpart", NULL };
> >
> > This breaks anyone who might have used (or might want to use) the ofpart
> > or cmdlinepart parsers. At a minimum, you need to include those in your
> > array here.
> 
> I have been under the wrong assumption there is cmdlinepart as last
> option (if compiled) so I have taken a wrong example.
> Grepping in /mt for probes gives many examples: what if I change it with
> 
> static const char * const probes[] = { "sharpslpart", "cmdlinepart", NULL };
> 
> ofpart is utopic at the moment: these machines are not yet converted
> to devicetree and it will take a while.
> 
> With this patchset we can move a step forward DT, removing all the
> static partition definition from spitz.c, tosa.c, corgi.c and poodle.c
> 
> I don't dare adding ofpart here: this will be done once Zaurus pxa
> platform is moved to devicetree.

What's the harm in including ofpart? It will be silently skipped if you
don't have a conforming device tree.

> > But really, I'd rather not add any more parser listings like this in
> > drivers. Parser selection should be determined by the platform, not by
> > the driver. See my last response to Rafal, who is trying to extend
> > support for device-tree based listing of parsers:
> >
> > http://lists.infradead.org/pipermail/linux-mtd/2017-April/073729.html
> 
> Ok then but remember these are obsolete devices and as far as I know
> these nand drivers are only used on Zaurus devices. No future use I
> guess.

Yes, but the point is I don't want new examples of a bad pattern. And if
you ever do gain device tree support, I would then be "breaking" your
device tree if I dropped "sharpslpart" from your probe list.

> > He has some more work posted to the mailing list since then; search the
> > archives.
> >
> > I'll take a look at the parser itself, and maybe we can merge that. But
> > I'm not likely to merge this patch, in any form.
> >
> The little parser itself is universal for all Zaurus pxa variants.
> As said above, please consider we can remove many lines of board code.

Speaking of board code: since this is all initialized by board files,
why can't you put the "platform information" (i.e., the partition parser
type(s)) in the platform data? e.g, struct sharpsl_nand_platform_data or
struct tmio_nand_data. That'd resolve my concern about hardcoding lists
in the driver.

Brian