Skip to contentSkip to conversation

f75383 /f75384 driver

PATCH New Brian Beardall 2 participants 6 messages Aug 19, 2006 – Aug 20, 2006

lore
Share & tools
lore

Brian Beardall

author
#0 lore raw

PATCH 1/1 f75383 /f75384 driver

This is the patch for the f75383/4 driver. The f75383.diff applies to
the kernel, and the lm_sensors-f75383.diff applies to lm_sensors. I
would just like to have the code reviewed so that changes can be made.
Hopefully within this week because school starts again on Aug. 28th, and
that will make the schedule tight for hacking on this driver. Both
patches are -p1 patches. If you have an ECS mainboard that is only about
2 years old it will most likely have this temperature sensor chip.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: f75383.diff
Type: text/x-patch
Size: 19618 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060819/73f994fa/attachment-0002.bin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lm_sensors-f75383.diff
Type: text/x-patch
Size: 26731 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060819/73f994fa/attachment-0003.bin

#1 lore raw

PATCH 1/1 f75383 /f75384 driver

Quoted 9 lines · Brian Beardall

This is the patch for the f75383/4 driver. The f75383.diff applies to the kernel, and the lm_sensors-f75383.diff applies to lm_sensors. I would just like to have the code reviewed so that changes can be made. Hopefully within this week because school starts again on Aug. 28th, and that will make the schedule tight for hacking on this driver. Both patches are -p1 patches. If you have an ECS mainboard that is only about 2 years old it will most likely have this temperature sensor chip.

These and other repeated callbacks can be folded down to 1 copy - theyre already extracting the index, which you can pre-initialize below..

Quoted 7 lines
(quoted diff) +6 -0
+
+static ssize_t show_temp1(struct device *dev, struct device_attribute *devattr,
+			   char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct f75383_data *data = f75383_update_device(dev);

Review

Quoted 10 lines
(quoted diff) +10 -0
+	return sprintf(buf, "%d\n", TEMP_FROM_REG(data->temp1[attr->index]));
+}
+
+static ssize_t show_temp2(struct device *dev, struct device_attribute *devattr,
+			   char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct f75383_data *data = f75383_update_device(dev);
+	return sprintf(buf, "%d\n", TEMP_FROM_REG(data->temp2[attr->index]));
+}

Youre already taking care of indexing in declarations too !

Review

Quoted 13 lines
(quoted diff) +9 -0
+static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp1, NULL, 0);
+static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_temp1,
+	set_temp1, 1);
+static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp1,
+	set_temp1, 2);
 
 
 
+static SENSOR_DEVICE_ATTR(temp2_min, S_IWUSR | S_IRUGO, show_temp2,
+	set_temp2, 1);
+static SENSOR_DEVICE_ATTR(temp2_max, S_IWUSR | S_IRUGO, show_temp2,
+	set_temp2, 2);

Unless Ive missed something, above should be following: dropped 1,2 on
function-names

Review

Quoted 5 lines
(quoted diff) +4 -0
+static SENSOR_DEVICE_ATTR(temp2_min, S_IWUSR | S_IRUGO, show_temp,
+	set_temp, 1);
+static SENSOR_DEVICE_ATTR(temp2_max, S_IWUSR | S_IRUGO, show_temp,
+	set_temp, 2);

Maybe I'll be able to take another look later..

hth
jimc

author
#2 lore raw

PATCH 1/1 f75383 /f75384 driver

In reply to Brian Beardall

I've been thinking about folding all of the duplicate functions. There are quite a few because both temperature sensors behave exactly the same, and so they can be handled with one code execution line. Is there a dev in lm_sensors to allow the change in read frequencies?

Quoted 64 lines · Jim Cromie
Quoted 9 lines · Brian Beardall

This is the patch for the f75383/4 driver. The f75383.diff applies to the kernel, and the lm_sensors-f75383.diff applies to lm_sensors. I would just like to have the code reviewed so that changes can be made. Hopefully within this week because school starts again on Aug. 28th, and that will make the schedule tight for hacking on this driver. Both patches are -p1 patches. If you have an ECS mainboard that is only about 2 years old it will most likely have this temperature sensor chip.

These and other repeated callbacks can be folded down to 1 copy - theyre already extracting the index, which you can pre-initialize below..

Quoted 7 lines
(quoted diff) +6 -0
+
+static ssize_t show_temp1(struct device *dev, struct device_attribute *devattr,
+			   char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct f75383_data *data = f75383_update_device(dev);

Review

Quoted 10 lines
(quoted diff) +10 -0
+	return sprintf(buf, "%d\n", TEMP_FROM_REG(data->temp1[attr->index]));
+}
+
+static ssize_t show_temp2(struct device *dev, struct device_attribute *devattr,
+			   char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct f75383_data *data = f75383_update_device(dev);
+	return sprintf(buf, "%d\n", TEMP_FROM_REG(data->temp2[attr->index]));
+}

