Re: [Qemu-devel] [PATCH v1] [Review Request] RTC Support in e500

[Alexander Graf <agraf@suse.de>]

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