[RFC PATCH 6/6] hwmon: OMAP4: On die temperature sensor driver

Hi,

On Thu, Aug 11, 2011 at 08:02:55PM +0530, J, KEERTHY wrote:

On Thu, Aug 11, 2011 at 7:42 PM, Felipe Balbi [off-list ref] wrote:

quoted

Hi,

On Thu, Aug 11, 2011 at 06:30:04PM +0530, J, KEERTHY wrote:

quoted

quoted

quoted

quoted

quoted

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@ti.com>
+ * Author: Moiz Sonasath <m-sonasath@ti.com>
+ *
+ * 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.

does the rate need to change ? Also, I have not mentioned hwmod anytime
simply because I'm not sure where this should be placed, but hopefully
not in the driver.

If the clock doesn't need to change after you set the correct rate, then
there's really no point in adding complexity to the driver.

Tony, would you have any comment here ?

quoted

quoted

quoted

quoted

quoted

quoted

+#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.

it's not omap-specific. It's specific to your driver and considering
it's platform_data, it should sit under include/linux/platform_data.h.
That directory was created (fairly recently) to hold platform_data...

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110811/af6058f5/attachment.sig>