Hello Senthil,
On Wed, 6 Oct 2010 16:44:56 +0530
Guruswamy Senthilvadivu <svadivu@xxxxxx> wrote:
> diff --git a/drivers/video/omap2/dss/venc.c b/drivers/video/omap2/dss/venc.c
> index ec17b28..3a121cb 100644
> --- a/drivers/video/omap2/dss/venc.c
> +++ b/drivers/video/omap2/dss/venc.c
> @@ -87,26 +87,6 @@
> #define VENC_OUTPUT_TEST 0xC8
> #define VENC_DAC_B__DAC_C 0xC8
>
> -/* VENC HW IP initialisation */
> -static int omap_venchw_probe(struct platform_device *pdev)
> -{
> - return 0;
> -}
> -
> -static int omap_venchw_remove(struct platform_device *pdev)
> -{
> - return 0;
> -}
> -
> -static struct platform_driver omap_venchw_driver = {
> - .probe = omap_venchw_probe,
> - .remove = omap_venchw_remove,
> - .driver = {
> - .name = "dss_venc",
> - .owner = THIS_MODULE,
> - },
> -};
Would be better in patch 7/16 to put this stuff at the correct place
(bottom of the file) so it does not need to be moved here.
> +/* VENC HW IP initialisation */
> +static int omap_venchw_probe(struct platform_device *pdev)
> +{
> + int r;
> + venc.pdev = pdev;
> +
> + r = venc_init(pdev);
> + if (r) {
> + DSSERR("Failed to initialize venc\n");
> + goto err_venc;
> + }
> +
> +err_venc:
> + return r;
> +}
> +
> +static int omap_venchw_remove(struct platform_device *pdev)
> +{
> + venc_exit();
> + return 0;
> +}
Same comment as before: include the code of venc_init() and venc_exit()
directly in the ->probe() and ->remove() hooks respectively.
> +struct regulator *dss_get_vdda_dac(void)
> +{
> + struct regulator *reg;
> +
> + if (venc.vdda_dac_reg != NULL)
> + return venc.vdda_dac_reg;
> +
> + reg = regulator_get(&venc.pdev->dev, "vdda_dac");
> + if (!IS_ERR(reg))
> + venc.vdda_dac_reg = reg;
>
> + return reg;
> +}
As far as I can see, this function is now only used in venc_init(),
which is in the same file, so the function should be static, and the
prototype removed from drivers/video/omap2/dss/core.h.
I'm also a bit skeptical about what this function does. It is called
this way in venc_init():
venc.vdda_dac_reg = dss_get_vdda_dac();
so it is dss_get_vdda_dac() responsability to set venc.vdda_dac_reg, or
is it the caller responsability ?
Moreover, the logic in dss_get_vdda_dac() that tests whether
venc.vdda_dac_reg is already initialized seems to indicate that this
function could be called several times. However, I only see it called
from venc_init(), which as far as I understand is called only once.
Isn't it possible to simplify this by removing the dss_get_vdda_dac()
function and just doing:
venc.vdda_dac_reg = regulator_get(&venc.pdev->dev, "vdda_dac");
in the venc_init() function (which should become the ->probe() method).
Thanks!
Thomas
--
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
