Re: [PATCH 6/9] hw/sensor: add Maxim MAX31785 device

[Jae Hyun Yoo]

Hello Titus,

On 6/22/2022 1:49 PM, Titus Rwantare wrote:
> On Wed, 22 Jun 2022 at 10:29, Jae Hyun Yoo <quic_jaehyoo@quicinc.com> wrote:
> > From: Maheswara Kurapati <quic_mkurapat@quicinc.com>
> >
> > MAX31785 is a PMBus compliant 6-Channel fan controller. It supports 6 fan
> > channels, 11 temperature sensors, and 6-Channel ADC to measure the remote
> > voltages. Datasheet can be found here:
> > https://datasheets.maximintegrated.com/en/ds/MAX31785.pdf
> >
> > This initial version of the driver has skeleton and support for the
> > fan channels. Requests for temperature sensors, and ADC Channels the
> > are serviced with the default values as per the datasheet.  No additional
> > instrumentation is done.  NV Log feature is not supported.
> >
> > Signed-off-by: Maheswara Kurapati <quic_mkurapat@quicinc.com>
> > ---
> >   hw/arm/Kconfig        |   1 +
> >   hw/arm/aspeed.c       |   6 +-
> >   hw/sensor/Kconfig     |   4 +
> >   hw/sensor/max31785.c  | 580 ++++++++++++++++++++++++++++++++++++++++++
> >   hw/sensor/meson.build |   1 +
> >   5 files changed, 590 insertions(+), 2 deletions(-)
> >   create mode 100644 hw/sensor/max31785.c
> >
> > diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> > index 219262a8da36..77ef0fa967b2 100644
> > --- a/hw/arm/Kconfig
> > +++ b/hw/arm/Kconfig
> > @@ -408,6 +408,7 @@ config NPCM7XX
> >       select SSI
> >       select UNIMP
> >       select PCA954X
> > +    select MAX31785
> >
> >   config FSL_IMX25
> >       bool
>
> As this is being used with the Aspeed 2600, you may need to select
> PMBUS and MAX31785 under config ASPEED_SOC in this same file.

Okay. Will fix it in v2.

> > diff --git a/hw/sensor/max31785.c b/hw/sensor/max31785.c
> > new file mode 100644
> > index 000000000000..11bf9977b6fd
> > --- /dev/null
> > +++ b/hw/sensor/max31785.c
>
>
> Also, style nit, the checkpatch.pl script doesn't check whitespace
> alignment. But the style guide
> https://qemu-project.gitlab.io/qemu/devel/style.html#multiline-indent
> specifies the variants we should use.

Will fix the indentation in v2.

> > +        pmdev->pages[i].vout_scale_monitor =
> > +        MAX31785_DEFAULT_VOUT_SCALE_MONITOR;
> > +        pmdev->pages[i].vout_ov_fault_limit =
> > +        MAX31785_DEFAULT_OV_FAULT_LIMIT;
> > +        pmdev->pages[i].vout_ov_warn_limit =
> > +        MAX31785_DEFAULT_OV_WARN_LIMIT;
> > +    }
> > +
> > +}
> > +
> > +static const VMStateDescription vmstate_max31785 = {
> > +    .name = TYPE_MAX31785,
> > +    .version_id = 0,
> > +    .minimum_version_id = 0,
> > +    .fields = (VMStateField[]){
> > +        VMSTATE_PMBUS_DEVICE(parent, MAX31785State),
> > +        VMSTATE_UINT16_ARRAY(mfr_mode, MAX31785State,
> > +        MAX31785_TOTAL_NUM_PAGES),
> > +        VMSTATE_UINT16_ARRAY(vout_peak, MAX31785State,
> > +        MAX31785_TOTAL_NUM_PAGES),
> > +        VMSTATE_UINT16_ARRAY(temperature_peak, MAX31785State,
> > +        MAX31785_TOTAL_NUM_PAGES),
> > +        VMSTATE_UINT16_ARRAY(vout_min, MAX31785State,
> > +        MAX31785_TOTAL_NUM_PAGES),
> > +        VMSTATE_UINT8_ARRAY(fault_response, MAX31785State,
> > +        MAX31785_TOTAL_NUM_PAGES),
> > +        VMSTATE_UINT32_ARRAY(time_count, MAX31785State,
> > +        MAX31785_TOTAL_NUM_PAGES),
> > +        VMSTATE_UINT16_ARRAY(temp_sensor_config, MAX31785State,
> > +        MAX31785_TOTAL_NUM_PAGES),
> > +        VMSTATE_UINT16_ARRAY(fan_config, MAX31785State,
> > +        MAX31785_TOTAL_NUM_PAGES),
> > +        VMSTATE_UINT16_ARRAY(read_fan_pwm, MAX31785State,
> > +        MAX31785_TOTAL_NUM_PAGES),
> > +        VMSTATE_UINT16_ARRAY(fan_fault_limit, MAX31785State,
> > +        MAX31785_TOTAL_NUM_PAGES),
> > +        VMSTATE_UINT16_ARRAY(fan_warn_limit, MAX31785State,
> > +        MAX31785_TOTAL_NUM_PAGES),
> > +        VMSTATE_UINT16_ARRAY(fan_run_time, MAX31785State,
> > +        MAX31785_TOTAL_NUM_PAGES),
> > +        VMSTATE_UINT16_ARRAY(fan_pwm_avg, MAX31785State,
> > +        MAX31785_TOTAL_NUM_PAGES),
> > +        VMSTATE_UINT64_ARRAY(fan_pwm2rpm, MAX31785State,
> > +        MAX31785_TOTAL_NUM_PAGES),
> > +        VMSTATE_UINT64(mfr_location, MAX31785State),
> > +        VMSTATE_UINT64(mfr_date, MAX31785State),
> > +        VMSTATE_UINT64(mfr_serial, MAX31785State),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> There's missing indentation here for example.

Will fix it too.

Thanks,
Jae