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