RE: [PATCH V1 05/10] thermal: da9062/61: Thermal junction temperature monitoring driver

[Steve Twiss]

On  07 October 2016 06:29 Keerthy wrote:

> On Thursday 06 October 2016 02:13 PM, Steve Twiss wrote:
> > From: Steve Twiss <stwiss.opensource@xxxxxxxxxxx>
> > 
> > +static int da9062_thermal_probe(struct platform_device *pdev)
> > +{
> > +	struct da9062 *chip = dev_get_drvdata(pdev->dev.parent);
> > +	struct da9062_thermal *thermal;
> > +	unsigned int pp_tmp = DA9062_DEFAULT_POLLING_MS_PERIOD;
> > +	const struct of_device_id *match;
> > +	int ret = 0;
> > +
> > +	match = of_match_node(da9062_compatible_reg_id_table,
> > +			      pdev->dev.of_node);
> > +	if (!match)
> > +		return -ENXIO;
> > +
> > +	if (pdev->dev.of_node) {
> > +		if (!of_property_read_u32(pdev->dev.of_node,
> > +					"dlg,tjunc-temp-polling-period-ms",
> > +					&pp_tmp)) {
> > +			if (pp_tmp < DA9062_MIN_POLLING_MS_PERIOD ||
> > +				pp_tmp > DA9062_MAX_POLLING_MS_PERIOD)
> > +				pp_tmp = DA9062_DEFAULT_POLLING_MS_PERIOD;
> > +		}
> > +
> > +		dev_dbg(&pdev->dev,
> > +			 "TJUNC temp polling period set at %d ms\n",
> > +			 pp_tmp);
> > +	}
> > +
> > +	thermal = devm_kzalloc(&pdev->dev, sizeof(struct da9062_thermal),
> > +			     GFP_KERNEL);
> > +	if (!thermal) {
> > +		ret = -ENOMEM;
> > +		goto err;
> > +	}
> > +
> > +	thermal->config = match->data;
> > +
> > +	INIT_DELAYED_WORK(&thermal->work, da9062_thermal_poll_on);
> > +	mutex_init(&thermal->lock);
> 
> thermal_zone_device_register itself does
> INIT_DELAYED_WORK(&(tz->poll_queue), thermal_zone_device_check);
> 
> refer: drivers/thermal/thermal_core.c
> 
> It does a periodic monitoring of the temperature as well. Do you really
> want to have an additional work for monitoring temperature in your
> driver also?

The thermal triggering mechanism is interrupt based and happens when the
temperature rises above a given threshold level. The component cannot
return an exact temperature, it only has knowledge if the temperature is
above or below a given threshold value. A status bit must be polled to
detect when the temperature falls below that threshold level again.

If I was to use the core's polling_delay it would request get_temp() every
x milliseconds. This would work: I could test the status bit to decide if
the temperature was above or below the threshold, and enable or disable the
interrupt accordingly. But during normal operation, when the temperature
is in the normal range, I would be polling through I2C every x milliseconds
instead of just waiting for an IRQ trigger event before starting my polling
operations to detect when the temperature level fell below the threshold.

Since an over-temperature is supposed to be a very rare event, I decided to
go with the IRQ based trigger and then kick-off a separate work-queue to
poll the status bit and detect when the temperature dropped below the
threshold level. This seemed a more efficient way of handling this.

I have looked through thermal core, and there doesn't seem to be an obvious
way of toggling on/off thermal core polling when I need to poll the
temperature through get_temp(). So, I was going to stick with this method for
PATCH V2.

Regards,
Steve