Re: [PATCH 2/5] cmd646: create separate header and QOM type for CMD646_IDE

[Mark Cave-Ayland]

On 12/06/2023 10:21, Bernhard Beschow wrote:

> Am 9. Juni 2023 18:51:16 UTC schrieb Mark Cave-Ayland 
> <mark.cave-ayland@ilande.co.uk>:
> > This will enable CMD646-specific fields to be added to CMD6464IDEState in
> > future.
> >
> > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > ---
> > hw/ide/cmd646.c         |  4 +++-
> > include/hw/ide/cmd646.h | 38 ++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 41 insertions(+), 1 deletion(-)
> > create mode 100644 include/hw/ide/cmd646.h
> >
> > diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
> > index 20f1e41d57..a3e227fba7 100644
> > --- a/hw/ide/cmd646.c
> > +++ b/hw/ide/cmd646.c
> > @@ -33,6 +33,7 @@
> > #include "sysemu/reset.h"
> >
> > #include "hw/ide/pci.h"
> > +#include "hw/ide/cmd646.h"
> > #include "trace.h"
> >
> > /* CMD646 specific */
> > @@ -339,9 +340,10 @@ static void cmd646_ide_class_init(ObjectClass *klass, void 
> > *data)
> > }
> >
> > static const TypeInfo cmd646_ide_info = {
> > -    .name          = "cmd646-ide",
> > +    .name          = TYPE_CMD646_IDE,
> >      .parent        = TYPE_PCI_IDE,
> >      .class_init    = cmd646_ide_class_init,
> > +    .instance_size = sizeof(CMD646IDEState),
> > };
> >
> > static void cmd646_ide_register_types(void)
> > diff --git a/include/hw/ide/cmd646.h b/include/hw/ide/cmd646.h
> > new file mode 100644
> > index 0000000000..4780b1132c
> > --- /dev/null
> > +++ b/include/hw/ide/cmd646.h
> > @@ -0,0 +1,38 @@
> > +/*
> > + * QEMU IDE Emulation: PCI cmd646 support.
> > + *
> > + * Copyright (c) 2003 Fabrice Bellard
> > + * Copyright (c) 2006 Openedhand Ltd.
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a copy
> > + * of this software and associated documentation files (the "Software"), to 
> > deal
> > + * in the Software without restriction, including without limitation the rights
> > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> > + * copies of the Software, and to permit persons to whom the Software is
> > + * furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> > FROM,
> > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> > + * THE SOFTWARE.
> > + */
> > +
> > +#ifndef HW_IDE_CMD646_H
> > +#define HW_IDE_CMD646_H
> > +
> > +#define TYPE_CMD646_IDE "cmd646-ide"
> > +OBJECT_DECLARE_SIMPLE_TYPE(CMD646IDEState, CMD646_IDE)
> > +
> > +#include "hw/ide/pci.h"
> > +
> > +struct CMD646IDEState {
> > +    PCIIDEState parent_obj;
>
> No public / private sections here? From the naming this attribute is often 
> considered private and the rest public. I guess what this scheme communicates 
> is that `parent_obj` should be accessed via `IDE_PCI()` only.

Whilst the public/private comments were included in the original QOM documentation, 
it's not something that tends to be used now. These days people are looking to use 
interfaces such as SysBus to indicated which MemoryRegions and IRQs are "public", but 
certainly the detail is still up for active discussion.

The QOM cast macros will allow the object referenced to be converted to any of the 
parent types directly, so indeed there should be no direct references to parent_obj.


ATB,

Mark.