Re: [lm-sensors] [PATCH] v3 of a adt7470 driver\

Re: [lm-sensors] [PATCH] v3 of a adt7470 driver\

[Darrick J. Wong]

[-- Attachment #1.1: Type: text/plain, Size: 4713 bytes --]

On Thu, Jul 12, 2007 at 11:42:43AM +0200, Hans de Goede wrote:
> Darrick J. Wong wrote:
> >Resend with correct title and patch included just once.
> >---
> >adt7470: new hwmon driver for adt7470 chip.
> >
> >Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
> 
> Looks good now, as far I'm concerned this is ready to get merged.
> Acked-by: Hans de Goede <j.w.r.degoede@hhs.nl>
> 
> Mark, any chance you can pick this up soonish, maybe it can make 2.6.23 
> then.
> 
> Darrick, have you already written support for this driver for libsensors 
> and sensors? If not could you please do that ASAP? Then I'll review it and 
> with some luck and Jean's permission we can still get support for this chip 
> included into lm_sensors 2.10.4

I ... can't say that I have a working config.  I've added this into /etc/sensors.conf:
chip "adt7470-*"
        label fan1 "CPU0 Fan"
        label fan2 "CPU1 Fan"
        label temp1 "CPU0 Temp"
        label temp3 "CPU1 temp"
        label pwm1 "CPU0 Fan Control"
        label pwm2 "CPU1 Fan Control"

(There are more pwm/fan/temp widgets but on the Z30 they're not
connected to anything.)

But every time I run sensors (from svn) I see only this:
root@elm3a181:/usr/local/src/lm_sensors/prog# sensors/sensors -c
/tmp/sensors.conf 
adt7470-i2c-0-2f
Can't get adapter name for bus 0

I _think_ this has to do with i2c-i801 not exporting something in sysfs
correctly, though I'm not sure.  Below is the strace output from sensors
after it finishes probing the hwmon devices.

--D

lstat("/sys/class/i2c-adapter/i2c-0", {st_mode=S_IFLNK|0777, st_size=0, ...}) = 0
lstat("/sys/class/i2c-adapter/i2c-0", {st_mode=S_IFLNK|0777, st_size=0, ...}) = 0
readlink("/sys/class/i2c-adapter/i2c-0", "../../devices/pci0000:00/0000:00:1f.3/i2c-adapter/i2c-0", 256) = 55
readlink("/sys/devices/pci0000:00/0000:00:1f.3/i2c-adapter/i2c-0/subsystem", "../../../../../class/i2c-adapter", 256) = 32
lstat("/sys/class/i2c-adapter", {st_mode=S_IFDIR|0755, st_size=0, ...}) = 0
lstat("/sys/devices/pci0000:00/0000:00:1f.3/i2c-adapter/i2c-0/device", {st_mode=S_IFLNK|0777, st_size=0, ...}) = 0
readlink("/sys/devices/pci0000:00/0000:00:1f.3/i2c-adapter/i2c-0/device", "../../../../../devices/pci0000:00/0000:00:1f.3", 256) = 46
lstat("/sys/devices/pci0000:00/0000:00:1f.3", {st_mode=S_IFDIR|0755, st_size=0, ...}) = 0
lstat("/sys/devices/pci0000:00/0000:00:1f.3/bus", 0x7fff146427e0) = -1 ENOENT (No such file or directory)
lstat("/sys/devices/pci0000:00/0000:00:1f.3/driver", {st_mode=S_IFLNK|0777, st_size=0, ...}) = 0
readlink("/sys/devices/pci0000:00/0000:00:1f.3/driver", "../../../bus/pci/drivers/i801_smbus", 256) = 35
lstat("/sys/devices/pci0000:00/0000:00:1f.3/subsystem", {st_mode=S_IFLNK|0777, st_size=0, ...}) = 0
readlink("/sys/devices/pci0000:00/0000:00:1f.3/subsystem", "../../../bus/pci", 256) = 16
lstat("/sys/devices/pci0000:00/0000:00:1f.3/name", 0x7fff14642d60) = -1 ENOENT (No such file or directory)
ioctl(3, SNDCTL_TMR_TIMEBASE or TCGETS, 0x7fff14642ea0) = -1 ENOTTY (Inappropriate ioctl for device)
fstat(3, {st_mode=S_IFREG|0644, st_size=458, ...}) = 0
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x2ad196483000
read(3, "chip \"adt7470-*\"\n\tlabel fan1 \"CP"..., 8192) = 458
read(3, "", 4096)                       = 0
read(3, "", 8192)                       = 0
ioctl(3, SNDCTL_TMR_TIMEBASE or TCGETS, 0x7fff146417b0) = -1 ENOTTY (Inappropriate ioctl for device)
close(3)                                = 0
munmap(0x2ad196483000, 4096)            = 0
open("/usr/lib/gconv/gconv-modules.cache", O_RDONLY) = 3
fstat(3, {st_mode=S_IFREG|0644, st_size=25460, ...}) = 0
mmap(NULL, 25460, PROT_READ, MAP_SHARED, 3, 0) = 0x2ad196483000
close(3)                                = 0
open("/usr/lib/gconv/ISO8859-1.so", O_RDONLY) = 3
read(3, "\177ELF\2\1\1\0\0\0\0\0\0\0\0\0\3\0>\0\1\0\0\0\300\4\0"..., 832) = 832
fstat(3, {st_mode=S_IFREG|0644, st_size=10200, ...}) = 0
mmap(NULL, 2105392, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3, 0) = 0x2ad1970a7000
mprotect(0x2ad1970a9000, 2093056, PROT_NONE) = 0
mmap(0x2ad1972a8000, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x1000) = 0x2ad1972a8000
close(3)                                = 0
fstat(1, {st_mode=S_IFCHR|0620, st_rdev=makedev(136, 2), ...}) = 0
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x2ad1972aa000
write(1, "adt7470-i2c-0-2f\n", 17adt7470-i2c-0-2f
)      = 17
write(2, "Can\'t get adapter name for bus 0"..., 33Can't get adapter name for bus 0
) = 33
write(1, "\n", 1
)                       = 1
exit_group(0)                           = ?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] v3 of a adt7470 driver\

