Hi Sughosh, On Fri, 19 Aug 2022 at 10:18, Sughosh Ganu <sughosh.g...@linaro.org> wrote: > > hi Simon, > > On Fri, 19 Aug 2022 at 20:53, Simon Glass <s...@chromium.org> wrote: > > > > Hi Sughosh, > > > > On Fri, 19 Aug 2022 at 07:36, Sughosh Ganu <sughosh.g...@linaro.org> wrote: > > > > > > hi Simon, > > > > > > On Thu, 18 Aug 2022 at 23:20, Simon Glass <s...@chromium.org> wrote: > > > > > > > > pHi Sughosh, > > > > > > > > On Thu, 18 Aug 2022 at 05:03, Sughosh Ganu <sughosh.g...@linaro.org> > > > > wrote: > > > > > > > > > > hi Simon, > > > > > > > > > > On Thu, 18 Aug 2022 at 06:43, Simon Glass <s...@chromium.org> wrote: > > > > > > > > > > > > Hi Sughosh, > > > > > > > > > > > > On Wed, 17 Aug 2022 at 06:44, Sughosh Ganu > > > > > > <sughosh.g...@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.g...@linaro.org> > > > > > > > Reviewed-by: Patrick Delaunay <patrick.delau...@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
pinctrl has pinctrl_select_state() and two GPIO functions that violate this. It should be fixed to store the default pinctrl driver in gd, like we do with serial_dev. Please do send a patch if you can. regulators have some functions which act on all regulators, or at least a selection. That is fine, I think, but let me know if you see somethign wrong. bootcount is not fully converted to driver model and assumes a single bootcount. That should be fixed. in that it should work like sysreset, loading / storing from anything that exists (e.g. the 'walk' functions). > 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. The 'real device' thing doesn't really matter. What is a real device, anyway? Anything we want to model with drivers, a uclass and methods is a device. It is also the primary way we call code from the top-level part of U-Boot. Regards, Simon