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

[Sascha Hauer <s.hauer@pengutronix.de>]

On Mon, Jun 10, 2024 at 10:33:43AM +0200, Ahmad Fatoum wrote:
> 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?

Sure. I decided for a linker list because otherwise we would have to add
an additional initcall to each subsystem using it. That's fine to do,
but we would also have to make sure this initcall executes before the
first user registers a device. Alternatively we could do something like
this in eth_register():

	static bool class_registered;

	if (!class_registered) {
		class_register(&eth_class);
		class_registered = true;
	}

I don't have a strong preference, but I think from these alternatives I
like linker lists best.

> 
> > +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?

Ok.

Sascha

-- 
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 |