Re: [PATCH V2 3/3] saa7115: Add register setup and config for gm7113c

[Ezequiel Garcia <ezequiel.garcia@free-electrons.com>]

On Mon, Apr 29, 2013 at 10:41:09PM +0200, Jon Arne Jørgensen wrote:
> 
> Signed-off-by: Jon Arne Jørgensen <jonarne@jonarne.no>

Every patch *must* have a proper commit message indicating what you're doing
and why you're doing it.

In particular, please add as much information as you can about this new clone decoder.

If you know *why* we have to add those quirks for PAL and NTSC, it would
be nice to add that information to the commit message and maybe even
in a comment somewhere in the code.

> ---
>  drivers/media/i2c/saa7115.c | 48 ++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 39 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/i2c/saa7115.c b/drivers/media/i2c/saa7115.c
> index eb2c19d..77c13dc 100644
> --- a/drivers/media/i2c/saa7115.c
> +++ b/drivers/media/i2c/saa7115.c
> @@ -126,6 +126,8 @@ static int saa711x_has_reg(const int id, const u8 reg)
>  		return 0;
>  
>  	switch (id) {
> +	case V4L2_IDENT_GM7113C:
> +		return reg != 0x14 && (reg < 0x18 || reg > 0x1e) && reg < 0x20;
>  	case V4L2_IDENT_SAA7113:
>  		return reg != 0x14 && (reg < 0x18 || reg > 0x1e) && (reg < 0x20 || reg > 0x3f) &&
>  		       reg != 0x5d && reg < 0x63;
> @@ -447,6 +449,30 @@ static const unsigned char saa7115_cfg_50hz_video[] = {
>  
>  /* ============== SAA7715 VIDEO templates (end) =======  */
>  
> +/* ============== GM7113C VIDEO templates =============  */
> +static const unsigned char gm7113c_cfg_60hz_video[] = {
> +	R_08_SYNC_CNTL, 0x68,			/* 0xBO: auto detection, 0x68 = NTSC */
> +	R_0E_CHROMA_CNTL_1, 0x07,		/* video autodetection is on */
> +
> +#if 0
> +	R_5A_V_OFF_FOR_SLICER, 0x06,		/* standard 60hz value for ITU656 line counting */
> +#endif

What's the meaning of this ifdef? In general it's better to remove dead
code, or at least to put some fat comment about it.

> +	0x00, 0x00
> +};
> +
> +static const unsigned char gm7113c_cfg_50hz_video[] = {
> +	R_08_SYNC_CNTL, 0x28,			/* 0x28 = PAL */
> +	R_0E_CHROMA_CNTL_1, 0x07,
> +
> +#if 0
> +	R_5A_V_OFF_FOR_SLICER, 0x03,		/* standard 50hz value */
> +#endif

Ditto.

I've tested this with an stk1160/gm7113c device and now
it's working fine (NTSC, PAL-M, PAL-Nc).

I also tested against regressions using stk1160/saa7113 and
em28xx/saa7115 devices.

I must admit I'm quite happy to have this issue finally fixed!

Could you please re-send the whole series, taking account of this
comments. You can add my:

Tested-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>

to every patch in the series.

Thanks!
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com