Re: [PATCH v3 06/13] hw/i2c/ppc4xx_i2c: Replace i2c_send_recv() by i2c_recv() & i2c_send()

[BALATON Zoltan <balaton@eik.bme.hu>]

[-- Attachment #1: Type: text/plain, Size: 3179 bytes --]

On Wed, 16 Jun 2021, Philippe Mathieu-Daudé wrote:
> On 6/16/21 10:01 PM, BALATON Zoltan wrote:
>> On Wed, 16 Jun 2021, Philippe Mathieu-Daudé wrote:
>>> On 6/16/21 9:16 PM, Corey Minyard wrote:
>>>> On Wed, Jun 16, 2021 at 06:14:11PM +0200, Philippe Mathieu-Daudé wrote:
>>>>> Instead of using the confuse i2c_send_recv(), rewrite to directly
>>>>> call i2c_recv() & i2c_send(), resulting in code easire to review.
>>>>>
>>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>> ---
>>>>>  hw/i2c/ppc4xx_i2c.c | 13 ++++++++-----
>>>>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
>>>>> index f4c5bc12d36..b3d3da56e38 100644
>>>>> --- a/hw/i2c/ppc4xx_i2c.c
>>>>> +++ b/hw/i2c/ppc4xx_i2c.c
>>>>> @@ -240,11 +240,14 @@ static void ppc4xx_i2c_writeb(void *opaque,
>>>>> hwaddr addr, uint64_t value,
>>>>>                          i2c->sts &= ~IIC_STS_ERR;
>>>>>                      }
>>>>>                  }
>>>>> -                if (!(i2c->sts & IIC_STS_ERR) &&
>>>>> -                    i2c_send_recv(i2c->bus, &i2c->mdata[i], !recv)) {
>>>>> -                    i2c->sts |= IIC_STS_ERR;
>>>>> -                    i2c->extsts |= IIC_EXTSTS_XFRA;
>>>>> -                    break;
>>>>> +                if (!(i2c->sts & IIC_STS_ERR)) {
>>>>> +                    if (recv) {
>>>>> +                        i2c->mdata[i] = i2c_recv(i2c->bus);
>>>>> +                    } else if (i2c_send(i2c->bus, i2c->mdata[i])) {
>>>>
>>>> In the previous patch you checked < 0, it would be nice to be
>>>> consistent.
>>>
>>> I did that first but thought Zoltan wouldn't be happy, then went back :)
>>>
>>> I'll fix for the next iteration, thanks.
>>
>> I generally had no problem with i2c_send_recv only that its argument
>> that decides which operation to do was inverted compared to other
>> similar i2c functions so my original patch just corrected that for
>> consistency and I was happy with that.
>
> But we have to make the maintainer happy too to get patch merged ;)
>
>> Having a send_recv in one func
>> allowed to avoid if-else in some places like these but if you think it's
>> better without this function at all I can work with that too. I'll have
>> to check if these changes could break anything. At first sight I'm not
>> sure errors are handled as before if recv fails but it was years ago I
>> did the sm501 and ati parts and I forgot how they work so I need to
>> check again. I'll wait for the final version of the series then and test
>> that.
>
> Thanks, that would be great!

I've tried AmigaOS and MorphOS on sam460ex and mac99 with sm501 and 
ati-vga and these still work so I think the patches are OK but I did not 
test other changes to other machines so I did not give a tested-by for the 
series just some reviewed-by to patches I've verified. (Found a regression 
with AROS but that's not related to these changes.)

Regards,
BALATON Zoltan