[PATCH v8 02/13] FWU: Add FWU metadata structure and driver for accessing metadata

[Sughosh Ganu]

hi Simon,

On Fri, 19 Aug 2022 at 20:53, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Sughosh,
>
> On Fri, 19 Aug 2022 at 07:36, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Thu, 18 Aug 2022 at 23:20, Simon Glass <sjg at chromium.org> wrote:
> > >
> > > pHi Sughosh,
> > >
> > > On Thu, 18 Aug 2022 at 05:03, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > >
> > > > hi Simon,
> > > >
> > > > On Thu, 18 Aug 2022 at 06:43, Simon Glass <sjg at chromium.org> wrote:
> > > > >
> > > > > Hi Sughosh,
> > > > >
> > > > > On Wed, 17 Aug 2022 at 06:44, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > > > >
> > > > > > In the FWU Multi Bank Update feature, the information about the
> > > > > > updatable images is stored as part of the metadata, which is stored on
> > > > > > a dedicated partition. Add the metadata structure, and a driver model
> > > > > > uclass which provides functions to access the metadata. These are
> > > > > > generic API's, and implementations can be added based on parameters
> > > > > > like how the metadata partition is accessed and what type of storage
> > > > > > device houses the metadata.
> > > > > >
> > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > > > > > Reviewed-by: Patrick Delaunay <patrick.delaunay at foss.st.com>
> > > > > > ---
> > > > > > Changes since V7:
> > > > > > * Rephrased the error message in fwu_update_active_index as per
> > > > > >   suggestion from Ilias.
> > > > > > * Reworked the logic in fwu_get_image_alt_num() as per the suggestion
> > > > > >   from Ilias.
> > > > > >
> > > > > >  drivers/Kconfig                      |   2 +
> > > > > >  drivers/Makefile                     |   1 +
> > > > > >  drivers/fwu-mdata/Kconfig            |   7 +
> > > > > >  drivers/fwu-mdata/Makefile           |   6 +
> > > > > >  drivers/fwu-mdata/fwu-mdata-uclass.c | 463 +++++++++++++++++++++++++++
> > > > > >  include/dm/uclass-id.h               |   1 +
> > > > > >  include/fwu.h                        |  49 +++
> > > > > >  include/fwu_mdata.h                  |  67 ++++
> > > > > >  8 files changed, 596 insertions(+)
> > > > > >  create mode 100644 drivers/fwu-mdata/Kconfig
> > > > > >  create mode 100644 drivers/fwu-mdata/Makefile
> > > > > >  create mode 100644 drivers/fwu-mdata/fwu-mdata-uclass.c
> > > > > >  create mode 100644 include/fwu.h
> > > > > >  create mode 100644 include/fwu_mdata.h
> > > > > >
>
> [..]
>
> > >
> > > I don't understand your comment about NULL. free(NULL) is valid in U-Boot, if that is what you are wondering?
> > >
> > > >
> > > > >
> > > > > > +       struct fwu_image_entry *img_entry;
> > > > > > +       const struct fwu_mdata_ops *ops = NULL;
> > > > > > +       struct fwu_image_bank_info *img_bank_info;
> > > > > > +
> > > > > > +       ret = fwu_get_dev_ops(&dev, &ops);
> > > > >
> > > > > Rather than this, see the pattern used by other uclasses
> > > >
> > > > Can you point me to some example code. Also, not sure what the issue
> > > > is with using this function.
> > >
> > > It is a pointless obfuscation, mainly. See for example clk_request(), but most uclasses use this pattern. I really encourage you to read more of the code before submitting a patch.
> > >
> > > int clk_request(struct udevice *dev, struct clk *clk)
> > > {
> > >    const struct clk_ops *ops;
> > >
> > >    ...
> > >    ops = clk_dev_ops(dev);
> > >
> > > (where that is defined in the header file)
> >
> > If you mean clk_dev_ops, it is defined in clk-uclass.c, and IMO that
> > makes sense, since it is being called multiple times in the driver.
> > Similarly, the fwu_get_dev_ops is being called multiple times in the
> > uclass driver, so I think it makes sense to have it as a function. If
> > you want me to move it to a header file or rename it, I can do that.
>
> Yes, rename and move to header, and use the same function signature as
> clk and the other uclasses (i.e. no dev parameter).
>
> Your uclass functions need to be defined with a device parameter as
> the first arg. They cannot go and hunt down a uclass themselves. See
> other uclasses for how this works.
>
> If you want a common function to find the first device, use
> uclass_first_device() in the caller, not internal to the uclass!

I can do that, sure. But there doesn't seem to be uniformity in these
rules. There are a bunch of uclass drivers which seem to be doing
exactly the same thing, including, but not limited to bootcount,
pinctrl, regulators. These uclass drivers all have instances of common
API being called and the uclass driver finding the device. Please note
that the FWU uclass is not representing a real device -- it is pretty
similar to bootcount in that regard. Which was the primary reason I
kept the FWU API's device agnostic.

-sughosh

>
> Regards,
> Simon