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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.