Hello Ladislav,
I agree. However, I'm talking only about header defining ioctl like
interface between (in sonypi case) drivers/char/sonypi.c and
drivers/media/video/meye.c.
Yes, I understand.
yes, but there is no need to name the same thing differently and put it
into separate files. your v4l driver knows what decoder is connected to
your capture hardware and I'm proposing following simple thing (see
diff -u include/linux/video_decoder.h include/linux/video_encoder.h
before reading more). Imagine those files merged under name
video_whatever.h where DECODER_GET_CAPABILITIES and ENCODER_GET_CAPABILITIES
becomes WHATEVER_GET_CAPABILITIES. I hope it is clear that now we have one
file less only by changing several identifier names. Then look at
include/linux/sonypi.h header and you'll find that all SONYPI_COMMAND_*
are only different names for what we already have (WHATEVER_SET_PICTURE
could be used instead sonypi_camera_command in VIDIOCSPICT ioctl at line
986 in drivers/media/video/meye.c (2.4.22)) and we just have to add defines
to query camera version and other specific stuff. In fact it doesn't mean
any code change, just naming changes.
Ok. But there is nothing really substantial in this api, right? Looking
at "video_decoder.h", I see GET_CAPABILITIES, SET_NORM (where many norms
are missing, please have a look at videodev2.h for what must be
supported here), SET_INPUT/OUTPUT and misc stuff. But today there is so
much more inside video decoders, that you would have to really extend
the api.
> your v4l driver knows what decoder is connected to
> your capture hardware
IMHO this is exactly the point. My experience was, that you'll almost
ever end up writing the whole configuration to the device at system
startup. 95% percent of these values are unchanged during operation, for
example vertical/horizontal sync start offsets, automatic color
correction settings, all this hardware dependend foo. I don't want an
IOCTL for every one of these settings.
Take for example the SET_INPUT ioctl. The saa7113 for example has 4
input pins, where 10 modes can be selected. 4 modes where the input pins
are actually cvbs inputs, and 6 modes where the y/c components are
separated. It depends on your hardware what and if these modes are
available.
So, if my driver gets a Video4Linux-2 IOCTL "S_INPUT", then I would need
to wrap this into several smaller IOCTLs, which would in turn set one
specific thing only, instead of writing some hardware-dependent values
to some i2c registers.
For example, my driver at drivers/media/video/hexium_gemini.c in
linux-2.6 has everything built in. It still uses the kernel I2C api for
transferring the actual data, but it does not have a dedicated driver
for the Samsung KS0127B video decoder.
I could of course do the same, but there is also MooseCam and other
products which could be connected to different capture devices and
that would mean code duplication.
Yes, but code duplication on what level? The code duplication would be
on the "Set Short/Long vertical blanking interval" IOCTL level. This
"IOCTL" can be done with one i2c read and one i2c write, if you really
need to ever change this value after initialization.
IMHO code duplication does only matter on a much higher level. No kernel
hacker will tell you to define an API just because you have duplicated a
small set of i2c read/write functions.
I have some startup data that's written to the device at driver load,
then some configurations for the different video norms (pal, ntsc, ...).
Brightness, contrast, saturation is not handled by the video decoder,
but it does not matter wheter I handle this in my hardware upon a
video4linux-ioctl or if I send some small message to the i2c client.
I wrote drivers for both and now I stopped on interface
between IndyCam driver and VINO driver.
My 2 ¢: I would make this as simple as possible and not waste much time
in defining an API that's won't be used outside of your driver.
I did so, but before even trying to push it into kernel it's always better
to discuss eventual better solution than blindly add just another unsorted
mess into kernel.
Yes, that's true. I did not want to offend you, just keep you from doing
IMHO unnecessary work. (Think of the time you can spend on other
projects... ;-)
I'd like to provide painless future nothing more. If you think above is
silly idea and I'm missing something obvious just let me know. In case that
you found it usefull I'm voluntering to create patch and send it here, but
note that I need to know your opinion (and opinion of v4l maintainer(s))
first.
No, I don't think it's silly and you're not missing something obvious.
It's just think that you should spend your time on something more
useful. I don't know which drivers still use the decoder/encoder api
(zoran people?), but some v4l drivers are really outdated.
Feel free to clean up the mess and simplify it as you have described
above. I'd really appreciate it, because it has annoyed me, too, but I
did not have the time and energy to fix it.
You can extend the APIs to fit your needs easily, because noone is
really working on this stuff.
regards,
ladis
CU
Michael.
