Re: [PATCH v3 0/5] hw/sensor: Add new device emulation for TI ADC128D818

Re: [PATCH v3 0/5] hw/sensor: Add new device emulation for TI ADC128D818

[Cédric Le Goater]

Hello,

On 6/28/26 02:45, Emmanuel Blot wrote:
> This is a 12-bit, 8-channel ADC I2C device, which is used on several
> OpenBMC platforms.
> 
> QOM properties can be used to change input ADC values and external VREF at
> run time.
> 
> Add it to AST2600-based Anacapa machine, along with some functional tests
> that use QOM properties to inject voltage values into analog inputs.
> 
> Datasheet: https://www.ti.com/lit/gpn/ADC128D818
> 
> Signed-off-by: Emmanuel Blot <eblot@meta.com>
> ---
> Changes in v3:
> - Update anacapa test image with proper DTB
> - Make I2C device in i2c_slave_create_simple a child of the I2C bus
> - Simplify tests/functional/arm/test_aspeed_anacapa.py
> - Use g_test_message rather than fprintf
> - Keep review trailers
> - Link to v2: https://lore.kernel.org/qemu-devel/20260617-i2c-adc128d818-anacapa-v2-0-fd38764f0465@meta.com


v3 hasn't reached https://lore.kernel.org/qemu-devel/ yet.

Any visible errors on your side ?

Thanks,

C.

Re: [PATCH v3 1/5] hw/sensor: adc128d818: add 12-bit 8-channel ADC device

[Philippe Mathieu-Daudé]

Hi Emmanuel,

Minor style comments inlined.

On 28/6/26 02:45, Emmanuel Blot wrote:
> The ADC128D818 is a TI 12-bit, 8-channel I2C ADC used on several
> OpenBMC platforms for voltage and temperature monitoring.
> 
> Implement the device with:
>   - four operating modes
>   - 12-bit voltage conversion from QOM inputs
>   - 9-bit temperature conversion from milli-degree Celsius QOM inputs
>   - switchable internal or external voltage reference
>   - per-channel high/low limit registers
>   - interrupt support
>   - software reset
>   - one-shot conversion support in shutdown mode
> 
> Reviewed-by: Alexander Hansen <alexander.hansen@9elements.com>
> Tested-by: Alexander Hansen <alexander.hansen@9elements.com>
> Signed-off-by: Emmanuel Blot <eblot@meta.com>
> ---
>   hw/sensor/Kconfig              |   4 +
>   hw/sensor/adc128d818.c         | 697 +++++++++++++++++++++++++++++++++++++++++
>   hw/sensor/meson.build          |   1 +
>   hw/sensor/trace-events         |   8 +
>   include/hw/sensor/adc128d818.h |  14 +
>   5 files changed, 724 insertions(+)
> 
> diff --git a/hw/sensor/Kconfig b/hw/sensor/Kconfig
> index bc6331b4ab..b459ac2240 100644
> --- a/hw/sensor/Kconfig
> +++ b/hw/sensor/Kconfig
> @@ -1,3 +1,7 @@
> +config ADC128D818
> +    bool
> +    depends on I2C
> +
>   config TMP105
>       bool
>       depends on I2C
> diff --git a/hw/sensor/adc128d818.c b/hw/sensor/adc128d818.c
> new file mode 100644
> index 0000000000..031c7598ef
> --- /dev/null
> +++ b/hw/sensor/adc128d818.c
> @@ -0,0 +1,697 @@
> +/*
> + * Texas Instruments ADC128D818 12-bit 8-channel ADC with I2C interface
> + *
> + * Copyright (c) 2026 Meta Platforms, Inc. and affiliates.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qapi/error.h"
> +#include "qapi/visitor.h"
> +#include "qom/object.h"
> +#include "hw/sensor/adc128d818.h"
> +#include "hw/core/irq.h"
> +#include "hw/core/qdev-properties.h"
> +#include "hw/i2c/i2c.h"
> +#include "migration/vmstate.h"
> +#include "trace.h"
> +
> +
> +/* Register addresses */
> +#define REG_CONFIG              0x00u
> +#define REG_INT_STATUS          0x01u
> +#define REG_INT_MASK            0x03u
> +#define REG_CONV_RATE           0x07u
> +#define REG_CH_DISABLE          0x08u
> +#define REG_ONE_SHOT            0x09u
> +#define REG_DEEP_SHUTDOWN       0x0Au
> +#define REG_ADV_CONFIG          0x0Bu
> +#define REG_BUSY_STATUS         0x0Cu

