On 18.12.14 22:32, Amit Tomar wrote:
>
> Thank you for reviewing the patches and providing the comments.
>
> I'm able to follow most of the comments except below two.
>
>> Is this true for a real MPC8544DS as well? If not, we'll need to
>> conditionalize it to only spawn >the i2c controller (and rtc clock) on the
>> virt machine.
>
> I could not able to parse the above comments,what I need to check here?
>
> I checked the MPC8544 dts file ,I2C is present at @3100(will double check it)
Ah, this is already great to hear.
I think we're ok to just assume the specific RTC is attached to the i2c
bus then. So your answer is basically "Yes, this is true for a real
MPC8544DS". Awesome.
>
>> + case MPC_I2C_CR:
>> + if (mpc_i2c_is_enabled(s) && ((value & CCR_MEN) == 0)) {
>> + mpc_i2c_soft_reset(s);
>
> > I think it's fine to do a break here and indent the below 4 bytes less.
>
> Do you want to have separate function for else, if so what should I name it ?
Right now the code flow is
if (...) {
foo;
} else {
bar;
}
where foo is really a special case that doesn't belong into the normal
code flow. Thus I think doing
if (...) {
foo;
break;
}
bar;
is more readable.
Also, please make sure to not top post on QEMU mailing lists. We usually
inline responses ;).
Alex
