From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:57794)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <agraf@suse.de>) id 1Y1jza-00057o-RG
	for qemu-devel@nongnu.org; Thu, 18 Dec 2014 17:56:30 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <agraf@suse.de>) id 1Y1jzQ-0003wq-Sj
	for qemu-devel@nongnu.org; Thu, 18 Dec 2014 17:56:22 -0500
Message-ID: <54935B8A.6000108@suse.de>
Date: Thu, 18 Dec 2014 23:56:10 +0100
From: Alexander Graf <agraf@suse.de>
MIME-Version: 1.0
References: <BL2PR03MB449F4F9FA02171164275824836D0@BL2PR03MB449.namprd03.prod.outlook.com>
	<1418938339720.40668@freescale.com>
In-Reply-To: <1418938339720.40668@freescale.com>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH v1] [Review Request]  RTC Support in e500
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Amit Tomar <Amit.Tomar@freescale.com>
Cc: "qemu-ppc@nongnu.org" <qemu-ppc@nongnu.org>, "qemu-devel@nongnu.org" <qemu-devel@nongnu.org>



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