Re: [Qemu-devel] [PATCH 3/5] aspeed: Add EEPROM I2C devices

[Cédric Le Goater]

On 09/19/2017 02:12 PM, Philippe Mathieu-Daudé wrote:
> Hi Cédric,
> 
> On 09/19/2017 08:26 AM, Cédric Le Goater wrote:
>> The Aspeed boards have at least one EEPROM to hold the Vital Product
>> Data (VPD).
>>
>> Signed-off-by: Cédric Le Goater <address@hidden>
>> ---
>>   hw/arm/aspeed.c | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>> index 362b683e9021..3e74970d7ad2 100644
>> --- a/hw/arm/aspeed.c
>> +++ b/hw/arm/aspeed.c
>> @@ -251,6 +251,17 @@ static void aspeed_board_init(MachineState *machine,
>>       arm_load_kernel(ARM_CPU(first_cpu), &aspeed_board_binfo);
>>   }
>>   +static void aspeed_i2c_add_eeprom(I2CBus *bus, uint8_t address, size_t 
>> size)
>> +{
>> +    uint8_t *eeprom_buf = g_malloc0(size);
>> +    DeviceState *dev;
>> +
>> +    dev = qdev_create((BusState *) bus, "smbus-eeprom");
>> +    qdev_prop_set_uint8(dev, "address", address);
>> +    qdev_prop_set_ptr(dev, "data", eeprom_buf);
>> +    qdev_init_nofail(dev);
> 
> Can you merge this with smbus_eeprom_init() ?
> I think the cleaner way would be add "include/hw/i2c/eeprom.h" defining 
> SMB_EEPROM_TYPE "smbus-eeprom" and declare smbus_eeprom_init() there or as 
> inlined.

yes. That would remove some duplicated code.

I will wait for some feedback before doing a resend or a followup.

Thanks,

C.


> 
>> +}
>> +
>>   static void palmetto_bmc_i2c_init(AspeedBoardState *bmc)
>>   {
>>       AspeedSoCState *soc = &bmc->soc;
>> @@ -260,6 +271,9 @@ static void palmetto_bmc_i2c_init(AspeedBoardState *bmc)
>>        * enough to provide basic RTC features. Alarms will be missing */
>>       i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 0), "ds1338", 
>> 0x68);
>>   +    aspeed_i2c_add_eeprom(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 0), 0x50,
>> +                          256 * 1024);
>> +
>>       /* add a TMP423 temperature sensor */
>>       dev = i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 2),
>>                              "tmp423", 0x4c);
>> @@ -298,6 +312,9 @@ static void ast2500_evb_i2c_init(AspeedBoardState *bmc)
>>   {
>>       AspeedSoCState *soc = &bmc->soc;
>>   +    aspeed_i2c_add_eeprom(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 3), 0x50,
>> +                          8 * 1024);
>> +
>>       /* The AST2500 EVB expects a LM75 but a TMP105 is compatible */
>>       i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 7), "tmp105", 
>> 0x4d);
>>   @@ -377,6 +394,9 @@ static void witherspoon_bmc_i2c_init(AspeedBoardState 
>> *bmc)
>>       /* The witherspoon board expects Epson RX8900 I2C RTC but a ds1338 is
>>        * good enough */
>>       i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), "ds1338", 
>> 0x32);
>> +
>> +    aspeed_i2c_add_eeprom(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), 0x51,
>> +                          8 * 1024);
>>   }
>>     static void witherspoon_bmc_init(MachineState *machine)
>>