Re: [PATCH v3 4/5] hw/misc: Add a PWM module for NPCM7XX

[Peter Maydell]

On Tue, 15 Dec 2020 at 00:13, Hao Wu <wuhaotsh@google.com> wrote:
>
> The PWM module is part of NPCM7XX module. Each NPCM7XX module has two
> identical PWM modules. Each module contains 4 PWM entries. Each PWM has
> two outputs: frequency and duty_cycle. Both are computed using inputs
> from software side.
>
> This module does not model detail pulse signals since it is expensive.
> It also does not model interrupts and watchdogs that are dependant on
> the detail models. The interfaces for these are left in the module so
> that anyone in need for these functionalities can implement on their
> own.
>
> The user can read the duty cycle and frequency using qom-get command.
>
> Reviewed-by: Havard Skinnemoen <hskinnemoen@google.com>
> Reviewed-by: Tyrone Ting <kfting@nuvoton.com>
> Signed-off-by: Hao Wu <wuhaotsh@google.com>


> +static void npcm7xx_pwm_init(Object *obj)
> +{
> +    NPCM7xxPWMState *s = NPCM7XX_PWM(obj);
> +    SysBusDevice *sbd = &s->parent;

This isn't right. A device shouldn't be poking around
in the 'parent' or 'parentobj' member of its struct --
that is a QOM internal. If you want "this device, cast
to a SysBusDevice", the way to write that is:
   SysBusDevice *sbd = SYS_BUS_DEVICE(s);

(or you could pass 'obj'; same thing).

Looking at the code currently in the tree it also is making this
same mistake:

$ git grep -- '->parent' hw/*/npcm*
hw/arm/npcm7xx_boards.c:    MachineClass *mc = &nmc->parent;
hw/mem/npcm7xx_mc.c:    sysbus_init_mmio(&s->parent, &s->mmio);
hw/misc/npcm7xx_clk.c:    sysbus_init_mmio(&s->parent, &s->iomem);
hw/misc/npcm7xx_gcr.c:    sysbus_init_mmio(&s->parent, &s->iomem);
hw/misc/npcm7xx_rng.c:    sysbus_init_mmio(&s->parent, &s->iomem);
hw/nvram/npcm7xx_otp.c:    SysBusDevice *sbd = &s->parent;
hw/ssi/npcm7xx_fiu.c:    SysBusDevice *sbd = &s->parent;
hw/timer/npcm7xx_timer.c:    SysBusDevice *sbd = &s->parent;

These all should be using QOM cast macros. Would somebody
who's working on these devices like to send a patch ?

thanks
-- PMM