Youre already taking care of indexing in declarations too !

Review

Quoted 13 lines
(quoted diff) +9 -0
+static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp1, NULL, 0);
+static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_temp1,
+	set_temp1, 1);
+static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp1,
+	set_temp1, 2);
 
 
 
+static SENSOR_DEVICE_ATTR(temp2_min, S_IWUSR | S_IRUGO, show_temp2,
+	set_temp2, 1);
+static SENSOR_DEVICE_ATTR(temp2_max, S_IWUSR | S_IRUGO, show_temp2,
+	set_temp2, 2);

Unless Ive missed something, above should be following: dropped 1,2 on
function-names

Review

Quoted 5 lines
(quoted diff) +4 -0
+static SENSOR_DEVICE_ATTR(temp2_min, S_IWUSR | S_IRUGO, show_temp,
+	set_temp, 1);
+static SENSOR_DEVICE_ATTR(temp2_max, S_IWUSR | S_IRUGO, show_temp,
+	set_temp, 2);

Maybe I'll be able to take another look later..

hth
jimc

#3 lore raw

PATCH 1/1 f75383 /f75384 driver

In reply to Brian Beardall
Quoted 3 lines · Brian Beardall

I've been thinking about folding all of the duplicate functions. There are quite a few because both temperature sensors behave exactly the same,

ok - that confims my casual observation.

Quoted 3 lines

and so they can be handled with one code execution line. Is there a dev in lm_sensors to allow the change in read frequencies?

Im not sure what you mean here.
Do you want some sensor reads to fetch *fresher* data than others ?

Quoted 2 lines
Quoted 2 lines

Review

Quoted 2 lines
(quoted diff) +1 -0
+	struct f75383_data *data = f75383_update_device(dev);

you could add a 2nd param to your update-device, allowing caller to indicate desired freshness.
or you could carve update_device for separate sensors, and fetch each separately.

Quoted 6 lines
Quoted 3 lines

hth
jimc

author
#4 lore raw

PATCH 1/1 f75383 /f75384 driver

In reply to Brian Beardall
Quoted 13 lines · Jim Cromie
Quoted 3 lines · Brian Beardall

I've been thinking about folding all of the duplicate functions. There are quite a few because both temperature sensors behave exactly the same,

ok - that confims my casual observation.

Quoted 3 lines

and so they can be handled with one code execution line. Is there a dev in lm_sensors to allow the change in read frequencies?

Im not sure what you mean here.
Do you want some sensor reads to fetch *fresher* data than others ?

Quoted 7 lines
Quoted 2 lines
Quoted 2 lines

Review

Quoted 2 lines
(quoted diff) +1 -0
+	struct f75383_data *data = f75383_update_device(dev);

you could add a 2nd param to your update-device, allowing caller to indicate desired freshness.
or you could carve update_device for separate sensors, and fetch each

separately.

The entire device reads the temperature sensors depending on a a register value. So I can make in read at different Hz, or I can also make it only update when a call is made to the device. However I have it setup for 15Hz update right now.

Quoted 8 lines
Quoted 8 lines
Quoted 5 lines

hth
jimc

#5 lore raw

PATCH 1/1 f75383 /f75384 driver

In reply to Brian Beardall
Quoted 20 lines · Brian Beardall
Quoted 18 lines · Jim Cromie
Quoted 4 lines · Brian Beardall

and so they can be handled with one code execution line. Is there a dev in lm_sensors to allow the change in read frequencies?

Im not sure what you mean here.
Do you want some sensor reads to fetch *fresher* data than others ?

Quoted 3 lines
Quoted 3 lines

Review

Quoted 3 lines
(quoted diff) +1 -0
+	struct f75383_data *data = f75383_update_device(dev);

you could add a 2nd param to your update-device, allowing caller to indicate desired freshness.
or you could carve update_device for separate sensors, and fetch each

Quoted 7 lines
Quoted 2 lines

separately.

The entire device reads the temperature sensors depending on a a register value. So I can make in read at different Hz, or I can also make it only update when a call is made to the device. However I have it setup for 15Hz update right now.

I still dont get what youre after - theres no question here.

IIUC, the standard usage of lm-sensor kernel modules is to support user-demanded queries,
such as those done by a user-space daemon.

In this context, why does your driver have/need 15hz anything
- is it 'scanning' its own inputs, looking for out-of-range values ?

It seems easy to expose the Freq as a mod-param,
which is then controllable at runtime via /proc/sys/<something> (IIRC) but Im unclear on what its for.

Quoted 10 lines
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines

hth
jimc