Re: [PATCH - v1 4/6] V4L - vpfe_capture bug fix and enhancements

From: Hans Verkuil <hverkuil@xs4all.nl>
To: m-karicheri2@ti.com
Cc: linux-media@vger.kernel.org, khilman@deeprootsystems.com,
	nsekhar@ti.com, hvaibhav@ti.com,
	davinci-linux-open-source@linux.davincidsp.com
Subject: Re: [PATCH - v1 4/6] V4L - vpfe_capture bug fix and enhancements
Date: Tue, 15 Dec 2009 22:20:41 +0100	[thread overview]
Message-ID: <200912152220.41459.hverkuil@xs4all.nl> (raw)
In-Reply-To: <200912152205.25491.hverkuil@xs4all.nl>

Note that the other patches from this series are fine as far as I am concerned.

One general note: I always have difficulties with constructions like this:

> +                     val = (bc->horz.win_count_calc &
> +                             ISIF_HORZ_BC_WIN_COUNT_MASK) |
> +                             ((!!bc->horz.base_win_sel_calc) <<
> +                             ISIF_HORZ_BC_WIN_SEL_SHIFT) |
> +                             ((!!bc->horz.clamp_pix_limit) <<
> +                             ISIF_HORZ_BC_PIX_LIMIT_SHIFT) |
> +                             ((bc->horz.win_h_sz_calc &
> +                             ISIF_HORZ_BC_WIN_H_SIZE_MASK) <<
> +                             ISIF_HORZ_BC_WIN_H_SIZE_SHIFT) |
> +                             ((bc->horz.win_v_sz_calc &
> +                             ISIF_HORZ_BC_WIN_V_SIZE_MASK) <<
> +                             ISIF_HORZ_BC_WIN_V_SIZE_SHIFT);

It's just about impossible for me to parse. And some of the patches in this
series are full of such constructs.

Unfortunately, I do not have a magic bullet solution. In some cases I suspect
that a static inline function can help.

In other cases it might help to split it up in smaller parts. For example:

u32 tmp;

val = bc->horz.win_count_calc &
	ISIF_HORZ_BC_WIN_COUNT_MASK;
val |= !!bc->horz.base_win_sel_calc <<
	ISIF_HORZ_BC_WIN_SEL_SHIFT;
val |= !!bc->horz.clamp_pix_limit <<
	ISIF_HORZ_BC_PIX_LIMIT_SHIFT;
tmp = bc->horz.win_h_sz_calc &
	ISIF_HORZ_BC_WIN_H_SIZE_MASK;
val |= tmp << ISIF_HORZ_BC_WIN_H_SIZE_SHIFT;
tmp = bc->horz.win_v_sz_calc &
	ISIF_HORZ_BC_WIN_V_SIZE_MASK;
val |= tmp << ISIF_HORZ_BC_WIN_V_SIZE_SHIFT;

Of course, in this particular piece of code from the function isif_config_bclamp()
I am also wondering why bc->horz.win_h_sz_calc and bc->horz.win_v_sz_calc need to
be ANDed anyway. I would expect that to happen when these values are set. But I
did not look at this in-depth, so I may well have missed some subtlety here.

It would be interesting to know if people know of good ways of making awkward
code like this more elegant (or at least less awkward).

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG