[U-Boot] [PATCH v2 2/8] fs/fat: introduce new director iterators

[Rob Clark]

On Tue, Sep 5, 2017 at 4:56 AM, Simon Glass <sjg at chromium.org> wrote:
> Hi Rob,
>
> On 3 September 2017 at 00:37, Rob Clark <robdclark at gmail.com> wrote:
>> Untangle directory traversal into a simple iterator, to replace the
>> existing multi-purpose do_fat_read_at() + get_dentfromdir().
>>
>> Signed-off-by: Rob Clark <robdclark at gmail.com>
>> ---
>>  fs/fat/fat.c | 326 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 326 insertions(+)
>>
>> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
>> index e1c0a15dc7..c72d6ca931 100644
>> --- a/fs/fat/fat.c
>> +++ b/fs/fat/fat.c
>> @@ -1245,6 +1245,332 @@ exit:
>>         return ret;
>>  }
>>
>> +
>> +/*
>> + * Directory iterator, to simplify filesystem traversal
>> + */
>> +
>> +typedef struct {
>
> Please avoid using a typedef here. It seems like it could just be a struct.

I'm typically not a fan of 'typedef struct' but that seems to be the
style that fs/fat code was already using, and "when in Rome..."

> Also pleaee add a comment about what the struct is for
>
>> +       fsdata    *fsdata;
>> +       unsigned   cursect;
>> +       dir_entry *dent;
>> +       int        remaining;     /* remaining dent's in current cluster */
>> +       int        last_cluster;
>> +       int        is_root;
>> +
>> +       /* current iterator position values: */
>> +       char       l_name[VFAT_MAXLEN_BYTES];
>> +       char       s_name[14];
>> +       char      *name;          /* l_name if there is one, else s_name */
>> +
>> +       u8         block[MAX_CLUSTSIZE] __aligned(ARCH_DMA_MINALIGN);
>
> Some members are missing comments.
>
> Also I'm not too sure how the alignment works here. I don't see you
> creating one of these objects in this patch, but could you add a
> comment to the struct about how to create one? Do you use memalign()?

I was just creating them on the stack normally.. expecting the
compiler to dtrt because of the __aligned() on block[].

BR,
-R