Re: [PATCH v8 5/7] xen/asm-generic: introduce generic device.h

[Julien Grall <julien@xxxxxxx>]

Hi Oleksii,

On 15/02/2024 16:54, Oleksii wrote:
> > On 14/02/2024 09:32, Oleksii wrote:
> > > On Tue, 2024-02-13 at 18:09 +0000, Julien Grall wrote:
> > > > > +#ifdef CONFIG_HAS_PASSTHROUGH
> > > > > +    struct iommu_fwspec *iommu_fwspec; /* per-device IOMMU
> > > > > instance data */
> > > > > +#endif
> > > > > +};
> > > > > +
> > > > > +typedef struct device device_t;
> > > > > +
> > > > > +#ifdef CONFIG_HAS_DEVICE_TREE
> > > > > +
> > > > > +#include <xen/device_tree.h>
> > > > > +
> > > > > +#define dev_is_dt(dev)  ((dev)->type == DEV_DT)
> > > > > +
> > > > > +/**
> > > > > + *  device_init - Initialize a device
> > > > > + *  @dev: device to initialize
> > > > > + *  @class: class of the device (serial, network...)
> > > > > + *  @data: specific data for initializing the device
> > > > > + *
> > > > > + *  Return 0 on success.
> > > > > + */
> > > > > +int device_init(struct dt_device_node *dev, enum device_class
> > > > > class,
> > > > > +                const void *data);
> > > > > +
> > > > > +/**
> > > > > + * device_get_type - Get the type of the device
> > > > > + * @dev: device to match
> > > > > + *
> > > > > + * Return the device type on success or DEVICE_ANY on failure
> > > > > + */
> > > > > +enum device_class device_get_class(const struct dt_device_node
> > > > > *dev);
> > > > > +
> > > > > +#define DT_DEVICE_START(name_, namestr_, class_)            \
> > > > > +static const struct device_desc __dev_desc_##name_ __used   \
> > > > > +__section(".dev.info") = {                                  \
> > > > > +    .name = namestr_,                                       \
> > > > > +    .class = class_,
> > > > > +
> > > > > +#define DT_DEVICE_END                                       \
> > > > > +};
> > > > > +
> > > > > +#else /* !CONFIG_HAS_DEVICE_TREE */
> > > > > +#define dev_is_dt(dev) ((void)(dev), false)
> > > > > +#endif /* CONFIG_HAS_DEVICE_TREE */
> > > > > +
> > > > > +#define dev_is_pci(dev) ((dev)->type == DEV_PCI)
> > > > > +
> > > > > +struct device_desc {
> > > > > +    /* Device name */
> > > > > +    const char *name;
> > > > > +    /* Device class */
> > > > > +    enum device_class class;
> > > > > +
> > > > > +#ifdef CONFIG_HAS_DEVICE_TREE
> > > > > +
> > > > > +    /* List of devices supported by this driver */
> > > > > +    const struct dt_device_match *dt_match;
> > > > > +    /*
> > > > > +     * Device initialization.
> > > > > +     *
> > > > > +     * -EAGAIN is used to indicate that device probing is
> > > > > deferred.
> > > > > +     */
> > > > > +    int (*init)(struct dt_device_node *dev, const void *data);
> > > > > +
> > > > > +#endif
> > > > > +};
> > > > I am not sure I fully understand why "device_desc" is not
> > > > protected
> > > > by
> > > > CONFIG_HAS_DEVICE_TREE. The structure doesn't mean much when the
> > > > config
> > > > is disabled. Can you clarify?
> > > I thought that one day struct device_desc and acpi_device_desc will
> > > be
> > > "merged", and so decided just to #ifdef only DEVICE_TREE specific
> > > fields.
> > It might be possible to merge the two if we were using an union for
> > the
> > ACPI/DT specific part. However the majority of the parsing code needs
> > to
> > differ. So I am not convinced there would be any value to merge the
> > two
> > structures.
> In this case, let's have two separate structures.
>
> This is not the current situation, and I don't have a specific example.
> It appears that all architectures will use Device Tree or ACPI.
> However, does it make sense to keep 'struct device_desc' more generic
> to accommodate non-DT or non-ACPI cases?
I am not entirely sure what else to say. As I wrote before yes it could 
be made generic. But right now I don't see any values.
If you have any idea how to share the structure. Then feel free to make 
a proposal and I will review it.
Cheers,

--
Julien Grall