On Fri, 10 Oct 2025 12:13:11 +0100 Simon Glass <[email protected]> wrote:
> Hi Kory, > > On Thu, 9 Oct 2025 at 15:51, Kory Maincent (TI.com) > <[email protected]> wrote: > > > > Introduce UCLASS-based extension board support to enable more > > standardized and automatic loading of extension board device tree > > overlays in preparation for integration with bootstd and pxe_utils. > > > > Signed-off-by: Kory Maincent (TI.com) <[email protected]> ... > > + > > +#include <alist.h> > > +#include <command.h> > > +#include <dm/device-internal.h> > > +#include <dm/lists.h> > > +#include <dm/uclass.h> > > +#include <env.h> > > +#include <extension_board.h> > > +#include <fdt_support.h> > > +#include <malloc.h> > > +#include <mapmem.h> > > nit: Please correct the ordering > > https://docs.u-boot.org/en/latest/develop/codingstyle.html#include-files Oh didn't know this rule. > > + > > +DECLARE_GLOBAL_DATA_PTR; > > + > > +/* For now, bind the extension device manually if none are found */ > > +static struct udevice *extension_get_dev(void) > > This should really return the error rather than gobbling it. Normally > we use 'int' as the return value, so: > > static int extension_get_dev(struct udevice *devp) You mean rather this then: static int extension_get_dev(struct udevice **devp) > > > +{ > > + struct driver *drv = ll_entry_start(struct driver, driver); > > + const int n_ents = ll_entry_count(struct driver, driver); > > + struct udevice *dev; > > + int i, ret; > > + > > + /* These are not needed before relocation */ > > + if (!(gd->flags & GD_FLG_RELOC)) > > + return NULL; > > + > > + uclass_first_device(UCLASS_EXTENSION, &dev); > > + if (dev) > > + return dev; > > + > > + /* Create the extension device if not already bound */ > > + for (i = 0; i < n_ents; i++, drv++) { > > + if (drv->id == UCLASS_EXTENSION) { > > If this is really what you want, you might as well just put a > U_BOOT_DRVINFO() declaration in the source. Then driver model does it > for you. We are supposed to use the devicetree, but it seems that that > idea is WIP at best. Oh I will use this MACRO instead. > > +int dm_extension_scan(void) > > +{ > > + struct alist *extension_list = dm_extension_get_list(); > > + struct udevice *dev = extension_get_dev(); > > + const struct extension_ops *ops; > > + > > + if (!dev || !extension_list) > > + return -ENODEV; > > + > > + ops = device_get_ops(dev); > > extension_get_ops() Is it really needed as it is used only once. > > > + if (!ops->scan) > > + return -ENODEV; > > That is normally -ENOSYS - at this point we have a device, it's just > that it doesn't have the require method. > > But it seem strange to me to allow a device that has no means to scan. Yes, maybe this check is useless indeed, as it is the only action done by the board code. ... > > @@ -13,9 +14,28 @@ LIST_HEAD(extension_list); > > static int do_extension_list(struct cmd_tbl *cmdtp, int flag, > > int argc, char *const argv[]) > > { > > +#if CONFIG_IS_ENABLED(SUPPORT_DM_EXTENSION_SCAN) > > + struct alist *dm_extension_list; > > +#endif > > struct extension *extension; > > int i = 0; > > > > +#if CONFIG_IS_ENABLED(SUPPORT_DM_EXTENSION_SCAN) > > Is it possible to use if() ? Perhaps at the end of your series, when > all the conversions are done and you can delete the old code? As all these macro check are removed when the conversion is done I didn't bother to use if() because I would have to take care of define but not used warnings. ... > > +/** > > + * dm_extension_get_list - Get the extension list > > + * Return: The extension alist pointer, or NULL if no such list exists. > > caller must free the list? No. > > + */ > > +struct alist *dm_extension_get_list(void); > > Is this the list of all extensions from all extension devices? yes. > > +/** > > + * dm_extension_apply - Apply extension board overlay to the devicetree > > + * @extension_num: Extension number to be applied > > + * Return: Zero on success, negative on failure. > > + */ > > +int dm_extension_apply(int extension_num); > > The uclass should have a method like apply(struct uclass *dev, int > extension_num). Is the numbering global across all devices? We currently support only one extension driver loaded at a time, therefore we don't currently need this uclass parameter. We will change the API when we will have several scan method possible at the same time but I can't test it for now. I don't think we will have such cases soon and maybe the devicetree WIP support will be ok at that time. > > + > > +/** > > + * dm_extension_apply_all - Apply all extension board overlays to the > > + * devicetree > > + * Return: Zero on success, negative on failure. > > + */ > > +int dm_extension_apply_all(void); > > + > > struct extension { > > struct list_head list; > > char name[32]; > > @@ -20,6 +62,28 @@ struct extension { > > char other[32]; > > }; > > At some point in this series, please comment this struct Ok I will add a patch to add comment on this. > > > > > +struct extension_ops { > > + /** > > + * scan - Add system-specific function to scan extension boards. > > + * @dev: extension device to use > > need to document extension_list here - I believe it is a list of > struct extension? Or is it struct extension * ? Oop some code leftover here. > > > + * Return: The number of extension or a negative value in case of > > + * error. > > + */ > > + int (*scan)(struct alist *extension_list); > > This is a bit of a strange uclass function! Since you are probing > drivers in this uclass, you can iterate through them using > uclass_foreach...() etc. > > I am guessing that you just need to add a struct udevice * as the first arg. As I said there is only one drivers loaded at a time supported for now. Regards, -- Köry Maincent, Bootlin Embedded Linux and kernel engineering https://bootlin.com
