* [lm-sensors] [PATCH] f75383 /f75384 driver
2006-08-19 14:20 [lm-sensors] [PATCH] f75383 /f75384 driver Brian Beardall
@ 2006-08-19 15:04 ` Jim Cromie
2006-08-19 20:02 ` Brian Beardall
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Jim Cromie @ 2006-08-19 15:04 UTC (permalink / raw)
To: lm-sensors
Brian Beardall wrote:
> 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..
> +
> +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);
> + 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 !
> +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
> +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
^ permalink raw reply [flat|nested] 6+ messages in thread* [lm-sensors] [PATCH] f75383 /f75384 driver
2006-08-19 14:20 [lm-sensors] [PATCH] f75383 /f75384 driver Brian Beardall
2006-08-19 15:04 ` Jim Cromie
@ 2006-08-19 20:02 ` Brian Beardall
2006-08-19 20:49 ` Jim Cromie
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Brian Beardall @ 2006-08-19 20:02 UTC (permalink / raw)
To: lm-sensors
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?
On Sat, 2006-08-19 at 09:04 -0600, Jim Cromie wrote:
> Brian Beardall wrote:
> > 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..
>
> > +
> > +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);
> > + 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 !
>
> > +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
>
>
> > +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
^ permalink raw reply [flat|nested] 6+ messages in thread* [lm-sensors] [PATCH] f75383 /f75384 driver
2006-08-19 14:20 [lm-sensors] [PATCH] f75383 /f75384 driver Brian Beardall
2006-08-19 15:04 ` Jim Cromie
2006-08-19 20:02 ` Brian Beardall
@ 2006-08-19 20:49 ` Jim Cromie
2006-08-20 1:27 ` Brian Beardall
2006-08-20 18:16 ` Jim Cromie
4 siblings, 0 replies; 6+ messages in thread
From: Jim Cromie @ 2006-08-19 20:49 UTC (permalink / raw)
To: lm-sensors
Brian Beardall wrote:
> 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.
> 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 ?
>>> + 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.
>> hth
>> jimc
>>
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [lm-sensors] [PATCH] f75383 /f75384 driver
2006-08-19 14:20 [lm-sensors] [PATCH] f75383 /f75384 driver Brian Beardall
` (2 preceding siblings ...)
2006-08-19 20:49 ` Jim Cromie
@ 2006-08-20 1:27 ` Brian Beardall
2006-08-20 18:16 ` Jim Cromie
4 siblings, 0 replies; 6+ messages in thread
From: Brian Beardall @ 2006-08-20 1:27 UTC (permalink / raw)
To: lm-sensors
On Sat, 2006-08-19 at 14:49 -0600, Jim Cromie wrote:
> Brian Beardall wrote:
> > 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.
>
> > 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 ?
>
> >>> + 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.
>
>
> >> hth
> >> jimc
> >>
> >
> >
> >
^ permalink raw reply [flat|nested] 6+ messages in thread* [lm-sensors] [PATCH] f75383 /f75384 driver
2006-08-19 14:20 [lm-sensors] [PATCH] f75383 /f75384 driver Brian Beardall
` (3 preceding siblings ...)
2006-08-20 1:27 ` Brian Beardall
@ 2006-08-20 18:16 ` Jim Cromie
4 siblings, 0 replies; 6+ messages in thread
From: Jim Cromie @ 2006-08-20 18:16 UTC (permalink / raw)
To: lm-sensors
Brian Beardall wrote:
> On Sat, 2006-08-19 at 14:49 -0600, Jim Cromie wrote:
>
>> Brian Beardall wrote:
>>
>>
>>> 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 ?
>>
>>
>>>>> + 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.
>
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.
>>
>>>> hth
>>>> jimc
>>>>
>>>>
>>>
>>>
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread