Re: [PULL] http://linuxtv.org/hg/~dheitmueller/hvr950q-analog2/
On Mon, 16 Mar 2009 14:12:19 -0400 Devin Heitmueller devin.heitmuel...@gmail.com wrote: On Mon, Mar 16, 2009 at 2:05 PM, Trent Piepho xy...@speakeasy.org wrote: On Sun, 15 Mar 2009, Devin Heitmueller wrote: au0828: remove memset calls in v4l2 routines. The userland callers are responsible for clearing the output buffers, so remove the unneeded memset calls. A driver should not assume that _userspace_ has cleared the buffers. In some cases userspace is supposed to clear certain fields, but you shouldn't assume it. AFAIK, for read-only ioctls there is no expectation at all that userspace will clear the buffer. Your patch is still right though, as now the videodev core will take care of this so individual drivers don't have to. Thanks for the feedback. Admittedly I just took Mauro's word that the buffers need to be cleared without fully investigating what is responsible. I guess I jumped to the conclusion that it was done in userland, when in fact it is done in the videodev core. My mistake. The cleanups are now done at videodev core, thanks to a Trent's patch. That's why we don't need to do it inside the drivers anymore. Cheers, Mauro -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://linuxtv.org/hg/~dheitmueller/hvr950q-analog2/
On Sun, 15 Mar 2009 20:30:54 -0400 Devin Heitmueller devin.heitmuel...@gmail.com wrote: Hello Mauro, Please issue a pull request from http://linuxtv.org/hg/~dheitmueller/hvr950q-analog2 for the following: au0828: remove memset calls in v4l2 routines. au0828: remove some unneeded braces au0828: add entry for undefined input type au0828/au8522: Codingstyle fixes au0828: rename macro for currently non-function VBI support au8522: move the analog decoder source file au0828: finish videodev/subdev conversion au8522: finish conversion to v4l2_device/subdev I believe this addresses all the outstanding comments from both you and Hans. Applied, thanks. I'll likely need to move some hunks from one changeset into another, due to au8522: move the analog decoder source file to avoid -git bisect breakages. I do recommend for all new drivers to have a completely separate patch for the Kbuild changes, since the trivial fix for git bisect is generally just moving the Kconfig/Makefile changes to be the last patch at the series. Cheers, Mauro -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://linuxtv.org/hg/~dheitmueller/hvr950q-analog2/
On Wed, Mar 18, 2009 at 11:13 AM, Mauro Carvalho Chehab mche...@infradead.org wrote: I'll likely need to move some hunks from one changeset into another, due to au8522: move the analog decoder source file to avoid -git bisect breakages. No problem. I did put the original move in the same changeset as the change to the Makefile in an attempt to avoid bisect breakage, but this obviously didn't take into account the first move which broke the in-kernel build. Thanks, Devin -- Devin J. Heitmueller http://www.devinheitmueller.com AIM: devinheitmueller -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://linuxtv.org/hg/~dheitmueller/hvr950q-analog2/
On Mon, Mar 16, 2009 at 3:58 AM, Hans Verkuil hverk...@xs4all.nl wrote: Looks great! I have one small final note (doesn't prevent this from going in): In au8522_decoder.c you can remove the 'normal_i2c[]' array and the I2C_CLIENT_INSMOD macro. It's only relevant for kernels 2.6.22. Note that you should replace the normal_i2c array with a comment telling the reader which i2c address is used by this device. Or, alternatively, put that info in au8522.h if it isn't there already (didn't check this). No problem. I had suspected that might need to be removed. I will add a patch to a series I have queuing up of stuff unrelated to the HVR-950q, and will submit a PULL request Tuesday night. Thanks for all your help, Devin -- Devin J. Heitmueller http://www.devinheitmueller.com AIM: devinheitmueller -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://linuxtv.org/hg/~dheitmueller/hvr950q-analog2/
On Sun, 15 Mar 2009, Devin Heitmueller wrote: au0828: remove memset calls in v4l2 routines. The userland callers are responsible for clearing the output buffers, so remove the unneeded memset calls. A driver should not assume that _userspace_ has cleared the buffers. In some cases userspace is supposed to clear certain fields, but you shouldn't assume it. AFAIK, for read-only ioctls there is no expectation at all that userspace will clear the buffer. Your patch is still right though, as now the videodev core will take care of this so individual drivers don't have to. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://linuxtv.org/hg/~dheitmueller/hvr950q-analog2/
On Mon, Mar 16, 2009 at 2:05 PM, Trent Piepho xy...@speakeasy.org wrote: On Sun, 15 Mar 2009, Devin Heitmueller wrote: au0828: remove memset calls in v4l2 routines. The userland callers are responsible for clearing the output buffers, so remove the unneeded memset calls. A driver should not assume that _userspace_ has cleared the buffers. In some cases userspace is supposed to clear certain fields, but you shouldn't assume it. AFAIK, for read-only ioctls there is no expectation at all that userspace will clear the buffer. Your patch is still right though, as now the videodev core will take care of this so individual drivers don't have to. Thanks for the feedback. Admittedly I just took Mauro's word that the buffers need to be cleared without fully investigating what is responsible. I guess I jumped to the conclusion that it was done in userland, when in fact it is done in the videodev core. My mistake. Regards, Devin -- Devin J. Heitmueller http://www.devinheitmueller.com AIM: devinheitmueller -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PULL] http://linuxtv.org/hg/~dheitmueller/hvr950q-analog2/
Hello Mauro, Please issue a pull request from http://linuxtv.org/hg/~dheitmueller/hvr950q-analog2 for the following: au0828: remove memset calls in v4l2 routines. au0828: remove some unneeded braces au0828: add entry for undefined input type au0828/au8522: Codingstyle fixes au0828: rename macro for currently non-function VBI support au8522: move the analog decoder source file au0828: finish videodev/subdev conversion au8522: finish conversion to v4l2_device/subdev I believe this addresses all the outstanding comments from both you and Hans. Thanks, Devin -- Devin J. Heitmueller http://www.devinheitmueller.com AIM: devinheitmueller -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://linuxtv.org/hg/~dheitmueller/hvr950q-analog2
Hello Mauro, The problem is that I check for UNDEFINED so that the .input section of the au0828 board definition can be left uninitialized. Otherwise, I would have to add something like num_inputs to the board definition and worry about the value matching the actual number of inputs defined. num_inputs is a really bad thing. Maybe you can just test if input type is UNDEFINED and return -EINVAL. I agree that num_inputs is a bad idea (which is why I didn't do it). There are two cases here. One is when people select an input, where you raised the concern that they pick an input that has a type of UNDEFINED. I agree that is bad, and will fix that. The other issue is why there is an UNDEFINED input in the first place and why it is zero instead of -1: I did this because when the .input section is not defined in the board definition, this is what the type will be set to, and I use this logic to tell if the analog subsystem should be loaded at all. Do you mean the checkpatch fixes should be done as a separate patch too? I assumed you wanted me to fix the original patch to pass make checkpatch. Is there a way to do the equivalent of make checkpatch against particular hg revisions or source files? I'm just trying to understand the best way to ensure that all of the issues actually get fixed. There are two ways: 1) v4l/check.pl file This accepts just one file each time; 2) hg diff -r the last review before your patch series file v4l/check.pl file The check.pl script is just a wrapper for the checkpatch.pl. Its output is equal to an standard C compiler. So, you can use it on a C error parser for some gui. Also, the wrapper removes some checks that are ok (the ones for the lines that adds kernel version checks - since this will be removed anyway by the submit script). Great. Thanks for taking the time to provide the info. I will use this to do the cleanup and try to get you the patches this weekend. Devin -- Devin J. Heitmueller http://www.devinheitmueller.com AIM: devinheitmueller -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://linuxtv.org/hg/~dheitmueller/hvr950q-analog2
On Wed, 11 Mar 2009 11:25:20 -0400 Devin Heitmueller devin.heitmuel...@gmail.com wrote: Hello Mauro, Please pull from: http://linuxtv.org/hg/~dheitmueller/hvr950q-analog2 for the following: xc5000: fix bug for hybrid xc5000 devices with IF other than 5380 au8522: rename the au8522.c source file au8522: move shared state and common functions into a separate header files au8522: fix register read/write high bits au8522: power down the digital demod when not in use au8522: make use of hybrid framework so analog/digital demod can share state au8522: add support for analog side of demodulator au0828: add support for analog functionality in bridge au0828: workaround a bug in the au0828 i2c handling au0828: add analog profile for the HVR-850 au8522: add mutex protecting use of hybrid state au0828: Rework the way the analog video binding occurs tveeprom: add the xc5000 tuner to the tveeprom definition au0828: advertise only NTSC-M (as opposed to all NTSC standards) au0828: disable VBI code since it doesn't yet work au0828: fix i2c enumeration bug au0828: make register debug lines easier to read au0828: make g_chip_ident call work properly au0828: properly handle missing analog USB endpoint au0828: properly handle non-existent analog inputs au0828: fix panic on disconnect if analog initialization failed au0828: Convert to use v4l2_device/subdev framework Cheers, Devin Hi Devin, There's a bug on your patch series: see this: Those are the locations of au8522 files at Kernel's tree: drivers/media/dvb/frontends/au8522.h drivers/media/dvb/frontends/au8522_dig.c drivers/media/dvb/frontends/au8522_priv.h drivers/media/video/au8522_decoder.c And those are the Makefile rules for au8522.h on drivers/media/dvb/frontends/Makefile: au8522-objs = au8522_dig.o au8522_decoder.o obj-$(CONFIG_DVB_AU8522) += au8522.o When you're compiling the out-of-tree version, everything works OK, but, for in-tree compilation, au8522_decoder won't be compiled, since the file will be in the wrong dir. If I'm understanding well, this chip has two functions: it is a dvb frontend and an analog video/audio demodulator, right? One solution would be to have all those files in the same directory. However, au8522_decoder doesn't fit well on dvb/frontends. It is also not a tuner, otherwise common/tuners would be another better place. Another alternative would be to create two kconfig rules (and two separate modules), being one for au8522_decoder and another for the frontend, since they are, in fact, two different things. I suspect,however, that compiling just one or another would break compilation. So, we need to create some sort of rules that will warrant that both modules will be compiled at the same time. This is not an easy task, since we cannot add depends on, since frontends are compiled by using select. So, we will need to re-design the Kconfig rules to use depends on instead of select (well, this is something good, anyway, since the usage of select is something that should be avoided, according with Kbuild docs). I'll keep reviewing the patch series. Maybe I'll merge it, but, in this case, I'll need to blacklist the module until we found a solution, or find a way to allow my -git trees to compile. Cheers, Mauro -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://linuxtv.org/hg/~dheitmueller/hvr950q-analog2
On Wed, 11 Mar 2009 21:00:19 -0400
Devin Heitmueller devin.heitmuel...@gmail.com wrote:
Hello Mauro,
Please apply one additional patch for the series I sent this morning:
- au0828: make sure v4l2_device name is unique
Thanks,
Devin
+static int vidioc_querycap(struct file *file, void *priv,
+ struct v4l2_capability *cap)
+{
+ struct au0828_fh *fh = priv;
+ struct au0828_dev *dev = fh-dev;
+
+ memset(cap, 0, sizeof(*cap));
Please remove all memsets for input/output arguments on vidioc_foo at
au0828-video.c.
The V4L2 core warrants that the non-input fields are zeroed.
+static int vidioc_enum_fmt_vid_cap(struct file *file, void *priv,
+ struct v4l2_fmtdesc *f)
+{
+ if(f-index)
+ return -EINVAL;
+
+ memset(f, 0, sizeof(*f));
+ f-type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+ strcpy(f-description, Packed YUV2);
+
+ f-flags = 0;
+ f-pixelformat = V4L2_PIX_FMT_UYVY;
+
+ memset(f-reserved, 0, sizeof(f-reserved));
+ return 0;
+}
hmm.. you are cleaning up f-reserved three times: at v4l2-ioctl, at the
memset(f) and at memset(f-reserved).
You really wanted to make sure that you've cleaned it, don't you? ;)
hmm...
+#ifdef VBI_NOT_YET_WORKING
+ .vidioc_g_fmt_vbi_cap = vidioc_g_fmt_vbi_cap,
+ .vidioc_try_fmt_vbi_cap = vidioc_s_fmt_vbi_cap,
+ .vidioc_s_fmt_vbi_cap = vidioc_s_fmt_vbi_cap,
+#endif
I don't see any reference of this macro. If VBI is working, please cleanup the
driver. Btw, your logic seems to be inverted on some cases. Why are you adding
VBI macros, if it is not working yet?
On the other hand, if VBI is broken we'll need some rules for removing vbi code
from upstream, at gentree.pl.
+enum au0828_itype {
+ AU0828_VMUX_UNDEFINED = 0,
+ AU0828_VMUX_COMPOSITE,
+ AU0828_VMUX_SVIDEO,
+ AU0828_VMUX_CABLE,
+ AU0828_VMUX_TELEVISION,
+ AU0828_VMUX_DVB,
+ AU0828_VMUX_DEBUG
+};
...
+static int vidioc_enum_input(struct file *file, void *priv,
+ struct v4l2_input *input)
+{
+ struct au0828_fh *fh = priv;
+ struct au0828_dev *dev = fh-dev;
+ unsigned int tmp;
+
+ static const char *inames[] = {
+ [AU0828_VMUX_COMPOSITE] = Composite,
+ [AU0828_VMUX_SVIDEO] = S-Video,
+ [AU0828_VMUX_CABLE] = Cable TV,
+ [AU0828_VMUX_TELEVISION] = Television,
+ [AU0828_VMUX_DVB] = DVB,
+ [AU0828_VMUX_DEBUG] = tv debug
+ };
If the user enumerates an entry marked as UNDEFINED, it will print NULL. Is it
what you really wanted? I would, instead, assign another value for
AU0828_VMUX_UNDEFINED, like -1.
+ switch(AUVI_INPUT(index).type) {
+ case AU0828_VMUX_SVIDEO:
+ {
+ dev-input_type = AU0828_VMUX_SVIDEO;
+ break;
+ }
+ case AU0828_VMUX_COMPOSITE:
+ {
+ dev-input_type = AU0828_VMUX_COMPOSITE;
+ break;
+ }
+ case AU0828_VMUX_TELEVISION:
+ {
+ dev-input_type = AU0828_VMUX_TELEVISION;
+ break;
+ }
+ default:
+ ;
+ }
You don't need all those braces.
Also, the default rule is missing. I don't see why you would preserve the same
dev-input_type if the user selects an undefined entry, or a DVB or a debug.
Ah, finally, there are a number of CodingStyle fun. I've enclosed what it got,
from the final code. Please, always use make checkpatch before committing a
patch.
Cheers,
Mauro
/home/v4l/master/linux/drivers/media/dvb/frontends/au8522_dig.c: In '
printk(arg); \':
/home/v4l/master/linux/drivers/media/dvb/frontends/au8522_dig.c:40: warning:
suspect code indent for conditional statements (8, 17)
/home/v4l/master/linux/drivers/media/dvb/frontends/au8522_dig.c: In ' u8 buf
[] = { (reg 8) | 0x80, reg 0xff, data };':
/home/v4l/master/linux/drivers/media/dvb/frontends/au8522_dig.c:48: ERROR:
space prohibited before open square bracket '['
/home/v4l/master/linux/drivers/media/dvb/frontends/au8522_dig.c: In ' u8 b0
[] = { (reg 8) | 0x40, reg 0xff };':
/home/v4l/master/linux/drivers/media/dvb/frontends/au8522_dig.c:65: ERROR:
space prohibited before open square bracket '['
/home/v4l/master/linux/drivers/media/dvb/frontends/au8522_dig.c: In ' u8 b1
[] = { 0 };':
/home/v4l/master/linux/drivers/media/dvb/frontends/au8522_dig.c:66: ERROR:
space prohibited before open square bracket '['
/home/v4l/master/linux/drivers/media/dvb/frontends/au8522_dig.c: In ' struct
i2c_msg msg [] = {':
/home/v4l/master/linux/drivers/media/dvb/frontends/au8522_dig.c:68: ERROR:
space prohibited before open square bracket '['
/home/v4l/master/linux/drivers/media/video/au0828/au0828-cards.c: In '
}':
/home/v4l/master/linux/drivers/media/video/au0828/au0828-cards.c:205: warning:
Re: [PULL] http://linuxtv.org/hg/~dheitmueller/hvr950q-analog2
On Thu, Mar 12, 2009 at 5:19 AM, Hans Verkuil hverk...@xs4all.nl wrote: On Wed, 11 Mar 2009 11:25:20 -0400 Devin Heitmueller devin.heitmuel...@gmail.com wrote: Hello Mauro, Please pull from: http://linuxtv.org/hg/~dheitmueller/hvr950q-analog2 for the following: xc5000: fix bug for hybrid xc5000 devices with IF other than 5380 au8522: rename the au8522.c source file au8522: move shared state and common functions into a separate header files au8522: fix register read/write high bits au8522: power down the digital demod when not in use au8522: make use of hybrid framework so analog/digital demod can share state au8522: add support for analog side of demodulator au0828: add support for analog functionality in bridge au0828: workaround a bug in the au0828 i2c handling au0828: add analog profile for the HVR-850 au8522: add mutex protecting use of hybrid state au0828: Rework the way the analog video binding occurs tveeprom: add the xc5000 tuner to the tveeprom definition au0828: advertise only NTSC-M (as opposed to all NTSC standards) au0828: disable VBI code since it doesn't yet work au0828: fix i2c enumeration bug au0828: make register debug lines easier to read au0828: make g_chip_ident call work properly au0828: properly handle missing analog USB endpoint au0828: properly handle non-existent analog inputs au0828: fix panic on disconnect if analog initialization failed au0828: Convert to use v4l2_device/subdev framework Hi Devin, Can you also do the last bit of the v4l2_device/subdev conversion by actually using the subdev callbacks (replace au0828_call_i2c_clients with v4l2_device_call_all), removing attach_inform and detach_inform (already deprecated in 2.6.29) and in au8522_decoder.c replacing v4l2-i2c-drv-legacy.h by v4l2-i2c-drv.h and removing the au8522_command. Basically, when you compile against 2.6.29 you shouldn't see any 'deprecated' warnings! I also suggest renaming au8522_decoder.c to just au8522.c, like all the other i2c modules. Hans, There was already a au8522.c in the master branch before Devin's patches -- au8522.c is located in drivers/media/dvb/frontends -- it is an ATSC/QAM demodulator modulator. Devin had renamed au8522.c to au8522_dig.c , so that it can be built with au8522_decoder.c into a single module. It is important that this remain a single module, because there is state being shared between the analog and digitial sides of this device. This is a hybrid demodulator, the first of it's kind within our codebase. Regards, Mike -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://linuxtv.org/hg/~dheitmueller/hvr950q-analog2
On Thu, Mar 12, 2009 at 5:19 AM, Hans Verkuil hverk...@xs4all.nl wrote: On Wed, 11 Mar 2009 11:25:20 -0400 Devin Heitmueller devin.heitmuel...@gmail.com wrote: Hello Mauro, Please pull from: http://linuxtv.org/hg/~dheitmueller/hvr950q-analog2 for the following: xc5000: fix bug for hybrid xc5000 devices with IF other than 5380 au8522: rename the au8522.c source file au8522: move shared state and common functions into a separate header files au8522: fix register read/write high bits au8522: power down the digital demod when not in use au8522: make use of hybrid framework so analog/digital demod can share state au8522: add support for analog side of demodulator au0828: add support for analog functionality in bridge au0828: workaround a bug in the au0828 i2c handling au0828: add analog profile for the HVR-850 au8522: add mutex protecting use of hybrid state au0828: Rework the way the analog video binding occurs tveeprom: add the xc5000 tuner to the tveeprom definition au0828: advertise only NTSC-M (as opposed to all NTSC standards) au0828: disable VBI code since it doesn't yet work au0828: fix i2c enumeration bug au0828: make register debug lines easier to read au0828: make g_chip_ident call work properly au0828: properly handle missing analog USB endpoint au0828: properly handle non-existent analog inputs au0828: fix panic on disconnect if analog initialization failed au0828: Convert to use v4l2_device/subdev framework Hi Devin, Can you also do the last bit of the v4l2_device/subdev conversion by actually using the subdev callbacks (replace au0828_call_i2c_clients with v4l2_device_call_all), removing attach_inform and detach_inform (already deprecated in 2.6.29) and in au8522_decoder.c replacing v4l2-i2c-drv-legacy.h by v4l2-i2c-drv.h and removing the au8522_command. Basically, when you compile against 2.6.29 you shouldn't see any 'deprecated' warnings! I also suggest renaming au8522_decoder.c to just au8522.c, like all the other i2c modules. Hans, There was already a au8522.c in the master branch before Devin's patches -- au8522.c is located in drivers/media/dvb/frontends -- it is an ATSC/QAM demodulator modulator. Devin had renamed au8522.c to au8522_dig.c , so that it can be built with au8522_decoder.c into a single module. It is important that this remain a single module, because there is state being shared between the analog and digitial sides of this device. This is a hybrid demodulator, the first of it's kind within our codebase. Ah, thank you for the explanation. No need for a rename then :-) Thanks, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://linuxtv.org/hg/~dheitmueller/hvr950q-analog2
On Thu, Mar 12, 2009 at 4:54 AM, Mauro Carvalho Chehab mche...@infradead.org wrote: Hi Devin, There's a bug on your patch series: see this: Those are the locations of au8522 files at Kernel's tree: drivers/media/dvb/frontends/au8522.h drivers/media/dvb/frontends/au8522_dig.c drivers/media/dvb/frontends/au8522_priv.h drivers/media/video/au8522_decoder.c And those are the Makefile rules for au8522.h on drivers/media/dvb/frontends/Makefile: au8522-objs = au8522_dig.o au8522_decoder.o obj-$(CONFIG_DVB_AU8522) += au8522.o When you're compiling the out-of-tree version, everything works OK, but, for in-tree compilation, au8522_decoder won't be compiled, since the file will be in the wrong dir. If I'm understanding well, this chip has two functions: it is a dvb frontend and an analog video/audio demodulator, right? One solution would be to have all those files in the same directory. However, au8522_decoder doesn't fit well on dvb/frontends. It is also not a tuner, otherwise common/tuners would be another better place. Another alternative would be to create two kconfig rules (and two separate modules), being one for au8522_decoder and another for the frontend, since they are, in fact, two different things. I suspect,however, that compiling just one or another would break compilation. So, we need to create some sort of rules that will warrant that both modules will be compiled at the same time. This is not an easy task, since we cannot add depends on, since frontends are compiled by using select. So, we will need to re-design the Kconfig rules to use depends on instead of select (well, this is something good, anyway, since the usage of select is something that should be avoided, according with Kbuild docs). I'll keep reviewing the patch series. Maybe I'll merge it, but, in this case, I'll need to blacklist the module until we found a solution, or find a way to allow my -git trees to compile. Cheers, Mauro Hello Mauro, Both files are required, as they share certain functions (such as the i2c transfer functions). Also, they share a common state mechanism, which is why both files end up in the same .ko file. This case is a little unique since it is the first case where we have a single chip that acts as a digital demod, an analog demod, and an analog video/audio decoder. I can certainly put the au8522_decoder.c into dvb/frontends even though this probably violates some rule about analog stuff being in the DVB section of the tree. Would that resolve your concern? I really don't want to make redesigning the KConfig rules a prerequisite to getting this rather large patch series merged. I would suggest we do what is required to get the code in (such as moving the _decoder.c to frontends), and then we can tune the solution to be something more optimal in terms of how the tree is structured. Devin -- Devin J. Heitmueller http://www.devinheitmueller.com AIM: devinheitmueller -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://linuxtv.org/hg/~dheitmueller/hvr950q-analog2
On Thu, Mar 12, 2009 at 5:19 AM, Hans Verkuil hverk...@xs4all.nl wrote: Hi Devin, Can you also do the last bit of the v4l2_device/subdev conversion by actually using the subdev callbacks (replace au0828_call_i2c_clients with v4l2_device_call_all), removing attach_inform and detach_inform (already deprecated in 2.6.29) and in au8522_decoder.c replacing v4l2-i2c-drv-legacy.h by v4l2-i2c-drv.h and removing the au8522_command. Basically, when you compile against 2.6.29 you shouldn't see any 'deprecated' warnings! I also suggest renaming au8522_decoder.c to just au8522.c, like all the other i2c modules. Hello Hans, I am certainly in favor of what you have proposed. However, I would like to do it as an incremental improvement over what the series currently contains. Any chance you can allow the current series to go in as-is, and I can work on this over the next couple of days (as I will need to retest everything)? The patch series has gotten kind of unwieldy. No problem. It's fairly trivial to do since you're 90% there already :-) Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://linuxtv.org/hg/~dheitmueller/hvr950q-analog2
Hello Mauro,
Thank you for reviewing. See comments inline.
On Thu, Mar 12, 2009 at 6:06 AM, Mauro Carvalho Chehab
mche...@infradead.org wrote:
+static int vidioc_querycap(struct file *file, void *priv,
+ struct v4l2_capability *cap)
+{
+ struct au0828_fh *fh = priv;
+ struct au0828_dev *dev = fh-dev;
+
+ memset(cap, 0, sizeof(*cap));
Please remove all memsets for input/output arguments on vidioc_foo at
au0828-video.c.
The V4L2 core warrants that the non-input fields are zeroed.
Ok.
+static int vidioc_enum_fmt_vid_cap(struct file *file, void *priv,
+ struct v4l2_fmtdesc *f)
+{
+ if(f-index)
+ return -EINVAL;
+
+ memset(f, 0, sizeof(*f));
+ f-type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+ strcpy(f-description, Packed YUV2);
+
+ f-flags = 0;
+ f-pixelformat = V4L2_PIX_FMT_UYVY;
+
+ memset(f-reserved, 0, sizeof(f-reserved));
+ return 0;
+}
hmm.. you are cleaning up f-reserved three times: at v4l2-ioctl, at the
memset(f) and at memset(f-reserved).
You really wanted to make sure that you've cleaned it, don't you? ;)
Well, I wanted to be *extra* sure. ;-) Yeah, I'll yank the duplicate code.
hmm...
+#ifdef VBI_NOT_YET_WORKING
+ .vidioc_g_fmt_vbi_cap = vidioc_g_fmt_vbi_cap,
+ .vidioc_try_fmt_vbi_cap = vidioc_s_fmt_vbi_cap,
+ .vidioc_s_fmt_vbi_cap = vidioc_s_fmt_vbi_cap,
+#endif
I don't see any reference of this macro. If VBI is working, please cleanup the
driver. Btw, your logic seems to be inverted on some cases. Why are you adding
VBI macros, if it is not working yet?
On the other hand, if VBI is broken we'll need some rules for removing vbi
code
from upstream, at gentree.pl.
Here's the situation with VBI: I did all the groundwork, but it
doesn't work yet. I am hoping on getting it working over the next
couple of weeks. I believed that #ifdef'ing out the code was the
safest way to ensure that the code does not get called, while not
having to remove it from the tree entirely.
If this is important to you that the code not appear in the source
tree at all until it works entirely, then I will remove the VBI code
entirely.
+enum au0828_itype {
+ AU0828_VMUX_UNDEFINED = 0,
+ AU0828_VMUX_COMPOSITE,
+ AU0828_VMUX_SVIDEO,
+ AU0828_VMUX_CABLE,
+ AU0828_VMUX_TELEVISION,
+ AU0828_VMUX_DVB,
+ AU0828_VMUX_DEBUG
+};
...
+static int vidioc_enum_input(struct file *file, void *priv,
+ struct v4l2_input *input)
+{
+ struct au0828_fh *fh = priv;
+ struct au0828_dev *dev = fh-dev;
+ unsigned int tmp;
+
+ static const char *inames[] = {
+ [AU0828_VMUX_COMPOSITE] = Composite,
+ [AU0828_VMUX_SVIDEO] = S-Video,
+ [AU0828_VMUX_CABLE] = Cable TV,
+ [AU0828_VMUX_TELEVISION] = Television,
+ [AU0828_VMUX_DVB] = DVB,
+ [AU0828_VMUX_DEBUG] = tv debug
+ };
If the user enumerates an entry marked as UNDEFINED, it will print NULL. Is it
what you really wanted? I would, instead, assign another value for
AU0828_VMUX_UNDEFINED, like -1.
Yeah, printing NULL is bad and I can obviously fix that. The real
reason for the addition of the UNDEFINED entry is I use that to
detect if there are *any* analog inputs defined, which dictates
whether the analog subsystem is initialized. Because the .input
section is a member of the au0828_dev struct, and not a pointer, I
needed some way to tell if it was populated with anything.
+ switch(AUVI_INPUT(index).type) {
+ case AU0828_VMUX_SVIDEO:
+ {
+ dev-input_type = AU0828_VMUX_SVIDEO;
+ break;
+ }
+ case AU0828_VMUX_COMPOSITE:
+ {
+ dev-input_type = AU0828_VMUX_COMPOSITE;
+ break;
+ }
+ case AU0828_VMUX_TELEVISION:
+ {
+ dev-input_type = AU0828_VMUX_TELEVISION;
+ break;
+ }
+ default:
+ ;
+ }
You don't need all those braces.
I will remove the braces.
Also, the default rule is missing. I don't see why you would preserve the same
dev-input_type if the user selects an undefined entry, or a DVB or a debug.
I will fix that.
Ah, finally, there are a number of CodingStyle fun. I've enclosed what it got,
from the final code. Please, always use make checkpatch before committing a
patch.
I rather foolishly assumed that make commit ran make checkpatch.
I looked at the list and all of these issues are easy to fix, and I
will do that tonight.
Please let me know if you have any other concerns (and what you want
me to do regarding the VBI stuff), since I would like to avoid having
do redo the tree again.
Thanks,
Devin
--
Devin J. Heitmueller
http://www.devinheitmueller.com
AIM:
Re: [PULL] http://linuxtv.org/hg/~dheitmueller/hvr950q-analog2
On Thu, 12 Mar 2009 10:24:02 -0400
Devin Heitmueller devin.heitmuel...@gmail.com wrote:
hmm.. you are cleaning up f-reserved three times: at v4l2-ioctl, at the
memset(f) and at memset(f-reserved).
You really wanted to make sure that you've cleaned it, don't you? ;)
Well, I wanted to be *extra* sure. ;-) Yeah, I'll yank the duplicate code.
LOL
hmm...
+#ifdef VBI_NOT_YET_WORKING
+ .vidioc_g_fmt_vbi_cap = vidioc_g_fmt_vbi_cap,
+ .vidioc_try_fmt_vbi_cap = vidioc_s_fmt_vbi_cap,
+ .vidioc_s_fmt_vbi_cap = vidioc_s_fmt_vbi_cap,
+#endif
I don't see any reference of this macro. If VBI is working, please cleanup
the
driver. Btw, your logic seems to be inverted on some cases. Why are you
adding
VBI macros, if it is not working yet?
On the other hand, if VBI is broken we'll need some rules for removing vbi
code
from upstream, at gentree.pl.
Here's the situation with VBI: I did all the groundwork, but it
doesn't work yet. I am hoping on getting it working over the next
couple of weeks. I believed that #ifdef'ing out the code was the
safest way to ensure that the code does not get called, while not
having to remove it from the tree entirely.
If this is important to you that the code not appear in the source
tree at all until it works entirely, then I will remove the VBI code
entirely.
Hmm... So, I understood just the opposite ;)
you wrote if VBI_IS_NOT_WORKING, when you should have written if
VBI_IS_WORKING.
Just rename it. I'll add it at gentree.pl to remove this symbol.
+enum au0828_itype {
+ AU0828_VMUX_UNDEFINED = 0,
+ AU0828_VMUX_COMPOSITE,
+ AU0828_VMUX_SVIDEO,
+ AU0828_VMUX_CABLE,
+ AU0828_VMUX_TELEVISION,
+ AU0828_VMUX_DVB,
+ AU0828_VMUX_DEBUG
+};
...
+static int vidioc_enum_input(struct file *file, void *priv,
+ struct v4l2_input *input)
+{
+ struct au0828_fh *fh = priv;
+ struct au0828_dev *dev = fh-dev;
+ unsigned int tmp;
+
+ static const char *inames[] = {
+ [AU0828_VMUX_COMPOSITE] = Composite,
+ [AU0828_VMUX_SVIDEO] = S-Video,
+ [AU0828_VMUX_CABLE] = Cable TV,
+ [AU0828_VMUX_TELEVISION] = Television,
+ [AU0828_VMUX_DVB] = DVB,
+ [AU0828_VMUX_DEBUG] = tv debug
+ };
If the user enumerates an entry marked as UNDEFINED, it will print NULL. Is
it
what you really wanted? I would, instead, assign another value for
AU0828_VMUX_UNDEFINED, like -1.
Yeah, printing NULL is bad and I can obviously fix that. The real
reason for the addition of the UNDEFINED entry is I use that to
detect if there are *any* analog inputs defined, which dictates
whether the analog subsystem is initialized. Because the .input
section is a member of the au0828_dev struct, and not a pointer, I
needed some way to tell if it was populated with anything.
if you attribute it to -1, the userspace calls will never set it to undefined.
You should take some care to avoid reading outside some array though.
Ah, finally, there are a number of CodingStyle fun. I've enclosed what it
got,
from the final code. Please, always use make checkpatch before committing a
patch.
I rather foolishly assumed that make commit ran make checkpatch.
It runs, and outputs its result at the commit comments.
I looked at the list and all of these issues are easy to fix, and I
will do that tonight.
Ok.
Please let me know if you have any other concerns (and what you want
me to do regarding the VBI stuff), since I would like to avoid having
do redo the tree again.
No, just the above. Please, instead of redo the tree, just add some patches
fixing those issues. This allows me to review faster your series.
Cheers,
Mauro
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://linuxtv.org/hg/~dheitmueller/hvr950q-analog2
On Thu, Mar 12, 2009 at 11:06 AM, Mauro Carvalho Chehab mche...@infradead.org wrote: Yeah, printing NULL is bad and I can obviously fix that. The real reason for the addition of the UNDEFINED entry is I use that to detect if there are *any* analog inputs defined, which dictates whether the analog subsystem is initialized. Because the .input section is a member of the au0828_dev struct, and not a pointer, I needed some way to tell if it was populated with anything. if you attribute it to -1, the userspace calls will never set it to undefined. You should take some care to avoid reading outside some array though. The problem is that I check for UNDEFINED so that the .input section of the au0828 board definition can be left uninitialized. Otherwise, I would have to add something like num_inputs to the board definition and worry about the value matching the actual number of inputs defined. I looked at the list and all of these issues are easy to fix, and I will do that tonight. Ok. Please let me know if you have any other concerns (and what you want me to do regarding the VBI stuff), since I would like to avoid having do redo the tree again. No, just the above. Please, instead of redo the tree, just add some patches fixing those issues. This allows me to review faster your series. Do you mean the checkpatch fixes should be done as a separate patch too? I assumed you wanted me to fix the original patch to pass make checkpatch. Is there a way to do the equivalent of make checkpatch against particular hg revisions or source files? I'm just trying to understand the best way to ensure that all of the issues actually get fixed. Devin -- Devin J. Heitmueller http://www.devinheitmueller.com AIM: devinheitmueller -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://linuxtv.org/hg/~dheitmueller/hvr950q-analog2
On Thu, 12 Mar 2009 10:11:12 -0400 Devin Heitmueller devin.heitmuel...@gmail.com wrote: On Thu, Mar 12, 2009 at 4:54 AM, Mauro Carvalho Chehab mche...@infradead.org wrote: Hi Devin, There's a bug on your patch series: see this: Those are the locations of au8522 files at Kernel's tree: drivers/media/dvb/frontends/au8522.h drivers/media/dvb/frontends/au8522_dig.c drivers/media/dvb/frontends/au8522_priv.h drivers/media/video/au8522_decoder.c And those are the Makefile rules for au8522.h on drivers/media/dvb/frontends/Makefile: au8522-objs = au8522_dig.o au8522_decoder.o obj-$(CONFIG_DVB_AU8522) += au8522.o When you're compiling the out-of-tree version, everything works OK, but, for in-tree compilation, au8522_decoder won't be compiled, since the file will be in the wrong dir. If I'm understanding well, this chip has two functions: it is a dvb frontend and an analog video/audio demodulator, right? One solution would be to have all those files in the same directory. However, au8522_decoder doesn't fit well on dvb/frontends. It is also not a tuner, otherwise common/tuners would be another better place. Another alternative would be to create two kconfig rules (and two separate modules), being one for au8522_decoder and another for the frontend, since they are, in fact, two different things. I suspect,however, that compiling just one or another would break compilation. So, we need to create some sort of rules that will warrant that both modules will be compiled at the same time. This is not an easy task, since we cannot add depends on, since frontends are compiled by using select. So, we will need to re-design the Kconfig rules to use depends on instead of select (well, this is something good, anyway, since the usage of select is something that should be avoided, according with Kbuild docs). I'll keep reviewing the patch series. Maybe I'll merge it, but, in this case, I'll need to blacklist the module until we found a solution, or find a way to allow my -git trees to compile. Cheers, Mauro Hello Mauro, Both files are required, as they share certain functions (such as the i2c transfer functions). Also, they share a common state mechanism, which is why both files end up in the same .ko file. This case is a little unique since it is the first case where we have a single chip that acts as a digital demod, an analog demod, and an analog video/audio decoder. I can certainly put the au8522_decoder.c into dvb/frontends even though this probably violates some rule about analog stuff being in the DVB section of the tree. Would that resolve your concern? I really don't want to make redesigning the KConfig rules a prerequisite to getting this rather large patch series merged. I would suggest we do what is required to get the code in (such as moving the _decoder.c to frontends), and then we can tune the solution to be something more optimal in terms of how the tree is structured. I don't like much the idea of moving decoder to frontends, but we may do this as an intermediate step. To make things easier for future changes, IMO, it would be better to create a separate dir for this driver, inside dvb/frontends. This will help to move it elsewhere. Cheers, Mauro. Cheers, Mauro -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://linuxtv.org/hg/~dheitmueller/hvr950q-analog2
On Thu, 12 Mar 2009 11:26:15 -0400 Devin Heitmueller devin.heitmuel...@gmail.com wrote: On Thu, Mar 12, 2009 at 11:06 AM, Mauro Carvalho Chehab mche...@infradead.org wrote: Yeah, printing NULL is bad and I can obviously fix that. The real reason for the addition of the UNDEFINED entry is I use that to detect if there are *any* analog inputs defined, which dictates whether the analog subsystem is initialized. Because the .input section is a member of the au0828_dev struct, and not a pointer, I needed some way to tell if it was populated with anything. if you attribute it to -1, the userspace calls will never set it to undefined. You should take some care to avoid reading outside some array though. The problem is that I check for UNDEFINED so that the .input section of the au0828 board definition can be left uninitialized. Otherwise, I would have to add something like num_inputs to the board definition and worry about the value matching the actual number of inputs defined. num_inputs is a really bad thing. Maybe you can just test if input type is UNDEFINED and return -EINVAL. I looked at the list and all of these issues are easy to fix, and I will do that tonight. Ok. Please let me know if you have any other concerns (and what you want me to do regarding the VBI stuff), since I would like to avoid having do redo the tree again. No, just the above. Please, instead of redo the tree, just add some patches fixing those issues. This allows me to review faster your series. Do you mean the checkpatch fixes should be done as a separate patch too? I assumed you wanted me to fix the original patch to pass make checkpatch. Is there a way to do the equivalent of make checkpatch against particular hg revisions or source files? I'm just trying to understand the best way to ensure that all of the issues actually get fixed. There are two ways: 1) v4l/check.pl file This accepts just one file each time; 2) hg diff -r the last review before your patch series file v4l/check.pl file The check.pl script is just a wrapper for the checkpatch.pl. Its output is equal to an standard C compiler. So, you can use it on a C error parser for some gui. Also, the wrapper removes some checks that are ok (the ones for the lines that adds kernel version checks - since this will be removed anyway by the submit script). Cheers, Mauro -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PULL] http://linuxtv.org/hg/~dheitmueller/hvr950q-analog2
Hello Mauro, Please pull from: http://linuxtv.org/hg/~dheitmueller/hvr950q-analog2 for the following: xc5000: fix bug for hybrid xc5000 devices with IF other than 5380 au8522: rename the au8522.c source file au8522: move shared state and common functions into a separate header files au8522: fix register read/write high bits au8522: power down the digital demod when not in use au8522: make use of hybrid framework so analog/digital demod can share state au8522: add support for analog side of demodulator au0828: add support for analog functionality in bridge au0828: workaround a bug in the au0828 i2c handling au0828: add analog profile for the HVR-850 au8522: add mutex protecting use of hybrid state au0828: Rework the way the analog video binding occurs tveeprom: add the xc5000 tuner to the tveeprom definition au0828: advertise only NTSC-M (as opposed to all NTSC standards) au0828: disable VBI code since it doesn't yet work au0828: fix i2c enumeration bug au0828: make register debug lines easier to read au0828: make g_chip_ident call work properly au0828: properly handle missing analog USB endpoint au0828: properly handle non-existent analog inputs au0828: fix panic on disconnect if analog initialization failed au0828: Convert to use v4l2_device/subdev framework Cheers, Devin -- Devin J. Heitmueller http://www.devinheitmueller.com AIM: devinheitmueller -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://linuxtv.org/hg/~dheitmueller/hvr950q-analog2
On Wed, 2009-03-11 at 11:25 -0400, Devin Heitmueller wrote: Hello Mauro, Please pull from: http://linuxtv.org/hg/~dheitmueller/hvr950q-analog2 for the following: [snip] au0828: Convert to use v4l2_device/subdev framework + /* Create the v4l2_device */ + snprintf(dev-v4l2_dev.name, sizeof(dev-v4l2_dev.name), %s-%03d, +au0828, 0); + retval = v4l2_device_register(dev-usbdev-dev, dev-v4l2_dev); If you're not going to differentiate between different instances of au0828 devices connectred to the system in log messages, you may wish to get rid of the '-%03d' and ',0' in the snprintf(). Or maybe you could use dev-usbdev-dev.bus_id somehow to indicate the instance in the name? Regards, Andy Cheers, Devin -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://linuxtv.org/hg/~dheitmueller/hvr950q-analog2
On Wed, Mar 11, 2009 at 3:44 PM, Andy Walls awa...@radix.net wrote: + /* Create the v4l2_device */ + snprintf(dev-v4l2_dev.name, sizeof(dev-v4l2_dev.name), %s-%03d, + au0828, 0); + retval = v4l2_device_register(dev-usbdev-dev, dev-v4l2_dev); If you're not going to differentiate between different instances of au0828 devices connectred to the system in log messages, you may wish to get rid of the '-%03d' and ',0' in the snprintf(). Or maybe you could use dev-usbdev-dev.bus_id somehow to indicate the instance in the name? Doh. Yeah, I jammed the zero in there when I was trying to bootstrap the conversion, and then I forgot about it. Mauro, any objection to me submitting this as a separate patch tonight when I get home? This is a large series of patches and I would prefer to get them in before somebody else submits something that requires me to rebase again (and while I agree with Andy that it should be fixed, it doesn't actually result in any sort of regression). Cheers, Devin -- Devin J. Heitmueller http://www.devinheitmueller.com AIM: devinheitmueller -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PULL] http://linuxtv.org/hg/~dheitmueller/hvr950q-analog2
Hello Mauro, Please apply one additional patch for the series I sent this morning: - au0828: make sure v4l2_device name is unique Thanks, Devin -- Devin J. Heitmueller http://www.devinheitmueller.com AIM: devinheitmueller -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
