[PATCH 0/3] Object properties for MAX31785 status, target, and input.

[PATCH 0/3] Object properties for MAX31785 status, target, and input.

[Maheswara Kurapati]

Hello,

I'm sending a series to add Object properties for MAX31785 device. These properties are for the fan status, tach target, tach input, and tach margin (+ve or -ve) percent.  To support the signed tach margin input, added infrastructure routines for 8 bit signed integer.

Please review.

Thanks,

Mahesh

Maheswara Kurapati (3):
  qom/object : add object_property_add_int8_ptr() property for 8 bit
    signed integers.
  hw/sensor: max31785 : add fan status, tach target, and tach input
    object properties
  hw/sensor: max31785 : update the tach input based on the tach margin
    percentage

 hw/sensor/max31785.c | 46 ++++++++++++++++++++++++++++++---
 include/qom/object.h | 21 ++++++++++++++++
 qom/object.c         | 60 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 123 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH 1/3] qom/object : add object_property_add_int8_ptr() property for 8 bit signed integers.

[Maheswara Kurapati]

Current implementation lacks the support to add signed 8 bit integer
property to an Object.  This fix adds the necessary infrastructure
routines.

Signed-off-by: Maheswara Kurapati <quic_mkurapat@quicinc.com>
---
 include/qom/object.h | 21 ++++++++++++++++
 qom/object.c         | 60 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 81 insertions(+)

diff --git a/include/qom/object.h b/include/qom/object.h
index ef7258a5e1..60b88ed685 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1764,6 +1764,27 @@ typedef enum {
     OBJ_PROP_FLAG_READWRITE = (OBJ_PROP_FLAG_READ | OBJ_PROP_FLAG_WRITE),
 } ObjectPropertyFlags;
 
+/**
+ * object_property_add_int8_ptr:
+ * @obj: the object to add a property to
+ * @name: the name of the property
+ * @v: pointer to value
+ * @flags: bitwise-or'd ObjectPropertyFlags
+ *
+ * Add a signed integer property in memory.  This function will add a
+ * property of type 'int8'.
+ *
+ * Returns: The newly added property on success, or %NULL on failure.
+ */
+ObjectProperty *object_property_add_int8_ptr(Object *obj, const char *name,
+                                              const int8_t *v,
+                                              ObjectPropertyFlags flags);
+
+ObjectProperty *object_class_property_add_int8_ptr(ObjectClass *klass,
+                                         const char *name,
+                                         const int8_t *v,
+                                         ObjectPropertyFlags flags);
+
 /**
  * object_property_add_uint8_ptr:
  * @obj: the object to add a property to
diff --git a/qom/object.c b/qom/object.c
index d34608558e..1a0e64157f 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -2450,6 +2450,26 @@ static char *object_get_type(Object *obj, Error **errp)
     return g_strdup(object_get_typename(obj));
 }
 
+static void property_get_int8_ptr(Object *obj, Visitor *v, const char *name,
+                                   void *opaque, Error **errp)
+{
+    int8_t value = *(int8_t *)opaque;
+    visit_type_int8(v, name, &value, errp);
+}
+
+static void property_set_int8_ptr(Object *obj, Visitor *v, const char *name,
+                                   void *opaque, Error **errp)
+{
+    int8_t *field = opaque;
+    int8_t value;
+
+    if (!visit_type_int8(v, name, &value, errp)) {
+        return;
+    }
+
+    *field = value;
+}
+
 static void property_get_uint8_ptr(Object *obj, Visitor *v, const char *name,
                                    void *opaque, Error **errp)
 {
@@ -2530,6 +2550,46 @@ static void property_set_uint64_ptr(Object *obj, Visitor *v, const char *name,
     *field = value;
 }
 
+ObjectProperty *
+object_property_add_int8_ptr(Object *obj, const char *name,
+                              const int8_t *v,
+                              ObjectPropertyFlags flags)
+{
+    ObjectPropertyAccessor *getter = NULL;
+    ObjectPropertyAccessor *setter = NULL;
+
+    if ((flags & OBJ_PROP_FLAG_READ) == OBJ_PROP_FLAG_READ) {
+        getter = property_get_int8_ptr;
+    }
+
+    if ((flags & OBJ_PROP_FLAG_WRITE) == OBJ_PROP_FLAG_WRITE) {
+        setter = property_set_int8_ptr;
+    }
+
+    return object_property_add(obj, name, "int8",
+                               getter, setter, NULL, (void *)v);
+}
+
+ObjectProperty *
+object_class_property_add_int8_ptr(ObjectClass *klass, const char *name,
+                                    const int8_t *v,
+                                    ObjectPropertyFlags flags)
+{
+    ObjectPropertyAccessor *getter = NULL;
+    ObjectPropertyAccessor *setter = NULL;
+
+    if ((flags & OBJ_PROP_FLAG_READ) == OBJ_PROP_FLAG_READ) {
+        getter = property_get_int8_ptr;
+    }
+
+    if ((flags & OBJ_PROP_FLAG_WRITE) == OBJ_PROP_FLAG_WRITE) {
+        setter = property_set_int8_ptr;
+    }
+
+    return object_class_property_add(klass, name, "int8",
+                                     getter, setter, NULL, (void *)v);
+}
+
 ObjectProperty *
 object_property_add_uint8_ptr(Object *obj, const char *name,
                               const uint8_t *v,
-- 
2.25.1

[PATCH 2/3] hw/sensor: max31785 : add fan status, tach target, and tach input object properties

[Maheswara Kurapati]

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.

Register                property
--------------------------------------
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 <quic_mkurapat@quicinc.com>
---
 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

[PATCH 3/3] hw/sensor: max31785 : update the tach input based on the tach margin percentage

[Maheswara Kurapati]

Update the tach input based on the percentage of tach target.  The tach margin
could be a +ve or -ve margin of the target tach rpm.

Signed-off-by: Maheswara Kurapati <quic_mkurapat@quicinc.com>
---
 hw/sensor/max31785.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/hw/sensor/max31785.c b/hw/sensor/max31785.c
index 1cb31c2e82..c37b154130 100644
--- a/hw/sensor/max31785.c
+++ b/hw/sensor/max31785.c
@@ -71,7 +71,7 @@
 /* FAN_CONFIG_1_2 */
 #define MAX31785_MFR_FAN_CONFIG                0xF1
 #define MAX31785_FAN_CONFIG_ENABLE             BIT(7)
