[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/3] hw/sensor: max31785 : add fan status, tach target, and t
From: |
Peter Maydell |
Subject: |
Re: [PATCH 2/3] hw/sensor: max31785 : add fan status, tach target, and tach input object properties |
Date: |
Fri, 15 Jul 2022 09:25:28 +0100 |
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