[PATCH v4 1/6] FWU: Add FWU metadata access driver for MTD storage regions

[Jassi Brar]

On Wed, 29 Mar 2023 at 07:00, Michal Simek <michal.simek at amd.com> wrote:
> On 3/27/23 23:15, jassisinghbrar at gmail.com wrote:

> > diff --git a/drivers/fwu-mdata/raw_mtd.c b/drivers/fwu-mdata/raw_mtd.c
> > new file mode 100644
> > index 0000000000..4b1a10073a
> > --- /dev/null
> > +++ b/drivers/fwu-mdata/raw_mtd.c
> > @@ -0,0 +1,272 @@
> > +// SPDX-License-Identifier: GPL-2.0+
>
> Just a note: Did you choose GPL-2.0+ by purpose? Or it is just c&p?
>
just c&p. though isn't that the same as GPL-2.0-or-later ?

.......
> > +
> > +extern struct fwu_mtd_image_info fwu_mtd_images[];
>
> if there is a need to share it. It should go to header.
>
include/fwu,h  would be the header. Which is supposed to be very
global, and doesn't seem very appropriate to mention private data
shared between a driver and helper library.
If people still insist, I will make the change.

> And fwu_mtd_images[] is not defined when only this patch is applied.
> It means order of patches is not right. 1/6 and 2/6 should be swapped
>
I think 1 and 2 should be merged rather.


......
> > +     if (!mtd_priv->mtd) {
> > +             log_err("Failed to find mtd device by fwu-mdata-store\n");
> > +             return -ENODEV;
> > +     }
> > +
> > +     /* Get the offset of primary and seconday mdata */
>
>
> typo
>
ok

......
> > +
> > +static const struct udevice_id fwu_mdata_ids[] = {
> > +     { .compatible = "u-boot,fwu-mdata-mtd" },
> > +     { }
> > +};
>
> As I said this DT binding should be approved first to make sure that we don't
> need to fix DT binding in future. Just simply do it right from the begining.
>
Yes, I will cc Rob in the next submission (I only forgot last time).
However, let us note that fwu-mdata-gpt.yaml isn't blessed either.
I am not sure if there is any reason for the fwu node to even be in
the dts for kernel. But sure it is good to have it eyeballed by the DT
gods.

cheers.