On Thu, Aug 11, 2011 at 7:42 PM, Felipe Balbi <balbi@xxxxxx> wrote: > Hi, > > On Thu, Aug 11, 2011 at 06:30:04PM +0530, J, KEERTHY wrote: >> >> >> diff --git a/drivers/hwmon/omap_temp_sensor.c b/drivers/hwmon/omap_temp_sensor.c >> >> >> new file mode 100644 >> >> >> index 0000000..15e2559 >> >> >> --- /dev/null >> >> >> +++ b/drivers/hwmon/omap_temp_sensor.c >> >> >> @@ -0,0 +1,950 @@ >> >> >> +/* >> >> >> + * OMAP4 Temperature sensor driver file >> >> >> + * >> >> >> + * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com/ >> >> >> + * Author: J Keerthy <j-keerthy@xxxxxx> >> >> >> + * Author: Moiz Sonasath <m-sonasath@xxxxxx> >> >> >> + * >> >> >> + * This program is free software; you can redistribute it and/or >> >> >> + * modify it under the terms of the GNU General Public License >> >> >> + * version 2 as published by the Free Software Foundation. >> >> >> + * >> >> >> + * This program is distributed in the hope that it will be useful, but >> >> >> + * WITHOUT ANY WARRANTY; without even the implied warranty of >> >> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> >> >> + * General Public License for more details. >> >> >> + * >> >> >> + * You should have received a copy of the GNU General Public License >> >> >> + * along with this program; if not, write to the Free Software >> >> >> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA >> >> >> + * 02110-1301 USA >> >> >> + * >> >> >> + */ >> >> >> + >> >> >> +#include <linux/interrupt.h> >> >> >> +#include <linux/clk.h> >> >> > >> >> > why ?? >> >> >> >> Clock rate setting functions. >> > >> > you shouldn't need in drivers. >> >> It is a one time setting of the rate so keeping it in drivers. > > you need some other way to handle this. Why do you need to manually set > the rate rather than having hwmod handle this for you ? > > your argument that "it's a one time setting" is not enough to have this > in the driver. Drivers should not care about clocks anymore, this should > have been done on another layer. Hwmod will have no idea on the rate required. > >> >> >> +#include <linux/delay.h> >> >> >> +#include <linux/slab.h> >> >> >> +#include <linux/platform_device.h> >> >> >> +#include <linux/init.h> >> >> >> +#include <plat/omap_device.h> >> >> > >> >> > why ?? >> >> > >> >> >> >> Context loss count >> > >> > shouldn't use in drivers. >> >> I guess already mmc and display are using >> omap_pm_get_dev_context_loss_count but are >> populating this function in pdata as a function pointer. >> I will add that code. > > at least ;-) better than accessing that function directly. But think > carefully how you will make this work when we move to DT. Ok > >> >> >> +#include <plat/temperature_sensor.h> >> >> > >> >> >> >> It is the header file with the structure definitions >> >> used in the driver. >> > >> > why don't you put in <linux/platform_data/....> ?? >> >> I saw many omap specific header files in plat-omap/include/plat. >> Any specific reason behind placing the header in linux/platform_data? > > because it's not a good idea to perpetuate the failure. May be Tony can give some inputs on where the OMAP specific header should be placed. > >> >> >> +#include <mach/ctrl_module_core_44xx.h> >> >> > >> >> > why ? >> >> >> >> It will be removed >> >> >> >> > >> >> >> +#include <mach/gpio.h> >> >> > >> >> > linux/gpio.h for crying out loud... how many times Russell has to say >> >> > the exact same thing ?????? >> >> > >> >> >> >> It will be removed >> > >> > oh, you don't even use any gpio ? Why do you blindly add so many headers >> > if you don't need them ??? >> >> It is not required. > > this is my point, be careful when adding new drivers... You're consuming > "review bandwidth" when you send code/patch/new-drivers and reviewers > are part of an endangered species. If you keep on making such silly > mistakes, you consume time from reviewers to let you know about things > which are obvious, to say the least. So be careful when sending code, > nobody is perfect, for sure, but by being careful you can help your code > being accepted earlier. My Bad. I will take care of this. > > -- > balbi > -- Regards and Thanks, Keerthy -- 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
