Mike Frysinger a écrit :
> On Thu, Jun 19, 2008 at 9:33 AM, Jean-Christian de Rivaz <[EMAIL PROTECTED]>
> wrote:
>> drivers/media/video/blackfin/Kconfig | 8 +-
>> drivers/media/video/blackfin/Makefile | 1 +
>> drivers/media/video/blackfin/adv7183b.c | 225
>> +++++++++++++++++++++++++++
>> drivers/media/video/blackfin/adv7183b.h | 85 ++++++++++
>> drivers/media/video/blackfin/blackfin_cam.c | 3 +
>> include/asm-blackfin/mach-bf561/cdefBF561.h | 12 ++
>> 6 files changed, 333 insertions(+), 1 deletions(-)
>
> i see you're using git to generate the patch, yet it has broken line
> wrapping ... what's up with that ? arent you using `git-send-email` ?
I used git to generate the diff and the stats, but assembled them
manually into the mail. I hope diff file in the patch tracker should be
free of this wrapping issue.
>> --- a/drivers/media/video/blackfin/Kconfig
>> +++ b/drivers/media/video/blackfin/Kconfig
>> @@ -5,7 +5,7 @@ config VIDEO_BLACKFIN_CAM
>> default n
>> help
>> V4L Support for ST VS6524, VS6624 sensor Micron MT9M001, MT9V022
>> - attached to the Blackfin PPI.
>> + sensor AD ADV7183B attached to the Blackfin PPI.
>
> the resulting sentence is incorrect:
> V4L Support for ST VS6524, VS6624 sensor Micron MT9M001, MT9V022
> sensor AD ADV7183B attached to the Blackfin PPI.
Oups. We will fix that.
>> +config ADV7183B
>> + bool "ADV7183B"
>> + depends on VIDEO_BLACKFIN_CAM
>> + help
>> + Analog Devices ADV7183B Sensor Support
>
> "Analog Devices AD...." is redundant. it's like saying "Compact Disc CDROM".
Ok. Will be fixed.
>> --- /dev/null
>> +++ b/drivers/media/video/blackfin/adv7183b.c
>> @@ -0,0 +1,225 @@
>> +static int adv7183b_set_gpio(unsigned gpio, int value)
>> + gpio_direction_output(gpio, 0);
>> + gpio_set_value(gpio, value);
>
> err, why not just combine these ?
>
>> + // Configuration Example, taken from datasheet ADV7183B
>
> no C++ comments
Ok. Will be fixed.
>> +MODULE_AUTHOR("");
>
> this really should be set
Ok. Will be fixed.
>> +/* Hardware settings for BF561-EZKIT */
>> +//#define ADV7183B_GPIO_RESET GPIO_PF13
>> +//#define ADV7183B_GPIO_OE GPIO_PF2
>> +//#undef ADV7183B_28MHZ
>> +//#undef ADV7183B_STRONG
>
> these should go into platform resources
In fact this BF561-EZKIT setting must be uncommented like in the patch
in the issue tracker. We hope this is acceptable to have the settings
for our custom board in comment. Very few people on this planet have
this board yet. Adding a platform just for it seem a bit too much to
clean up 5 lines of comment.
>> +/* Parallel Peripheral Interface (PPI) is simply the same as PPI0 */
>
> ugh, no. this is as bad as the PORT hack we had for BF533->BF537. we
> should be abstracting PPI so that platform resources can declare which
> PPI the sensor is hooked up to. base registers perhaps like we do
> with the UART ?
Well. We expected this kind of comment. But really, the patch is about
the ADV7183B. We only do this simple hack to let people test it. A far
as we understand, none of the Blackfin PPI V4L sensors can be used with
a BF561 without this hack. The problem is in the blackfin_cam code, not
in the adv7183b code, this outside the scope of the ADV7183B support.
So I propose to remove the hack from the patch and to open a ticket to
allow setup the V4L on BF561 using the PPI0 or the PPI1, without
touching any sensors codes.
Will something (pseudo code) like this be acceptable in blackfin_cam.h ?
#ifdef BF561
#ifdef CONFIG_BF561_PPI1
#define bfin_PPI_xxx bfin_PPI1_xxx
#else
#define bfin_PPI_xxx bfin_PPI0_xxx
#endif
#endif
Regards,
--
Jean-Christian de Rivaz
_______________________________________________
Uclinux-dist-devel mailing list
Uclinux-dist-devel@blackfin.uclinux.org
http://blackfin.uclinux.org/mailman/listinfo/uclinux-dist-devel
