Peter,
Thanks for you review.
I have a few questions below.
JC
On 05/02/2013 02:16 PM, Peter Crosthwaite wrote:
Hi Jean-Christophe,
Thanks for your contribution. Please run the patch through
scripts/checkpatch.pl to check for formatting errors.
On Thu, May 2, 2013 at 5:53 AM, Jean-Christophe DUBOIS
<j...@tribudubois.net> wrote:
ERROR: return is not a function, parentheses are not required
#148: FILE: hw/i2c/imx_i2c.c:114:
+ return (s->i2cr & I2CR_IEN);
>From checkpatch, here and below.
Will do.
+}
+
+static inline bool imx_i2c_interrupt_is_enabled(imx_i2c_state *s)
+{
+ return (s->i2cr & I2CR_IIEN);
+}
+
+static inline bool imx_i2c_is_master(imx_i2c_state *s)
+{
+ return (s->i2cr & I2CR_MSTA);
+}
+
+static inline bool imx_i2c_direction_is_tx(imx_i2c_state *s)
+{
+ return (s->i2cr & I2CR_MTX);
+}
+
+static void imx_i2c_reset(DeviceState *d)
+{
+ imx_i2c_state *s = FROM_SYSBUS(imx_i2c_state, SYS_BUS_DEVICE(d));
+
Please don't use FROM_SYSBUS in new code. Use QOM cast macros.
http://wiki.qemu.org/QOMConventions
Has useful guidelines for the rules around this for new devices.
It is not very clear what you do expect. Could you point me to a driver
that is up to date.
+ if (s->address != ADDR_RESET) {
+ i2c_end_transfer(s->bus);
+ }
+
+ s->address = ADDR_RESET;
+ s->iadr = IADR_RESET;
+ s->ifdr = IFDR_RESET;
+ s->i2cr = I2CR_RESET;
+ s->i2sr = I2SR_RESET;
+ s->i2dr_read = I2DR_RESET;
+ s->i2dr_write = I2DR_RESET;
+}
+
+static inline void imx_i2c_raise_interrupt(imx_i2c_state *s)
+{
+ /*
+ * raise an interrupt if the device is enabled and it is configured
+ * to generate some interrupts.
+ */
+ if (imx_i2c_is_enabled(s) && imx_i2c_interrupt_is_enabled(s)) {
+ s->i2sr |= I2SR_IIF;
+ qemu_irq_raise(s->irq);
+ }
+}
+
+static uint64_t imx_i2c_read(void *opaque, hwaddr offset,
+ unsigned size)
+{
+ imx_i2c_state *s = (imx_i2c_state *)opaque;
QOM cast macro.
Could you point me to an up to date driver using QOM cast macro?
+
+static const MemoryRegionOps imx_i2c_ops = {
+ .read = imx_i2c_read,
+ .write = imx_i2c_write,
+ .endianness = DEVICE_NATIVE_ENDIAN,
I think you may need a .valid definition here as it looks like you
have restrictions on access size and alignment? (looks like 4bytes
accesses only).
I'll look into this.
+};
+
+static const VMStateDescription imx_i2c_vmstate = {
+ .name = TYPE_IMX_I2C,
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT16(address, imx_i2c_state),
+ VMSTATE_UINT16(iadr, imx_i2c_state),
+ VMSTATE_UINT16(ifdr, imx_i2c_state),
+ VMSTATE_UINT16(i2cr, imx_i2c_state),
+ VMSTATE_UINT16(i2sr, imx_i2c_state),
+ VMSTATE_UINT16(i2dr_read, imx_i2c_state),
+ VMSTATE_UINT16(i2dr_write, imx_i2c_state),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static int imx_i2c_init(SysBusDevice *dev)
+{
Use of the SysBusDeviceClass::init function is deprecated. Please use
DeviceClass::realise or Object::init. With no reliance on properties I
would suggest this one can be done as just an Object::init fn.
Could you point me to the documentation or an up to date example?
