Re: [PATCH 2/3] hw/sensor: max31785 : add fan status, tach target, and tach input object properties
On Thu, 14 Jul 2022 at 22:14, Maheswara Kurapati wrote: > On 7/14/22 8:10 AM, Peter Maydell wrote: > > On Thu, 14 Jul 2022 at 14:04, Maheswara Kurapati > > wrote: > >> This fix adds object properties for the FAN_COMMAND_1 (3Bh), > >> STATUS_FANS_1_2 (81h), > >> READ_FAN_SPEED_1 (90h) registers for the MAX31785 instrumentation. An > >> additional > >> property tach_margin_percent updates the tachs for a configured percent of > >> FAN_COMMAND_1 value. > >> > >> Registerproperty > >> -- > >> FAN_COMMAND_1 (3Bh) fan_target > >> STATUS_FANS_1_2 (81h) status_fans_1_2 > >> READ_FAN_SPEED_1 (90h) fan_input > > This commit message is missing the rationale -- why do we need this? > The STATUS_FANS_1_2, and READ_FAN_SPEED_1 registers are read-only. I > added these properties to simulate the error device faults. I'm not entirely sure what you have in mind here, but QEMU doesn't generally simulate device error injection. > > I am also not sure that we should be defining properties that are > > just straight 1:1 with the device registers. Compare the way we > > handle temperature-sensor values, where the property values are > > defined in a generic manner (same units representation) regardless > > of the underlying device and the device's property-set-get implementation > > then handles converting that to and from whatever internal implementation > > representation the device happens to use. > I am not sure I understood your comment. I checked hw/sensors/tmp105.c, > in which a "temperature" property is added for the tmp_input field in > almost the similar way what I did, except that the registers in the > MAX31785 are in direct format. Yes, that is my point. My impression is that you've provided properties that directly match the register format of this device because that's easy. I think that instead we should consider what the properties are intended to do, and perhaps have a standard convention for what format to use for particular kinds of data, as we do for temperature already. -- PMM
Re: [PATCH 2/3] hw/sensor: max31785 : add fan status, tach target, and tach input object properties
Hello Peter, Thank you for the review. Please see my comments inline. Thank you, Mahesh On 7/14/22 8:10 AM, Peter Maydell wrote: On Thu, 14 Jul 2022 at 14:04, Maheswara Kurapati wrote: This fix adds object properties for the FAN_COMMAND_1 (3Bh), STATUS_FANS_1_2 (81h), READ_FAN_SPEED_1 (90h) registers for the MAX31785 instrumentation. An additional property tach_margin_percent updates the tachs for a configured percent of FAN_COMMAND_1 value. Registerproperty -- FAN_COMMAND_1 (3Bh) fan_target STATUS_FANS_1_2 (81h) status_fans_1_2 READ_FAN_SPEED_1 (90h) fan_input This commit message is missing the rationale -- why do we need this? The STATUS_FANS_1_2, and READ_FAN_SPEED_1 registers are read-only. I added these properties to simulate the error device faults. I am also not sure that we should be defining properties that are just straight 1:1 with the device registers. Compare the way we handle temperature-sensor values, where the property values are defined in a generic manner (same units representation) regardless of the underlying device and the device's property-set-get implementation then handles converting that to and from whatever internal implementation representation the device happens to use. I am not sure I understood your comment. I checked hw/sensors/tmp105.c, in which a "temperature" property is added for the tmp_input field in almost the similar way what I did, except that the registers in the MAX31785 are in direct format. thanks -- PMM
Re: [PATCH 2/3] hw/sensor: max31785 : add fan status, tach target, and tach input object properties
On Thu, 14 Jul 2022 at 14:04, Maheswara Kurapati wrote: > > This fix adds object properties for the FAN_COMMAND_1 (3Bh), STATUS_FANS_1_2 > (81h), > READ_FAN_SPEED_1 (90h) registers for the MAX31785 instrumentation. An > additional > property tach_margin_percent updates the tachs for a configured percent of > FAN_COMMAND_1 value. > > Registerproperty > -- > FAN_COMMAND_1 (3Bh) fan_target > STATUS_FANS_1_2 (81h) status_fans_1_2 > READ_FAN_SPEED_1 (90h) fan_input This commit message is missing the rationale -- why do we need this? I am also not sure that we should be defining properties that are just straight 1:1 with the device registers. Compare the way we handle temperature-sensor values, where the property values are defined in a generic manner (same units representation) regardless of the underlying device and the device's property-set-get implementation then handles converting that to and from whatever internal implementation representation the device happens to use. thanks -- PMM
[PATCH 2/3] hw/sensor: max31785 : add fan status, tach target, and tach input object properties
This fix adds object properties for the FAN_COMMAND_1 (3Bh), STATUS_FANS_1_2
(81h),
READ_FAN_SPEED_1 (90h) registers for the MAX31785 instrumentation. An additional
property tach_margin_percent updates the tachs for a configured percent of
FAN_COMMAND_1 value.
Registerproperty
--
FAN_COMMAND_1 (3Bh) fan_target
STATUS_FANS_1_2 (81h) status_fans_1_2
READ_FAN_SPEED_1 (90h) fan_input
Signed-off-by: Maheswara Kurapati
---
hw/sensor/max31785.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/hw/sensor/max31785.c b/hw/sensor/max31785.c
index 8b95e32481..1cb31c2e82 100644
--- a/hw/sensor/max31785.c
+++ b/hw/sensor/max31785.c
@@ -164,6 +164,7 @@ typedef struct MAX31785State {
uint64_t mfr_date;
uint64_t mfr_serial;
uint16_t mfr_revision;
+int8_t tach_margin_percent[MAX31785_FAN_PAGES];
} MAX31785State;
static uint8_t max31785_read_byte(PMBusDevice *pmdev)
@@ -530,6 +531,27 @@ static void max31785_init(Object *obj)
for (int i = MAX31785_MIN_FAN_PAGE; i <= MAX31785_MAX_FAN_PAGE; i++) {
pmbus_page_config(pmdev, i, PB_HAS_VOUT_MODE);
+
+/* STATUS_FANS_1_2 (81h) for FAULT and WARN bits */
+object_property_add_uint8_ptr(obj, "status_fans_1_2[*]",
+&pmdev->pages[i].status_fans_1_2,
+OBJ_PROP_FLAG_READWRITE);
+
+/* FAN_COMMAND_1 (3Bh) target fan speed (pwm/rpm) */
+object_property_add_uint16_ptr(obj, "fan_target[*]",
+&pmdev->pages[i].fan_command_1,
+OBJ_PROP_FLAG_READWRITE);
+
+/* margin fan speed in percent (could be +ve or -ve) */
+object_property_add_int8_ptr(obj, "tach_margin_percent[*]",
+&(MAX31785(obj))->tach_margin_percent[i],
+OBJ_PROP_FLAG_READWRITE);
+
+/* READ_FAN_SPEED_1 (90h) returns the fan speed in RPM */
+object_property_add_uint16_ptr(obj, "fan_input[*]",
+&pmdev->pages[i].read_fan_speed_1,
+OBJ_PROP_FLAG_READWRITE);
+
}
for (int i = MAX31785_MIN_TEMP_PAGE; i <= MAX31785_MAX_TEMP_PAGE; i++) {
--
2.25.1