[Hans de Goede]

Darrick J. Wong wrote:
> On Thu, Jul 12, 2007 at 11:42:43AM +0200, Hans de Goede wrote:
>> Darrick J. Wong wrote:
>>> Resend with correct title and patch included just once.
>>> ---
>>> adt7470: new hwmon driver for adt7470 chip.
>>>
>>> Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
>> Looks good now, as far I'm concerned this is ready to get merged.
>> Acked-by: Hans de Goede <j.w.r.degoede@hhs.nl>
>>
>> Mark, any chance you can pick this up soonish, maybe it can make 2.6.23 
>> then.
>>
>> Darrick, have you already written support for this driver for libsensors 
>> and sensors? If not could you please do that ASAP? Then I'll review it and 
>> with some luck and Jean's permission we can still get support for this chip 
>> included into lm_sensors 2.10.4
> 
> I ... can't say that I have a working config.  I've added this into /etc/sensors.conf:
> chip "adt7470-*"
>         label fan1 "CPU0 Fan"
>         label fan2 "CPU1 Fan"
>         label temp1 "CPU0 Temp"
>         label temp3 "CPU1 temp"
>         label pwm1 "CPU0 Fan Control"
>         label pwm2 "CPU1 Fan Control"
> 
> (There are more pwm/fan/temp widgets but on the Z30 they're not
> connected to anything.)
> 
> But every time I run sensors (from svn) I see only this:
> root@elm3a181:/usr/local/src/lm_sensors/prog# sensors/sensors -c
> /tmp/sensors.conf 
> adt7470-i2c-0-2f
> Can't get adapter name for bus 0
> 

While the "Can't get adapter name for bus 0" message is strange, its also 
harmless. Its normal for sensors to not print anything for chips libsensors 
doesn't know, thats why I asked you to write libsensors support. See 
lib/chips.{c,h} for many many examples, just add an entry at the end for the 
adt7470. If you then recompile and install libsensors, sensors should give you 
some (ugly) output.

In order to get better output for sensors, you must also add support to sensors 
itself see prog/sensors/chips.{c,h} and don't forget to also add the printing 
routine to the table in prog/sensors/main.c once you are done writing it.

