[U-Boot] [PATCH 4/5] sunxi: display: Add support for interlaced modes

[Hans de Goede]

Hi,

On 05-08-15 10:05, Ian Campbell wrote:
> On Mon, 2015-08-03 at 23:54 +0200, Hans de Goede wrote:
>> @@ -775,13 +781,18 @@ static void sunxi_lcdc_tcon1_mode_set(const
>> struct ctfb_res_modes *mode,
>>
>>   	clk_delay = sunxi_lcdc_get_clk_delay(mode, 1);
>>   	writel(SUNXI_LCDC_TCON1_CTRL_ENABLE |
>> +	       SUNXI_LCDC_TCON1_CTRL_INTERLACE(
>> +			mode->vmode == FB_VMODE_INTERLACED) |
>
> I think this would be clearer if SUNXI_LCDC_TCON1_CTRL_INTERLACE was
> actually the enable bit (perhaps with _ENABLE on the name), rather than
> a macro which takes a boolean and returns 0 or the single bit, so you
> could just write
> 	mode->vmode == FB_VMODE_INTERLACED ?
> 		SUNXI_LCDC_TCON1_CTRL_INTERLACE : 0
> (in whichever wrapping style you prefer).
>
> I think the macro is the bit style is more common in this code for
> boolean options too, we mainly use the macro-with-argument style for
> fields with more than 1 bit to them.

Agreed, fixed.

>
> But ultimately the code is correct as you have it so either way as you
> prefer:
>
> Acked-by: Ian Campbell <ijc at hellion.org.uk>
>
> Although if you want to keep it the way it is then perhaps the macro sh
> ould have !!n instead of just n, to prevent surprises if someone uses a
> bitop rather than a full boolean op as an argument?
>
> @@ -1240,6 +1245,9 @@ void *video_hw_init(void)
>>   		return NULL;
>>   	}
>>
>> +> 	> printf("Setting up a %dx%d %s console\n", mode->xres,
>> +> 	>        mode->yres, sunxi_get_mon_desc(sunxi_display.monitor));
>
> Is it worth including the string "interlaced" here when appropriate?
> (Ack stands either way)

Also fixed.

Thanks,

Hans