Re: [PATCH 3/5] hw/sensor: add Intersil ISL69260 device model

[Peter Maydell]

On Thu, 27 Jan 2022 at 20:54, Titus Rwantare <titusr@google.com> wrote:
>
> On Thu, 27 Jan 2022 at 11:39, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Thu, 6 Jan 2022 at 23:19, Titus Rwantare <titusr@google.com> wrote:
> > >
>
> > > +static uint8_t isl_pmbus_read_byte(PMBusDevice *pmdev)
> > > +{
> > > +    qemu_log_mask(LOG_GUEST_ERROR,
> > > +                  "%s: reading from unsupported register: 0x%02x\n",
> > > +                  __func__, pmdev->code);
> > > +    return 0xFF;
> > > +}
> > > +
> > > +static int isl_pmbus_write_data(PMBusDevice *pmdev, const uint8_t *buf,
> > > +                              uint8_t len)
> > > +{
> > > +    qemu_log_mask(LOG_GUEST_ERROR,
> > > +                  "%s: write to unsupported register: 0x%02x\n",
> > > +                  __func__, pmdev->code);
> > > +    return 0xFF;
> > > +}
> >
> > This device appears to have no implemented guest visible
> > interface at all, and yet it has a lot of object properties.
> > What's going on here ?

> This device relies on read_byte and write_data implemented in
> pmbus_device.c. Those generic implementations fall through to the
> device specific implementations for registers not in the standard.
> This qemu model happens not to include additional registers. However,
> I must change these to LOG_UNIMP which is more appropriate to what's
> going on.

Ah, right. A comment noting something like that might also
be helpful.

Something I forgot to note -- the filenames isl_pmbus.[ch] seem a
bit overly generic for "PMBus device for Renesas Digital Multiphase
Voltage Regulators" -- could you rename to something a bit more
specific? This will help to avoid in future people lumping
lots of different unrelated device models into the same source file.

thanks
-- PMM