Hi JC,
On Fri, May 3, 2013 at 4:33 AM, Jean-Christophe DUBOIS
<j...@tribudubois.net> wrote:
> 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.
>
Its actually quite hard to find them but hw/dma/pl330.c was fairly
recently reviewed and accepted sysbus device. There is one QOM
violation in there just with a quick scan for the more common errors,
that is best fixed in new device models. fix below:
--- a/hw/dma/pl330.c
+++ b/hw/dma/pl330.c
@@ -227,7 +227,7 @@ static const VMStateDescription vmstate_pl330_queue = {
};
struct PL330State {
- SysBusDevice busdev;
+ SysBusDevice parent_obj;
Code of interest:
Type cast macro:
#define TYPE_PL330 "pl330"
#define PL330(obj) OBJECT_CHECK(PL330State, (obj), TYPE_PL330)
Realize fn:
static void pl330_realize(DeviceState *dev, Error **errp)
{
int i;
PL330State *s = PL330(dev);
(Theres an example usage of QOM cast macro).
and its wiring up:
static void pl330_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
dc->realize = pl330_realize;
dc->reset = pl330_reset;
dc->props = pl330_properties;
dc->vmsd = &vmstate_pl330;
}
>
>>> + 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?
>
Covered above
>
>>
>> +
>> +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.
>
More example:
static const MemoryRegionOps pl330_ops = {
.read = pl330_iomem_read,
.write = pl330_iomem_write,
.endianness = DEVICE_NATIVE_ENDIAN,
.impl = {
.min_access_size = 4,
.max_access_size = 4,
}
};
>
>>> +};
>>> +
>>> +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?
>
Not sure if there's any formal documentation around. But its covered
by the pl330 examples. There are some relevant code comments around
the place (include/hw/qdev-core.h).
* # Realization #
* Devices are constructed in two stages,
* 1) object instantiation via object_initialize() and
* 2) device realization via #DeviceState:realized property.
* The former may not fail (it might assert or exit), the latter may return
* error information to the caller and must be re-entrant.
* Trivial field initializations should go into #TypeInfo.instance_init.
* Operations depending on @props static properties should go into @realize.
* After successful realization, setting static properties will fail.
Regards,
Peter
