On 26 September 2012 18:50, Jonathan Corbet <corbet@xxxxxxx> wrote: > On Wed, 26 Sep 2012 11:47:55 +0200 > Javier Martin <javier.martin@xxxxxxxxxxxxxxxxx> wrote: > >> According to the datasheet ov7675 uses a formula to achieve >> the desired framerate that is different from the operations >> done in the current code. >> >> In fact, this formula should apply to ov7670 too. This would >> mean that current code is wrong but, in order to preserve >> compatibility, the new formula will be used for ov7675 only. > > At this point I couldn't tell you what the real situation is; it's been a > while and there's always a fair amount of black magic involved with > ov7670 configuration. I do appreciate attention to not breaking existing > users. Indeed, this sensor is the quirkier I've dealt with, with those magic values in non documented registers... >> +static void ov7670_get_framerate(struct v4l2_subdev *sd, >> + struct v4l2_fract *tpf) > > This bugs me, though. It's called ov7670_get_framerate() but it's getting > the rate for the ov7675 - confusing. Meanwhile the real ov7670 code > remains inline while ov7675 has its own function. Actually, I did this on purpose because I wanted to remark that this function should be valid for both models and because I expected that the old behaviour was removed sometime in the future. > Please make two functions, one of which is ov7675_get_framerate(), and call > the right one for the model. Same for the "set" functions, obviously. > Maybe what's really needed is a structure full of sensor-specific > operations? The get_wsizes() function could go there too. That would take > a lot of if statements out of the code. The idea of a structure of sensor-specific operations seems reasonable. Furthermore, I think we should encourage users to use the right formula in the future. For this reason we could define 4 functions ov7670_set_framerate_legacy() ov7670_get_framerate_legacy() ov7675_set_framerate() ov7675_get_framerate() >> + /* >> + * The datasheet claims that clkrc = 0 will divide the input clock by 1 >> + * but we've checked with an oscilloscope that it divides by 2 instead. >> + * So, if clkrc = 0 just bypass the divider. >> + */ > > Thanks for documenting this kind of thing. You are welcome. -- Javier Martin Vista Silicon S.L. CDTUC - FASE C - Oficina S-345 Avda de los Castros s/n 39005- Santander. Cantabria. Spain +34 942 25 32 60 www.vista-silicon.com -- 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
