Messages in this thread | | | From | Andrea Adami <> | Date | Fri, 26 May 2017 00:21:17 +0200 | Subject | Re: [PATCH v2 3/3] mtd: nand: tmio_nand.c: prefer sharpslpart MTD partition parser |
| |
On Thu, May 25, 2017 at 11:10 PM, Brian Norris <computersforpeace@gmail.com> wrote: > 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@gmail.com> 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@gmail.com> >> >> --- >> >> 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
Brian,
I now understand your objections about hardcoding parsers in the drivers. I'll ask the maintainers in case there were any objections: the patch will cross mach-pxa and drivers/mtd.
Thanks again
Andrea
| |