I assume this means sofar you've only been testing by catting sysfs files? In 
that case it is a good idea to test with the 3.0.0 branch of lm_sensors, which 
on longer needs support code for each chip, instead it "queries" sysfs to find 
out what features a chip has.

To try it do:
svn checkout http://lm-sensors.org/svn/lm-sensors/branches/lm-sensors-3.0.0
cd lm-sensors-3.0.0
make
sudo make install                          # will put it in /usr/local
sensors3

Notice that you can install and use sensors3 and the 2.10.x brnch in parallel.

Regards,

Hans


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] v3 of a adt7470 driver\

[Vadim Zeitlin]

Hans de Goede <j.w.r.degoede <at> hhs.nl> writes:

> I assume this means sofar you've only been testing by catting sysfs files? In 
> that case it is a good idea to test with the 3.0.0 branch of lm_sensors, which 
> on longer needs support code for each chip, instead it "queries" sysfs to find 
> out what features a chip has.

 Hello again,

 I've just tried to do this with rev 4608 and I was getting a lot of errors like
this:

libsensors error, more sensors of one type then MAX_SENSORS_PER_TYPE, ignoring
feature: temp32_alarm

and the AMB sensors 2 and 3 were not being taken into account. Without knowing
anything about libsensors I thought it was because the file names in
/sys/devices/platform/i5k_amb.0 were named {temp0,temp16,temp32,temp48}_xxx and
not temp[1234] as usual and so I tested this trivial patch:

--- i5k_amb.c.orig      2007-07-14 16:52:42.000000000 +0200
+++ i5k_amb.c   2007-07-14 16:54:36.000000000 +0200
@@ -219,5 +219,5 @@
 static int __devinit i5k_amb_hwmon_init(struct platform_device *pdev)
 {
-   int i, j, k;
+   int i, j, k, d = 0;
    u16 c;
    char *n;
@@ -250,7 +250,9 @@
                continue;

+           ++d;
+
            /* Temperature sysfs knob */
            n = kmalloc(13, GFP_KERNEL);
-           sprintf(n, "temp%d_input", k);
+           sprintf(n, "temp%d_input", d);
            data->attrs[data->num_attrs].dev_attr.attr.name = n;
            data->attrs[data->num_attrs].dev_attr.attr.mode @@ -267,5 +269,5 @@
            /* Temperature min sysfs knob */
            n = kmalloc(13, GFP_KERNEL);
-           sprintf(n, "temp%d_min", k);
+           sprintf(n, "temp%d_min", d);
            data->attrs[data->num_attrs].dev_attr.attr.name = n;
            data->attrs[data->num_attrs].dev_attr.attr.mode @@ -284,5 +286,5 @@
            /* Temperature mid sysfs knob */
            n = kmalloc(13, GFP_KERNEL);
-           sprintf(n, "temp%d_mid", k);
+           sprintf(n, "temp%d_mid", d);
            data->attrs[data->num_attrs].dev_attr.attr.name = n;
            data->attrs[data->num_attrs].dev_attr.attr.mode @@ -301,5 +303,5 @@
            /* Temperature max sysfs knob */
            n = kmalloc(13, GFP_KERNEL);
-           sprintf(n, "temp%d_max", k);
+           sprintf(n, "temp%d_max", d);
            data->attrs[data->num_attrs].dev_attr.attr.name = n;
            data->attrs[data->num_attrs].dev_attr.attr.mode @@ -318,5 +320,5 @@
            /* Temperature alarm sysfs knob */
            n = kmalloc(13, GFP_KERNEL);
-           sprintf(n, "temp%d_alarm", k);
+           sprintf(n, "temp%d_alarm", d);
            data->attrs[data->num_attrs].dev_attr.attr.name = n;
            data->attrs[data->num_attrs].dev_attr.attr.mode 
and now everything seems to work fine for me except for the 2 problems mentioned
in my previous message (long delay when accessing the sensors and manifestly
incorrect values for temp[256]):

% sensors3
adt7470-i2c-0-2e
Adapter: SMBus I801 adapter at 18c0
fan1:       2015 RPM  (min =    0 RPM)
fan2:          0 RPM  (min =    0 RPM)
fan3:       1704 RPM  (min =    0 RPM)
fan4:       1545 RPM  (min =    0 RPM)
temp1:        +1.0 C  (low  = +129.0 C, high = +127.0 C)
temp2:      +229.0 C  (low  = +129.0 C, high = +127.0 C)
temp3:       +59.0 C  (low  = +129.0 C, high = +127.0 C)
temp4:       +54.0 C  (low  = +129.0 C, high = +127.0 C)
temp5:      +227.0 C  (low  = +129.0 C, high = +127.0 C)
temp6:      +228.0 C  (low  = +129.0 C, high = +127.0 C)
temp7:       +59.0 C  (low  = +129.0 C, high = +127.0 C)
temp8:       +55.0 C  (low  = +129.0 C, high = +127.0 C)
temp9:       +50.0 C  (low  = +129.0 C, high = +127.0 C)
temp10:      +49.0 C  (low  = +129.0 C, high = +127.0 C)

i5k_amb-isa-0000
Adapter: ISA adapter
temp1:       +78.5 C  (low  = +106.0 C, high = +124.0 C)
temp2:       +75.0 C  (low  = +106.0 C, high = +124.0 C)
temp3:       +64.5 C  (low  = +106.0 C, high = +124.0 C)
temp4:       +63.5 C  (low  = +106.0 C, high = +124.0 C)

 I hope this information can be useful,
VZ


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] v3 of a adt7470 driver\

