Re: [PATCH 3/3] thermal: of: notify sensor driver on trip updates

[Eduardo Valentin]

On Mon, Dec 01, 2014 at 12:45:52PM -0800, navneet kumar wrote:
> Hi Eduardo,
> 
> On 11/27/2014 06:32 AM, Eduardo Valentin wrote:
> > * PGP Signed by an unknown key
> > 
> > Hello Navneet,
> > 
> > On Wed, Nov 26, 2014 at 05:16:29PM -0800, Navneet Kumar wrote:
> >> From: navneet kumar <navneetk@xxxxxxxxxx>
> >>
> >> some thermal sensor hardwares include logic which
> >> can raise interrupts at certain programmed temperature
> >> thresholds.
> >>
> >> Drivers for such sensors should be able to learn the
> >> appropriate threshold temperatures for interrupts by querying
> >> the thermal framework.
> >>
> >> This change provides a mechanism to allow a sensor driver to
> >> update it's thresholds when userspace changes a trip point
> >> temperature.
> >>
> >> While this behavior may not make sense in thermal zones
> >> with more than one sensor, no such examples exist in
> >> the kernel.
> >>
> >> Signed-off-by: navneet kumar <navneetk@xxxxxxxxxx>
> >> ---
> >>  drivers/thermal/of-thermal.c | 7 +++++++
> >>  include/linux/thermal.h      | 1 +
> >>  2 files changed, 8 insertions(+)
> >>
> >> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> >> index 3d47a0cf3825..3568e4a586dc 100644
> >> --- a/drivers/thermal/of-thermal.c
> >> +++ b/drivers/thermal/of-thermal.c
> >> @@ -258,6 +258,9 @@ static int of_thermal_set_trip_temp(struct thermal_zone_device *tz, int trip,
> >>  	/* thermal framework should take care of data->mask & (1 << trip) */
> >>  	data->trips[trip].temperature = temp;
> >>  
> >> +	if (data->sops.trip_update)
> >> +		data->sops.trip_update(data->sensor_data, trip);
> >> +
> >>  	return 0;
> >>  }
> >>  
> >> @@ -285,6 +288,9 @@ static int of_thermal_set_trip_hyst(struct thermal_zone_device *tz, int trip,
> >>  	/* thermal framework should take care of data->mask & (1 << trip) */
> >>  	data->trips[trip].hysteresis = hyst;
> >>  
> >> +	if (data->sops.trip_update)
> >> +		data->sops.trip_update(data->sensor_data, trip);
> >> +
> >>  	return 0;
> >>  }
> >>  
> >> @@ -500,6 +506,7 @@ void thermal_zone_of_sensor_unregister(struct device *dev,
> >>  
> >>  	tz->sops.get_temp = NULL;
> >>  	tz->sops.get_trend = NULL;
> >> +	tz->sops.trip_update = NULL;
> >>  	tz->sensor_data = NULL;
> >>  	mutex_unlock(&tzd->lock);
> >>  }
> >> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> >> index 58341c56a01f..b93e65815175 100644
> >> --- a/include/linux/thermal.h
> >> +++ b/include/linux/thermal.h
> >> @@ -292,6 +292,7 @@ struct thermal_genl_event {
> >>  struct thermal_of_sensor_ops {
> >>  	int (*get_temp)(void *, long *);
> >>  	int (*get_trend)(void *, long *);
> >> +	int (*trip_update)(void *, int);
> > 
> > First thing I ask you is to update your work on top of my -linus branch,
> > as I already mentioned. Reasoning is that part of the changes you are
> > sending is already there.
> will do.
> > 
> > As for this new callback, I am fine with it as long as it is also
> > available for drivers that do not use of-thermal. Once again, of-thermal
> > is not a competitor of thermal core. It will never be. It is not a new
> > thermal API. 
> I agree that this callback is not a part of the thermal_core functionality.
> However, when a driver registers directly with the thermal_core (doesn't use
> of-thermal), it 'owns' the set_trip_XX callbacks in the first place; which is
> the sole purpose of using the 'trip_update' callback in of-thermal.
> 
> Adding an additional 'update' to the thermal_core ops would be a no-op. right?

Yes, you are right. Now I understand your point. 

Can we then re-use the .set_trips nomenclature?

Cheers,

> > 
> > That said, it does not make sense to have functionality in of-thermal that
> > do not belong to thermal core. Exceptions are, of course, for helping
> > doing the same operations we already have in thermal core.
> > 
> > All the best,
> > 
> > Eduardo Valentin
> > 
> >>  };
> >>  
> >>  /* Function declarations */
> >> -- 
> >> 1.8.1.5
> >>
> > 
> > * Unknown Key
> > * 0x7DA4E256
> >

Attachment: signature.asc
Description: Digital signature