Re: [PATCH 1/1] mtd: block2mtd: add support for an optional custom MTD label

Miquel Raynal
2021-10-08T08:35:22Z

Hi Joachim,

troglobit@gmail.com wrote on Fri, 08 Oct 2021 10:26:46 +0200:

> Hi Miquèl!
>
> On Fri, Oct 08, 2021 at 09:27, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > troglobit@gmail.com wrote on Tue, 5 Oct 2021 10:03:59 +0200:
> >> [snip]
> >> @@ -304,7 +307,7 @@ static struct block2mtd_dev *add_device(char *devname, int erase_size,
> >>
> >> list_add(&dev->list, &blkmtd_device_list);
> >> pr_info("mtd%d: [%s] erase_size = %dKiB [%d]\n",
> >> - dev->mtd.index,
> >> + dev->mtd.index, label ? label :
> > Can you put this on the next line?
>
> Sure thing!
>
> >> @@ -381,8 +384,8 @@ static int block2mtd_setup2(const char *val)
> >> /* 80 for device, 12 for erase size, 80 for name, 8 for timeout */
> >> char buf[80 + 12 + 80 + 8];
> >> char *str = buf;
> >> - char *token[2];
> >> - char *name;
> >> + char *token[3];
> > This number deserves a define and should be used later on as well.
>
> OK, I'll see what I can do, it's for the positional module arguments,
> so a FOO_MAX_ARGS or sth, I guess ...
>
> >> @@ -395,7 +398,7 @@ static int block2mtd_setup2(const char *val)
> >> strcpy(str, val);
> >> kill_final_newline(str);
> >>
> >> - for (i = 0; i < 2; i++)
> >> + for (i = 0; i < 3; i++)
>
> used here as well ...
>
> >> token[i] = strsep(&str, ",");
>
> But what about each separate arg, is there any convention? I'll see
> what I can find in other modules.
>
> >> if (str) {
> >> @@ -414,7 +417,7 @@ static int block2mtd_setup2(const char *val)
> >> return 0;
> >> }
> >>
> >> - if (token[1]) {
> >> + if (token[1] && strlen(token[1])) {
> >
> > This change is not related to your commit I believe, please split if
> > my assumption is right.
>
> Well, it's a logical consequence of adding another module argument, but
> you're right, this could be hit before as well if someone added multiple
> commas without argument. (That breaks erase_size default value.)

In this case you can keep this change in the commit but you need to
explain it in the commit message.

> Maybe this and the defines fit in the same separate "cleanup" patch?
>
> >> - add_device(name, erase_size, timeout);
> >> + if (token[2]) {
> >> + label = token[2];
> >> + pr_info("Using custom MTD label '%s' for dev %s\n", label, name);
> > Not sure this qualifies as an info log message, perhaps a debug one
> > would be more appropriate?
>
> OK! (Just tried to follow the same log level used for erase_size.)

Ok then keep the driver consistent, it's not a bit deal and we can
lower the level later if it's too noisy.

> Thank you for the review, I'll post a v2 later today, or during the
> weekend :)
>
> Cheers
> /Joachim
>

Thanks,
Miquèl