[lm-sensors] [PATCH] f75383 /f75384 driver

[lm-sensors] [PATCH] f75383 /f75384 driver

[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.
-------------- 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 

[lm-sensors] [PATCH] f75383 /f75384 driver

[Jim Cromie]

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

[lm-sensors] [PATCH] f75383 /f75384 driver

[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?

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

[lm-sensors] [PATCH] f75383 /f75384 driver

[Jim Cromie]

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
>>     
>
>
>   

[lm-sensors] [PATCH] f75383 /f75384 driver

[Brian Beardall]

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
> >>     
> >
> >
> >   

[lm-sensors] [PATCH] f75383 /f75384 driver

[Jim Cromie]

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
>>>>     
>>>>         
>>>   
>>>       
>
>
>