[Hans de Goede]

Vadim Zeitlin wrote:
> On Sat, 14 Jul 2007 18:19:22 +0200 Hans de Goede <j.w.r.degoede@hhs.nl> wrote:
> 
> HdG> > and the AMB sensors 2 and 3 were not being taken into account. Without knowing
> HdG> > anything about libsensors I thought it was because the file names in
> HdG> > /sys/devices/platform/i5k_amb.0 were named {temp0,temp16,temp32,temp48}_xxx and
> HdG> > not temp[1234] as usual
> HdG> 
> HdG> Yes, thats a rather severe violation of the hwmon sysfs standard. I do not see 
> HdG> this i5k_amb driver in the standard kernel, I assume its an addon driver?
> 
>  Hello Hans,
> 
>  This is the patch recently posted to the list:
> 
> 	http://article.gmane.org/gmane.linux.drivers.sensors/14004
> 
> sorry for not mentioning it (I forgot I was replying in a different thread
> from my reply in that one).
> 

Duh, I actually still have that mail in my Inbox, as I'm planning on reviewing 
it as time permits.

> HdG> > and now everything seems to work fine for me except for the 2
> HdG> > problems mentioned in my previous message (long delay when accessing
> HdG> > the sensors and manifestly incorrect values for temp[256]):
> HdG> 
> HdG> Does this long delay always happen? If you run sensors in 2 quick
> HdG> successions, the second call should be quite fast.
> 
>  No, the delay is always present and always the same.
> 

Hmm,

I see it refresh the readings every 2 seconds, since reading things takes 1 sec 
minimum, I think it would be a good idea to make this somewhat bigger. Darrick, 
can you post a new version and or an incremental patch with a slower read 
frequency, say once every 5 or 10 seconds?

Also I notice that you are still doing a full read of the chip to data->raw, 
this might very well explain some if the slowness. Please remove raw[] from 
data and the full read code from the update() function.

Thanks & Regards,

Hans

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] v3 of a adt7470 driver\

[Jean Delvare]

Hi Hans, hi Darrick,

On Fri, 13 Jul 2007 14:49:12 +0200, Hans de Goede wrote:
> Darrick J. Wong wrote:
> > But every time I run sensors (from svn) I see only this:
> > root@elm3a181:/usr/local/src/lm_sensors/prog# sensors/sensors -c
> > /tmp/sensors.conf 
> > adt7470-i2c-0-2f
> > Can't get adapter name for bus 0
> 
> While the "Can't get adapter name for bus 0" message is strange, its also 
> harmless.

