Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1650766 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 3/3] mtd: nand: tmio_nand.c: prefer sharpslpart MTD  partition parser Brian Norris <computersforpeace@gmail.com> - 2017-05-25 21:30 +0200
    Re: [PATCH v2 3/3] mtd: nand: tmio_nand.c: prefer sharpslpart MTD  partition parser Andrea Adami <andrea.adami@gmail.com> - 2017-05-25 22:50 +0200
      Re: [PATCH v2 3/3] mtd: nand: tmio_nand.c: prefer sharpslpart MTD  partition parser Brian Norris <computersforpeace@gmail.com> - 2017-05-25 23:20 +0200
        Re: [PATCH v2 3/3] mtd: nand: tmio_nand.c: prefer sharpslpart MTD  partition parser Andrea Adami <andrea.adami@gmail.com> - 2017-05-26 00:30 +0200

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

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
> 

[toc] | [next] | [standalone]

#1650816

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
>>

[toc] | [prev] | [next] | [standalone]

#1650825

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

[toc] | [prev] | [next] | [standalone]

#1650935

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web