Re: [PATCHv3 02/16] drivers: thermal: introduce device tree parser

On 18-09-2013 15:59, Joe Perches wrote:

On Wed, 2013-09-18 at 15:44 -0400, Eduardo Valentin wrote:

quoted

Hello Joe,

Hello Eduardo.

quoted

 18-09-2013 15:11, Joe Perches wrote:

quoted

On Wed, 2013-09-18 at 15:02 -0400, Eduardo Valentin wrote:

quoted

This patch introduces a device tree bindings for
describing the hardware thermal behavior and limits.
Also a parser to read and interpret the data and feed
it in the thermal framework is presented.

trivial notes:

[]

quoted

quoted

If readings are within some non zero noise level,
perhaps stable should be returned.

Yes, there should be some sort of threshold for temperature trend. But I
am not sure this is the right place to implement this. This type of
feature is in my TODO list, but I am planing to get it done within the
core code of the thermal framework.

No worries, one day...  Just wanted to bring it to attention.

Thanks, no worries, I should be working on it at some point in near
future. It does not hurt to add a TODO mark above this code though.

quoted

quoted

quoted

+static struct __thermal_zone *
+thermal_of_build_thermal_zone(struct device_node *np)
+{

[]

quoted

quoted

quoted

+	tz = kzalloc(sizeof(*tz), GFP_KERNEL);
+	if (!tz) {
+		pr_err("not enough memory for thermal of zone\n");

Unnecessary OOM message.
All allocs without GFP_NOWARN get a dump_stack()

[]

quoted

Hmmm.. I am pretty sure you have a good point. But to me seams to be
still a common practice to have drivers outputing error messages when
allocation fails. A simple git grep -A 4 kzalloc for instance, shows
that there are still quite a considerable amount of occurrences of such
practice.

True though a considerable number have been removed.
I think these should all be deleted eventually and
there's no good reason to add more.

I see.  I will repost with your suggestion. Thanks.

-- 
You have got to be excited about what you are doing. (L. Lamport)

Eduardo Valentin