| Message ID | 1492860013-20848-4-git-send-email-andrea.adami@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
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. 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 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. Brian > + > static int tmio_probe(struct platform_device *dev) > { > struct tmio_nand_data *data = dev_get_platdata(&dev->dev); > @@ -440,7 +442,7 @@ static int tmio_probe(struct platform_device *dev) > goto err_irq; > > /* Register the partitions */ > - retval = mtd_device_parse_register(mtd, NULL, NULL, > + retval = mtd_device_parse_register(mtd, probes, NULL, > data ? data->partition : NULL, > data ? data->num_partitions : 0); > if (!retval) > -- > 2.7.4 >
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. > > 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. > > 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. Thanks Andrea > Brian > >> + >> static int tmio_probe(struct platform_device *dev) >> { >> struct tmio_nand_data *data = dev_get_platdata(&dev->dev); >> @@ -440,7 +442,7 @@ static int tmio_probe(struct platform_device *dev) >> goto err_irq; >> >> /* Register the partitions */ >> - retval = mtd_device_parse_register(mtd, NULL, NULL, >> + retval = mtd_device_parse_register(mtd, probes, NULL, >> data ? data->partition : NULL, >> data ? data->num_partitions : 0); >> if (!retval) >> -- >> 2.7.4 >>
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
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
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 }; + static int tmio_probe(struct platform_device *dev) { struct tmio_nand_data *data = dev_get_platdata(&dev->dev); @@ -440,7 +442,7 @@ static int tmio_probe(struct platform_device *dev) goto err_irq; /* Register the partitions */ - retval = mtd_device_parse_register(mtd, NULL, NULL, + retval = mtd_device_parse_register(mtd, probes, NULL, data ? data->partition : NULL, data ? data->num_partitions : 0); if (!retval)
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(-)