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

Joachim Wiberg
2021-10-08T08:27:29Z

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

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

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

Cheers
/Joachim