Hi Wolfram,
On 03/13/2018 09:16 PM, Wolfram Sang wrote:
> Hi Philippe,
>
>>> static Property at24c_eeprom_props[] = {
>>> - DEFINE_PROP_UINT32("rom-size", EEPROMState, rsize, 0),
>>> + DEFINE_PROP_UINT32("rom-size", EEPROMState, rsize, 128),
>>
>> This patch should goes before your 2/3 in your series.
>
> I don't mind much, but why? My reasoning was "let's first fix the cause
> and then the symptom"?
The '0' case is worst than incorrect, it segfaults, so you are right :)
>
>> Can you add a #define for this value? Such AT24C_ROMSIZE_MIN.
>
> Can do, of course. But won't that give room for regressions because
> people are already using it with lower values?
Your patch already introduce the regression :)
I prefer self-explanatory #defines than magic value, but I see your
point, so if we can not decide a value, can you add a comment to explain
the magic value? I think the clearer is to add a #define with a comment.
>
> Ideally, we would have a "model" variable. The model type would define
> the size of the memory. The "rom-size" variable could then be kept as is
> (except for the 0 bugfix) or deprecated?
IMO there are too many AT24C eeproms to model, so the "rom-size"
variable is the easiest way. Your patch #2 is simple enough to avoid the
#DIV/0!
>
> Thanks for the review,
>
> Wolfram
>
Attachment:
signature.asc
Description: OpenPGP digital signature
