Message-ID: <6ED8E3B22081A4459DAC7699F3695FB7018CCE459A@SW-EX-MBX02.diasemi.com>
Date:   Thu, 20 Oct 2016 14:21:27 +0000
From:   Steve Twiss <stwiss.opensource@...semi.com>
To:     'Keerthy' <a0393675@...com>,
        'Eduardo Valentin' <edubezval@...il.com>,
        'LINUX-KERNEL' <linux-kernel@...r.kernel.org>,
        'LINUX-PM' <linux-pm@...r.kernel.org>,
        'Zhang Rui' <rui.zhang@...el.com>
Subject: RE: [PATCH V1 05/10] thermal: da9062/61: Thermal junction
 temperature monitoring driver

On 20 October 2016 14:03 Steve Twiss wrote:

> On  07 October 2016 06:29 Keerthy wrote:
> > On Thursday 06 October 2016 02:13 PM, Steve Twiss wrote:
> > > From: Steve Twiss <stwiss.opensource@...semi.com>
> > >
> > > +	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.

Ok. There doesn't seem to be any formal way to do this in the thermal core, 
but after a second look, it seems like I can hijack the polling_delay value and
turn the polling on and off from my device driver. There is no API to do this,
but it seems to be possible.

I start with polling off, wait for an over-temperature IRQ trigger and tweak
the polling_delay value, say, like this:

static irqreturn_t da9062_thermal_irq_handler(int irq, void *data) {
	struct da9062_thermal *thermal = data;
	disable_irq_nosync(thermal->irq);
	thermal->zone->polling_delay = 3000;
	thermal_zone_device_update(thermal->zone);
	return IRQ_HANDLED;
}

Then when polling starts and get_temp() is called, I can re-enable the IRQ again
if the temperature has dropped.
At that point I can also turn off the thermal core polling by re-writing the 
polling_delay. Like this:

static int da9062_thermal_get_temp(struct thermal_zone_device *z,
				   int *temp)
{
	struct da9062_thermal *thermal = z->devdata;
	...
	/* if temp has dropped */
	thermal->zone->polling_delay = 0;
	enable_irq(thermal->irq);
}

I'm not certain if this is acceptable, accessing the thermal core like this.
I will send this patch separately as a RFC I think.

Regards,
Steve

Powered by blists - more mailing lists