Re: [PATCH 2/4] base: add class device support

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hello Sascha,

On 10.06.24 10:12, Sascha Hauer wrote:
> This introduces the concept of class devices in barebox. Class devices
> group together devices of the same type. Several subsystems like
> watchdog and network devices have a struct device embedded in their
> subsystem specific struct anyway, so we can use this as a class device.
> As these class devices are collected on a list we can use this list to
> iterate over all network/watchdog devices and thus free the subsystems
> from the burden of keeping a list themselves.
> 
> There is a 'class' command added in this patch which can be used to show
> all registered classes along with the devices registered for each class.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  commands/Kconfig                  |  6 +++++
>  commands/Makefile                 |  1 +
>  commands/class.c                  | 30 ++++++++++++++++++++++
>  drivers/base/Makefile             |  1 +
>  drivers/base/class.c              | 41 +++++++++++++++++++++++++++++++
>  include/asm-generic/barebox.lds.h |  7 ++++++
>  include/device.h                  | 21 ++++++++++++++++
>  7 files changed, 107 insertions(+)
>  create mode 100644 commands/class.c
>  create mode 100644 drivers/base/class.c
> 
> diff --git a/commands/Kconfig b/commands/Kconfig
> index 899673bfee..7831e6276d 100644
> --- a/commands/Kconfig
> +++ b/commands/Kconfig
> @@ -68,6 +68,12 @@ config CMD_BOOTROM
>  
>  	  bootrom [-la]
>  
> +config CMD_CLASS
> +	tristate
> +	prompt "class"
> +	help
> +	  Show information about registered classes and devices

s/classes and devices/device classes/.

> +#define BAREBOX_CLASSES				\
> +	STRUCT_ALIGN();				\
> +	__barebox_class_start = .;		\
> +	KEEP(*(SORT_BY_NAME(.barebox_class*)))	\
> +	__barebox_class_end = .;

While I don't mind using linker lists for this, can you add a note
to the commit message why you decided against dynamic allocation?

> +struct class {
> +	const char *name;
> +	struct list_head devices;
> +	struct list_head list;
> +};
> +
> +#define DECLARE_CLASS(_name, _classname)					\

This is a definition, not ony a declaration and
Linux has DEFINE_CLASS defined in <linux/cleanup.h>.

How about DEFINE_DEV_CLASS?

> +	struct class _name __ll_elem(.barebox_class_##_name) = {		\
> +		.name = _classname,						\
> +		.devices = LIST_HEAD_INIT(_name.devices),			\
> +	}

Cheers,
Ahmad

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |