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

[Hao Wu via <qemu-devel@nongnu.org>]

[-- Attachment #1: Type: text/plain, Size: 2268 bytes --]

Thanks for the review. We can add a patch in this patchset to fix this
issue.

On Wed, Dec 16, 2020 at 11:02 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

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

[-- Attachment #2: Type: text/html, Size: 3198 bytes --]