Re: [Mjpeg-users] [PATCH v2 06/10] staging: media: zoran: fusion all modules
Le Thu, Oct 14, 2021 at 11:01:55AM +0300, Dan Carpenter a écrit :
> On Wed, Oct 13, 2021 at 06:58:08PM +, Corentin Labbe wrote:
> > The zoran driver is split in many modules, but this lead to some
> > problems.
> > One of them is that load order is incorrect when everything is built-in.
> >
> > Having more than one module is useless, so fusion all zoran modules in
> ^^
> "fusion" isn't the right word. It should be "fuse" or even better
> "merge". Same in the subject.
>
Hello
I will use merge, thanks for the suggestion.
> > +static int load_codec(struct zoran *zr, u16 codecid)
> > +{
> > + switch (codecid) {
> > + case CODEC_TYPE_ZR36060:
> > +#ifdef CONFIG_VIDEO_ZORAN_ZR36060
> > + return zr36060_init_module();
> > +#else
> > + pci_err(zr->pci_dev, "ZR36060 support is not enabled\n");
> > + return -EINVAL;
> > +#endif
> > + break;
> > + case CODEC_TYPE_ZR36050:
> > +#ifdef CONFIG_VIDEO_ZORAN_DC30
> > + return zr36050_init_module();
> > +#else
> > + pci_err(zr->pci_dev, "ZR36050 support is not enabled\n");
> > + return -EINVAL;
> > +#endif
> > + break;
> > + case CODEC_TYPE_ZR36016:
> > +#ifdef CONFIG_VIDEO_ZORAN_DC30
> > + return zr36016_init_module();
> > +#else
> > + pci_err(zr->pci_dev, "ZR36016 support is not enabled\n");
> > + return -EINVAL;
> > +#endif
> > + break;
> > + }
>
> Not related to your patch but if load_codec() fails, the probe function
> still calls zoran_setup_videocodec() on the failed codec. It might be
> better to set the codec to zero?
>
> result = load_codec(zr, zr->card.video_codec);
> if (result < 0) {
> pci_err(pdev, "failed to load codec %s: %d\n",
> codec_name, result);
> zr->card.video_codec = 0;
> }
>
I will rework by adding a video_codec_init/exit, it will help tracking error
(current behavour to ignore error is bad).
Furthermore, my patch forgot to add call to all "old module" exits, so
dedicated function will be better.
Thanks for the review
Regards
___
Mjpeg-users mailing list
Mjpeg-users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mjpeg-users
Re: [Mjpeg-users] [PATCH v2 00/10] staging: media: zoran: fusion in one module
Hi Corentin, I noticed some code review comments from Dan and a kernel test robot issue. Can you post a v3 fixing those by the end of the week? Next week I will have access again to my zoran board, so then I can test the v3 series. BTW, I agree with Dan, just drop the 'Enable zoran debugfs' config option. It's not worth the additional complexity. Instead, just #ifdef CONFIG_DEBUG_FS where necessary (in most cases you shouldn't even have to do that since the since you have dummy debug_fs_* functions if CONFIG_DEBUG_FS isn't set). Regards, Hans On 13/10/2021 20:58, Corentin Labbe wrote: > Hello > > The main change of this serie is to fusion all zoran related modules in > one. > This fixes the load order problem when everything is built-in. > > Regards > > Changes since v1: > - add missing debugfs cleaning > - clean some remaining module_get/put functions which made impossible to > remove the zoran module > - added the two latest patchs > > Corentin Labbe (10): > staging: media: zoran: move module parameter checks to zoran_probe > staging: media: zoran: use module_pci_driver > staging: media: zoran: rename debug module parameter > staging: media: zoran: add debugfs > staging: media: zoran: videocode: remove procfs > staging: media: zoran: fusion all modules > staging: media: zoran: remove vidmem > staging: media: zoran: move videodev alloc > staging: media: zoran: move config select on primary kconfig > staging: media: zoran: introduce zoran_i2c_init > > drivers/staging/media/zoran/Kconfig| 46 +-- > drivers/staging/media/zoran/Makefile | 8 +- > drivers/staging/media/zoran/videocodec.c | 68 + > drivers/staging/media/zoran/videocodec.h | 6 +- > drivers/staging/media/zoran/zoran.h| 6 +- > drivers/staging/media/zoran/zoran_card.c | 328 ++--- > drivers/staging/media/zoran/zoran_driver.c | 5 +- > drivers/staging/media/zoran/zr36016.c | 24 +- > drivers/staging/media/zoran/zr36016.h | 2 + > drivers/staging/media/zoran/zr36050.c | 21 +- > drivers/staging/media/zoran/zr36050.h | 2 + > drivers/staging/media/zoran/zr36060.c | 21 +- > drivers/staging/media/zoran/zr36060.h | 2 + > 13 files changed, 291 insertions(+), 248 deletions(-) > ___ Mjpeg-users mailing list Mjpeg-users@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mjpeg-users
Re: [Mjpeg-users] [PATCH v2 04/10] staging: media: zoran: add debugfs
Le Thu, Oct 14, 2021 at 10:37:52AM +0300, Dan Carpenter a écrit :
> On Wed, Oct 13, 2021 at 06:58:06PM +, Corentin Labbe wrote:
> > +config VIDEO_ZORAN_DEBUG
> > + bool "Enable zoran debugfs"
> > + depends on VIDEO_ZORAN
> > + depends on DEBUG_FS
> > + help
> > + Say y to enable zoran debug file.
> > + This will create /sys/kernel/debug/CARD_NAME/debug for displaying
> > + stats and debug information.
>
> Why bother with a CONFIG? Just make it always on?
>
Hello
I love to provides choice to user (and so avoid a dep on DEBUG_FS), even if I
think I am the only one remaining user.
> > @@ -1286,6 +1321,12 @@ static int zoran_probe(struct pci_dev *pdev, const
> > struct pci_device_id *ent)
> >
> > zr->map_mode = ZORAN_MAP_MODE_RAW;
> >
> > +#ifdef CONFIG_VIDEO_ZORAN_DEBUG
> > + zr->dbgfs_dir = debugfs_create_dir(ZR_DEVNAME(zr), NULL);
> > + debugfs_create_file("debug", 0444,
> > + zr->dbgfs_dir, zr,
> > + &zoran_debugfs_fops);
>
> This whitespace is weird.
Definitively Yes, fixed!
Thanks
Regards
___
Mjpeg-users mailing list
Mjpeg-users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mjpeg-users
