From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([140.186.70.92]:58754)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <afaerber@suse.de>) id 1RfX5Q-0004xY-09
	for qemu-devel@nongnu.org; Tue, 27 Dec 2011 08:29:00 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <afaerber@suse.de>) id 1RfX5O-0000QL-W3
	for qemu-devel@nongnu.org; Tue, 27 Dec 2011 08:29:00 -0500
Received: from cantor2.suse.de ([195.135.220.15]:43803 helo=mx2.suse.de)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <afaerber@suse.de>) id 1RfX5O-0000QB-N4
	for qemu-devel@nongnu.org; Tue, 27 Dec 2011 08:28:58 -0500
Message-ID: <4EF9C7CB.4020007@suse.de>
Date: Tue, 27 Dec 2011 14:27:39 +0100
From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= <afaerber@suse.de>
MIME-Version: 1.0
References: <1324893824-13558-1-git-send-email-i.mitsyanko@samsung.com>
	<1324893824-13558-3-git-send-email-i.mitsyanko@samsung.com>
	<CAFEAcA-6JD8rQd_RYFmxaPO2jdZS2bzY--eCmgp2Nc=XcCAdSg@mail.gmail.com>
In-Reply-To: <CAFEAcA-6JD8rQd_RYFmxaPO2jdZS2bzY--eCmgp2Nc=XcCAdSg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Subject: Re: [Qemu-devel] [PATCH 2/3] hw/sd.c: add SD card save/load support
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: Peter Maydell <peter.maydell@linaro.org>
Cc: Mitsyanko Igor <i.mitsyanko@samsung.com>, e.voevodin@samsung.com, Juan Quintela <quintela@redhat.com>, qemu-devel@nongnu.org, kyungmin.park@samsung.com, d.solodkiy@samsung.com, m.kozlov@samsung.com, jehyung.lee@samsung.com

Am 26.12.2011 15:58, schrieb Peter Maydell:
>> diff --git a/hw/sd.c b/hw/sd.c
>> index 07eb263..2b489d3 100644
>> --- a/hw/sd.c
>> +++ b/hw/sd.c

>> @@ -81,22 +85,22 @@ struct SDState {
>>     uint8_t sd_status[64];
>>     uint32_t vhs;
>>     int wp_switch;
>> -    int *wp_groups;
>> +    uint8_t *wp_groups;
>>     uint64_t size;
>> -    int blk_len;
>> +    uint32_t blk_len;
>>     uint32_t erase_start;
>>     uint32_t erase_end;
>>     uint8_t pwd[16];
>> -    int pwd_len;
>> -    int function_group[6];
>> +    uint32_t pwd_len;
>> +    uint8_t function_group[6];
>>
>> -    int spi;
>> -    int current_cmd;
>> +    uint8_t spi;
>> +    uint8_t current_cmd;
>>     /* True if we will handle the next command as an ACMD. Note that t=
his does
>>      * *not* track the APP_CMD status bit!
>>      */
>> -    int expecting_acmd;
>> -    int blk_written;
>> +    bool expecting_acmd;
>=20
> Why have you changed expecting_acmd and enable to bool, but spi
> to a uint8_t and wp_groups[] to a uint8_t[] ? This isn't very
> consistent. (I'm vaguely against bool because you then end up
> making other changes to change '1' to 'true' and so on.)

>> @@ -1706,5 +1710,44 @@ int sd_data_ready(SDState *sd)
>>
>>  void sd_enable(SDState *sd, int enable)
>>  {
>> -    sd->enable =3D enable;
>> +    sd->enable =3D enable ? true : false;
>=20
> This kind of thing is why I don't like bool :-)

x =3D 1 should work with bool when not doing x =3D=3D true anywhere, here
!!enable would be an alternative. Maybe change int enable to bool, too?

Andreas

--=20
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3=BC=
rnberg