-#define MAX31785_FAN_CONFIG_RPM_PWM            BIT(6)
+#define MAX31785_FAN_CONFIG_FAN_MODE_RPM       BIT(6)
 #define MAX31785_FAN_CONFIG_PULSE(pulse)       (pulse << 4)
 #define MAX31785_DEFAULT_FAN_CONFIG_1_2(pulse)                                 \
     (MAX31785_FAN_CONFIG_ENABLE | MAX31785_FAN_CONFIG_PULSE(pulse))
@@ -316,6 +316,8 @@ static int max31785_write_data(PMBusDevice *pmdev, const uint8_t *buf,
                                uint8_t len)
 {
     MAX31785State *s = MAX31785(pmdev);
+    int16_t   tach_margin = 0;
+
     if (len == 0) {
         qemu_log_mask(LOG_GUEST_ERROR, "%s: writing empty data\n", __func__);
         return -1;
@@ -342,9 +344,23 @@ static int max31785_write_data(PMBusDevice *pmdev, const uint8_t *buf,
     case PMBUS_FAN_COMMAND_1:
         if (pmdev->page <= MAX31785_MAX_FAN_PAGE) {
             pmdev->pages[pmdev->page].fan_command_1 = pmbus_receive16(pmdev);
-            pmdev->pages[pmdev->page].read_fan_speed_1 =
-                ((MAX31785_DEFAULT_FAN_SPEED / MAX31785_DEFAULT_FAN_MAX_PWM) *
-                pmdev->pages[pmdev->page].fan_command_1);
+            if ((pmdev->pages[pmdev->page].fan_config_1_2 &
+                 MAX31785_FAN_CONFIG_FAN_MODE_RPM)
+                  == MAX31785_FAN_CONFIG_FAN_MODE_RPM) {
+                /* calculate the tach margin (+ve or -ve) */
+                tach_margin = (int16_t)pmdev->pages[pmdev->page].fan_command_1 *
+                        ((float)s->tach_margin_percent[pmdev->page] / 100.0);
+
+                /* set the tach */
+                pmdev->pages[pmdev->page].read_fan_speed_1 =
+                 (uint16_t)(pmdev->pages[pmdev->page].fan_command_1 +
+                            tach_margin);
+            } else {
+                /* pwm mode */
+                pmdev->pages[pmdev->page].read_fan_speed_1 =
+                  ((MAX31785_DEFAULT_FAN_SPEED / MAX31785_DEFAULT_FAN_MAX_PWM) *
+                   pmdev->pages[pmdev->page].fan_command_1);
+            }
         }
         break;
 
-- 
2.25.1

Re: [PATCH 2/3] hw/sensor: max31785 : add fan status, tach target, and tach input object properties

[Peter Maydell]

On Thu, 14 Jul 2022 at 14:04, Maheswara Kurapati
<quic_mkurapat@quicinc.com> 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.
>
> Register                property
> --------------------------------------
> 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

Re: [PATCH 2/3] hw/sensor: max31785 : add fan status, tach target, and tach input object properties

[Maheswara Kurapati]

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
> <quic_mkurapat@quicinc.com> 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.
>>
>> Register                property
>> --------------------------------------
>> 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

[Peter Maydell]

On Thu, 14 Jul 2022 at 22:14, Maheswara Kurapati
<quic_mkurapat@quicinc.com> wrote:
> On 7/14/22 8:10 AM, Peter Maydell wrote:
> > On Thu, 14 Jul 2022 at 14:04, Maheswara Kurapati
> > <quic_mkurapat@quicinc.com> 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.
> >>
> >> Register                property
> >> --------------------------------------
> >> 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