RE: [PATCH v1 05/16] OMAP3 DSS Driver register moved to mach_omap2

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: Thomas Petazzoni [mailto:thomas.petazzoni@xxxxxxxxxxxxxxxxxx]
> Sent: Friday, October 08, 2010 12:36 AM
> To: Guruswamy, Senthilvadivu
> Cc: khilman@xxxxxxxxxxxxxxxxxxx; tomi.valkeinen@xxxxxxxxx; paul@xxxxxxxxx;
> Hiremath, Vaibhav; linux-omap@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v1 05/16] OMAP3 DSS Driver register moved to
> mach_omap2
> 
> Hello,
> 
> The patch title is a bit misleading, maybe it should rather be
> something like "Move OMAP3 DSS driver registration to
> mach-omap2/devices.c"/
> 
> On Wed,  6 Oct 2010 16:44:48 +0530
> Guruswamy Senthilvadivu <svadivu@xxxxxx> wrote:
> 
> >  /*---------------------------------------------------------------------
> ------*/
> > +#ifdef CONFIG_OMAP2_DSS
> > +
> > +static struct platform_device omap_display_device = {
> > +	.name          = "omapdisplay",
> > +	.id            = -1,
> > +	.dev            = {
> > +		.platform_data = NULL,
> > +	},
> 
> This .dev = {} part is useless. The compiler will automatically
> initialize unset fields to zero.
> 
[Senthil]  Thanks.  When I do code movement I don't change it.
I can submit additional patches to improvise the code.

> > +};
> > +
> > +void __init omap_display_init(struct omap_dss_board_info
> > +					*board_data)
> 
> *board_data should probably be on the same line as the argument type.
> 
[Senthil] Taken.
> > +{
> > +
> 
> The general kernel coding style seems to be that there shouldn't be
> such empty newlines at the beginning of functions.
> 
> > +	omap_display_device.dev.platform_data = board_data;
> > +
> > +	if (platform_device_register(&omap_display_device) < 0)
> > +		printk(KERN_ERR "Unable to register OMAP-Display device\n");
> > +
> > +
> 
> Unneeded newlines.
> 
[Senthil] Taken.
> > +	return ;
> 
> This return is not needed, we are at the end of a void function.
> 
> > @@ -712,7 +712,7 @@ static struct platform_driver omap_dss_driver = {
> >  	.suspend	= omap_dss_suspend,
> >  	.resume		= omap_dss_resume,
> >  	.driver         = {
> > -		.name   = "omapdss",
> > +		.name   = "omapdisplay",
> >  		.owner  = THIS_MODULE,
> >  	},
> >  };
> 
> There are other boards instantiating a platform_device with the omapdss
> name, so I think this change is going to break those boards. In my
> not-so-old linux-omap tree :
> 
> $ grep "\.name.*omapdss" *
> board-3430sdp.c:	.name		= "omapdss",
> board-am3517evm.c:	.name		= "omapdss",
> board-cm-t35.c:	.name		= "omapdss",
> board-devkit8000.c:	.name		= "omapdss",
> board-igep0020.c:	.name	= "omapdss",
> board-omap3beagle.c:	.name          = "omapdss",
> board-omap3evm.c:	.name		= "omapdss",
> board-omap3pandora.c:	.name		= "omapdss",
> board-omap3stalker.c:	.name	= "omapdss",
> board-rx51-video.c:	.name	= "omapdss",
> 
> Shouldn't these board files also be updated to use the new
> omap_display_init() function ?
> 
[Senthil] Yes, Taken. I would request for testing these additional changes.
> Regards,
> 
> 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


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux