Re: [PATCH v1 13/16] OMAP3: hwmod DSS: VENC Move init,exit to driver

[Thomas Petazzoni <thomas.petazzoni@free-electrons.com>]

Hello Senthil,

On Wed,  6 Oct 2010 16:44:56 +0530
Guruswamy Senthilvadivu <svadivu@ti.com> 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