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