QEMU coding style is usually lower case hex values, with no explicit
unsigned suffix. Not sure this is right, but using distinct style
make reviews harder.

> +/* Channel Reading Registers (16-bit, read-only) */
> +#define REG_CH_READING_BASE     0x20u
> +#define REG_CH_READING_LAST     0x27u
> +
> +/* Limit Registers (8-bit, read/write) */
> +#define REG_LIMIT_BASE          0x2Au
> +#define REG_LIMIT_LAST          0x39u
> +
> +/* ID Registers (read-only) */
> +#define REG_MANUFACTURER_ID     0x3Eu
> +#define REG_REVISION_ID         0x3Fu
> +
> +/* Configuration Register (0x00) bitfields */
> +#define CONFIG_START            BIT(0)
> +#define CONFIG_INT_ENABLE       BIT(1)
> +#define CONFIG_INT_CLEAR        BIT(3)
> +#define CONFIG_INITIALIZATION   BIT(7)
> +#define CONFIG_WR_MASK \
> +    (CONFIG_START | CONFIG_INT_ENABLE | CONFIG_INT_CLEAR)
> +
> +/* Advanced Configuration Register (0x0B) bitfields */
> +#define ADV_CONFIG_EXT_REF_EN   BIT(0)
> +#define ADV_CONFIG_MODE_SHIFT   1u
> +#define ADV_CONFIG_MODE_MASK    (0x3u << ADV_CONFIG_MODE_SHIFT)
> +#define ADV_CONFIG_WR_MASK \
> +    (ADV_CONFIG_EXT_REF_EN | ADV_CONFIG_MODE_MASK)
> +
> +/* Busy Status Register (0x0C) bitfields */
> +#define BUSY_STATUS_NOT_READY   BIT(1)
> +
> +/* Conversion Rate Register (0x07) bitfields */
> +#define CONV_RATE_MASK          0x01u
> +
> +/* Deep Shutdown Register (0x0A) bitfields */
> +#define DEEP_SHUTDOWN_EN        0x01u
> +
> +/* Device constants */
> +#define ADC128D818_NUM_CHANNELS         8u
> +#define ADC128D818_NUM_REGS             0x40u
> +
> +#define ADC128D818_INTERNAL_VREF_MV     2560u
> +#define ADC128D818_MAX_VDD_MV           5500u
> +#define ADC128D818_MANUFACTURER_ID_VAL  0x01u
> +#define ADC128D818_REVISION_ID_VAL      0x09u
> +
> +/* ADC resolution */
> +#define ADC128D818_ADC_RESOLUTION       4096u
> +#define ADC128D818_ADC_MAX              4095u
> +
> +/* Temperature: 0.5 deg C per LSb = 500 milli-degrees per LSb */
> +#define ADC128D818_TEMP_LSB_MC          500
> +#define ADC128D818_TEMP_RAW_MIN         (-256)
> +#define ADC128D818_TEMP_RAW_MAX         255
> +
> +
> +typedef struct ADC128D818State ADC128D818State;
> +DECLARE_INSTANCE_CHECKER(ADC128D818State, ADC128D818, TYPE_ADC128D818)

No need to forward declare if you use:

OBJECT_DECLARE_SIMPLE_TYPE(ADC128D818State, ADC128D818)

> +
> +struct ADC128D818State {
> +    I2CSlave i2c;

Again, per our coding style:

        I2CSlave parent_obj;

> +
> +    qemu_irq irq;
> +
> +    uint8_t len;
> +    uint8_t pointer;
> +    uint8_t rx_byte;
> +
> +    uint8_t regs[ADC128D818_NUM_REGS];
> +    uint16_t channel[ADC128D818_NUM_CHANNELS];
> +
> +    int16_t ain[ADC128D818_NUM_CHANNELS]; /* mV */
> +    int32_t temperature; /* milli-degrees Celsius */
> +    uint16_t ext_vref; /* mV, 0 means not connected */
> +    bool temp_alarm; /* temperature high-limit alarm latched */
> +
> +    char *description;
> +};

