Re: [PATCH 1/1] thermal: of: improve of-thermal sensor registration API

[Eduardo Valentin]

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