Hi Mauro,
Please see below.
On Wednesday 28 November 2012 09:22:13 Mauro Carvalho Chehab wrote:
> Hi Prabhakar,
>
> Em Wed, 28 Nov 2012 16:12:09 +0530
>
> Prabhakar Lad <prabhakar.csengg@xxxxxxxxx> escreveu:
> > +Introduction
> > +============
> > +
> > + This file documents the Texas Instruments Davinci Video processing Front
> > + End (VPFE) driver located under drivers/media/platform/davinci. The
> > + original driver exists for Davinci VPFE, which is now being changed to
> > + Media Controller Framework.
>
> Hmm... please correct me if I'm wrong, but are you wanting to replace an
> existing driver at drivers/media/platform/davinci, by another one at
> staging that has lots of known issues, as pointed at your TODO????
>
> If so, please don't do that. Replacing a driver by some other one is
> generally a very bad idea, especially in this case, where the new driver
> has clearly several issues, the main one being to define its own proprietary
> and undocumented API:
>
> > +As of now since the interface will undergo few changes all the include
> > +files are present in staging itself, to build for dm365 follow below
> > +steps,
> > +
> > +- copy vpfe.h from drivers/staging/media/davinci_vpfe/ to
> > + include/media/davinci/ folder for building the uImage.
> > +- copy davinci_vpfe_user.h from drivers/staging/media/davinci_vpfe/ to
> > + include/uapi/linux/davinci_vpfe.h, and add a entry in Kbuild (required
> > + for building application).
> > +- copy dm365_ipipeif_user.h from drivers/staging/media/davinci_vpfe/ to
> > + include/uapi/linux/dm365_ipipeif.h and a entry in Kbuild (required
> > + for building application).
>
> Among other things, with those ugly and very likely mandatory API calls:
>
> >+/*
> >+ * Private IOCTL
> >+ * VIDIOC_VPFE_IPIPEIF_S_CONFIG: Set IPIEIF configuration
> >+ * VIDIOC_VPFE_IPIPEIF_G_CONFIG: Get IPIEIF configuration
> >+ */
> >+#define VIDIOC_VPFE_IPIPEIF_S_CONFIG \
> >+ _IOWR('I', BASE_VIDIOC_PRIVATE + 1, struct ipipeif_params)
> >+#define VIDIOC_VPFE_IPIPEIF_G_CONFIG \
> >+ _IOWR('I', BASE_VIDIOC_PRIVATE + 2, struct ipipeif_params)
> >+
> >+#endif
>
> I remember we rejected already drivers like that with obscure "S_CONFIG"
> private ioctl that were suspect to send a big initialization undocumented
> blob to the driver, as only the vendor's application would be able to use
> such driver.
That's correct, and that's why the driver is going to staging. From there it
will be incrementally fixed and then moved to drivers/media/, or dropped if
not maintained.
> So, instead, of submitting it to staging, you should be sending incremental
> patches for the existing driver, adding newer functionality there, and
> using the proper V4L2 API, with makes life easier for reviewers and
> application developers.
I agree that it would be the best thing to do, but I don't think it's going to
happen. We need to decide between two options.
- Push back now and insist in incremental patches for the existing driver, and
get nothing back as TI will very likely give up completely.
- Accept the driver in staging, get it fixed incrementally, and finally move
it to drivers/media/
There's a political side to this issue, we need to decide whether we want to
insist vendors getting everything right before any code reaches mainline, in
which case I believe we will lose some of them in the process, including major
vendors such as TI, or if we can make the mainline learning curve and
experience a bit more smooth by accepting such code in staging.
I would vote for the second option, with a very clear rule that getting the
driver in staging is only one step in the journey: if the development effort
stops there, the driver *will* be removed.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
