From mboxrd@z Thu Jan  1 00:00:00 1970
From: Michal Simek <monstr@seznam.cz>
Date: Sun, 5 Aug 2007 18:16:30 +0200
Subject: [U-Boot-Users] ROMFS
References: <003901c7b366$e4c09190$0200a8c0@monstrone>
	<fa686aa40708050859u2762b8e8re8c3a63fdbc30a0e@mail.gmail.com>
Message-ID: <0a6201c7d77b$fc8851d0$0500a8c0@monstrone>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Grant,

> On 6/20/07, Michal Simek <monstr@seznam.cz> wrote:
>> ROMFS: Add support for ROMFS filesystem based on MTD system.
>>
>> signed-off-by: Michal Simek <monstr@monstr.eu>
>>
>> Patch is in attachement.
>
> Please send your patches inline.  It is harder to comment on patches
> that are attachments because I cannot hit 'reply' and start typing.
> Patches that don't get reviewed are harder to get merged.
> (git-send-email is your friend here)
>
> I needed to cut and paste to write this email, but here are some comments.

OK. Next driver.

> Also, it's looking more and more like there needs to be an abstraction
> between filesystems and block devices.  (More of a general comment
> than a comment on your patch)  Overriding cmd_jffs2 to do more and
> more filesystems just smells wrong.
>
> Otherwise, it's a good looking driver.

Yes. I agree with you. System around block devices and filesystems needs 
rebuild.

>>  /*
>> - * JFFS2/CRAMFS support
>> + * JFFS2/CRAMFS/ROMFS support
>
> Hmmm, perhaps the filename cmd_jffs2.c needs to be changed.

It's the same design goal as CRAMFS.

>> + str[0] = 'h';
>> + break;
>> + case 1:
>> + str[0] = 'd';
>> + break;
>> + case 2:
>> + str[0] = 'f';
>> + break;
>> + case 3:
>> + str[0] = 'l';
>> + break;
>> + case 4:
>> + str[0] = 'b';
>> + break;
>> + case 5:
>> + str[0] = 'c';
>> + break;
>> + case 6:
>> + str[0] = 's';
>> + break;
>> + case 7:
>> + str[0] = 'p';
>> + break;
>> + default:
>> + str[0] = '?';
>> + }

> This can be done in a less verbose way by using an offset into a table
> or string.

Yes, it can be.

>> +
>> + if (inode->next & 0x8) {
>> + str[1] = 'x';
>> + } else {
>> + str[1] = '-';
>> + }
>
> Maybe?
> str[1] = inode->next & 0x8 ? 'x' : '-';

It can be accepted.

Michal Simek