Re: [PATCH v3 02/14] xen/asm-generic: introduce generic device.h

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Fri, 2023-11-24 at 11:01 +0000, Julien Grall wrote:
> Hi Oleksii,
> 
> On 17/11/2023 12:24, Oleksii Kurochko wrote:
> > Arm, PPC and RISC-V use the same device.h thereby device.h
> > was moved to asm-generic. 
> 
> I read "was moved" as the patch should also contain some deleted
> lines. 
> But below, I only see the file introduced. Did you intend to also
> remove 
> the version in arm/include/asm?
Yes, I just messed up with the patch version. Here is the version of
the patch which remove Arm's device.h:
https://gitlab.com/xen-project/people/olkur/xen/-/commit/ce845abfb57d27b0c08984f5433085c767550495
> 
> > Arm's device.h was taken as a base with
> > the following changes:
> >   - #ifdef PCI related things.
> >   - #ifdef ACPI related things.
> >   - Rename #ifdef guards.
> >   - Add SPDX tag.
> >   - #ifdef CONFIG_HAS_DEVICE_TREE related things.
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > ---
> > It is still open question if device.h should be in asm-generic.
> > Need more opinions.
> 
> I still think it should. But I guess you want others to answer? If so
> it 
> would be worth to point out from who you seek opinions.
> 
> > ---
> > Changes in V3:
> >   - ifdef device tree related things.
> >   - update the commit message
> > ---
> > Changes in V2:
> >     - take ( as common ) device.h from Arm as PPC and RISC-V
> > use it as a base.
> >     - #ifdef PCI related things.
> >     - #ifdef ACPI related things.
> >     - rename DEVICE_GIC to DEVIC_IC.
> >     - rename #ifdef guards.
> >     - switch Arm and PPC to generic device.h
> >     - add SPDX tag
> >     - update the commit message
> > 
> > ---
> >   xen/include/asm-generic/device.h | 147
> > +++++++++++++++++++++++++++++++
> >   1 file changed, 147 insertions(+)
> >   create mode 100644 xen/include/asm-generic/device.h
> > 
> > diff --git a/xen/include/asm-generic/device.h b/xen/include/asm-
> > generic/device.h
> > new file mode 100644
> > index 0000000000..7ef5aa955a
> > --- /dev/null
> > +++ b/xen/include/asm-generic/device.h
> > @@ -0,0 +1,147 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef __ASM_GENERIC_DEVICE_H__
> > +#define __ASM_GENERIC_DEVICE_H__
> > +
> > +enum device_type
> > +{
> > +#ifdef CONFIG_HAS_DEVICE_TREE
> > +    DEV_DT,
> > +#endif
> > +
> > +#ifdef HAS_PCI
> > +    DEV_PCI,
> > +#endif
> > +};
> > +
> > +struct dev_archdata {
> > +    void *iommu;    /* IOMMU private data */
> > +};
> > +
> > +/* struct device - The basic device structure */
> > +struct device
> > +{
> > +    enum device_type type;
> > +#ifdef CONFIG_HAS_DEVICE_TREE
> > +    struct dt_device_node *of_node; /* Used by drivers imported
> > from Linux */
> > +#endif
> > +    struct dev_archdata archdata;
> > +    struct iommu_fwspec *iommu_fwspec; /* per-device IOMMU
> > instance data */
> > +};
> > +
> > +typedef struct device device_t;
> > +
> > +#ifdef CONFIG_HAS_DEVICE_TREE
> > +#include <xen/device_tree.h>
> > +#endif
> 
> NIT: Could we try to rationalize the number of #ifdef CONFIG_HAS_*?
> For 
> example ...
> 
> > +
> > +#ifdef HAS_PCI
> > +#define dev_is_pci(dev) ((dev)->type == DEV_PCI)
> > +#endif
> > +
> > +#ifdef CONFIG_HAS_DEVICE_TREE
> > +#define dev_is_dt(dev)  ((dev)->type == DEV_DT)
> > +#endif
> 
> ... this is another definition for Device-Tree only. It could be
> easily 
> moved above the definitnion of dev_is_pci(). The others would be the 
> DT_DEVICE_*() helpers.
Yes, sure. I can combine CONFIG_HAS_DEVICE_TREE related things better.

> 
> > +
> > +enum device_class
> > +{
> > +    DEVICE_SERIAL,
> > +    DEVICE_IOMMU,
> > +    DEVICE_IC,
> 
> I guess you mean INTERRUPT_CONTROLLER? If so, can this be spelt out?
> (I 
> don't think shorthand version is worth it here)
Sure, I'll change to INTERRUPT_CONTROLLER.

~ Oleksii