From mboxrd@z Thu Jan  1 00:00:00 1970
From: Hans de Goede <hdegoede@redhat.com>
Date: Thu, 20 Nov 2014 15:13:33 +0100
Subject: [U-Boot] [PATCH v5 3/3] sunxi: video: Add simplefb support
In-Reply-To: <1416475347.29243.62.camel@hellion.org.uk>
References: <1416403920-24561-1-git-send-email-hdegoede@redhat.com>	
	<1416403920-24561-4-git-send-email-hdegoede@redhat.com>
	<1416475347.29243.62.camel@hellion.org.uk>
Message-ID: <546DF70D.8020900@redhat.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi,

On 11/20/2014 10:22 AM, Ian Campbell wrote:
> On Wed, 2014-11-19 at 14:32 +0100, Hans de Goede wrote:
>> From: Luc Verhaegen <libv@skynet.be>
>>
>> Add simplefb support, note this depends on the kernel having support for
>> the clocks property which has recently been added to the simplefb devicetree
>> binding.
>>
>> Signed-off-by: Luc Verhaegen <libv@skynet.be>
>> [hdegoede at redhat.com: Use pre-populated simplefb node under /chosen as
>>  disussed on the devicetree list]
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Acked-by: Ian Campbell <ijc@hellion.org.uk>.

Thanks, any chance you could also review v2 of the CONFIG_USB_KEYBOARD patch ?

> One non-blocking queries:
> 
>> +	/* Find a framebuffer node, with pipeline == "de_be0-lcd0-hdmi" */
>> +	offset = fdt_node_offset_by_compatible(blob, -1,
>> +					       "allwinner,simple-framebuffer");
>> +	while (offset >= 0) {
>> +		ret = fdt_find_string(blob, offset, "allwinner,pipeline",
>> +				      "de_be0-lcd0-hdmi");
>> +		if (ret == 0)
>> +			break;
>> +		offset = fdt_node_offset_by_compatible(blob, offset,
>> +					       "allwinner,simple-framebuffer");
>> +	}
> 
> Is this variant non-conformant with coding style?:
> 
> 	int offset = -1;
> 	while ( (offset = fdt_node_offset_by_compatible(blob, offset,
>                                                         "allwinner,simple-framebuffer") ) {
> 		LOOP BODY
> 	}
> 
> I expect it is because of the assignment within the while condition,
> which is a shame, since this is one case where it is IMHO leads to
> clearer code.

AFAIK this indeed would go against the coding style, and TBH I'm also not
sure if I agree it would be cleaner (mainly because indentation would become
(even more) messy due to the 80 columns limit).

Regards,

Hans