Re: [PATCH v3 9/9] davinci: vpfe: Add documentation and TODO

[Mauro Carvalho Chehab]

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.

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.

Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/