Re: [PATCH 2/4] hw/arm/stellaris: Convert I2C controller to Resettable interface

[Philippe Mathieu-Daudé]

On 1/2/24 17:24, Peter Maydell wrote:
> On Tue, 30 Jan 2024 at 19:03, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> >   hw/arm/stellaris.c | 9 ++++++---
> >   1 file changed, 6 insertions(+), 3 deletions(-)


> > @@ -658,8 +661,6 @@ static void stellaris_i2c_init(Object *obj)
> >       memory_region_init_io(&s->iomem, obj, &stellaris_i2c_ops, s,
> >                             "i2c", 0x1000);
> >       sysbus_init_mmio(sbd, &s->iomem);
> > -    /* ??? For now we only implement the master interface.  */
>
> I'm not 100%, but I think this comment is a general one,
> not reset specific, so it should stay in the init function.
>
> > -    stellaris_i2c_reset(s);
> >   }
>
> I think that the i2c_end_transfer() should be in the
> "enter" phase, and the clearing of the state fields in
> "hold", and then the stellaris_i2c_update() call in "exit".

Indeed.

> Though usually we don't bother to do an update in reset
> if it's just "device has reset and now its outbound IRQ
> line is not set", so we could alternatively just delete that.

I'll let that as an possible cleanup on top.

Thanks for the review,

Phil.