Re: [PATCH 1/3] qtest: Add functions for accessing devices on Aspeed I2C controller

[Stefan Berger <stefanb@linux.ibm.com>]

On 3/28/23 02:39, Cédric Le Goater wrote:
> On 3/27/23 22:24, Stefan Berger wrote:
>> Add read and write functions for accessing registers of I2C devices
>> connected to the Aspeed I2C controller.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> [... ]
> 
>> +#ifndef QTEST_ASPEED_H
>> +#define QTEST_ASPEED_H
>> +
>> +#include <stdint.h>
>> +
>> +#define AST2600_ASPEED_I2C_BASE_ADDR 0x1e78a000
>> +
>> +static inline uint32_t ast2600_aspeed_i2c_calc_dev_addr(uint8_t bus_num)
> 
> I think you could simplify the name ast2600_aspeed_i2c_calc_dev_addr()
> to aspeed_i2c_calc_bus_addr() and add a comment saying it implements only
> the AST2600 I2C controller. I don't think we will need the others.


What are 'the others'?

> 
>> +{
>> +    return AST2600_ASPEED_I2C_BASE_ADDR + 0x80 + bus_num * 0x80;
>> +}
> 
> The formula is :
> 
>     return soc_base + (soc_i2c_offset + bus_num) * soc_i2c_reg_size;

That's what I thought:

return soc_base + soc_i2c_offset + (bus_num * soc_i2c_reg_size);

I will keep it as it is, though.

     Stefan

> 
> May be turn it that way in case you respin. This is minor.
> 
> Thanks,
> 
> C.
>