On 23/05/16 12:50, Jyri Sarha wrote:
> On 05/20/16 16:21, Tomi Valkeinen wrote:
>>>> +
>>>> +static int dispc_errata_i734_wa_init(void)
>>>> +{
>>>> +	size_t buff_size = i734.ovli.width * i734.ovli.height *
>> "buf_size" would match the other names =).
>>
>> Usually I try to do only simple initializations when declaring the
>> locals. This is a bit on the complex side, and also there's a check
>> below that can make all this init calculation not needed.
>>
> 
> Compiler optimizations should delay the initialization to happen only if
> it is needed.

That is true.

My worry is more on the functional side: if you do, say, calculations
there and, e.g. do a division, it's easy to get division by zero if all
the variables are not actually valid. Or if you do get_foo(channel), and
get_foo() only works for certain channels, again you could hit a crash.

Those kind of things happen easily when you have code like:

int foo = get_foo(channel);

if (channel != XYZ)
	return;

>>>> +
>>>> +static void dispc_errata_i734_wa(void)
>>>> +{
>>>> +	u32 vsync_irq = dispc_mgr_get_vsync_irq(OMAP_DSS_CHANNEL_LCD);
>>>> +	u32 framedone_irq = dispc_mgr_get_framedone_irq(OMAP_DSS_CHANNEL_LCD);
>>>> +	u32 gatestate = REG_GET(DISPC_CONTROL, 8, 4);
>> Here too, too complex initializations. Especially doing register
>> reads/writes should be something done in the "real" code. The bits you
>> read might not even be valid on some platforms which don't have the i734.
>>
> 
> You are right about the register read. Compiler should not optimize the
> register access. But - but with your permission - I'll keep the IRQ mask
> initializations as they are quite constant in nature anyway.

Yes, those should be fine. Presuming all DSS versions have CHANNEL_LCD,
which happens to be the case =).

 Tomi