On 19 Jun 2026, at 13:55, [email protected] wrote:
From: Alexander Hansen <[email protected]>
diff --git a/hw/sensor/max11615.c b/hw/sensor/max11615.c
new file mode 100644
index 0000000000..ac2ccda4fa
--- /dev/null
+++ b/hw/sensor/max11615.c
Would it make sense to call it max1161x.c and create a specific version
for the 8-channel version?
It seems Analog Devices produces MAX 11612 .. 11617 which mostly vary
with the input channel count, so your driver could later be extended to
support the 4- and 12- channel version. This would avoid being stuck
with a specific model in its name, as it is unfortunately the case with
several I2C emulated device in QEMU such as PCA9552, PCA9554, …
+#include "qapi/visitor.h"
I think this file inclusion is not needed.
+struct MAX11615Class {
+ I2CSlaveClass parent_class;
+};
+
+OBJECT_DECLARE_TYPE(MAX11615State, MAX11615Class, MAX11615)
Maybe stick with the I2CSlaveClass as no feature is added here, with
OBJECT_DECLARE_TYPED_SIMPLE(MAX11615State, MAX11615)
If you opt-in for a multi-channel implementation (MAX1161x), this does
not apply, as the class should contain the number of ADC channels,
initialized for the MAX11615 version with MAX11615_NUM_CHANNELS.
+static int16_t max11615_differential_value(MAX11615State *s)
+{
+ /* Table 4. Channel Selection in Differential Mode */
+ size_t i1 = s->pointer;
+ size_t i2 = (s->pointer % 2 == 0) ? s->pointer + 1 : s->pointer -
1;
+
+ const int16_t ch1 = s->channels[i1];
+ const int16_t ch2 = s->channels[i2];
+ return ch1 - ch2;
+}
Maybe const MAX11615State *s
+static void max11615_write_config_byte(MAX11615State *s, uint8_t
data)
+{
+ trace_max11615_write_config(s->i2c.address, data);
+
+ uint8_t scan_select = (data >> 5) & 0x3;
+
+ if (scan_select != 0x3) {
+ qemu_log_mask(LOG_UNIMP, "%s: unimplemented scan select\n",
__func__);
+ }
+
+ uint8_t channel_select = (data >> 1) & 0xf;
+
+ /* Table 3. Channel Selection */
+ if (channel_select >= MAX11615_NUM_CHANNELS) {
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid channel
select\n",
+ __func__);
+ channel_select = MAX11615_NUM_CHANNELS - 1;
+ }
+ s->pointer = channel_select;
+ s->single_ended = data & 0x1;
+}
I’m not sure about the invalid channel management. The datasheet
specifies:
“For the MAX11615, CS3 is internally set to 0”.
My understanding is that channel_select is therefore masked with
MAX11615_NUM_CHANNELS - 1 here, not 0xf;
the actual selected channel is wrapped rather than been maxed.
BTW a nice addition - for a future update, I do not think it is worth
changing it now - would be to use the registerfields.h macros, it would
make the code more readable as hardcoded mask & shift would be managed.
+I2CSlave *max11615_init_with_values(I2CBus *bus, uint8_t address,
+ const uint16_t *init_values,
+ uint32_t init_values_size)
+{
+ MAX11615State *s;
+
+ s = MAX11615(i2c_slave_new(TYPE_MAX11615, address));
+
+ i2c_slave_realize_and_unref(I2C_SLAVE(s), bus, &error_abort);
+
+ for (int i = 0; i < MAX11615_NUM_CHANNELS; i++) {
+ /* arbitrary value if there is no data*/
+ s->channels[i] = i < init_values_size ? init_values[i] :
0x2d2;
+ }
+
+ return I2C_SLAVE(s);
+}
max11615_reset_enter is called once the device is realized, overriding
those values with the default reset ones. I’m not sure to understand
how this function can be used.
I think it would be more flexible to use QOM properties rather than a
custom dedicated function to initialize default values.
+
+static void max11615_reset_enter(MAX11615State *s)
+{
+ trace_max11615_reset_enter(s->i2c.address);
+
+ s->single_ended = true;
+ s->bipolar = false;
+ s->vref = 2048;
+ s->pointer = 0;
+ s->outlen = 0;
+
+ for (int i = 0; i < MAX11615_NUM_CHANNELS; i++) {
+ s->channels[i] = 0x2d2;
+ }
+}
+
+static void max11615_reset(Object *obj, ResetType type)
+{
+ MAX11615State *s = MAX11615(obj);
+
+ max11615_reset_enter(s);
+}
I tend to think it should be the opposite call logic:
- max11615_reset should be called max11615_reset_hold as per
rc->phases.hold = max11615_reset;
- max11615_reset_hold should call max11615_reset
- max11615_reset should initialize the default values
- max11615_reset should also be called from write to SETUP./rst (hence
the need for two reset functions)
