> -----Original Message----- > From: Thomas Petazzoni [mailto:thomas.petazzoni@xxxxxxxxxxxxxxxxxx] > Sent: Friday, October 08, 2010 1:28 AM > To: Guruswamy, Senthilvadivu > Cc: khilman@xxxxxxxxxxxxxxxxxxx; tomi.valkeinen@xxxxxxxxx; paul@xxxxxxxxx; > Hiremath, Vaibhav; linux-omap@xxxxxxxxxxxxxxx > Subject: Re: [PATCH v1 13/16] OMAP3: hwmod DSS: VENC Move init,exit to > driver > > 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. > [Senthil] Taken. Will do the movement in the first patch itself. > > +/* 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. > [Senthil] when code was in core.c there were chances of it getting called more than once. Now it is called only once as you see, so it could be made static. > 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
