Messages in this thread | | | Date | Wed, 18 Sep 2013 15:44:16 -0400 | From | Eduardo Valentin <> | Subject | Re: [PATCHv3 02/16] drivers: thermal: introduce device tree parser |
| |
Hello Joe,
Thanks for reviewing this code. Couple of replies.
18-09-2013 15:11, Joe Perches wrote: > On Wed, 2013-09-18 at 15:02 -0400, Eduardo Valentin wrote: >> 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:
No issues.
> >> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c > [] >> +static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip, >> + enum thermal_trend *trend) >> +{ >> + struct __thermal_zone *data = tz->devdata; >> + long dev_trend; >> + int r; >> + >> + if (!data->get_trend) >> + return -EINVAL; >> + >> + r = data->get_trend(data->sensor_data, &dev_trend); >> + if (r) >> + return r; >> + >> + if (dev_trend > 0) >> + *trend = THERMAL_TREND_RAISING; >> + else if (dev_trend < 0) >> + *trend = THERMAL_TREND_DROPPING; >> + else >> + *trend = THERMAL_TREND_STABLE; >> + >> + return 0; >> +} > > 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.
> >> +static struct __thermal_zone * >> +thermal_of_build_thermal_zone(struct device_node *np) >> +{ >> + struct device_node *child, *gchild; >> + struct __thermal_zone *tz; >> + int ret, i; >> + u32 prop; >> + >> + if (!np) { >> + pr_err("no thermal zone np\n"); >> + return ERR_PTR(-EINVAL); >> + } >> + >> + 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() > >> +int __init of_parse_thermal_zones(void) >> +{ > [] >> + ops = kzalloc(sizeof(*ops), GFP_KERNEL); >> + if (!ops) { >> + pr_err("no memory available for thermal ops\n"); >> + return 0; >> + } >> + memcpy(ops, &of_thermal_ops, sizeof(*ops)); >> + >> + tzp = kzalloc(sizeof(*tzp), GFP_KERNEL); >> + if (!ops) { >> + pr_err("no memory available for thermal zone params\n"); >> + return 0; >> + } > > a couple more OOMs. >
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.
> > >
-- You have got to be excited about what you are doing. (L. Lamport)
Eduardo Valentin
[unhandled content-type:application/pgp-signature] | |