Re: [PATCH v3 2/5] tests/qtest: adc128d818: add test suite

[Philippe Mathieu-Daudé]

On 28/6/26 02:45, Emmanuel Blot wrote:
> Add QOS-based QTest coverage for the ADC128D818:
> 
>   - manufacturer and revision ID registers
>   - power-on-reset defaults
>   - software reset
>   - voltage conversion and edge cases
>   - all channels with distinct values simultaneously
>   - temperature conversion and edge cases
>   - external voltage reference
>   - per-channel interrupt status
>   - INT_CLEAR bit suppresses interrupt assertion
>   - channel disable prevents conversion
>   - one-shot conversion in shutdown mode
>   - deep shutdown blocks conversion
>   - BUSY_STATUS NOT_READY flag lifecycle
>   - operating mode selection
>   - mode 2 pseudo-differential pairs
>   - mode 3 mixed single-ended and differential
>   - mode-change reset of readings and interrupt status
>   - QOM-triggered reconversion
> 
> Signed-off-by: Emmanuel Blot <eblot@meta.com>
> ---
>   tests/qtest/adc128d818-test.c | 857 ++++++++++++++++++++++++++++++++++++++++++
>   tests/qtest/meson.build       |   1 +
>   2 files changed, 858 insertions(+)

> +static void adc128d818_register_nodes(void)
> +{
> +    QOSGraphEdgeOptions opts = {
> +        .extra_device_opts = "id=" ADC128D818_TEST_ID
> +                             ",address=0x1f"
> +    };
> +    add_qi2c_address(&opts, &(QI2CAddress) { ADC128D818_TEST_ADDR });
> +
> +    qos_node_create_driver("adc128d818", i2c_device_create);
> +    qos_node_consumes("adc128d818", "i2c-bus", &opts);
> +
> +    qos_add_test("id-registers", "adc128d818", test_id_registers, NULL);
> +    qos_add_test("defaults", "adc128d818", test_defaults, NULL);
> +    qos_add_test("soft-reset", "adc128d818", test_soft_reset, NULL);
> +    qos_add_test("voltage-conversion", "adc128d818", test_voltage_conversion,
> +                 NULL);
> +    qos_add_test("temperature-conversion", "adc128d818",
> +                 test_temperature_conversion, NULL);

Could you split this patch, adding group of tests (4, 5?) in order to
keep the overall reviewable? More than 800 LoC is way too big to review.

> +    qos_add_test("interrupt-status", "adc128d818", test_interrupt_status, NULL);
> +    qos_add_test("int-clear", "adc128d818", test_int_clear, NULL);
> +    qos_add_test("channel-disable", "adc128d818", test_channel_disable, NULL);
> +    qos_add_test("one-shot", "adc128d818", test_one_shot, NULL);
> +    qos_add_test("mode-selection", "adc128d818", test_mode_selection, NULL);
> +    qos_add_test("mode2-diff", "adc128d818", test_mode2_diff, NULL);
> +    qos_add_test("mode3-mixed", "adc128d818", test_mode3_mixed, NULL);
> +    qos_add_test("mode-change-reset", "adc128d818", test_mode_change_reset,
> +                 NULL);
> +    qos_add_test("diff-qom-trigger", "adc128d818", test_diff_qom_trigger,
> +                 NULL);
> +    qos_add_test("all-channels", "adc128d818", test_all_channels, NULL);
> +    qos_add_test("voltage-edges", "adc128d818", test_voltage_edges, NULL);
> +    qos_add_test("temperature-edges", "adc128d818", test_temperature_edges,
> +                 NULL);
> +    qos_add_test("ext-vref", "adc128d818", test_ext_vref, NULL);
> +    qos_add_test("deep-shutdown", "adc128d818", test_deep_shutdown, NULL);
> +    qos_add_test("busy-status", "adc128d818", test_busy_status, NULL);
> +    qos_add_test("low-limit", "adc128d818", test_low_limit, NULL);
> +    qos_add_test("chan-disable-clears", "adc128d818", test_chan_disable_clears,
> +                 NULL);
> +    qos_add_test("adv-config-clears", "adc128d818", test_adv_config_clears,
> +                 NULL);
> +    qos_add_test("temp-hysteresis", "adc128d818", test_temp_hysteresis, NULL);
> +    qos_add_test("conv-rate", "adc128d818", test_conv_rate, NULL);
> +    qos_add_test("ain-property", "adc128d818", test_ain_property, NULL);
> +}
> +libqos_init(adc128d818_register_nodes);

