lkml.org 
[lkml]   [2013]   [Sep]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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]
    \
     
     \ /
      Last update: 2013-09-18 22:21    [W:5.264 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site