On Sun, 19 Apr 2009 12:46:00 +0200 Hans Verkuil <hverkuil@xxxxxxxxx> wrote: > Hi Mauro, > > Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-davinci for the > following: > > - v4l: TI THS7303 video amplifier driver code +static int ths7303_setvalue(struct v4l2_subdev *sd, v4l2_std_id std) +{ + int err = 0; + u8 val; + struct i2c_client *client; + + client = v4l2_get_subdevdata(sd); + + if ((std & V4L2_STD_NTSC) || (std & V4L2_STD_PAL)) { + val = 0x02; + v4l2_dbg(1, debug, sd, "setting value for SDTV format\n"); + } else { + val = 0x00; + v4l2_dbg(1, debug, sd, "disabling all channels\n"); + } + Hmm... Are you sure that the above check is ok? The standards you're allowing are: PAL/BGDKHI and NTSC (except for NTSC/443). So, standards like PAL/N, PAL/N', PAL/60, PAL/M will stay away. If what you want is all standards but SECAM, then the correct syntax would be something like: if (std & (V4L2_STD_ALL & ~V4L2_STD_SECAM)) > - Analog Devices ADV7343 video encoder driver +#define SD_STD_NTSC (0x00) +#define SD_STD_PAL_BDGHI (0x01) +#define SD_STD_PAL_M (0x02) +#define SD_STD_PAL_N (0x03) Hmm... from the comments at the beginning of the .c file, it seems that the hardware supports all standards but SECAM. The above register definitions also specifies PAL/M and PAL/M as supported standards. However, by looking at the std_into table: +static const struct adv7343_std_info + adv7343_composite_std_info[] = { + { + SD_STD_NTSC, 0x1F, 0x7C, 0xF0, 0x21, V4L2_STD_NTSC, + }, + { + SD_STD_PAL_BDGHI, 0xCB, 0x8A, 0x09, 0x2A, V4L2_STD_PAL, + }, +}; + +static const struct adv7343_std_info + adv7343_component_std_info[] = { + { + SD_STD_NTSC, 0x1F, 0x7C, 0xF0, 0x21, V4L2_STD_NTSC, + }, + { + SD_STD_PAL_BDGHI, 0x1F, 0x7C, 0xF0, 0x21, V4L2_STD_PAL, + }, +}; Hmm.. on the last table, you are using a FSC = 3,579,545.45 Hz for both PAL and NTSC? This seems wrong to my eyes. Also, both tables have several supported standards missed. Instead of specifying the FSC as 4 u8 values, obfuscating its value, it would be much more valuable to write it as a u32 and explaining how to obtain the value. Unfortunately, according with the datasheet [1], those values are specified in function of the 27MHz reference clock, and not in Hz, so the data is somewhat obfuscated. The datasheet says that this is calculated by: number of subcarrier periods per video line FSC(reg) = ------------------------------------------- x 2^32 number of 27MHz clk cycles per video line And rounded to the nearest value. Since both factors are divided by the same constant (video line), this is the same as defining FSC(reg) as given by this formula: 2^32 FSC(reg) = FSC (Hz) * ------------- 27000000 (Hz) By using the above formula, we have: Standard FSC (Hz) * 2^32/27000000 = FSC(reg) PAL/M 3,575,611.00 * 159.0728628 = 568,782,678.08 PAL/Nc 3,582,056.00 * 159.0728628 = 569,807,902.68 NTSC 3,579,545.45 * 159.0728628 = 569,408,542.31 PAL/NBGDKHI 4,433,618.75 * 159.0728628 = 705,268,427.19 With the above, we can fill the missing entries at the table. Ah, if you want, you can also add PAL/60 and NTSC/443 to the above table, since both also use the same 4.42 MHz FSC. NTSC 4.43 should be as simple as just using 4.43 MHz for FSC. I'm not sure what would be the proper SD register for PAL/60, and the datasheet doesn't mention (although it says it is supported). I guess it will work properly with the same value as PAL/M, since, AFAIK, the only difference between PAL/60 and PAL/M is the FSC. So, the complete STD for all supported standards seems to be: struct adv7343_std_info { u32 standard_reg; u32 fsc_regs; v4l2_std_id stdid; }; /* * 2^32 * FSC(reg) = FSC (HZ) * -------- * 27000000 */ adv7343_std_info[] = { { /* FSC(Hz) = 3,579,545.45 Hz */ SD_STD_NTSC, 569408542, V4L2_STD_NTSC, }, { /* FSC(Hz) = 3,575,611.00 Hz */ SD_STD_PAL_M, 568782678, V4L2_STD_PAL_M, }, { /* FSC(Hz) = 3,582,056.00 */ SD_STD_PAL_N, 569807903, V4L2_STD_PAL_Nc, }, { /* FSC(Hz) = 4,433,618.75 Hz */ SD_STD_PAL_N, 705268427, V4L2_STD_PAL_N, }, { /* FSC(Hz) = 4,433,618.75 Hz */ SD_STD_PAL_BDGHI, 705268427, V4L2_STD_PAL, }, { /* FSC(Hz) = 4,433,618.75 Hz */ SD_STD_NTSC, 705268427, V4L2_STD_NTSC443, }, { /* FSC(Hz) = 4,433,618.75 Hz */ SD_STD_PAL_M, 705268427, V4L2_STD_PAL60, }, { }, + /* Filter settings */ + if (std & V4L2_STD_NTSC) + val &= 0x03; + else if (std & V4L2_STD_PAL) + val |= 0x04; There is a double whitespace above. Also, again, not all standards are supported. The proper code seems to be, instead: /* Filter settings */ if (std & (V4L2_STD_NTSC | V4L2_STD_NTSC443)) val &= 0x03; else if (std & ~V4L2_STD_SECAM) val |= 0x04; [1] http://www.analog.com/static/imported-files/Data_Sheets/ADV7342_7343.pdf > - v4l: improve consistency of the config menu. This one seems ok. > These new drivers for the davinci platform have been reviewed and OKed > almost a month ago, so it's time to get these merged. Especially since the > davinci display driver is also almost ready. Cheers, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
