| Message ID | 20220630045133.32251-11-me@pjd.dev |
|---|---|
| State | New |
| Headers | show |
| Series | hw/i2c/aspeed: I2C slave mode DMA RX w/ new regs | expand |
On 6/30/22 06:51, Peter Delevoryas 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}; This looks like an ISLClass attribute to me. In which case, you wouldn't need the reset handler nor the 'ic_device_id_len' field. Thanks, C. > + g_assert_cmphex(sizeof(ic_device_id), <=, sizeof(s->ic_device_id)); > + > + isl_pmbus_vr_exit_reset(obj); > + > + s->ic_device_id_len = sizeof(ic_device_id); > + memcpy(s->ic_device_id, ic_device_id, sizeof(ic_device_id)); > +} > + > static void isl_pmbus_vr_add_props(Object *obj, uint64_t *flags, uint8_t pages) > { > PMBusDevice *pmdev = PMBUS_DEVICE(obj); > @@ -257,6 +269,21 @@ static void raa229004_class_init(ObjectClass *klass, void *data) > isl_pmbus_vr_class_init(klass, data, 2); > } > > +static void isl69259_class_init(ObjectClass *klass, void *data) > +{ > + ResettableClass *rc = RESETTABLE_CLASS(klass); > + DeviceClass *dc = DEVICE_CLASS(klass); > + dc->desc = "Renesas ISL69259 Digital Multiphase Voltage Regulator"; > + rc->phases.exit = isl69259_exit_reset; > + isl_pmbus_vr_class_init(klass, data, 2); > +} > + > +static const TypeInfo isl69259_info = { > + .name = TYPE_ISL69259, > + .parent = TYPE_ISL69260, > + .class_init = isl69259_class_init, > +}; > + > static const TypeInfo isl69260_info = { > .name = TYPE_ISL69260, > .parent = TYPE_PMBUS_DEVICE, > @@ -283,6 +310,7 @@ static const TypeInfo raa228000_info = { > > static void isl_pmbus_vr_register_types(void) > { > + type_register_static(&isl69259_info); > type_register_static(&isl69260_info); > type_register_static(&raa228000_info); > type_register_static(&raa229004_info);
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)); otherwise, LGTM. Titus
On Wed, 29 Jun 2022 at 23:30, Cédric Le Goater <clg@kaod.org> wrote: > > On 6/30/22 06:51, Peter Delevoryas 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}; > > This looks like an ISLClass attribute to me. In which case, you wouldn't need the > reset handler nor the 'ic_device_id_len' field. > > Thanks, > > C. I asked for this because, so far, I've been doing all the register defaults in reset handlers, including read-only registers. I don't mind either way, but it seemed preferable to have the devices consistent. Titus
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
On 6/30/22 21:16, Titus Rwantare wrote: > On Wed, 29 Jun 2022 at 23:30, Cédric Le Goater <clg@kaod.org> wrote: >> >> On 6/30/22 06:51, Peter Delevoryas 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}; >> >> This looks like an ISLClass attribute to me. In which case, you wouldn't need the >> reset handler nor the 'ic_device_id_len' field. >> >> Thanks, >> >> C. > > I asked for this because, so far, I've been doing all the register > defaults in reset handlers, including read-only registers. > I don't mind either way, but it seemed preferable to have the devices > consistent. Sure. Fine for me. Thanks, C.
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)); + + isl_pmbus_vr_exit_reset(obj); + + s->ic_device_id_len = sizeof(ic_device_id); + memcpy(s->ic_device_id, ic_device_id, sizeof(ic_device_id)); +} + static void isl_pmbus_vr_add_props(Object *obj, uint64_t *flags, uint8_t pages) { PMBusDevice *pmdev = PMBUS_DEVICE(obj); @@ -257,6 +269,21 @@ static void raa229004_class_init(ObjectClass *klass, void *data) isl_pmbus_vr_class_init(klass, data, 2); } +static void isl69259_class_init(ObjectClass *klass, void *data) +{ + ResettableClass *rc = RESETTABLE_CLASS(klass); + DeviceClass *dc = DEVICE_CLASS(klass); + dc->desc = "Renesas ISL69259 Digital Multiphase Voltage Regulator"; + rc->phases.exit = isl69259_exit_reset; + isl_pmbus_vr_class_init(klass, data, 2); +} + +static const TypeInfo isl69259_info = { + .name = TYPE_ISL69259, + .parent = TYPE_ISL69260, + .class_init = isl69259_class_init, +}; + static const TypeInfo isl69260_info = { .name = TYPE_ISL69260, .parent = TYPE_PMBUS_DEVICE, @@ -283,6 +310,7 @@ static const TypeInfo raa228000_info = { static void isl_pmbus_vr_register_types(void) { + type_register_static(&isl69259_info); type_register_static(&isl69260_info); type_register_static(&raa228000_info); type_register_static(&raa229004_info);