I confirm it is harmless. Typically, it happens when you are using
lm-sensors <= 2.10.2, a kernel >= 2.6.22, and you did not enable
CONFIG_SYSFS_DEPRECATED in your kernel. So the fix is to either upgrade
to lm-sensors 2.10.3 or later, or rebuild your kernel with
CONFIG_SYSFS_DEPRECATED=y.

This is explained on this page for reference:
http://www.lm-sensors.org/wiki/Kernel2.6

Now Darrick says he has the problem with lm-sensors SVN, which is more
surprising. Maybe sensors is linking with an older version of
libsensors present on the system. This theory can be confirmed by
running "sensors -v".

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] v3 of a adt7470 driver\

[Darrick J. Wong]

[-- Attachment #1.1: Type: text/plain, Size: 1030 bytes --]

On Sat, Jul 14, 2007 at 07:22:41PM +0200, Hans de Goede wrote:
> Hmm,
> 
> I see it refresh the readings every 2 seconds, since reading things takes 1 
> sec minimum, I think it would be a good idea to make this somewhat bigger. 
> Darrick, can you post a new version and or an incremental patch with a 
> slower read frequency, say once every 5 or 10 seconds?

Will do.  The data sheet says you have to wait 200ms *
number_of_temperature_sensor_chips.  Unfortunately, there's no way to find
out how many sensor chips there are before you read them, but 1s seemed to
work ok for all my systems.  I suppose I could modify the init function
to do the read once with the long delay, then lower it to 200ms *
however many temperature sensors read a "sane" value.

> Also I notice that you are still doing a full read of the chip to 
> data->raw, this might very well explain some if the slowness. Please remove 
> raw[] from data and the full read code from the update() function.

That was pulled from v3.

--D

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] v3 of a adt7470 driver\

[Darrick J. Wong]

[-- Attachment #1.1: Type: text/plain, Size: 371 bytes --]

On Sat, Jul 14, 2007 at 10:00:27PM +0200, Jean Delvare wrote:

> Now Darrick says he has the problem with lm-sensors SVN, which is more
> surprising. Maybe sensors is linking with an older version of
> libsensors present on the system. This theory can be confirmed by
> running "sensors -v".

Yep, I'd linked it with an old version.  2.10.3 and 3.0.0 are both fine.

--D

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] v3 of a adt7470 driver\

[Hans de Goede]

Darrick J. Wong wrote:
> On Sat, Jul 14, 2007 at 07:22:41PM +0200, Hans de Goede wrote:
>> Hmm,
>>
>> I see it refresh the readings every 2 seconds, since reading things takes 1 
>> sec minimum, I think it would be a good idea to make this somewhat bigger. 
>> Darrick, can you post a new version and or an incremental patch with a 
>> slower read frequency, say once every 5 or 10 seconds?
> 
> Will do.  The data sheet says you have to wait 200ms *
> number_of_temperature_sensor_chips.  Unfortunately, there's no way to find
> out how many sensor chips there are before you read them, but 1s seemed to
> work ok for all my systems.  I suppose I could modify the init function
> to do the read once with the long delay, then lower it to 200ms *
> however many temperature sensors read a "sane" value.
> 

I think this will be rather tricky, its esp tricky to define what is a sane value.

How about the following instead:
-convert TEMP_COLLECTION_TIME to jiffies
-add a started_reading member to data
-driver initializes
-sets start reading temp sensors bit
-set started_reading to jiffies
-in update_device:
  if (time_before(jiffies, data->last_updated + REFRESH_INTERVAL)
	&& data->valid)
	goto out;

  /* cache jiffies to make sure it doesn't change, possible causing an unwanted
     overflow in the substraction when calculating how long to sleep */
  unsigned long current_jiffies = jiffies;
  if (time_before(current_jiffies, data->started_reading +
		TEMP_COLLECTION_TIME))
	msleep(jiffies_to_msecs((data->started_reading + TEMP_COLLECTION_TIME) 			- 
current_jiffies));

  /* done reading temperature sensors */
  ...

  /* read registers */
  ...


  /* start reading temperature sensors */
  ...

  data->started_reading = jiffies;

  ...

  "exit update_device"

