[PATCH 0/8] kernel: mtdsplit_uimage: use device tree properties for non-standard uimage parsing

[Sander Vanheule]

Hi Bjørn,

Just some thoughts in addition to what was already mentioned about the
property names.

On Wed, 2020-11-25 at 12:45 +0100, Bjørn Mork wrote:
> This ended up a bit more invasive than I imagined, but here goes..
> 
> I wanted to add a couple of new rtl83xx devices.  They both use
> standard uimages, but with device specific magic values.  Like
> most devices in this target, it seems.
> 
> The easy route would be to just add another magic to one of the
> existing parsers in mtdsplit_uimage, or maybe even a new parser
> grouping devices "belonging together" on some scale.  But this seems
> so unnecessary complicated to add something which is basically a
> device specific configuration and nothing more.  Not to mention that
> it does have some side effects.  New parsers make the code grow.
> Adding new magic numbers to existing parsers make them less fool
> proof, allowing any of the magic values to match while there is only
> one correct value most of the time.
> 
> So I thought I'd rather put these magic values in the device tree, for
> the cases where the parser is otherwise identical to the generic
> "denx,uimage" parser.  I believe this makes sense.
> 
> No so sure about the rest of the series.  But the snow ball rolled,
> and I thought "why not do the same for the other device specific
> parameters?".  So I added device tree properties for those as well.
> And ended up removing all the parsers except for the generic one.
> 
> Still not sure that was a good idea.  In particular because it's hard
> to test this without having the hardware, which I don't have.  And
> console is required if something breaks.
> 
> Anyway, here it is.  Please be gentle :-)
> 
> 
> 
> Bjørn
> ---
> 
> Bjørn Mork (8):
>   kernel: mtdsplit_uimage: read extralen from device tree
>   kernel: mtdsplit_uimage: replace "fonfxc" and "sge" parsers
>   kernel: mtdsplit_uimage: add "ih-magic" device-tree property
>   kernel: mtdsplit_uimage: replace "openwrt,okli" parser
>   kernel: mtdsplit_uimage: replace "allnet,uimage" parser
>   kernel: mtdsplit_uimage: add "ih-type" device-tree property

I don't know how others feel about this, but I find something like
	"ih-type = <IH_TYPE_FILESYSTEM>",
to be clearer than
	"ih-type = <7>".
Would it be an option to create a dt-bindings header? For
example,'include/dt-indings/mtd/partition/uimage.h' (to mirror the DT
documentation paths) could include definitions for IH_TYPE_*.
mtdsplit_uimage.c could even include this header to prevent duplicate
definitions.

>   kernel: mtdsplit_uimage: replace "netgear,uimage" parser
>   kernel: mtdsplit_uimage: replace "edimax,uimage" parser

This patch removes "edimax,uimage", while also introducing the DT
properties to do this. For the other properties you split this up. If
there won't be any formal documentation, maybe a separate patch could
serve this purpose?


>  27 files changed, 140 insertions(+), 377 deletions(-)

🎉️

Best,
Sander