Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sun, 17 Oct 2021 21:57:27 +0200
From:   LABBE Corentin <clabbe@baylibre.com>
To:     Dan Carpenter <dan.carpenter@oracle.com>
Cc:     mchehab@kernel.org, hverkuil@xs4all.nl, gregkh@linuxfoundation.org,
        linux-kernel@vger.kernel.org, linux-media@vger.kernel.org,
        linux-staging@lists.linux.dev, mjpeg-users@lists.sourceforge.net
Subject: Re: [PATCH v2 06/10] staging: media: zoran: fusion all modules
Message-ID: <YWyAJ1gqDrSIqAu7@Red>
References: <20211013185812.590931-1-clabbe@baylibre.com>
 <20211013185812.590931-7-clabbe@baylibre.com>
 <20211014080155.GY2083@kadam>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20211014080155.GY2083@kadam>
Precedence: bulk

Le Thu, Oct 14, 2021 at 11:01:55AM +0300, Dan Carpenter a ?crit :
> On Wed, Oct 13, 2021 at 06:58:08PM +0000, 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