Re: [PATCH v3 10/14] hw/sensor: Add Renesas ISL69259 device model

[Peter Delevoryas]

On Thu, Jun 30, 2022 at 12:16:05PM -0700, Titus Rwantare wrote:
> On Wed, 29 Jun 2022 at 21:52, Peter Delevoryas <me@pjd.dev> wrote:
> >
> > From: Peter Delevoryas <pdel@fb.com>
> >
> > This adds the ISL69259, using all the same functionality as the existing
> > ISL69260 but overriding the IC_DEVICE_ID.
> >
> > Signed-off-by: Peter Delevoryas <pdel@fb.com>
> > ---
> >  hw/sensor/isl_pmbus_vr.c | 28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> >
> > diff --git a/hw/sensor/isl_pmbus_vr.c b/hw/sensor/isl_pmbus_vr.c
> > index 799ea9d89e..853d70536f 100644
> > --- a/hw/sensor/isl_pmbus_vr.c
> > +++ b/hw/sensor/isl_pmbus_vr.c
> > @@ -119,6 +119,18 @@ static void raa228000_exit_reset(Object *obj)
> >      pmdev->pages[0].read_temperature_3 = 0;
> >  }
> >
> > +static void isl69259_exit_reset(Object *obj)
> > +{
> > +    ISLState *s = ISL69260(obj);
> > +    static const uint8_t ic_device_id[] = {0x04, 0x00, 0x81, 0xD2, 0x49, 
> > 0x3c};
> > +    g_assert_cmphex(sizeof(ic_device_id), <=, sizeof(s->ic_device_id));
> > +
> 
> This generates an error from the checkpatch script:
> Checking 0010-hw-sensor-Add-Renesas-ISL69259-device-model.patch...
> ERROR: Use g_assert or g_assert_not_reached
> #27: FILE: hw/sensor/isl_pmbus_vr.c:126:
> +    g_assert_cmphex(sizeof(ic_device_id), <=, sizeof(s->ic_device_id));

Argghhh I should have caught this, thanks. I'll replace it with g_assert. I
didn't realize there was some kind of portability issue with using
g_assert_cmphex in non-test code.

> 
> otherwise, LGTM.

That's great! Thanks for the review. I'll let you and Cedric sort
out if we want to make IC_DEVICE_ID a class property or keep it
in exit_reset as everything else class-specific is right now.

I'll still resubmit the patches as a separate series though with
the g_assert fix and your reviewed-by tags.

> 
> 
> Titus