Re: [PATCH 1/2] hw/i2c/dw: Add DesignWare I2C controller emulator

[Jonathan Cameron via <qemu-arm@nongnu.org>]

On Tue,  6 Jan 2026 21:12:52 +0800
AlanoSong@163.com wrote:

> Add DesignWare I2C controller according to DesignWare
> I2C databook v2.01a.
> Confirmed the model with i2c-tools under v6.18
> linux driver.
> 
> The slave mode is not implemented, cause this feature
> is usually not used.
> 
> The 10 bit slave address is not implemented, cause
> this feature is usually not used, and not supported
> by qemu I2C core bus currently.
> 
> Signed-off-by: Alano Song <AlanoSong@163.com>
Hi Alano,

A very superficial review. Sadly I don't have time to dig
much deeper at the moment.

Good to see this support.

Jonathan

> diff --git a/hw/i2c/dw_i2c.c b/hw/i2c/dw_i2c.c
> new file mode 100644
> index 0000000000..a5a31ec78c
> --- /dev/null
> +++ b/hw/i2c/dw_i2c.c
> @@ -0,0 +1,517 @@
> +/*
> + *  DesignWare I2C Bus Serial Interface Emulation
> + *
> + *  Copyright (C) 2026 Alano Song <AlanoSong@163.com>
> + *
> + *  This program is free software; you can redistribute it and/or modify it
> + *  under the terms of the GNU General Public License as published by the
> + *  Free Software Foundation; either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful, but WITHOUT
> + *  ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + *  FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + *  for more details.
> + *
> + *  You should have received a copy of the GNU General Public License along
> + *  with this program; if not, see <http://www.gnu.org/licenses/>.
> + *
> + */
As below. Can probably replace the boilerplate with an SPDX license
line and just have the copyright + intro text here.


> diff --git a/include/hw/i2c/dw_i2c.h b/include/hw/i2c/dw_i2c.h
> new file mode 100644
> index 0000000000..512c749f21
> --- /dev/null
> +++ b/include/hw/i2c/dw_i2c.h
> @@ -0,0 +1,151 @@
> +/*
> + *  DesignWare I2C Bus Serial Interface Emulation
> + *
> + *  Copyright (C) 2026 Alano Song <AlanoSong@163.com>
> + *
> + *  This program is free software; you can redistribute it and/or modify it
> + *  under the terms of the GNU General Public License as published by the
> + *  Free Software Foundation; either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful, but WITHOUT
> + *  ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + *  FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + *  for more details.
> + *
> + *  You should have received a copy of the GNU General Public License along
> + *  with this program; if not, see <http://www.gnu.org/licenses/>.

For new code in QEMU I think we are fine with just SPDX.  See many examples
in tree.  It saves on repeating this boilerplate.

> + *
> + */
> +
> +#ifndef DW_I2C_H
> +#define DW_I2C_H
> +
> +#include "hw/core/sysbus.h"
> +#include "qom/object.h"
> +#include "qemu/fifo8.h"
> +
> +#define TYPE_DW_I2C "dw.i2c"
> +OBJECT_DECLARE_SIMPLE_TYPE(DWI2CState, DW_I2C)
> +
> +#define DW_I2C_TX_FIFO_DEPTH        16
> +#define DW_I2C_RX_FIFO_DEPTH        16

> +
> +#define DW_IC_CON_MASTER            BIT(0)
> +#define DW_IC_CON_SPEED_STANDARD    (0x1 << 1)
> +#define DW_IC_CON_SPEED_FAST        (0x2 << 1)
> +#define DW_IC_CON_SPEED_HIGH        (0x3 << 1)

Smells of value being written to a field rather than
a series of values with meaning on their own. Qemu
has a rich set of register field macros. I'd look
at using those to make your life easier.

> +#define DW_IC_CON_RESTART_EN        BIT(5)