On Wed, 12 Jan 2022 at 00:25, Titus Rwantare wrote:
>
> Signed-off-by: Titus Rwantare
> Reviewed-by: Hao Wu
Hi; thanks for this patchset. For future revisions, could you
make sure you send a cover-letter for patchsets that have
more than one patch? It helps to keep the automated tooling
from getting confused.
> ---
> MAINTAINERS | 8 +-
> hw/arm/Kconfig| 1 +
> hw/sensor/Kconfig | 4 +
> hw/sensor/max31790_fan_ctrl.c | 454 ++
> hw/sensor/meson.build | 1 +
> include/hw/sensor/max31790_fan_ctrl.h | 93 ++
> 6 files changed, 560 insertions(+), 1 deletion(-)
> create mode 100644 hw/sensor/max31790_fan_ctrl.c
> create mode 100644 include/hw/sensor/max31790_fan_ctrl.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c98a61caee..0791b6be42 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2304,6 +2304,12 @@ F: hw/timer/mips_gictimer.c
> F: include/hw/intc/mips_gic.h
> F: include/hw/timer/mips_gictimer.h
>
> +MAX31790 Fan controller
> +M: Titus Rwantare
> +S: Maintained
> +F: hw/sensor/max31790_fan_ctrl.c
> +F: include/hw/sensor/max31790_fan_ctrl.h
> +
> Subsystems
> --
> Overall Audio backends
> @@ -2798,7 +2804,7 @@ R: Paolo Bonzini
> R: Bandan Das
> R: Stefan Hajnoczi
> R: Thomas Huth
> -R: Darren Kenny
> +R: Darren Kenny
Why did this line get changed ? It looks like maybe there was
a trailing space that got deleted. If you want to do that kind
of cleanup it's best done as a separate patch.
> R: Qiuhao Li
> S: Maintained
> F: tests/qtest/fuzz/
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index e652590943..00bfbaf1c4 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -393,6 +393,7 @@ config NPCM7XX
> select SMBUS
> select AT24C # EEPROM
> select MAX34451
> +select MAX31790
> select PL310 # cache controller
> select PMBUS
> select SERIAL
> diff --git a/hw/sensor/Kconfig b/hw/sensor/Kconfig
> index 9c8a049b06..54d269d642 100644
> --- a/hw/sensor/Kconfig
> +++ b/hw/sensor/Kconfig
> @@ -21,3 +21,7 @@ config ADM1272
> config MAX34451
> bool
> depends on I2C
> +
> +config MAX31790
> +bool
> +depends on I2C
> diff --git a/hw/sensor/max31790_fan_ctrl.c b/hw/sensor/max31790_fan_ctrl.c
> new file mode 100644
> index 00..b5334c1130
> --- /dev/null
> +++ b/hw/sensor/max31790_fan_ctrl.c
> @@ -0,0 +1,454 @@
> +/*
> + * MAX31790 Fan controller
> + *
> + * Independently control 6 fans, up to 12 tachometer inputs,
> + * controlled through i2c
> + *
> + * This device model has read/write support for:
> + * - 9-bit pwm through i2c and qom/qmp
> + * - 11-bit tach_count through i2c
> + * - RPM through qom/qmp
> + *
> + * Copyright 2021 Google LLC
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/sensor/max31790_fan_ctrl.h"
> +#include "hw/irq.h"
> +#include "hw/qdev-properties.h"
> +#include "migration/vmstate.h"
> +#include "qapi/visitor.h"
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +
> +static uint16_t max31790_get_sr(uint8_t fan_dynamics)
> +{
> +uint16_t sr = 1 << ((fan_dynamics >> 5) & 0b111);
> +return sr > 16 ? 32 : sr;
> +}
> +
> +static void max31790_place_bits(uint16_t *dest, uint16_t byte, uint8_t
> offset)
> +{
> +uint16_t val = *dest;
> +val &= ~(0x00FF << offset);
> +val |= byte << offset;
> +*dest = val;
> +}
> +
> +/*
> + * calculating fan speed
> + * f_TOSC/4 is the clock, 8192Hz
> + * NP = tachometer pulses per revolution (usually 2)
> + * SR = number of periods(pulses) over which the clock ticks are counted
> + * TACH Count = SR x 8192 x 60 / (NP x RPM)
> + * RPM = SR x 8192 x 60 / (NP x TACH count)
> + *
> + * RPM mode - desired tach count is written to TACH Target Count
> + * PWM mode - desired duty cycle is written to PWMOUT Target Duty reg
> + */
> +static void max31790_calculate_tach_count(MAX31790State *ms, uint8_t id)
> +{
> +uint32_t rpm;
> +uint32_t sr = max31790_get_sr(ms->fan_dynamics[id]);
> +ms->pwm_duty_cycle[id] = ms->pwmout[id] >> 7;
> +rpm = (ms->max_rpm[id] * ms->pwm_duty_cycle[id]) / 0x1FF;
> +
> +if (rpm) {
> +ms->tach_count[id] = (sr * MAX31790_CLK_FREQ * 60) /
> + (MAX31790_PULSES_PER_REV * rpm);
This is all being done with 32-bit arithmetic. Is it definitely
never possible for it to overflow ?
> +} else {
> +ms->tach_count[id] = 0;
> +}
> +
> +}
> +
> +if ((ms->command + 1) % 8) {
> +ms->command++;
> +} else {
> +ms->command -= 7;
> +}
You could write this:
ms->command = (ms->command & ~7) | ((ms->command + 1) & 7);
Maybe that's clearer, maybe not -- your choice whether to change it or not.
> +/* assumes that the fans have the same speed range (SR) */
> +static void max31790_get_rpm(Object *obj, Visitor *v, const char