From mboxrd@z Thu Jan 1 00:00:00 1970 From: "J, KEERTHY" Subject: Re: [RFC PATCH 6/6] hwmon: OMAP4: On die temperature sensor driver Date: Thu, 11 Aug 2011 20:02:55 +0530 Message-ID: References: <1312979122-5896-1-git-send-email-j-keerthy@ti.com> <1312979122-5896-7-git-send-email-j-keerthy@ti.com> <20110810124629.GJ12882@legolas.emea.dhcp.ti.com> <20110811103658.GG27742@legolas.emea.dhcp.ti.com> <20110811141248.GK28500@legolas.emea.dhcp.ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from na3sys009aog125.obsmtp.com ([74.125.149.153]:43414 "EHLO na3sys009aog125.obsmtp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751217Ab1HKOc6 convert rfc822-to-8bit (ORCPT ); Thu, 11 Aug 2011 10:32:58 -0400 Received: by mail-iy0-f176.google.com with SMTP id 35so126608iyn.7 for ; Thu, 11 Aug 2011 07:32:57 -0700 (PDT) In-Reply-To: <20110811141248.GK28500@legolas.emea.dhcp.ti.com> Sender: linux-omap-owner@vger.kernel.org List-Id: linux-omap@vger.kernel.org To: balbi@ti.com, Tony Lindgren Cc: lm-sensors@lm-sensors.org, vishwanath.bs@ti.com, linux-omap@vger.kernel.org, b-cousson@ti.com, rnayak@ti.com, Russell King , Linux ARM Kernel Mailing List , khali@linux-fr.org, guenter.roeck@ericsson.com On Thu, Aug 11, 2011 at 7:42 PM, Felipe Balbi 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 >> >> >> + * Author: Moiz Sonasath >> >> >> + * >> >> >> + * This program is free software; you can redistribute it and= /or >> >> >> + * modify it under the terms of the GNU General Public Licens= e >> >> >> + * version 2 as published by the Free Software Foundation. >> >> >> + * >> >> >> + * This program is distributed in the hope that it will be us= eful, but >> >> >> + * WITHOUT ANY WARRANTY; without even the implied warranty of >> >> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. =A0Se= e 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 Softwar= e >> >> >> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA >> >> >> + * 02110-1301 USA >> >> >> + * >> >> >> + */ >> >> >> + >> >> >> +#include >> >> >> +#include >> >> > >> >> > 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 s= et > the rate rather than having hwmod handle this for you ? > > your argument that "it's a one time setting" is not enough to have th= is > in the driver. Drivers should not care about clocks anymore, this sho= uld > have been done on another layer. Hwmod will have no idea on the rate required. > >> >> >> +#include >> >> >> +#include >> >> >> +#include >> >> >> +#include >> >> >> +#include >> >> > >> >> > 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 >> >> > >> >> >> >> It is the header file with the structure definitions >> >> used in the driver. >> > >> > why don't you put in ?? >> >> 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 >> >> > >> >> > why ? >> >> >> >> It will be removed >> >> >> >> > >> >> >> +#include >> >> > >> >> > 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 h= eaders >> > if you don't need them ??? >> >> It is not required. > > this is my point, be careful when adding new drivers... You're consum= ing > "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 thing= s > 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 c= ode > being accepted earlier. My Bad. I will take care of this. > > -- > balbi > --=20 Regards and Thanks, Keerthy -- To unsubscribe from this list: send the line "unsubscribe linux-omap" i= n the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 From: j-keerthy@ti.com (J, KEERTHY) Date: Thu, 11 Aug 2011 20:02:55 +0530 Subject: [RFC PATCH 6/6] hwmon: OMAP4: On die temperature sensor driver In-Reply-To: <20110811141248.GK28500@legolas.emea.dhcp.ti.com> References: <1312979122-5896-1-git-send-email-j-keerthy@ti.com> <1312979122-5896-7-git-send-email-j-keerthy@ti.com> <20110810124629.GJ12882@legolas.emea.dhcp.ti.com> <20110811103658.GG27742@legolas.emea.dhcp.ti.com> <20110811141248.GK28500@legolas.emea.dhcp.ti.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Thu, Aug 11, 2011 at 7:42 PM, Felipe Balbi 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 >> >> >> + * Author: Moiz Sonasath >> >> >> + * >> >> >> + * 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 >> >> >> +#include >> >> > >> >> > 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 >> >> >> +#include >> >> >> +#include >> >> >> +#include >> >> >> +#include >> >> > >> >> > 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 >> >> > >> >> >> >> It is the header file with the structure definitions >> >> used in the driver. >> > >> > why don't you put in ?? >> >> 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 >> >> > >> >> > why ? >> >> >> >> It will be removed >> >> >> >> > >> >> >> +#include >> >> > >> >> > 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: "J, KEERTHY" Date: Thu, 11 Aug 2011 14:44:55 +0000 Subject: Re: [lm-sensors] [RFC PATCH 6/6] hwmon: OMAP4: On die temperature Message-Id: List-Id: References: <1312979122-5896-1-git-send-email-j-keerthy@ti.com> <1312979122-5896-7-git-send-email-j-keerthy@ti.com> <20110810124629.GJ12882@legolas.emea.dhcp.ti.com> <20110811103658.GG27742@legolas.emea.dhcp.ti.com> <20110811141248.GK28500@legolas.emea.dhcp.ti.com> In-Reply-To: <20110811141248.GK28500@legolas.emea.dhcp.ti.com> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable To: balbi@ti.com, Tony Lindgren Cc: lm-sensors@lm-sensors.org, vishwanath.bs@ti.com, linux-omap@vger.kernel.org, b-cousson@ti.com, rnayak@ti.com, Russell King , Linux ARM Kernel Mailing List , khali@linux-fr.org, guenter.roeck@ericsson.com On Thu, Aug 11, 2011 at 7:42 PM, Felipe Balbi 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 >> >> >> + * Author: Moiz Sonasath >> >> >> + * >> >> >> + * 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. =A0See th= e GNU >> >> >> + * General Public License for more details. >> >> >> + * >> >> >> + * You should have received a copy of the GNU General Public Lice= nse >> >> >> + * along with this program; if not, write to the Free Software >> >> >> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA >> >> >> + * 02110-1301 USA >> >> >> + * >> >> >> + */ >> >> >> + >> >> >> +#include >> >> >> +#include >> >> > >> >> > 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 >> >> >> +#include >> >> >> +#include >> >> >> +#include >> >> >> +#include >> >> > >> >> > 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 >> >> > >> >> >> >> It is the header file with the structure definitions >> >> used in the driver. >> > >> > why don't you put in ?? >> >> 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 >> >> > >> >> > why ? >> >> >> >> It will be removed >> >> >> >> > >> >> >> +#include >> >> > >> >> > linux/gpio.h for crying out loud... how many times Russell has to s= ay >> >> > the exact same thing ?????? >> >> > >> >> >> >> It will be removed >> > >> > oh, you don't even use any gpio ? Why do you blindly add so many heade= rs >> > 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 > --=20 Regards and Thanks, Keerthy _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors