Hey Lukasz,
On Tue, Nov 18, 2014 at 08:38:57AM +0100, Lukasz Majewski wrote:
> Hi Eduardo,
>
> In the mail topic we have PATCH 1/1 but I think that it should be PATCH
> v3 1/1.
>
Yeah, sent it without checking that. Fixing in V4, no issues.
<big cut>
> > @@ -107,10 +106,7 @@ static int of_thermal_get_temp(struct
> > thermal_zone_device *tz, {
> > struct __thermal_zone *data = tz->devdata;
> >
> > - if (!data->get_temp)
> > - return -EINVAL;
>
> To be consistent, I think that we should keep the above check [1].
>
> if (!data->ops->get_temp)
> return -EINVAL;
>
> The same check is done with get_trend callback.
>
OK. I agree, and disagree, :-). Now that you mention here, I will resend
with your request applied. The reasoning is to, yes, keep the
consistency. However, not to be the same as .get_trend, but in fact, to
keep same behavior as the code as it is currently. The thing is
.get_temp is required field, while .get_trend is not. So, checking for
required fields in the registration makes more sense than checking it
only when the field is needed.
However, as I mentioned, to keep the same behavior, before and after the
patch, it makes sense we keep the checks as they are. I will send v4
with this amendment.
> > -
> > - return data->get_temp(data->sensor_data, temp);
> > + return data->ops->get_temp(data->sensor_data, temp);
> > }
> >
> > static int of_thermal_get_trend(struct thermal_zone_device *tz, int
> > trip, @@ -120,10 +116,10 @@ static int of_thermal_get_trend(struct
> > thermal_zone_device *tz, int trip, long dev_trend;
> > int r;
> >
> > - if (!data->get_trend)
> > + if (!data->ops->get_trend)
> > return -EINVAL;
> >
> > - r = data->get_trend(data->sensor_data, &dev_trend);
> > + r = data->ops->get_trend(data->sensor_data, &dev_trend);
> > if (r)
> > return r;
> >
> > @@ -324,8 +320,7 @@ static struct thermal_zone_device_ops
> > of_thermal_ops = { static struct thermal_zone_device *
> > thermal_zone_of_add_sensor(struct device_node *zone,
> > struct device_node *sensor, void *data,
> > - int (*get_temp)(void *, long *),
> > - int (*get_trend)(void *, long *))
> > + const struct thermal_zone_of_device_ops
> > *ops) {
> > struct thermal_zone_device *tzd;
> > struct __thermal_zone *tz;
> > @@ -336,9 +331,11 @@ thermal_zone_of_add_sensor(struct device_node
> > *zone,
> > tz = tzd->devdata;
> >
> > + if (!(ops && ops->get_temp))
> > + return ERR_PTR(-EINVAL);
>
> IMHO, here we should only check:
> if (!ops)
> return ERR_PTR(-EINVAL);
>
> And check if specific callbacks are available in other
> functions (like [1])
>
OK. For the sake of this change only, I agree. However, I might be
sending patches on top of this one to keep the checks of required fields in the
registration itself.
Cheers,
> > }
>
> Despite this minor comments, feel free to add :-)
>
> Reviewed-by: Lukasz Majewski <l.majewski@xxxxxxxxxxx>
OK. Thanks.
>
> --
> Best regards,
>
> Lukasz Majewski
>
> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
Eduardo Valentin
Attachment:
signature.asc
Description: Digital signature