-Notice that the msleep, now will only be triggered if update_device gets
  called within TEMP_COLLECTION_TIME of driver loading, assuming that
  REFRESH_INTERVAL > TEMP_COLLECTION_TIME. I did it this way hoping that the
  startup scripts of the PC will have enough time between insmod and the first
  read, thus not delaying PC startup.


---

In a releated note, depending on where the 0ther 0.7 seconds are, you could try 
todo less reading, for example each time you start / stop the temp reading you 
first read the cfg register, why not just use a cached value, is there a reason 
to assumee it will change? Esp when stopping I would be using the value already 
read when starting (in the current code).

Also you read the limits and other non sensor reading registers each update, 
why not just read these once at driver initialisation, is there a reason to 
assume they will change?

Regards,

Hans


p.s.

I saw your longer read intervals and make temps signed patches, since the 
driver hasn't been merged yet, maybe its a good idea todo a v4 ?

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] v3 of a adt7470 driver\

[Darrick J. Wong]

[-- Attachment #1.1: Type: text/plain, Size: 2704 bytes --]

On Tue, Jul 17, 2007 at 07:13:52AM +0200, Hans de Goede wrote:
> I think this will be rather tricky, its esp tricky to define what is a sane 
> value.
<snip>
> -Notice that the msleep, now will only be triggered if update_device gets
>  called within TEMP_COLLECTION_TIME of driver loading, assuming that
>  REFRESH_INTERVAL > TEMP_COLLECTION_TIME. I did it this way hoping that the
>  startup scripts of the PC will have enough time between insmod and the 
>  first
>  read, thus not delaying PC startup.

As I understand your pseudocode, you want to start the temperature
reading at the end of a call to update_device and init, and pick up
the temp values the next time update_device gets called, sleeping as
necessary.  I've a few questions about this approach:

(1) Each sysfs attribute calls update_device before calling sprintf
to write the sysfs attribute.  If a user decides to read all sysfs
attributes at once (for example, via that grep trick), won't this
cause a new round of temperature sensor reading every time a sysfs
attribute is read, as well as a sleep for the next sysfs attribute read?

(2) Let's say a user reads a sensor and causes the temperature sensors
to start reading.  Then, the user goes to sleep for an hour, and reads
the temperature sensor via sysfs.  Since the last temperature sensor
refresh was an hour previously, won't this cause the user to see
temperature readings that are an hour old?

(3) If the startup scripts load the driver and immediately begin
querying the sensor readings, the scripts will take the msleep hit
anyway.

> In a releated note, depending on where the 0ther 0.7 seconds are, you could 
> try todo less reading, for example each time you start / stop the temp 
> reading you first read the cfg register, why not just use a cached value, 
> is there a reason to assumee it will change? Esp when stopping I would be 
> using the value already read when starting (in the current code).

I suspect that would work fine.  It looks like the bits in the config
register only get changed by the driver, so I'd only need to read it
once.

> Also you read the limits and other non sensor reading registers each 
> update, why not just read these once at driver initialisation, is there a 
> reason to assume they will change?

Nope.  I can make these two changes.

> I saw your longer read intervals and make temps signed patches, since the 
> driver hasn't been merged yet, maybe its a good idea todo a v4 ?

Not hard, though I'll have to collapse the two subsequent incremental
patches.  It'd be less work to just make one more incremental patch and
send the entire patchset to lm-sensors.

--D

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] v3 of a adt7470 driver\

[Hans de Goede]

Darrick J. Wong wrote:
> On Tue, Jul 17, 2007 at 07:13:52AM +0200, Hans de Goede wrote:
>> I think this will be rather tricky, its esp tricky to define what is a sane 
>> value.
> <snip>
>> -Notice that the msleep, now will only be triggered if update_device gets
>>  called within TEMP_COLLECTION_TIME of driver loading, assuming that
>>  REFRESH_INTERVAL > TEMP_COLLECTION_TIME. I did it this way hoping that the
>>  startup scripts of the PC will have enough time between insmod and the 
>>  first
>>  read, thus not delaying PC startup.
> 
> As I understand your pseudocode, you want to start the temperature
> reading at the end of a call to update_device and init, and pick up
> the temp values the next time update_device gets called, sleeping as
> necessary.  I've a few questions about this approach:
> 