Re: [PATCH v3 3/5] hw/i2c: parent slaves created with i2c_slave_create_simple

[Philippe Mathieu-Daudé]

On 28/6/26 02:45, Emmanuel Blot wrote:
> Slaves created with i2c_slave_create_simple() were left unparented and
> showed up under /machine/unattached with no stable QOM path. Add each
> slave as a QOM child of its bus, named after its I2C address, so it has
> a deterministic and addressable QOM path.

+Markus

Personally I never liked the idea of having buses being parents
of the devices attached to them, but maybe it is simpler this way.

> Signed-off-by: Emmanuel Blot <eblot@meta.com>
> ---
>   hw/i2c/core.c        | 3 +++
>   include/hw/i2c/i2c.h | 7 +++++--
>   2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
> index 54f6bdca88..0bbce45d48 100644
> --- a/hw/i2c/core.c
> +++ b/hw/i2c/core.c
> @@ -382,6 +382,9 @@ I2CSlave *i2c_slave_create_simple(I2CBus *bus, const char *name, uint8_t addr)
>   {
>       I2CSlave *dev = i2c_slave_new(name, addr);
>   
> +    g_autofree char *childname = g_strdup_printf("0x%02x", addr);
> +    object_property_add_child(OBJECT(bus), childname, OBJECT(dev));
> +
>       i2c_slave_realize_and_unref(dev, bus, &error_abort);
>   
>       return dev;
> diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
> index dd5930f4b5..dc557bbf3f 100644
> --- a/include/hw/i2c/i2c.h
> +++ b/include/hw/i2c/i2c.h
> @@ -166,13 +166,16 @@ bool i2c_scan_bus(I2CBus *bus, uint8_t address, bool broadcast,
>   I2CSlave *i2c_slave_new(const char *name, uint8_t addr);
>   
>   /**
> - * Create and realize an I2C slave device on the heap.
> + * Create and realize an I2C slave device on the heap, add the device as a
> + * child of its parent bus.
> + *
>    * @bus: I2C bus to put it on
>    * @name: I2C slave device type name
>    * @addr: I2C address of the slave when put on a bus
>    *
>    * Create the device state structure, initialize it, put it on the
> - * specified @bus, and drop the reference to it (the device is realized).
> + * specified @bus, parent it, and drop the reference to it (the device is
> + * realized).
>    */
>   I2CSlave *i2c_slave_create_simple(I2CBus *bus, const char *name, uint8_t addr);
>   
> 

Re: [PATCH v3 3/5] hw/i2c: parent slaves created with i2c_slave_create_simple

[Cédric Le Goater]

On 7/1/26 08:29, Philippe Mathieu-Daudé wrote:
> On 28/6/26 02:45, Emmanuel Blot wrote:
>> Slaves created with i2c_slave_create_simple() were left unparented and
>> showed up under /machine/unattached with no stable QOM path. Add each
>> slave as a QOM child of its bus, named after its I2C address, so it has
>> a deterministic and addressable QOM path.
> 
> +Markus
> 
> Personally I never liked the idea of having buses being parents
> of the devices attached to them, 

What was the problem ?

> but maybe it is simpler this way.

It is much simpler for functional tests when looking for board
devices which don't have ids.

patch 5 didn't reach the list, but here is an extract :

+    ADC128D818_QOM_PATH = \
+        "/machine/soc/i2c/bus[8]/aspeed.i2c.bus.8/0x72/i2c.0/0x1f"
+    ADC128D818_MUX_CHANNEL = "/sys/bus/i2c/devices/8-0072/channel-0"

+        adc = self.ADC128D818_QOM_PATH
+        for ch0_mv, ch1_mv in ((108, 2000), (1280, 500)):
+            self.vm.cmd("qom-set", path=adc, property="ain0", value=ch0_mv)
+            self.vm.cmd("qom-set", path=adc, property="ain1", value=ch1_mv)
+            self.wait_adc128d818_value("in0_input", ch0_mv)
+            self.wait_adc128d818_value("in1_input", ch1_mv)

Thanks,

C.

> 
>> Signed-off-by: Emmanuel Blot <eblot@meta.com>
>> ---
>>   hw/i2c/core.c        | 3 +++
>>   include/hw/i2c/i2c.h | 7 +++++--
>>   2 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
>> index 54f6bdca88..0bbce45d48 100644
>> --- a/hw/i2c/core.c
>> +++ b/hw/i2c/core.c
>> @@ -382,6 +382,9 @@ I2CSlave *i2c_slave_create_simple(I2CBus *bus, const char *name, uint8_t addr)
>>   {
>>       I2CSlave *dev = i2c_slave_new(name, addr);
>> +    g_autofree char *childname = g_strdup_printf("0x%02x", addr);
>> +    object_property_add_child(OBJECT(bus), childname, OBJECT(dev));
>> +
>>       i2c_slave_realize_and_unref(dev, bus, &error_abort);
>>       return dev;
>> diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
>> index dd5930f4b5..dc557bbf3f 100644
>> --- a/include/hw/i2c/i2c.h
>> +++ b/include/hw/i2c/i2c.h
>> @@ -166,13 +166,16 @@ bool i2c_scan_bus(I2CBus *bus, uint8_t address, bool broadcast,
>>   I2CSlave *i2c_slave_new(const char *name, uint8_t addr);
>>   /**
>> - * Create and realize an I2C slave device on the heap.
>> + * Create and realize an I2C slave device on the heap, add the device as a
>> + * child of its parent bus.
>> + *
>>    * @bus: I2C bus to put it on
>>    * @name: I2C slave device type name
>>    * @addr: I2C address of the slave when put on a bus
>>    *
>>    * Create the device state structure, initialize it, put it on the
>> - * specified @bus, and drop the reference to it (the device is realized).
>> + * specified @bus, parent it, and drop the reference to it (the device is
>> + * realized).
>>    */
>>   I2CSlave *i2c_slave_create_simple(I2CBus *bus, const char *name, uint8_t addr);
>>
> 

Re: [PATCH v3 3/5] hw/i2c: parent slaves created with i2c_slave_create_simple

[Markus Armbruster]

Cédric Le Goater <clg@kaod.org> writes:

> On 7/1/26 08:29, Philippe Mathieu-Daudé wrote:
>> On 28/6/26 02:45, Emmanuel Blot wrote:
>>> Slaves created with i2c_slave_create_simple() were left unparented and
>>> showed up under /machine/unattached with no stable QOM path. Add each
>>> slave as a QOM child of its bus, named after its I2C address, so it has
>>> a deterministic and addressable QOM path.
>>
>> +Markus
>>
>> Personally I never liked the idea of having buses being parents
>> of the devices attached to them, 
>
> What was the problem ?
>
>> but maybe it is simpler this way.
>
> It is much simpler for functional tests when looking for board
> devices which don't have ids.

A QOM object must be the child of exactly one parent.  This defines the
QOM composition tree.  The link from parent to child is a property of
the parent, and therefore has a name that is unique within its parent.
An object's canonical QOM path is these names on the path from root to
object in the QOM composition tree separated by '/'.

For devices:

* If a device is plugged in with -device / device_add, and it has an ID,
  we make it a child of /machine/peripheral/ with name ID.  If it
  doesn't have an ID, we make it a child of /machine/peripheral-anon/
  with name device[N], where N counts up from zero.  The canonical QOM
  path /machine/peripheral/ID is stable.  The canonical QOM path
  /machine/peripheral-anon/device[N] isn't: it depends on the number of
  devices already there.

* If a device is part of another device, it should be its child.  The
  child's canonical QOM path is the parent's plus '/CHILD-NAME'.  Stable
  as long as the parent's path and the child name are.

* "Should" because we have a lot of code that fails to pick the parent.
  When such a device gets realized, we make it a child of
  /machine/unattached/ orphanage with name device[N], where N counts up
  from zero.  The canonical QOM path /machine/unattached/device[N]
  depends on the number of children already in the orphanage, which
  makes it unstable.

  Letting code get away with not picking a parent was a mistake.  I
  guess it "saved" us some thinking about what's part of what when
  converting existing devices to QOM.  In other words, it enabled sloppy
  hardware modeling.  We've been "saving" thinking ever since.

  I want /machine/unattached/ to be empty.  If an onboard device isn't
  part of another device, put it into /machine/ with a sensible name.

  Whether a device plugged into a bus is part of the device providing
  the bus is a hardware modeling question.

Now to the patch in question.  I understand it makes
i2c_slave_create_simple() pick a parent.  Many machines and devices use
that.  I examined its effect for machine anacapa-bmc with v5 of this
series using "info qom-tree".

The patch moves a bunch of devices from /machine/unattached to

    /machine/soc/i2c/bus[0]/aspeed.i2c.bus.0/0x70 (pca9546)
    /machine/soc/i2c/bus[0]/aspeed.i2c.bus.0/0xff (aspeed.i2c.slave)
    /machine/soc/i2c/bus[1]/aspeed.i2c.bus.1/0x70 (pca9546)
    /machine/soc/i2c/bus[1]/aspeed.i2c.bus.1/0xff (aspeed.i2c.slave)
    /machine/soc/i2c/bus[2]/aspeed.i2c.bus.2/0xff (aspeed.i2c.slave)
    /machine/soc/i2c/bus[3]/aspeed.i2c.bus.3/0xff (aspeed.i2c.slave)
    /machine/soc/i2c/bus[4]/aspeed.i2c.bus.4/0x70 (pca9548)
    /machine/soc/i2c/bus[4]/aspeed.i2c.bus.4/0xff (aspeed.i2c.slave)
    /machine/soc/i2c/bus[5]/aspeed.i2c.bus.5/0xff (aspeed.i2c.slave)
    /machine/soc/i2c/bus[6]/aspeed.i2c.bus.6/0xff (aspeed.i2c.slave)
    /machine/soc/i2c/bus[7]/aspeed.i2c.bus.7/0xff (aspeed.i2c.slave)
    /machine/soc/i2c/bus[8]/aspeed.i2c.bus.8/0x72 (pca9546)
    /machine/soc/i2c/bus[8]/aspeed.i2c.bus.8/0x72/i2c.0/0x22 (pca9552)
    /machine/soc/i2c/bus[8]/aspeed.i2c.bus.8/0x72/i2c.0/0x24 (pca9552)
    /machine/soc/i2c/bus[8]/aspeed.i2c.bus.8/0x72/i2c.1/0x22 (pca9552)
    /machine/soc/i2c/bus[8]/aspeed.i2c.bus.8/0x72/i2c.1/0x24 (pca9552)
    /machine/soc/i2c/bus[8]/aspeed.i2c.bus.8/0xff (aspeed.i2c.slave)
    /machine/soc/i2c/bus[9]/aspeed.i2c.bus.9/0xff (aspeed.i2c.slave)
    /machine/soc/i2c/bus[10]/aspeed.i2c.bus.10/0x71 (pca9548)
    /machine/soc/i2c/bus[10]/aspeed.i2c.bus.10/0x71/i2c.5/0x22 (pca9552)
    /machine/soc/i2c/bus[10]/aspeed.i2c.bus.10/0xff (aspeed.i2c.slave)
    /machine/soc/i2c/bus[11]/aspeed.i2c.bus.11/0x71 (pca9548)
    /machine/soc/i2c/bus[11]/aspeed.i2c.bus.11/0x71/i2c.5/0x22 (pca9552)
    /machine/soc/i2c/bus[11]/aspeed.i2c.bus.11/0xff (aspeed.i2c.slave)
    /machine/soc/i2c/bus[12]/aspeed.i2c.bus.12/0xff (aspeed.i2c.slave)
    /machine/soc/i2c/bus[13]/aspeed.i2c.bus.13/0x70 (pca9548)
    /machine/soc/i2c/bus[13]/aspeed.i2c.bus.13/0xff (aspeed.i2c.slave)
    /machine/soc/i2c/bus[14]/aspeed.i2c.bus.14/0xff (aspeed.i2c.slave)
    /machine/soc/i2c/bus[15]/aspeed.i2c.bus.15/0xff (aspeed.i2c.slave)

Is this a reasonable way to model the hardware?  I can't tell; I don't
know the hardware.  But it sure feels less unreasonable than the
/unattached/ crap!

[...]

Re: [PATCH v3 3/5] hw/i2c: parent slaves created with i2c_slave_create_simple

[Cédric Le Goater]

On 7/2/26 09:05, Markus Armbruster wrote:
> Cédric Le Goater <clg@kaod.org> writes:
> 
>> On 7/1/26 08:29, Philippe Mathieu-Daudé wrote:
>>> On 28/6/26 02:45, Emmanuel Blot wrote:
>>>> Slaves created with i2c_slave_create_simple() were left unparented and
>>>> showed up under /machine/unattached with no stable QOM path. Add each
>>>> slave as a QOM child of its bus, named after its I2C address, so it has
>>>> a deterministic and addressable QOM path.
>>>
>>> +Markus
>>>
>>> Personally I never liked the idea of having buses being parents
>>> of the devices attached to them,
>>
>> What was the problem ?
>>
>>> but maybe it is simpler this way.
>>
>> It is much simpler for functional tests when looking for board
>> devices which don't have ids.
> 
> A QOM object must be the child of exactly one parent.  This defines the
> QOM composition tree.  The link from parent to child is a property of
> the parent, and therefore has a name that is unique within its parent.
> An object's canonical QOM path is these names on the path from root to
> object in the QOM composition tree separated by '/'.
> 
> For devices:
> 
> * If a device is plugged in with -device / device_add, and it has an ID,
>    we make it a child of /machine/peripheral/ with name ID.  If it
>    doesn't have an ID, we make it a child of /machine/peripheral-anon/
>    with name device[N], where N counts up from zero.  The canonical QOM
>    path /machine/peripheral/ID is stable.  The canonical QOM path
>    /machine/peripheral-anon/device[N] isn't: it depends on the number of
>    devices already there.
> 
> * If a device is part of another device, it should be its child.  The
>    child's canonical QOM path is the parent's plus '/CHILD-NAME'.  Stable
>    as long as the parent's path and the child name are.
> 
> * "Should" because we have a lot of code that fails to pick the parent.
>    When such a device gets realized, we make it a child of
>    /machine/unattached/ orphanage with name device[N], where N counts up
>    from zero.  The canonical QOM path /machine/unattached/device[N]
>    depends on the number of children already in the orphanage, which
>    makes it unstable.
> 
>    Letting code get away with not picking a parent was a mistake.  I
>    guess it "saved" us some thinking about what's part of what when
>    converting existing devices to QOM.  In other words, it enabled sloppy
>    hardware modeling.  We've been "saving" thinking ever since.
> 
>    I want /machine/unattached/ to be empty.  If an onboard device isn't
>    part of another device, put it into /machine/ with a sensible name.
> 
>    Whether a device plugged into a bus is part of the device providing
>    the bus is a hardware modeling question.
> 
> Now to the patch in question.  I understand it makes
> i2c_slave_create_simple() pick a parent.  Many machines and devices use
> that.  I examined its effect for machine anacapa-bmc with v5 of this
> series using "info qom-tree".
> 
> The patch moves a bunch of devices from /machine/unattached to
> 
>      /machine/soc/i2c/bus[0]/aspeed.i2c.bus.0/0x70 (pca9546)
>      /machine/soc/i2c/bus[0]/aspeed.i2c.bus.0/0xff (aspeed.i2c.slave)
>      /machine/soc/i2c/bus[1]/aspeed.i2c.bus.1/0x70 (pca9546)
>      /machine/soc/i2c/bus[1]/aspeed.i2c.bus.1/0xff (aspeed.i2c.slave)
>      /machine/soc/i2c/bus[2]/aspeed.i2c.bus.2/0xff (aspeed.i2c.slave)
>      /machine/soc/i2c/bus[3]/aspeed.i2c.bus.3/0xff (aspeed.i2c.slave)
>      /machine/soc/i2c/bus[4]/aspeed.i2c.bus.4/0x70 (pca9548)
>      /machine/soc/i2c/bus[4]/aspeed.i2c.bus.4/0xff (aspeed.i2c.slave)
>      /machine/soc/i2c/bus[5]/aspeed.i2c.bus.5/0xff (aspeed.i2c.slave)
>      /machine/soc/i2c/bus[6]/aspeed.i2c.bus.6/0xff (aspeed.i2c.slave)
>      /machine/soc/i2c/bus[7]/aspeed.i2c.bus.7/0xff (aspeed.i2c.slave)
>      /machine/soc/i2c/bus[8]/aspeed.i2c.bus.8/0x72 (pca9546)
>      /machine/soc/i2c/bus[8]/aspeed.i2c.bus.8/0x72/i2c.0/0x22 (pca9552)
>      /machine/soc/i2c/bus[8]/aspeed.i2c.bus.8/0x72/i2c.0/0x24 (pca9552)
>      /machine/soc/i2c/bus[8]/aspeed.i2c.bus.8/0x72/i2c.1/0x22 (pca9552)
>      /machine/soc/i2c/bus[8]/aspeed.i2c.bus.8/0x72/i2c.1/0x24 (pca9552)
>      /machine/soc/i2c/bus[8]/aspeed.i2c.bus.8/0xff (aspeed.i2c.slave)
>      /machine/soc/i2c/bus[9]/aspeed.i2c.bus.9/0xff (aspeed.i2c.slave)
>      /machine/soc/i2c/bus[10]/aspeed.i2c.bus.10/0x71 (pca9548)
>      /machine/soc/i2c/bus[10]/aspeed.i2c.bus.10/0x71/i2c.5/0x22 (pca9552)
>      /machine/soc/i2c/bus[10]/aspeed.i2c.bus.10/0xff (aspeed.i2c.slave)
>      /machine/soc/i2c/bus[11]/aspeed.i2c.bus.11/0x71 (pca9548)
>      /machine/soc/i2c/bus[11]/aspeed.i2c.bus.11/0x71/i2c.5/0x22 (pca9552)
>      /machine/soc/i2c/bus[11]/aspeed.i2c.bus.11/0xff (aspeed.i2c.slave)
>      /machine/soc/i2c/bus[12]/aspeed.i2c.bus.12/0xff (aspeed.i2c.slave)
>      /machine/soc/i2c/bus[13]/aspeed.i2c.bus.13/0x70 (pca9548)
>      /machine/soc/i2c/bus[13]/aspeed.i2c.bus.13/0xff (aspeed.i2c.slave)
>      /machine/soc/i2c/bus[14]/aspeed.i2c.bus.14/0xff (aspeed.i2c.slave)
>      /machine/soc/i2c/bus[15]/aspeed.i2c.bus.15/0xff (aspeed.i2c.slave)


it "matches" what can be found under /sys/bus/i2c/devices

Thanks for taking the time to run some test.

> Is this a reasonable way to model the hardware?  I can't tell; I don't
> know the hardware.  But it sure feels less unreasonable than the
> /unattached/ crap!

Hey :)

I have always tried to find a QOM parent to avoid orphans objects.
I2C ans SPI devices are the leftovers on Aspeed machines.


C.

Re: [PATCH v3 3/5] hw/i2c: parent slaves created with i2c_slave_create_simple

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@oss.qualcomm.com> writes:

> On 28/6/26 02:45, Emmanuel Blot wrote:
>> Slaves created with i2c_slave_create_simple() were left unparented and
>> showed up under /machine/unattached with no stable QOM path. Add each
>> slave as a QOM child of its bus, named after its I2C address, so it has
>> a deterministic and addressable QOM path.
>
> +Markus
>
> Personally I never liked the idea of having buses being parents
> of the devices attached to them, but maybe it is simpler this way.
>
>> Signed-off-by: Emmanuel Blot <eblot@meta.com>

Patches haven't reached the list as far as I can tell.  I need them to
contribute to the discussion.  Emmanuel, please resend them.