Correct, as said notice that the sleep will only be needed if an attribute gets 
read within TEMP_COLLECTION_TIME of the driver load, see below for more on this.

> (1) Each sysfs attribute calls update_device before calling sprintf
> to write the sysfs attribute.  If a user decides to read all sysfs
> attributes at once (for example, via that grep trick), won't this
> cause a new round of temperature sensor reading every time a sysfs
> attribute is read, as well as a sleep for the next sysfs attribute read?
> 

No, the regular do we need to update, iow is jiffies > (last_update + 
REFRESH_INTERVAL) check will be done first, and only when that succeeds the 
check to see if we still need to sleep for TEMP_COLLECTION_TIME to complete 
will be done, this is why I also wrote:

 >>  Notice that the msleep, now will only be triggered if update_device gets
 >>  called within TEMP_COLLECTION_TIME of driver loading, assuming that
 >>  REFRESH_INTERVAL > TEMP_COLLECTION_TIME.

The whole do we need to sleep because the  TEMP_COLLECTION_TIME hasn't passed 
really is only there for when an attribute gets read immediately after driver 
init. As said I've written the pseudo code like this, so that driver init 
itself doesn't need to sleep, hoping that the attr reading will happen later 
and that thus Pc-booting isn't delayed.

> (2) Let's say a user reads a sensor and causes the temperature sensors
> to start reading.  Then, the user goes to sleep for an hour, and reads
> the temperature sensor via sysfs.  Since the last temperature sensor
> refresh was an hour previously, won't this cause the user to see
> temperature readings that are an hour old?
> 

I don't know, possibly yes. That would be a problem, you could add some logic 
to redo the temp reading if its older then say a minute. The problem with the 
current code is that regular non threaded sensors apps like ksensors, gkrellm 
and gnome-applet-sensors will block for an extended amount of time now.

Maybe the best way would be to use a timer to driver the device_update, 
avoiding your hour old readings scenario that way.

> (3) If the startup scripts load the driver and immediately begin
> querying the sensor readings, the scripts will take the msleep hit
> anyway.
> 

It will, but only on the first query, after that there will be no more sleeping.

>> In a releated note, depending on where the 0ther 0.7 seconds are, you could 
>> try todo less reading, for example each time you start / stop the temp 
>> reading you first read the cfg register, why not just use a cached value, 
>> is there a reason to assumee it will change? Esp when stopping I would be 
>> using the value already read when starting (in the current code).
> 
> I suspect that would work fine.  It looks like the bits in the config
> register only get changed by the driver, so I'd only need to read it
> once.
> 
>> Also you read the limits and other non sensor reading registers each 
>> update, why not just read these once at driver initialisation, is there a 
>> reason to assume they will change?
> 
> Nope.  I can make these two changes.
> 
>> I saw your longer read intervals and make temps signed patches, since the 
>> driver hasn't been merged yet, maybe its a good idea todo a v4 ?
> 
> Not hard, though I'll have to collapse the two subsequent incremental
> patches.  It'd be less work to just make one more incremental patch and
> send the entire patchset to lm-sensors.
> 

Never mind about this I've already requested Mark to pick up the v3 patch + the 
2 fixes.

Jean, if you are reading this what do you think about this whole sleep for a 
sesond in device_update stuff and possible workarounds?

Regards,

Hans


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] v3 of a adt7470 driver\

[Darrick J. Wong]

[-- Attachment #1.1: Type: text/plain, Size: 799 bytes --]

On Tue, Jul 24, 2007 at 08:56:51PM +0200, Hans de Goede wrote:

> Never mind about this I've already requested Mark to pick up the v3 patch + 
> the 2 fixes.
> 
> Jean, if you are reading this what do you think about this whole sleep for 
> a sesond in device_update stuff and possible workarounds?

I could modify the driver to update the sensor values periodically with
a timer, though the msleep forces me to push the actual work of reading
the sensor values into a workqueue.  This has the effect of making sysfs
reads nearly instantaneous though I don't know if sleeping for a second
in the system workqueue is agreeable.  It might be useful to create a
separate workqueue for slow sensor chip reads.

Ugh, the patch is buggy, so I'll have to hack on it a little longer.

--D

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors