Re: [PATCH 0/5] I8K driver facelift
On Wed, Apr 13, 2005 at 01:33:31AM -0500, Dmitry Torokhov wrote:
> On Thursday 24 March 2005 02:24, Greg KH wrote:
> > On Thu, Mar 17, 2005 at 03:16:24AM -0500, [EMAIL PROTECTED] wrote:
> > > On Wed, 16 Mar 2005 14:38:50 MST, Frank Sorenson said:
> > > > Okay, I replaced the sysfs_ops with ops of my own, and now all the show
> > > > and store functions also accept the name of the attribute as a
> > > > parameter.
> > > > This lets the functions know what attribute is being accessed, and
> > > > allows
> > > > us to create attributes that share show and store functions, so things
> > > > don't need to be defined at compile time (I feel slightly evil!).
> > > >
> > > > This patch puts the correct number of temp sensors and fans into sysfs,
> > > > and only exposes power_status if enabled by the power_status module
> > > > parameter.
> > >
> > > Works for me:
> > >
> > > [/sys/bus/platform/drivers/i8k/i8k]2 ls -l
> > > total 0
> > > lrwxrwxrwx 1 root root0 Mar 17 03:02 bus -> ../../../bus/platform
> > > -r--r--r-- 1 root root 4096 Mar 17 03:02 cpu_temp
> > > -rw-r--r-- 1 root root 4096 Mar 17 03:01 detach_state
> > > lrwxrwxrwx 1 root root0 Mar 17 03:02 driver ->
> > > ../../../bus/platform/drivers/i8k
> > > -r--r--r-- 1 root root 4096 Mar 17 03:02 fan1_speed
> > > -rw-r--r-- 1 root root 4096 Mar 17 03:02 fan1_state
> > > -r--r--r-- 1 root root 4096 Mar 17 03:02 fan2_speed
> > > -rw-r--r-- 1 root root 4096 Mar 17 03:02 fan2_state
> > > drwxr-xr-x 2 root root0 Mar 17 03:02 power
> > > -r--r--r-- 1 root root 4096 Mar 17 03:02 power_status
> > > -r--r--r-- 1 root root 4096 Mar 17 03:02 temp1
> > > -r--r--r-- 1 root root 4096 Mar 17 03:02 temp2
> > >
> > > The valyes of the fan* settings, and cpu_temp match what's reported in
> > > /proc/i8k.
> >
> > Please match the same units and filename as the other i2c sensors. See
> > the documentation in the Documentation/i2c/ directory for what that
> > standard is, so userspace programs will "just work" with your devices.
> >
>
> Greg,
>
> I almost started doing what you just said but then I realized that none of
> the programs will "just work" because all of them will look into /sys/bus/i2c
> instead of /sys/bus/platform/i8k.
>
> For userspace tools to work transparently we would need something like
> /sys/class/sensor/{fan|temp|current}, but it is something I am not ready to do
> now - I need to finish input layer first.
The sensors developers are creating just such a class, see their hwmon
patch in their email archive if you are curious.
> So given above I think having private scheme for now is ok. Sooo... Can I get
> my attributes goups patch in so I can use it in i8k, please?
Ick, no, use the upcoming hwmon stuff instead :)
thanks,
greg k-h
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] I8K driver facelift
On Thursday 24 March 2005 02:24, Greg KH wrote:
> On Thu, Mar 17, 2005 at 03:16:24AM -0500, [EMAIL PROTECTED] wrote:
> > On Wed, 16 Mar 2005 14:38:50 MST, Frank Sorenson said:
> > > Okay, I replaced the sysfs_ops with ops of my own, and now all the show
> > > and store functions also accept the name of the attribute as a parameter.
> > > This lets the functions know what attribute is being accessed, and allows
> > > us to create attributes that share show and store functions, so things
> > > don't need to be defined at compile time (I feel slightly evil!).
> > >
> > > This patch puts the correct number of temp sensors and fans into sysfs,
> > > and only exposes power_status if enabled by the power_status module
> > > parameter.
> >
> > Works for me:
> >
> > [/sys/bus/platform/drivers/i8k/i8k]2 ls -l
> > total 0
> > lrwxrwxrwx 1 root root0 Mar 17 03:02 bus -> ../../../bus/platform
> > -r--r--r-- 1 root root 4096 Mar 17 03:02 cpu_temp
> > -rw-r--r-- 1 root root 4096 Mar 17 03:01 detach_state
> > lrwxrwxrwx 1 root root0 Mar 17 03:02 driver ->
> > ../../../bus/platform/drivers/i8k
> > -r--r--r-- 1 root root 4096 Mar 17 03:02 fan1_speed
> > -rw-r--r-- 1 root root 4096 Mar 17 03:02 fan1_state
> > -r--r--r-- 1 root root 4096 Mar 17 03:02 fan2_speed
> > -rw-r--r-- 1 root root 4096 Mar 17 03:02 fan2_state
> > drwxr-xr-x 2 root root0 Mar 17 03:02 power
> > -r--r--r-- 1 root root 4096 Mar 17 03:02 power_status
> > -r--r--r-- 1 root root 4096 Mar 17 03:02 temp1
> > -r--r--r-- 1 root root 4096 Mar 17 03:02 temp2
> >
> > The valyes of the fan* settings, and cpu_temp match what's reported in
> > /proc/i8k.
>
> Please match the same units and filename as the other i2c sensors. See
> the documentation in the Documentation/i2c/ directory for what that
> standard is, so userspace programs will "just work" with your devices.
>
Greg,
I almost started doing what you just said but then I realized that none of
the programs will "just work" because all of them will look into /sys/bus/i2c
instead of /sys/bus/platform/i8k.
For userspace tools to work transparently we would need something like
/sys/class/sensor/{fan|temp|current}, but it is something I am not ready to do
now - I need to finish input layer first.
So given above I think having private scheme for now is ok. Sooo... Can I get
my attributes goups patch in so I can use it in i8k, please?
Thanks!
--
Dmitry
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] I8K driver facelift
On Thursday 24 March 2005 02:24, Greg KH wrote:
On Thu, Mar 17, 2005 at 03:16:24AM -0500, [EMAIL PROTECTED] wrote:
On Wed, 16 Mar 2005 14:38:50 MST, Frank Sorenson said:
Okay, I replaced the sysfs_ops with ops of my own, and now all the show
and store functions also accept the name of the attribute as a parameter.
This lets the functions know what attribute is being accessed, and allows
us to create attributes that share show and store functions, so things
don't need to be defined at compile time (I feel slightly evil!).
This patch puts the correct number of temp sensors and fans into sysfs,
and only exposes power_status if enabled by the power_status module
parameter.
Works for me:
[/sys/bus/platform/drivers/i8k/i8k]2 ls -l
total 0
lrwxrwxrwx 1 root root0 Mar 17 03:02 bus - ../../../bus/platform
-r--r--r-- 1 root root 4096 Mar 17 03:02 cpu_temp
-rw-r--r-- 1 root root 4096 Mar 17 03:01 detach_state
lrwxrwxrwx 1 root root0 Mar 17 03:02 driver -
../../../bus/platform/drivers/i8k
-r--r--r-- 1 root root 4096 Mar 17 03:02 fan1_speed
-rw-r--r-- 1 root root 4096 Mar 17 03:02 fan1_state
-r--r--r-- 1 root root 4096 Mar 17 03:02 fan2_speed
-rw-r--r-- 1 root root 4096 Mar 17 03:02 fan2_state
drwxr-xr-x 2 root root0 Mar 17 03:02 power
-r--r--r-- 1 root root 4096 Mar 17 03:02 power_status
-r--r--r-- 1 root root 4096 Mar 17 03:02 temp1
-r--r--r-- 1 root root 4096 Mar 17 03:02 temp2
The valyes of the fan* settings, and cpu_temp match what's reported in
/proc/i8k.
Please match the same units and filename as the other i2c sensors. See
the documentation in the Documentation/i2c/ directory for what that
standard is, so userspace programs will just work with your devices.
Greg,
I almost started doing what you just said but then I realized that none of
the programs will just work because all of them will look into /sys/bus/i2c
instead of /sys/bus/platform/i8k.
For userspace tools to work transparently we would need something like
/sys/class/sensor/{fan|temp|current}, but it is something I am not ready to do
now - I need to finish input layer first.
So given above I think having private scheme for now is ok. Sooo... Can I get
my attributes goups patch in so I can use it in i8k, please?
Thanks!
--
Dmitry
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] I8K driver facelift
On Wed, Apr 13, 2005 at 01:33:31AM -0500, Dmitry Torokhov wrote:
On Thursday 24 March 2005 02:24, Greg KH wrote:
On Thu, Mar 17, 2005 at 03:16:24AM -0500, [EMAIL PROTECTED] wrote:
On Wed, 16 Mar 2005 14:38:50 MST, Frank Sorenson said:
Okay, I replaced the sysfs_ops with ops of my own, and now all the show
and store functions also accept the name of the attribute as a
parameter.
This lets the functions know what attribute is being accessed, and
allows
us to create attributes that share show and store functions, so things
don't need to be defined at compile time (I feel slightly evil!).
This patch puts the correct number of temp sensors and fans into sysfs,
and only exposes power_status if enabled by the power_status module
parameter.
Works for me:
[/sys/bus/platform/drivers/i8k/i8k]2 ls -l
total 0
lrwxrwxrwx 1 root root0 Mar 17 03:02 bus - ../../../bus/platform
-r--r--r-- 1 root root 4096 Mar 17 03:02 cpu_temp
-rw-r--r-- 1 root root 4096 Mar 17 03:01 detach_state
lrwxrwxrwx 1 root root0 Mar 17 03:02 driver -
../../../bus/platform/drivers/i8k
-r--r--r-- 1 root root 4096 Mar 17 03:02 fan1_speed
-rw-r--r-- 1 root root 4096 Mar 17 03:02 fan1_state
-r--r--r-- 1 root root 4096 Mar 17 03:02 fan2_speed
-rw-r--r-- 1 root root 4096 Mar 17 03:02 fan2_state
drwxr-xr-x 2 root root0 Mar 17 03:02 power
-r--r--r-- 1 root root 4096 Mar 17 03:02 power_status
-r--r--r-- 1 root root 4096 Mar 17 03:02 temp1
-r--r--r-- 1 root root 4096 Mar 17 03:02 temp2
The valyes of the fan* settings, and cpu_temp match what's reported in
/proc/i8k.
Please match the same units and filename as the other i2c sensors. See
the documentation in the Documentation/i2c/ directory for what that
standard is, so userspace programs will just work with your devices.
Greg,
I almost started doing what you just said but then I realized that none of
the programs will just work because all of them will look into /sys/bus/i2c
instead of /sys/bus/platform/i8k.
For userspace tools to work transparently we would need something like
/sys/class/sensor/{fan|temp|current}, but it is something I am not ready to do
now - I need to finish input layer first.
The sensors developers are creating just such a class, see their hwmon
patch in their email archive if you are curious.
So given above I think having private scheme for now is ok. Sooo... Can I get
my attributes goups patch in so I can use it in i8k, please?
Ick, no, use the upcoming hwmon stuff instead :)
thanks,
greg k-h
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] I8K driver facelift
On Thu, 24 Mar 2005 00:00:48 -0800, Greg KH <[EMAIL PROTECTED]> wrote: > On Thu, Mar 24, 2005 at 02:39:32AM -0500, Dmitry Torokhov wrote: > > On Thursday 24 March 2005 02:25, Greg KH wrote: > > > On Thu, Mar 17, 2005 at 01:40:48AM -0500, Dmitry Torokhov wrote: > > > > On Wednesday 16 March 2005 16:38, Frank Sorenson wrote: > > > > > Okay, I replaced the sysfs_ops with ops of my own, and now all the > > > > > show > > > > > and store functions also accept the name of the attribute as a > > > > > parameter. > > > > > This lets the functions know what attribute is being accessed, and > > > > > allows > > > > > us to create attributes that share show and store functions, so things > > > > > don't need to be defined at compile time (I feel slightly evil!). > > > > > > > > Hrm, can we be a little more explicit and not poke in the sysfs guts > > > > right > > > > in the driver? What do you think about the patch below athat implements > > > > "attribute arrays"? And I am attaching cumulative i8k patch using these > > > > arrays so they can be tested. > > > > > > > > I am CC-ing Greg to see what he thinks about it. > > > > > > Hm, I think it's proably of limited use, right? What other code would > > > want this (the i2c sensor code doesn't, as it's naming scheme is > > > different.) > > > > > > > Looking at their attributes they would benefit from arrays of goups of > > attributes... They have: > > ... > Yeah, but then you break the userspace api that tools are already > expecting to see :( > Yes, although i2c did change inreface somewhat recently - I remember I had to upgrade sensor package couple of month ago to get sensors output, so we have some precedent. Also, even if i2c decides not to use it it does not mean that at some point nobody else would use attribute arrays and arrays of groups. I am willing to bet if these would be available they could be used: /drivers/macintosh/therm_pm72.c /drivers/usb/misc/cytherm.c Attributes with a private pointer passed in would be very useful too. -- Dmitry - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] I8K driver facelift
On Thu, Mar 24, 2005 at 02:39:32AM -0500, Dmitry Torokhov wrote: > On Thursday 24 March 2005 02:25, Greg KH wrote: > > On Thu, Mar 17, 2005 at 01:40:48AM -0500, Dmitry Torokhov wrote: > > > On Wednesday 16 March 2005 16:38, Frank Sorenson wrote: > > > > Okay, I replaced the sysfs_ops with ops of my own, and now all the show > > > > and store functions also accept the name of the attribute as a > > > > parameter. > > > > This lets the functions know what attribute is being accessed, and > > > > allows > > > > us to create attributes that share show and store functions, so things > > > > don't need to be defined at compile time (I feel slightly evil!). > > > > > > Hrm, can we be a little more explicit and not poke in the sysfs guts right > > > in the driver? What do you think about the patch below athat implements > > > "attribute arrays"? And I am attaching cumulative i8k patch using these > > > arrays so they can be tested. > > > > > > I am CC-ing Greg to see what he thinks about it. > > > > Hm, I think it's proably of limited use, right? What other code would > > want this (the i2c sensor code doesn't, as it's naming scheme is > > different.) > > > > Looking at their attributes they would benefit from arrays of goups of > attributes... They have: > > temp[1-4]_max > temp[1-3]_min > temp[1-3]_max_hyst > > It could be: > > temp//max > min > max_hyst > Yeah, but then you break the userspace api that tools are already expecting to see :( thanks, greg k-h - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] I8K driver facelift
On Thu, Mar 24, 2005 at 02:39:32AM -0500, Dmitry Torokhov wrote: On Thursday 24 March 2005 02:25, Greg KH wrote: On Thu, Mar 17, 2005 at 01:40:48AM -0500, Dmitry Torokhov wrote: On Wednesday 16 March 2005 16:38, Frank Sorenson wrote: Okay, I replaced the sysfs_ops with ops of my own, and now all the show and store functions also accept the name of the attribute as a parameter. This lets the functions know what attribute is being accessed, and allows us to create attributes that share show and store functions, so things don't need to be defined at compile time (I feel slightly evil!). Hrm, can we be a little more explicit and not poke in the sysfs guts right in the driver? What do you think about the patch below athat implements attribute arrays? And I am attaching cumulative i8k patch using these arrays so they can be tested. I am CC-ing Greg to see what he thinks about it. Hm, I think it's proably of limited use, right? What other code would want this (the i2c sensor code doesn't, as it's naming scheme is different.) Looking at their attributes they would benefit from arrays of goups of attributes... They have: temp[1-4]_max temp[1-3]_min temp[1-3]_max_hyst It could be: temp/n/max min max_hyst Yeah, but then you break the userspace api that tools are already expecting to see :( thanks, greg k-h - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] I8K driver facelift
On Thu, 24 Mar 2005 00:00:48 -0800, Greg KH [EMAIL PROTECTED] wrote: On Thu, Mar 24, 2005 at 02:39:32AM -0500, Dmitry Torokhov wrote: On Thursday 24 March 2005 02:25, Greg KH wrote: On Thu, Mar 17, 2005 at 01:40:48AM -0500, Dmitry Torokhov wrote: On Wednesday 16 March 2005 16:38, Frank Sorenson wrote: Okay, I replaced the sysfs_ops with ops of my own, and now all the show and store functions also accept the name of the attribute as a parameter. This lets the functions know what attribute is being accessed, and allows us to create attributes that share show and store functions, so things don't need to be defined at compile time (I feel slightly evil!). Hrm, can we be a little more explicit and not poke in the sysfs guts right in the driver? What do you think about the patch below athat implements attribute arrays? And I am attaching cumulative i8k patch using these arrays so they can be tested. I am CC-ing Greg to see what he thinks about it. Hm, I think it's proably of limited use, right? What other code would want this (the i2c sensor code doesn't, as it's naming scheme is different.) Looking at their attributes they would benefit from arrays of goups of attributes... They have: ... Yeah, but then you break the userspace api that tools are already expecting to see :( Yes, although i2c did change inreface somewhat recently - I remember I had to upgrade sensor package couple of month ago to get sensors output, so we have some precedent. Also, even if i2c decides not to use it it does not mean that at some point nobody else would use attribute arrays and arrays of groups. I am willing to bet if these would be available they could be used: /drivers/macintosh/therm_pm72.c /drivers/usb/misc/cytherm.c Attributes with a private pointer passed in would be very useful too. -- Dmitry - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] I8K driver facelift
On Thursday 24 March 2005 02:25, Greg KH wrote: > On Thu, Mar 17, 2005 at 01:40:48AM -0500, Dmitry Torokhov wrote: > > On Wednesday 16 March 2005 16:38, Frank Sorenson wrote: > > > Okay, I replaced the sysfs_ops with ops of my own, and now all the show > > > and store functions also accept the name of the attribute as a parameter. > > > This lets the functions know what attribute is being accessed, and allows > > > us to create attributes that share show and store functions, so things > > > don't need to be defined at compile time (I feel slightly evil!). > > > > Hrm, can we be a little more explicit and not poke in the sysfs guts right > > in the driver? What do you think about the patch below athat implements > > "attribute arrays"? And I am attaching cumulative i8k patch using these > > arrays so they can be tested. > > > > I am CC-ing Greg to see what he thinks about it. > > Hm, I think it's proably of limited use, right? What other code would > want this (the i2c sensor code doesn't, as it's naming scheme is > different.) > Looking at their attributes they would benefit from arrays of goups of attributes... They have: temp[1-4]_max temp[1-3]_min temp[1-3]_max_hyst It could be: temp//max min max_hyst -- Dmitry - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] I8K driver facelift
On Thu, Mar 17, 2005 at 01:40:48AM -0500, Dmitry Torokhov wrote: > On Wednesday 16 March 2005 16:38, Frank Sorenson wrote: > > Okay, I replaced the sysfs_ops with ops of my own, and now all the show > > and store functions also accept the name of the attribute as a parameter. > > This lets the functions know what attribute is being accessed, and allows > > us to create attributes that share show and store functions, so things > > don't need to be defined at compile time (I feel slightly evil!). > > Hrm, can we be a little more explicit and not poke in the sysfs guts right > in the driver? What do you think about the patch below athat implements > "attribute arrays"? And I am attaching cumulative i8k patch using these > arrays so they can be tested. > > I am CC-ing Greg to see what he thinks about it. Hm, I think it's proably of limited use, right? What other code would want this (the i2c sensor code doesn't, as it's naming scheme is different.) What drivers _do_ want is a way to create attributes on the fly easily, and be able to have a "private" pointer to determine easier what file was opened (to allow a single file handler, instead of the current one-per-attribute type). thanks, greg k-h - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] I8K driver facelift
On Thu, Mar 17, 2005 at 03:16:24AM -0500, [EMAIL PROTECTED] wrote: > On Wed, 16 Mar 2005 14:38:50 MST, Frank Sorenson said: > > Okay, I replaced the sysfs_ops with ops of my own, and now all the show > > and store functions also accept the name of the attribute as a parameter. > > This lets the functions know what attribute is being accessed, and allows > > us to create attributes that share show and store functions, so things > > don't need to be defined at compile time (I feel slightly evil!). > > > > This patch puts the correct number of temp sensors and fans into sysfs, > > and only exposes power_status if enabled by the power_status module > > parameter. > > Works for me: > > [/sys/bus/platform/drivers/i8k/i8k]2 ls -l > total 0 > lrwxrwxrwx 1 root root0 Mar 17 03:02 bus -> ../../../bus/platform > -r--r--r-- 1 root root 4096 Mar 17 03:02 cpu_temp > -rw-r--r-- 1 root root 4096 Mar 17 03:01 detach_state > lrwxrwxrwx 1 root root0 Mar 17 03:02 driver -> > ../../../bus/platform/drivers/i8k > -r--r--r-- 1 root root 4096 Mar 17 03:02 fan1_speed > -rw-r--r-- 1 root root 4096 Mar 17 03:02 fan1_state > -r--r--r-- 1 root root 4096 Mar 17 03:02 fan2_speed > -rw-r--r-- 1 root root 4096 Mar 17 03:02 fan2_state > drwxr-xr-x 2 root root0 Mar 17 03:02 power > -r--r--r-- 1 root root 4096 Mar 17 03:02 power_status > -r--r--r-- 1 root root 4096 Mar 17 03:02 temp1 > -r--r--r-- 1 root root 4096 Mar 17 03:02 temp2 > > The valyes of the fan* settings, and cpu_temp match what's reported in > /proc/i8k. Please match the same units and filename as the other i2c sensors. See the documentation in the Documentation/i2c/ directory for what that standard is, so userspace programs will "just work" with your devices. thanks, greg k-h - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] I8K driver facelift
On Thu, Mar 17, 2005 at 01:40:48AM -0500, Dmitry Torokhov wrote: On Wednesday 16 March 2005 16:38, Frank Sorenson wrote: Okay, I replaced the sysfs_ops with ops of my own, and now all the show and store functions also accept the name of the attribute as a parameter. This lets the functions know what attribute is being accessed, and allows us to create attributes that share show and store functions, so things don't need to be defined at compile time (I feel slightly evil!). Hrm, can we be a little more explicit and not poke in the sysfs guts right in the driver? What do you think about the patch below athat implements attribute arrays? And I am attaching cumulative i8k patch using these arrays so they can be tested. I am CC-ing Greg to see what he thinks about it. Hm, I think it's proably of limited use, right? What other code would want this (the i2c sensor code doesn't, as it's naming scheme is different.) What drivers _do_ want is a way to create attributes on the fly easily, and be able to have a private pointer to determine easier what file was opened (to allow a single file handler, instead of the current one-per-attribute type). thanks, greg k-h - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] I8K driver facelift
On Thu, Mar 17, 2005 at 03:16:24AM -0500, [EMAIL PROTECTED] wrote: On Wed, 16 Mar 2005 14:38:50 MST, Frank Sorenson said: Okay, I replaced the sysfs_ops with ops of my own, and now all the show and store functions also accept the name of the attribute as a parameter. This lets the functions know what attribute is being accessed, and allows us to create attributes that share show and store functions, so things don't need to be defined at compile time (I feel slightly evil!). This patch puts the correct number of temp sensors and fans into sysfs, and only exposes power_status if enabled by the power_status module parameter. Works for me: [/sys/bus/platform/drivers/i8k/i8k]2 ls -l total 0 lrwxrwxrwx 1 root root0 Mar 17 03:02 bus - ../../../bus/platform -r--r--r-- 1 root root 4096 Mar 17 03:02 cpu_temp -rw-r--r-- 1 root root 4096 Mar 17 03:01 detach_state lrwxrwxrwx 1 root root0 Mar 17 03:02 driver - ../../../bus/platform/drivers/i8k -r--r--r-- 1 root root 4096 Mar 17 03:02 fan1_speed -rw-r--r-- 1 root root 4096 Mar 17 03:02 fan1_state -r--r--r-- 1 root root 4096 Mar 17 03:02 fan2_speed -rw-r--r-- 1 root root 4096 Mar 17 03:02 fan2_state drwxr-xr-x 2 root root0 Mar 17 03:02 power -r--r--r-- 1 root root 4096 Mar 17 03:02 power_status -r--r--r-- 1 root root 4096 Mar 17 03:02 temp1 -r--r--r-- 1 root root 4096 Mar 17 03:02 temp2 The valyes of the fan* settings, and cpu_temp match what's reported in /proc/i8k. Please match the same units and filename as the other i2c sensors. See the documentation in the Documentation/i2c/ directory for what that standard is, so userspace programs will just work with your devices. thanks, greg k-h - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] I8K driver facelift
On Thursday 24 March 2005 02:25, Greg KH wrote: On Thu, Mar 17, 2005 at 01:40:48AM -0500, Dmitry Torokhov wrote: On Wednesday 16 March 2005 16:38, Frank Sorenson wrote: Okay, I replaced the sysfs_ops with ops of my own, and now all the show and store functions also accept the name of the attribute as a parameter. This lets the functions know what attribute is being accessed, and allows us to create attributes that share show and store functions, so things don't need to be defined at compile time (I feel slightly evil!). Hrm, can we be a little more explicit and not poke in the sysfs guts right in the driver? What do you think about the patch below athat implements attribute arrays? And I am attaching cumulative i8k patch using these arrays so they can be tested. I am CC-ing Greg to see what he thinks about it. Hm, I think it's proably of limited use, right? What other code would want this (the i2c sensor code doesn't, as it's naming scheme is different.) Looking at their attributes they would benefit from arrays of goups of attributes... They have: temp[1-4]_max temp[1-3]_min temp[1-3]_max_hyst It could be: temp/n/max min max_hyst -- Dmitry - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] I8K driver facelift
On Monday 21 March 2005 17:53, Frank Sorenson wrote: > Dmitry Torokhov wrote: > > I have implemented arrays of groups of attributes: > > Works great here. The i8k-cumulative patch claimed to be malformed, but > I merged it in just fine by hand. In arch/i386/kernel/dmi_scan.c, I had > to EXPORT dmi_get_system_info in order to get the i8k module to load. > That may have been a mistake on my end (lots of odd patches in my tree > right now). No, I bet I forgot to export it - I have i8k compiled in and would not notice missing export. > I'm a little curious to see how many people are going to > find they need the ignore_dmi flag versus working without it. I want this driver to be first of all safe for loading on a random box so it can be enabled by default. > > Everything works great, and it is a big step up from the existing code. > I say lets go with it. > Yep, I will add missing export and forward it to Andrew - it could use some time in -mm. Thank you very much for testing it. -- Dmitry - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] I8K driver facelift
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Dmitry Torokhov wrote: > I have implemented arrays of groups of attributes: Works great here. The i8k-cumulative patch claimed to be malformed, but I merged it in just fine by hand. In arch/i386/kernel/dmi_scan.c, I had to EXPORT dmi_get_system_info in order to get the i8k module to load. That may have been a mistake on my end (lots of odd patches in my tree right now). I'm a little curious to see how many people are going to find they need the ignore_dmi flag versus working without it. Everything works great, and it is a big step up from the existing code. I say lets go with it. Frank - -- Frank Sorenson - KD7TZK Systems Manager, Computer Science Department Brigham Young University [EMAIL PROTECTED] -BEGIN PGP SIGNATURE- Version: GnuPG v1.2.6 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org iD8DBQFCP1B7aI0dwg4A47wRAufNAJ9tJODed2rcndtytGCZbJHr5AQJPgCgqbA1 zWnRrePRdJ/+5K1yu5HM3jg= =OzaL -END PGP SIGNATURE- - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] I8K driver facelift
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Dmitry Torokhov wrote: I have implemented arrays of groups of attributes: Works great here. The i8k-cumulative patch claimed to be malformed, but I merged it in just fine by hand. In arch/i386/kernel/dmi_scan.c, I had to EXPORT dmi_get_system_info in order to get the i8k module to load. That may have been a mistake on my end (lots of odd patches in my tree right now). I'm a little curious to see how many people are going to find they need the ignore_dmi flag versus working without it. Everything works great, and it is a big step up from the existing code. I say lets go with it. Frank - -- Frank Sorenson - KD7TZK Systems Manager, Computer Science Department Brigham Young University [EMAIL PROTECTED] -BEGIN PGP SIGNATURE- Version: GnuPG v1.2.6 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org iD8DBQFCP1B7aI0dwg4A47wRAufNAJ9tJODed2rcndtytGCZbJHr5AQJPgCgqbA1 zWnRrePRdJ/+5K1yu5HM3jg= =OzaL -END PGP SIGNATURE- - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] I8K driver facelift
On Monday 21 March 2005 17:53, Frank Sorenson wrote: Dmitry Torokhov wrote: I have implemented arrays of groups of attributes: Works great here. The i8k-cumulative patch claimed to be malformed, but I merged it in just fine by hand. In arch/i386/kernel/dmi_scan.c, I had to EXPORT dmi_get_system_info in order to get the i8k module to load. That may have been a mistake on my end (lots of odd patches in my tree right now). No, I bet I forgot to export it - I have i8k compiled in and would not notice missing export. I'm a little curious to see how many people are going to find they need the ignore_dmi flag versus working without it. I want this driver to be first of all safe for loading on a random box so it can be enabled by default. Everything works great, and it is a big step up from the existing code. I say lets go with it. Yep, I will add missing export and forward it to Andrew - it could use some time in -mm. Thank you very much for testing it. -- Dmitry - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] I8K driver facelift
Hi,
On Thursday 17 March 2005 04:46, Frank Sorenson wrote:
> Dmitry Torokhov wrote:
> | Hrm, can we be a little more explicit and not poke in the sysfs guts right
> | in the driver? What do you think about the patch below athat implements
> | "attribute arrays"? And I am attaching cumulative i8k patch using these
> | arrays so they can be tested.
>
> Also, with power_status being a module parameter defaulting to 0/off, we
> can leave out the device_create_file for the i8k_power_status_attr. No
> need to expose it in sysfs if it will always return -EIO.
>
Since root can toggle power_status through sysfs at runtime (see
/sys/modules/i8k/parameters/power_status) I think it is simplier to have
the file always created.
I have implemented arrays of groups of attributes:
[EMAIL PROTECTED] ~]$ ls -R /sys/bus/platform/devices/i8k/fan/
/sys/bus/platform/devices/i8k/fan/:
0 1
/sys/bus/platform/devices/i8k/fan/0:
speed state
/sys/bus/platform/devices/i8k/fan/1:
speed state
Please give it a try when you have time. Thanks!
--
Dmitry
===
sysfs: add support for attribute arrays so it is possible to create
a number of similar attributes all sharing the same show and
store handlers:
..//0
..//1
...
..//
Number of attributes can be specified at run-time.
Signed-off-by: Dmitry Torokhov <[EMAIL PROTECTED]>
fs/sysfs/Makefile |2
fs/sysfs/array.c | 162 ++
include/linux/sysfs.h | 20 +-
3 files changed, 182 insertions(+), 2 deletions(-)
Index: dtor/fs/sysfs/array.c
===
--- /dev/null
+++ dtor/fs/sysfs/array.c
@@ -0,0 +1,162 @@
+/*
+ * fs/sysfs/array.c - Operations for adding/removing arrays of attributes.
+ *
+ * Copyright (c) 2005 Dmitry Torokhov <[EMAIL PROTECTED]>
+ *
+ * This file is released under the GPL v2.
+ *
+ */
+
+#include
+#include
+#include
+#include "sysfs.h"
+
+struct attr_element {
+ struct attribute attr;
+ unsigned int idx;
+ char name[4];
+};
+#define to_attr_element(e) container_of(e, struct attr_element, attr)
+
+struct attr_array {
+ const struct enumerated_attr *base_attr;
+ unsigned int n_elements;
+ struct kobject kobj;
+ struct attr_element elements[0];
+};
+#define to_attr_array(obj) container_of(obj, struct attr_array, kobj)
+
+static ssize_t
+attr_array_show(struct kobject *kobj, struct attribute *attr, char *buf)
+{
+ struct attr_element *element = to_attr_element(attr);
+ struct attr_array *array = to_attr_array(kobj);
+ ssize_t ret = 0;
+
+ if (array->base_attr->show)
+ ret = array->base_attr->show(kobj->parent, element->idx, buf);
+
+ return ret;
+}
+
+static ssize_t
+attr_array_store(struct kobject *kobj, struct attribute *attr,
+ const char *buf, size_t count)
+{
+ struct attr_element *element = to_attr_element(attr);
+ struct attr_array *array = to_attr_array(kobj);
+ ssize_t ret = 0;
+
+ if (array->base_attr->store)
+ ret = array->base_attr->store(kobj->parent, element->idx, buf, count);
+
+ return ret;
+}
+
+static struct sysfs_ops attr_array_sysfs_ops = {
+ .show = attr_array_show,
+ .store = attr_array_store,
+};
+
+static void attr_array_release(struct kobject *kobj)
+{
+ kfree(to_attr_array(kobj));
+}
+
+static struct kobj_type ktype_attr_array = {
+ .release = attr_array_release,
+ .sysfs_ops = _array_sysfs_ops,
+};
+
+
+static int create_array_element(struct attr_array *array,
+unsigned int idx)
+{
+ struct attr_element *element = >elements[idx];
+
+ snprintf(element->name, sizeof(element->name), "%d", idx);
+ element->idx = idx;
+ element->attr = array->base_attr->attr;
+ element->attr.name = element->name;
+
+ return sysfs_create_file(>kobj, >attr);
+}
+
+static inline void remove_array_element(struct attr_array *array,
+ unsigned int idx)
+{
+ sysfs_remove_file(>kobj, >elements[idx].attr);
+}
+
+int sysfs_create_array(struct kobject *kobj,
+ const struct enumerated_attr *attr,
+ unsigned int n_elements)
+{
+ struct attr_array *array;
+ size_t size;
+ unsigned int i;
+ int error;
+
+ BUG_ON(!kobj || !kobj->dentry);
+ BUG_ON(!attr_name(*attr));
+
+ if (n_elements > 999)
+ return -EINVAL;
+
+ size = sizeof(struct attr_array) +
+ sizeof(struct attr_element) * n_elements;
+
+ array = kmalloc(size, GFP_KERNEL);
+ if (!array)
+ return -ENOMEM;
+
+ memset(array, 0, size);
+ array->base_attr = attr;
+ array->n_elements = n_elements;
+
+ array->kobj.ktype = _attr_array;
+ array->kobj.parent = kobj;
+ kobject_set_name(>kobj, attr_name(*attr));
+
+ error = kobject_register(>kobj);
+ if (error)
+ goto fail;
+
+ for (i = 0; i < n_elements; i++) {
+ error = create_array_element(array, i);
+ if (error) {
+ while (--i)
+remove_array_element(array, i);
+ goto fail;
+ }
+ }
+
+ return 0;
+
+ fail:
+ kfree(array);
+ return error;
+}
+
+void sysfs_remove_array(struct
Re: [PATCH 0/5] I8K driver facelift
Hi,
On Thursday 17 March 2005 04:46, Frank Sorenson wrote:
Dmitry Torokhov wrote:
| Hrm, can we be a little more explicit and not poke in the sysfs guts right
| in the driver? What do you think about the patch below athat implements
| attribute arrays? And I am attaching cumulative i8k patch using these
| arrays so they can be tested.
Also, with power_status being a module parameter defaulting to 0/off, we
can leave out the device_create_file for the i8k_power_status_attr. No
need to expose it in sysfs if it will always return -EIO.
Since root can toggle power_status through sysfs at runtime (see
/sys/modules/i8k/parameters/power_status) I think it is simplier to have
the file always created.
I have implemented arrays of groups of attributes:
[EMAIL PROTECTED] ~]$ ls -R /sys/bus/platform/devices/i8k/fan/
/sys/bus/platform/devices/i8k/fan/:
0 1
/sys/bus/platform/devices/i8k/fan/0:
speed state
/sys/bus/platform/devices/i8k/fan/1:
speed state
Please give it a try when you have time. Thanks!
--
Dmitry
===
sysfs: add support for attribute arrays so it is possible to create
a number of similar attributes all sharing the same show and
store handlers:
../attr_name/0
../attr_name/1
...
../attr_name/n
Number of attributes can be specified at run-time.
Signed-off-by: Dmitry Torokhov [EMAIL PROTECTED]
fs/sysfs/Makefile |2
fs/sysfs/array.c | 162 ++
include/linux/sysfs.h | 20 +-
3 files changed, 182 insertions(+), 2 deletions(-)
Index: dtor/fs/sysfs/array.c
===
--- /dev/null
+++ dtor/fs/sysfs/array.c
@@ -0,0 +1,162 @@
+/*
+ * fs/sysfs/array.c - Operations for adding/removing arrays of attributes.
+ *
+ * Copyright (c) 2005 Dmitry Torokhov [EMAIL PROTECTED]
+ *
+ * This file is released under the GPL v2.
+ *
+ */
+
+#include linux/kobject.h
+#include linux/module.h
+#include linux/dcache.h
+#include sysfs.h
+
+struct attr_element {
+ struct attribute attr;
+ unsigned int idx;
+ char name[4];
+};
+#define to_attr_element(e) container_of(e, struct attr_element, attr)
+
+struct attr_array {
+ const struct enumerated_attr *base_attr;
+ unsigned int n_elements;
+ struct kobject kobj;
+ struct attr_element elements[0];
+};
+#define to_attr_array(obj) container_of(obj, struct attr_array, kobj)
+
+static ssize_t
+attr_array_show(struct kobject *kobj, struct attribute *attr, char *buf)
+{
+ struct attr_element *element = to_attr_element(attr);
+ struct attr_array *array = to_attr_array(kobj);
+ ssize_t ret = 0;
+
+ if (array-base_attr-show)
+ ret = array-base_attr-show(kobj-parent, element-idx, buf);
+
+ return ret;
+}
+
+static ssize_t
+attr_array_store(struct kobject *kobj, struct attribute *attr,
+ const char *buf, size_t count)
+{
+ struct attr_element *element = to_attr_element(attr);
+ struct attr_array *array = to_attr_array(kobj);
+ ssize_t ret = 0;
+
+ if (array-base_attr-store)
+ ret = array-base_attr-store(kobj-parent, element-idx, buf, count);
+
+ return ret;
+}
+
+static struct sysfs_ops attr_array_sysfs_ops = {
+ .show = attr_array_show,
+ .store = attr_array_store,
+};
+
+static void attr_array_release(struct kobject *kobj)
+{
+ kfree(to_attr_array(kobj));
+}
+
+static struct kobj_type ktype_attr_array = {
+ .release = attr_array_release,
+ .sysfs_ops = attr_array_sysfs_ops,
+};
+
+
+static int create_array_element(struct attr_array *array,
+unsigned int idx)
+{
+ struct attr_element *element = array-elements[idx];
+
+ snprintf(element-name, sizeof(element-name), %d, idx);
+ element-idx = idx;
+ element-attr = array-base_attr-attr;
+ element-attr.name = element-name;
+
+ return sysfs_create_file(array-kobj, element-attr);
+}
+
+static inline void remove_array_element(struct attr_array *array,
+ unsigned int idx)
+{
+ sysfs_remove_file(array-kobj, array-elements[idx].attr);
+}
+
+int sysfs_create_array(struct kobject *kobj,
+ const struct enumerated_attr *attr,
+ unsigned int n_elements)
+{
+ struct attr_array *array;
+ size_t size;
+ unsigned int i;
+ int error;
+
+ BUG_ON(!kobj || !kobj-dentry);
+ BUG_ON(!attr_name(*attr));
+
+ if (n_elements 999)
+ return -EINVAL;
+
+ size = sizeof(struct attr_array) +
+ sizeof(struct attr_element) * n_elements;
+
+ array = kmalloc(size, GFP_KERNEL);
+ if (!array)
+ return -ENOMEM;
+
+ memset(array, 0, size);
+ array-base_attr = attr;
+ array-n_elements = n_elements;
+
+ array-kobj.ktype = ktype_attr_array;
+ array-kobj.parent = kobj;
+ kobject_set_name(array-kobj, attr_name(*attr));
+
+ error = kobject_register(array-kobj);
+ if (error)
+ goto fail;
+
+ for (i = 0; i n_elements; i++) {
+ error = create_array_element(array, i);
+ if (error) {
+ while (--i)
+remove_array_element(array, i);
+ goto fail;
+ }
+ }
+
+ return 0;
+
+ fail:
+
Re: [PATCH 0/5] I8K driver facelift
On Thu, 17 Mar 2005 02:37:56 -0700, Frank Sorenson <[EMAIL PROTECTED]> wrote: > 1: My Inspiron 9200 (and perhaps others) doesn't seem to respond to the > I8K_SMM_BIOS_VERSION function call, so it fails the check in i8k_probe. > ~ The check of i8k_get_bios_version doesn't seem critical, and removing > the return -ENODEV makes it work again for me. That's the current > behavior, so perhaps the printk level should just be changed to > KERN_WARNING rather than KERN_ALERT. You are probably right, I shoudl change that. > 2: To compile 2.6.11 cleanly, I needed two hunks from your original > patch 2 (perhaps you're working from a more up-to-date tree than I am? > If so, these are probably already addressed.): > Oh, sorry - when I was pereparing cumulative patch I simply missed this bit. It is still nowhere near the official tree. > > The 'temp' entries make sense, however I'm not sure about the fan_speed > and fan_state entries. From the perspective of how the objects are > ordered, a fan would have 'speed' and 'state' attributes, but a > 'fan_state' attribute wouldn't normally have a fan. Maybe something > along these lines would make more sense from that perspective: > > ./fan/0 > ./fan/0/speed > ./fan/0/state > ./fan/1 > ./fan/1/speed > ./fan/1/state > Yes, as soon as I did attribute array I realized that something like attr_array_group would reflect the structure better... We'll see what can be done. Thank you for your comments and suggestions! -- Dmitry - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] I8K driver facelift
On Wed, 16 Mar 2005 14:38:50 MST, Frank Sorenson said: > Okay, I replaced the sysfs_ops with ops of my own, and now all the show > and store functions also accept the name of the attribute as a parameter. > This lets the functions know what attribute is being accessed, and allows > us to create attributes that share show and store functions, so things > don't need to be defined at compile time (I feel slightly evil!). > > This patch puts the correct number of temp sensors and fans into sysfs, > and only exposes power_status if enabled by the power_status module > parameter. Works for me: [/sys/bus/platform/drivers/i8k/i8k]2 ls -l total 0 lrwxrwxrwx 1 root root0 Mar 17 03:02 bus -> ../../../bus/platform -r--r--r-- 1 root root 4096 Mar 17 03:02 cpu_temp -rw-r--r-- 1 root root 4096 Mar 17 03:01 detach_state lrwxrwxrwx 1 root root0 Mar 17 03:02 driver -> ../../../bus/platform/drivers/i8k -r--r--r-- 1 root root 4096 Mar 17 03:02 fan1_speed -rw-r--r-- 1 root root 4096 Mar 17 03:02 fan1_state -r--r--r-- 1 root root 4096 Mar 17 03:02 fan2_speed -rw-r--r-- 1 root root 4096 Mar 17 03:02 fan2_state drwxr-xr-x 2 root root0 Mar 17 03:02 power -r--r--r-- 1 root root 4096 Mar 17 03:02 power_status -r--r--r-- 1 root root 4096 Mar 17 03:02 temp1 -r--r--r-- 1 root root 4096 Mar 17 03:02 temp2 The valyes of the fan* settings, and cpu_temp match what's reported in /proc/i8k. temp1 is the same as cpu_temp. temp2 is running about 7C higher and is still unidentified (though probably the NVidia chip). I'll give Dmitry's sysfs/array stuff a test tomorrow-ish unless Greg has comments before then. pgpgol89bS67j.pgp Description: PGP signature
Re: [PATCH 0/5] I8K driver facelift
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Dmitry Torokhov wrote: | Hrm, can we be a little more explicit and not poke in the sysfs guts right | in the driver? What do you think about the patch below athat implements | "attribute arrays"? And I am attaching cumulative i8k patch using these | arrays so they can be tested. Also, with power_status being a module parameter defaulting to 0/off, we can leave out the device_create_file for the i8k_power_status_attr. No need to expose it in sysfs if it will always return -EIO. Frank - -- Frank Sorenson - KD7TZK Systems Manager, Computer Science Department Brigham Young University [EMAIL PROTECTED] -BEGIN PGP SIGNATURE- Version: GnuPG v1.2.6 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org iD8DBQFCOVHjaI0dwg4A47wRAgquAJ49qf5qFZX9twSbLetJiliEnES5GwCg41z2 r6AWC22/zAcz54xAIfNQJ4I= =0BtE -END PGP SIGNATURE- - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] I8K driver facelift
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
Dmitry Torokhov wrote:
| Hrm, can we be a little more explicit and not poke in the sysfs guts right
| in the driver? What do you think about the patch below athat implements
| "attribute arrays"? And I am attaching cumulative i8k patch using these
| arrays so they can be tested.
|
| I am CC-ing Greg to see what he thinks about it.
Well, yes. That would probably be the better way to go about it, though
I have to admit I was somewhat pleased with myself that I came up with
something that worked. :)
Your patches work well, with a few minor notes:
1: My Inspiron 9200 (and perhaps others) doesn't seem to respond to the
I8K_SMM_BIOS_VERSION function call, so it fails the check in i8k_probe.
~ The check of i8k_get_bios_version doesn't seem critical, and removing
the return -ENODEV makes it work again for me. That's the current
behavior, so perhaps the printk level should just be changed to
KERN_WARNING rather than KERN_ALERT.
2: To compile 2.6.11 cleanly, I needed two hunks from your original
patch 2 (perhaps you're working from a more up-to-date tree than I am?
If so, these are probably already addressed.):
- --- dtor.orig/arch/i386/kernel/dmi_scan.c
+++ dtor/arch/i386/kernel/dmi_scan.c
@@ -416,6 +416,7 @@ static void __init dmi_decode(struct dmi
~ dmi_save_ident(dm, DMI_PRODUCT_VERSION, 6);
~ dmi_printk(("Serial Number: %s\n",
~ dmi_string(dm, data[7])));
+ dmi_save_ident(dm, DMI_PRODUCT_SERIAL, 7);
~ break;
~ case 2:
~ dmi_printk(("Board Vendor: %s\n",
- --- dtor.orig/include/linux/dmi.h
+++ dtor/include/linux/dmi.h
@@ -9,6 +9,7 @@ enum dmi_field {
~ DMI_SYS_VENDOR,
~ DMI_PRODUCT_NAME,
~ DMI_PRODUCT_VERSION,
+ DMI_PRODUCT_SERIAL,
~ DMI_BOARD_VENDOR,
~ DMI_BOARD_NAME,
~ DMI_BOARD_VERSION,
I also have a question about the structure created using sysfs attribute
arrays. Applying it in this case, I get:
./temp
./temp/3
./temp/2
./temp/1
./temp/0
./fan_speed
./fan_speed/1
./fan_speed/0
./fan_state
./fan_state/1
./fan_state/0
The 'temp' entries make sense, however I'm not sure about the fan_speed
and fan_state entries. From the perspective of how the objects are
ordered, a fan would have 'speed' and 'state' attributes, but a
'fan_state' attribute wouldn't normally have a fan. Maybe something
along these lines would make more sense from that perspective:
./fan/0
./fan/0/speed
./fan/0/state
./fan/1
./fan/1/speed
./fan/1/state
I'm not certain about the best way to do this, so this may just be a
thought. It would certainly be more complex to reorder it, and it
appears usable in its current form.
Frank
- --
Frank Sorenson - KD7TZK
Systems Manager, Computer Science Department
Brigham Young University
[EMAIL PROTECTED]
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.2.6 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org
iD4DBQFCOU/0aI0dwg4A47wRAjgDAJwLsvd14J/qAmgv7JzkXG2xgAmTGwCY6RUc
Nomk0pwTSfymHtIuF7ylzQ==
=85eA
-END PGP SIGNATURE-
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] I8K driver facelift
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
Dmitry Torokhov wrote:
| Hrm, can we be a little more explicit and not poke in the sysfs guts right
| in the driver? What do you think about the patch below athat implements
| attribute arrays? And I am attaching cumulative i8k patch using these
| arrays so they can be tested.
|
| I am CC-ing Greg to see what he thinks about it.
Well, yes. That would probably be the better way to go about it, though
I have to admit I was somewhat pleased with myself that I came up with
something that worked. :)
Your patches work well, with a few minor notes:
1: My Inspiron 9200 (and perhaps others) doesn't seem to respond to the
I8K_SMM_BIOS_VERSION function call, so it fails the check in i8k_probe.
~ The check of i8k_get_bios_version doesn't seem critical, and removing
the return -ENODEV makes it work again for me. That's the current
behavior, so perhaps the printk level should just be changed to
KERN_WARNING rather than KERN_ALERT.
2: To compile 2.6.11 cleanly, I needed two hunks from your original
patch 2 (perhaps you're working from a more up-to-date tree than I am?
If so, these are probably already addressed.):
- --- dtor.orig/arch/i386/kernel/dmi_scan.c
+++ dtor/arch/i386/kernel/dmi_scan.c
@@ -416,6 +416,7 @@ static void __init dmi_decode(struct dmi
~ dmi_save_ident(dm, DMI_PRODUCT_VERSION, 6);
~ dmi_printk((Serial Number: %s\n,
~ dmi_string(dm, data[7])));
+ dmi_save_ident(dm, DMI_PRODUCT_SERIAL, 7);
~ break;
~ case 2:
~ dmi_printk((Board Vendor: %s\n,
- --- dtor.orig/include/linux/dmi.h
+++ dtor/include/linux/dmi.h
@@ -9,6 +9,7 @@ enum dmi_field {
~ DMI_SYS_VENDOR,
~ DMI_PRODUCT_NAME,
~ DMI_PRODUCT_VERSION,
+ DMI_PRODUCT_SERIAL,
~ DMI_BOARD_VENDOR,
~ DMI_BOARD_NAME,
~ DMI_BOARD_VERSION,
I also have a question about the structure created using sysfs attribute
arrays. Applying it in this case, I get:
./temp
./temp/3
./temp/2
./temp/1
./temp/0
./fan_speed
./fan_speed/1
./fan_speed/0
./fan_state
./fan_state/1
./fan_state/0
The 'temp' entries make sense, however I'm not sure about the fan_speed
and fan_state entries. From the perspective of how the objects are
ordered, a fan would have 'speed' and 'state' attributes, but a
'fan_state' attribute wouldn't normally have a fan. Maybe something
along these lines would make more sense from that perspective:
./fan/0
./fan/0/speed
./fan/0/state
./fan/1
./fan/1/speed
./fan/1/state
I'm not certain about the best way to do this, so this may just be a
thought. It would certainly be more complex to reorder it, and it
appears usable in its current form.
Frank
- --
Frank Sorenson - KD7TZK
Systems Manager, Computer Science Department
Brigham Young University
[EMAIL PROTECTED]
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.2.6 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org
iD4DBQFCOU/0aI0dwg4A47wRAjgDAJwLsvd14J/qAmgv7JzkXG2xgAmTGwCY6RUc
Nomk0pwTSfymHtIuF7ylzQ==
=85eA
-END PGP SIGNATURE-
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] I8K driver facelift
On Wed, 16 Mar 2005 14:38:50 MST, Frank Sorenson said: Okay, I replaced the sysfs_ops with ops of my own, and now all the show and store functions also accept the name of the attribute as a parameter. This lets the functions know what attribute is being accessed, and allows us to create attributes that share show and store functions, so things don't need to be defined at compile time (I feel slightly evil!). This patch puts the correct number of temp sensors and fans into sysfs, and only exposes power_status if enabled by the power_status module parameter. Works for me: [/sys/bus/platform/drivers/i8k/i8k]2 ls -l total 0 lrwxrwxrwx 1 root root0 Mar 17 03:02 bus - ../../../bus/platform -r--r--r-- 1 root root 4096 Mar 17 03:02 cpu_temp -rw-r--r-- 1 root root 4096 Mar 17 03:01 detach_state lrwxrwxrwx 1 root root0 Mar 17 03:02 driver - ../../../bus/platform/drivers/i8k -r--r--r-- 1 root root 4096 Mar 17 03:02 fan1_speed -rw-r--r-- 1 root root 4096 Mar 17 03:02 fan1_state -r--r--r-- 1 root root 4096 Mar 17 03:02 fan2_speed -rw-r--r-- 1 root root 4096 Mar 17 03:02 fan2_state drwxr-xr-x 2 root root0 Mar 17 03:02 power -r--r--r-- 1 root root 4096 Mar 17 03:02 power_status -r--r--r-- 1 root root 4096 Mar 17 03:02 temp1 -r--r--r-- 1 root root 4096 Mar 17 03:02 temp2 The valyes of the fan* settings, and cpu_temp match what's reported in /proc/i8k. temp1 is the same as cpu_temp. temp2 is running about 7C higher and is still unidentified (though probably the NVidia chip). I'll give Dmitry's sysfs/array stuff a test tomorrow-ish unless Greg has comments before then. pgpgol89bS67j.pgp Description: PGP signature
Re: [PATCH 0/5] I8K driver facelift
On Thu, 17 Mar 2005 02:37:56 -0700, Frank Sorenson [EMAIL PROTECTED] wrote: 1: My Inspiron 9200 (and perhaps others) doesn't seem to respond to the I8K_SMM_BIOS_VERSION function call, so it fails the check in i8k_probe. ~ The check of i8k_get_bios_version doesn't seem critical, and removing the return -ENODEV makes it work again for me. That's the current behavior, so perhaps the printk level should just be changed to KERN_WARNING rather than KERN_ALERT. You are probably right, I shoudl change that. 2: To compile 2.6.11 cleanly, I needed two hunks from your original patch 2 (perhaps you're working from a more up-to-date tree than I am? If so, these are probably already addressed.): Oh, sorry - when I was pereparing cumulative patch I simply missed this bit. It is still nowhere near the official tree. The 'temp' entries make sense, however I'm not sure about the fan_speed and fan_state entries. From the perspective of how the objects are ordered, a fan would have 'speed' and 'state' attributes, but a 'fan_state' attribute wouldn't normally have a fan. Maybe something along these lines would make more sense from that perspective: ./fan/0 ./fan/0/speed ./fan/0/state ./fan/1 ./fan/1/speed ./fan/1/state Yes, as soon as I did attribute array I realized that something like attr_array_group would reflect the structure better... We'll see what can be done. Thank you for your comments and suggestions! -- Dmitry - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] I8K driver facelift
On Wednesday 16 March 2005 16:38, Frank Sorenson wrote:
> Okay, I replaced the sysfs_ops with ops of my own, and now all the show
> and store functions also accept the name of the attribute as a parameter.
> This lets the functions know what attribute is being accessed, and allows
> us to create attributes that share show and store functions, so things
> don't need to be defined at compile time (I feel slightly evil!).
Hrm, can we be a little more explicit and not poke in the sysfs guts right
in the driver? What do you think about the patch below athat implements
"attribute arrays"? And I am attaching cumulative i8k patch using these
arrays so they can be tested.
I am CC-ing Greg to see what he thinks about it.
--
Dmitry
===
sysfs: add support for attribure arrays so it is possible to create
a (variable) number of similar attributes all sharing the
same show and store handlers:
..//0
..//1
...
..//
Signed-off-by: Dmitry Torokhov <[EMAIL PROTECTED]>
fs/sysfs/Makefile |2
fs/sysfs/array.c | 167 ++
include/linux/sysfs.h | 20 +
3 files changed, 187 insertions(+), 2 deletions(-)
Index: dtor/fs/sysfs/array.c
===
--- /dev/null
+++ dtor/fs/sysfs/array.c
@@ -0,0 +1,167 @@
+/*
+ * fs/sysfs/array.c - Operations for adding/removing arrays of attributes.
+ *
+ * Copyright (c) 2005 Dmitry Torokhov <[EMAIL PROTECTED]>
+ *
+ * This file is released under the GPL v2.
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include "sysfs.h"
+
+struct attr_element {
+ struct attribute attr;
+ unsigned int idx;
+ char name[4];
+};
+#define to_attr_element(e) container_of(e, struct attr_element, attr)
+
+struct array_object {
+ const struct attribute_array *array;
+ unsigned int n_elements;
+ struct kobject kobj;
+ struct attr_element elements[0];
+};
+#define to_array_object(obj) container_of(obj, struct array_object, kobj)
+
+static ssize_t
+attr_array_show(struct kobject *kobj, struct attribute *attr, char *buf)
+{
+ struct attr_element *element = to_attr_element(attr);
+ struct array_object *obj = to_array_object(kobj);
+ ssize_t ret = 0;
+
+ if (obj->array->show) {
+ struct kobject *parent = kobject_get(kobj->parent);
+ ret = obj->array->show(parent, element->idx, buf);
+ kobject_put(parent);
+ }
+
+ return ret;
+}
+
+static ssize_t
+attr_array_store(struct kobject *kobj, struct attribute *attr,
+const char *buf, size_t count)
+{
+ struct attr_element *element = to_attr_element(attr);
+ struct array_object *obj = to_array_object(kobj);
+ ssize_t ret = 0;
+
+ if (obj->array->store) {
+ struct kobject *parent = kobject_get(kobj->parent);
+ ret = obj->array->store(parent, element->idx, buf, count);
+ kobject_put(parent);
+ }
+
+ return ret;
+}
+
+static struct sysfs_ops attr_array_sysfs_ops = {
+ .show = attr_array_show,
+ .store = attr_array_store,
+};
+
+
+static void attr_array_release(struct kobject *kobj)
+{
+ kfree(to_array_object(kobj));
+}
+
+static struct kobj_type ktype_attr_array = {
+ .release= attr_array_release,
+ .sysfs_ops = _array_sysfs_ops,
+};
+
+
+static int create_array_element(struct array_object *obj,
+ unsigned int idx)
+{
+ struct attr_element *element = >elements[idx];
+
+ snprintf(element->name, sizeof(element->name), "%d", idx);
+ element->idx = idx;
+ element->attr = obj->array->attr;
+ element->attr.name = element->name;
+
+ return sysfs_create_file(>kobj, >attr);
+}
+
+static inline void remove_array_element(struct array_object *obj,
+ unsigned int idx)
+{
+ sysfs_remove_file(>kobj, >elements[idx].attr);
+}
+
+int sysfs_create_array(struct kobject *kobj,
+ const struct attribute_array *array,
+ unsigned int n_elements)
+{
+ struct array_object *obj;
+ size_t size;
+ unsigned int i;
+ int error;
+
+ BUG_ON(!kobj || !kobj->dentry);
+ BUG_ON(!array->attr.name);
+
+ size = sizeof(struct array_object) +
+ sizeof(struct attr_element) * n_elements;
+
+ obj = kmalloc(size, GFP_KERNEL);
+ if (!obj)
+ return -ENOMEM;
+
+ memset(obj, 0, size);
+ obj->array = array;
+ obj->n_elements = n_elements;
+
+ obj->kobj.ktype = _attr_array;
+ obj->kobj.parent = kobj;
+ kobject_set_name(>kobj, array->attr.name);
+
+ error = kobject_register(>kobj);
+ if (error)
+ goto fail;
+
+
Re: [PATCH 0/5] I8K driver facelift
Okay, I replaced the sysfs_ops with ops of my own, and now all the show
and store functions also accept the name of the attribute as a parameter.
This lets the functions know what attribute is being accessed, and allows
us to create attributes that share show and store functions, so things
don't need to be defined at compile time (I feel slightly evil!).
This patch puts the correct number of temp sensors and fans into sysfs,
and only exposes power_status if enabled by the power_status module
parameter.
This patch applies on top of Dmitry Torokov's patches. Comments welcome!
Thanks,
Frank
Signed-off-by: Frank Sorenson <[EMAIL PROTECTED]>
--- 2.6.11-a/drivers/char/i8k.c 2005-03-12 18:47:55.0 -0700
+++ 2.6.11-b/drivers/char/i8k.c 2005-03-16 14:23:40.0 -0700
@@ -23,12 +23,14 @@
#include
#include
#include
+#include
+#include
#include
#include
#include
-#define I8K_VERSION"1.14 21/02/2005"
+#define I8K_VERSION"2005-03-16"
#define I8K_SMM_FN_STATUS 0x0025
#define I8K_SMM_POWER_STATUS 0x0069
@@ -36,7 +38,8 @@
#define I8K_SMM_GET_FAN0x00a3
#define I8K_SMM_GET_SPEED 0x02a3
#define I8K_SMM_GET_TEMP 0x10a3
-#define I8K_SMM_GET_DELL_SIG 0xffa3
+#define I8K_SMM_GET_DELL_SIG1 0xfea3
+#define I8K_SMM_GET_DELL_SIG2 0xffa3
#define I8K_SMM_BIOS_VERSION 0x00a6
#define I8K_FAN_MULT 30
@@ -54,7 +57,12 @@
#define I8K_TEMPERATURE_BUG1
+#define to_dev(d) container_of(d, struct device, kobj)
+#define to_dev_attr(_attr) container_of(_attr,struct device_attribute,attr)
+
static char bios_version[4];
+static int num_temps = 0;
+static int num_fans = 0;
MODULE_AUTHOR("Massimo Dal Zotto ([EMAIL PROTECTED])");
MODULE_DESCRIPTION("Driver for accessing SMM BIOS on Dell laptops");
@@ -80,6 +88,8 @@
static int i8k_ioctl(struct inode *, struct file *, unsigned int,
unsigned long);
+static struct kobj_type *backup_ktype;
+
static struct file_operations i8k_fops = {
.open = i8k_open_fs,
.read = seq_read,
@@ -94,7 +104,7 @@
};
static struct platform_device *i8k_device;
-
+
struct smm_regs {
unsigned int eax;
unsigned int ebx __attribute__ ((packed));
@@ -156,7 +166,7 @@
{
struct smm_regs regs = { .eax = I8K_SMM_BIOS_VERSION, };
- return i8k_smm() < 0 ? : regs.eax;
+ return i8k_smm() ? : regs.eax;
}
/*
@@ -201,10 +211,10 @@
*/
static int i8k_get_fan_status(int fan)
{
- struct smm_regs regs = { .eax = I8K_SMM_GET_FAN, };
+ struct smm_regs regs = { .eax = I8K_SMM_GET_FAN, .ebx = (fan & 0xff)};
regs.ebx = fan & 0xff;
- return i8k_smm() < 0 ? : regs.eax & 0xff;
+ return i8k_smm() ? : regs.eax & 0xff;
}
/*
@@ -215,7 +225,7 @@
struct smm_regs regs = { .eax = I8K_SMM_GET_SPEED, };
regs.ebx = fan & 0xff;
- return i8k_smm() < 0 ? : (regs.eax & 0x) * I8K_FAN_MULT;
+ return i8k_smm() ? : (regs.eax & 0x) * I8K_FAN_MULT;
}
/*
@@ -228,15 +238,15 @@
speed = (speed < 0) ? 0 : ((speed > I8K_FAN_MAX) ? I8K_FAN_MAX : speed);
regs.ebx = (fan & 0xff) | (speed << 8);
- return i8k_smm() < 0 ? : i8k_get_fan_status(fan);
+ return i8k_smm() ? : i8k_get_fan_status(fan);
}
/*
* Read the cpu temperature.
*/
-static int i8k_get_cpu_temp(void)
+static int i8k_get_temp(int sensor)
{
- struct smm_regs regs = { .eax = I8K_SMM_GET_TEMP, };
+ struct smm_regs regs = { .eax = I8K_SMM_GET_TEMP, .ebx = sensor };
int rc;
int temp;
@@ -268,9 +278,14 @@
return temp;
}
-static int i8k_get_dell_signature(void)
+static int i8k_get_cpu_temp(void)
{
- struct smm_regs regs = { .eax = I8K_SMM_GET_DELL_SIG, };
+ return i8k_get_temp(0);
+}
+
+static int i8k_get_dell_sig_aux(int fn)
+{
+ struct smm_regs regs = { .eax = fn, };
int rc;
if ((rc = i8k_smm()) < 0)
@@ -279,6 +294,17 @@
return regs.eax == 1145651527 && regs.edx == 1145392204 ? 0 : -1;
}
+static int i8k_get_dell_signature(void)
+{
+ int rc;
+
+ if (((rc=i8k_get_dell_sig_aux(I8K_SMM_GET_DELL_SIG1)) < 0) &&
+ ((rc=i8k_get_dell_sig_aux(I8K_SMM_GET_DELL_SIG2)) < 0)) {
+ return rc;
+ }
+ return 0;
+}
+
static int i8k_ioctl(struct inode *ip, struct file *fp, unsigned int cmd,
unsigned long arg)
{
@@ -414,87 +440,112 @@
return single_open(file, i8k_proc_show, NULL);
}
-static ssize_t i8k_sysfs_cpu_temp_show(struct device *dev, char *buf)
+static ssize_t i8k_sysfs_cpu_temp_show(struct device *dev, char *buf,
+ const char *name)
{
int temp = i8k_get_cpu_temp();
return temp < 0 ? -EIO : sprintf(buf, "%d\n", temp);
}
-static ssize_t i8k_sysfs_fan1_show(struct device *dev, char *buf)
+static ssize_t i8k_sysfs_temp_show(struct device *dev, char *buf,
+
Re: [PATCH 0/5] I8K driver facelift
Okay, I replaced the sysfs_ops with ops of my own, and now all the show
and store functions also accept the name of the attribute as a parameter.
This lets the functions know what attribute is being accessed, and allows
us to create attributes that share show and store functions, so things
don't need to be defined at compile time (I feel slightly evil!).
This patch puts the correct number of temp sensors and fans into sysfs,
and only exposes power_status if enabled by the power_status module
parameter.
This patch applies on top of Dmitry Torokov's patches. Comments welcome!
Thanks,
Frank
Signed-off-by: Frank Sorenson [EMAIL PROTECTED]
--- 2.6.11-a/drivers/char/i8k.c 2005-03-12 18:47:55.0 -0700
+++ 2.6.11-b/drivers/char/i8k.c 2005-03-16 14:23:40.0 -0700
@@ -23,12 +23,14 @@
#include linux/seq_file.h
#include linux/dmi.h
#include linux/device.h
+#include linux/sysfs.h
+#include linux/kobject.h
#include asm/uaccess.h
#include asm/io.h
#include linux/i8k.h
-#define I8K_VERSION1.14 21/02/2005
+#define I8K_VERSION2005-03-16
#define I8K_SMM_FN_STATUS 0x0025
#define I8K_SMM_POWER_STATUS 0x0069
@@ -36,7 +38,8 @@
#define I8K_SMM_GET_FAN0x00a3
#define I8K_SMM_GET_SPEED 0x02a3
#define I8K_SMM_GET_TEMP 0x10a3
-#define I8K_SMM_GET_DELL_SIG 0xffa3
+#define I8K_SMM_GET_DELL_SIG1 0xfea3
+#define I8K_SMM_GET_DELL_SIG2 0xffa3
#define I8K_SMM_BIOS_VERSION 0x00a6
#define I8K_FAN_MULT 30
@@ -54,7 +57,12 @@
#define I8K_TEMPERATURE_BUG1
+#define to_dev(d) container_of(d, struct device, kobj)
+#define to_dev_attr(_attr) container_of(_attr,struct device_attribute,attr)
+
static char bios_version[4];
+static int num_temps = 0;
+static int num_fans = 0;
MODULE_AUTHOR(Massimo Dal Zotto ([EMAIL PROTECTED]));
MODULE_DESCRIPTION(Driver for accessing SMM BIOS on Dell laptops);
@@ -80,6 +88,8 @@
static int i8k_ioctl(struct inode *, struct file *, unsigned int,
unsigned long);
+static struct kobj_type *backup_ktype;
+
static struct file_operations i8k_fops = {
.open = i8k_open_fs,
.read = seq_read,
@@ -94,7 +104,7 @@
};
static struct platform_device *i8k_device;
-
+
struct smm_regs {
unsigned int eax;
unsigned int ebx __attribute__ ((packed));
@@ -156,7 +166,7 @@
{
struct smm_regs regs = { .eax = I8K_SMM_BIOS_VERSION, };
- return i8k_smm(regs) 0 ? : regs.eax;
+ return i8k_smm(regs) ? : regs.eax;
}
/*
@@ -201,10 +211,10 @@
*/
static int i8k_get_fan_status(int fan)
{
- struct smm_regs regs = { .eax = I8K_SMM_GET_FAN, };
+ struct smm_regs regs = { .eax = I8K_SMM_GET_FAN, .ebx = (fan 0xff)};
regs.ebx = fan 0xff;
- return i8k_smm(regs) 0 ? : regs.eax 0xff;
+ return i8k_smm(regs) ? : regs.eax 0xff;
}
/*
@@ -215,7 +225,7 @@
struct smm_regs regs = { .eax = I8K_SMM_GET_SPEED, };
regs.ebx = fan 0xff;
- return i8k_smm(regs) 0 ? : (regs.eax 0x) * I8K_FAN_MULT;
+ return i8k_smm(regs) ? : (regs.eax 0x) * I8K_FAN_MULT;
}
/*
@@ -228,15 +238,15 @@
speed = (speed 0) ? 0 : ((speed I8K_FAN_MAX) ? I8K_FAN_MAX : speed);
regs.ebx = (fan 0xff) | (speed 8);
- return i8k_smm(regs) 0 ? : i8k_get_fan_status(fan);
+ return i8k_smm(regs) ? : i8k_get_fan_status(fan);
}
/*
* Read the cpu temperature.
*/
-static int i8k_get_cpu_temp(void)
+static int i8k_get_temp(int sensor)
{
- struct smm_regs regs = { .eax = I8K_SMM_GET_TEMP, };
+ struct smm_regs regs = { .eax = I8K_SMM_GET_TEMP, .ebx = sensor };
int rc;
int temp;
@@ -268,9 +278,14 @@
return temp;
}
-static int i8k_get_dell_signature(void)
+static int i8k_get_cpu_temp(void)
{
- struct smm_regs regs = { .eax = I8K_SMM_GET_DELL_SIG, };
+ return i8k_get_temp(0);
+}
+
+static int i8k_get_dell_sig_aux(int fn)
+{
+ struct smm_regs regs = { .eax = fn, };
int rc;
if ((rc = i8k_smm(regs)) 0)
@@ -279,6 +294,17 @@
return regs.eax == 1145651527 regs.edx == 1145392204 ? 0 : -1;
}
+static int i8k_get_dell_signature(void)
+{
+ int rc;
+
+ if (((rc=i8k_get_dell_sig_aux(I8K_SMM_GET_DELL_SIG1)) 0)
+ ((rc=i8k_get_dell_sig_aux(I8K_SMM_GET_DELL_SIG2)) 0)) {
+ return rc;
+ }
+ return 0;
+}
+
static int i8k_ioctl(struct inode *ip, struct file *fp, unsigned int cmd,
unsigned long arg)
{
@@ -414,87 +440,112 @@
return single_open(file, i8k_proc_show, NULL);
}
-static ssize_t i8k_sysfs_cpu_temp_show(struct device *dev, char *buf)
+static ssize_t i8k_sysfs_cpu_temp_show(struct device *dev, char *buf,
+ const char *name)
{
int temp = i8k_get_cpu_temp();
return temp 0 ? -EIO : sprintf(buf, %d\n, temp);
}
-static ssize_t i8k_sysfs_fan1_show(struct
Re: [PATCH 0/5] I8K driver facelift
On Wednesday 16 March 2005 16:38, Frank Sorenson wrote:
Okay, I replaced the sysfs_ops with ops of my own, and now all the show
and store functions also accept the name of the attribute as a parameter.
This lets the functions know what attribute is being accessed, and allows
us to create attributes that share show and store functions, so things
don't need to be defined at compile time (I feel slightly evil!).
Hrm, can we be a little more explicit and not poke in the sysfs guts right
in the driver? What do you think about the patch below athat implements
attribute arrays? And I am attaching cumulative i8k patch using these
arrays so they can be tested.
I am CC-ing Greg to see what he thinks about it.
--
Dmitry
===
sysfs: add support for attribure arrays so it is possible to create
a (variable) number of similar attributes all sharing the
same show and store handlers:
../attr_name/0
../attr_name/1
...
../attr_name/n
Signed-off-by: Dmitry Torokhov [EMAIL PROTECTED]
fs/sysfs/Makefile |2
fs/sysfs/array.c | 167 ++
include/linux/sysfs.h | 20 +
3 files changed, 187 insertions(+), 2 deletions(-)
Index: dtor/fs/sysfs/array.c
===
--- /dev/null
+++ dtor/fs/sysfs/array.c
@@ -0,0 +1,167 @@
+/*
+ * fs/sysfs/array.c - Operations for adding/removing arrays of attributes.
+ *
+ * Copyright (c) 2005 Dmitry Torokhov [EMAIL PROTECTED]
+ *
+ * This file is released under the GPL v2.
+ *
+ */
+
+#include linux/kobject.h
+#include linux/module.h
+#include linux/dcache.h
+#include linux/err.h
+#include sysfs.h
+
+struct attr_element {
+ struct attribute attr;
+ unsigned int idx;
+ char name[4];
+};
+#define to_attr_element(e) container_of(e, struct attr_element, attr)
+
+struct array_object {
+ const struct attribute_array *array;
+ unsigned int n_elements;
+ struct kobject kobj;
+ struct attr_element elements[0];
+};
+#define to_array_object(obj) container_of(obj, struct array_object, kobj)
+
+static ssize_t
+attr_array_show(struct kobject *kobj, struct attribute *attr, char *buf)
+{
+ struct attr_element *element = to_attr_element(attr);
+ struct array_object *obj = to_array_object(kobj);
+ ssize_t ret = 0;
+
+ if (obj-array-show) {
+ struct kobject *parent = kobject_get(kobj-parent);
+ ret = obj-array-show(parent, element-idx, buf);
+ kobject_put(parent);
+ }
+
+ return ret;
+}
+
+static ssize_t
+attr_array_store(struct kobject *kobj, struct attribute *attr,
+const char *buf, size_t count)
+{
+ struct attr_element *element = to_attr_element(attr);
+ struct array_object *obj = to_array_object(kobj);
+ ssize_t ret = 0;
+
+ if (obj-array-store) {
+ struct kobject *parent = kobject_get(kobj-parent);
+ ret = obj-array-store(parent, element-idx, buf, count);
+ kobject_put(parent);
+ }
+
+ return ret;
+}
+
+static struct sysfs_ops attr_array_sysfs_ops = {
+ .show = attr_array_show,
+ .store = attr_array_store,
+};
+
+
+static void attr_array_release(struct kobject *kobj)
+{
+ kfree(to_array_object(kobj));
+}
+
+static struct kobj_type ktype_attr_array = {
+ .release= attr_array_release,
+ .sysfs_ops = attr_array_sysfs_ops,
+};
+
+
+static int create_array_element(struct array_object *obj,
+ unsigned int idx)
+{
+ struct attr_element *element = obj-elements[idx];
+
+ snprintf(element-name, sizeof(element-name), %d, idx);
+ element-idx = idx;
+ element-attr = obj-array-attr;
+ element-attr.name = element-name;
+
+ return sysfs_create_file(obj-kobj, element-attr);
+}
+
+static inline void remove_array_element(struct array_object *obj,
+ unsigned int idx)
+{
+ sysfs_remove_file(obj-kobj, obj-elements[idx].attr);
+}
+
+int sysfs_create_array(struct kobject *kobj,
+ const struct attribute_array *array,
+ unsigned int n_elements)
+{
+ struct array_object *obj;
+ size_t size;
+ unsigned int i;
+ int error;
+
+ BUG_ON(!kobj || !kobj-dentry);
+ BUG_ON(!array-attr.name);
+
+ size = sizeof(struct array_object) +
+ sizeof(struct attr_element) * n_elements;
+
+ obj = kmalloc(size, GFP_KERNEL);
+ if (!obj)
+ return -ENOMEM;
+
+ memset(obj, 0, size);
+ obj-array = array;
+ obj-n_elements = n_elements;
+
+ obj-kobj.ktype = ktype_attr_array;
+ obj-kobj.parent = kobj;
+ kobject_set_name(obj-kobj, array-attr.name);
+
+ error =
Re: [PATCH 0/5] I8K driver facelift
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 [EMAIL PROTECTED] wrote: > Well, (a) the next rev of the patch will hopefully provide more access to the > second thermal probe than just detecting its existence (it still doesn't do > the sysfs or whatever magic to make the actual value accessible), and (b) the > probe I *know* about is on the CPU, and runs over 40C easily as well (it's > sitting > at 49C right now). Remember we're talking about a laptop here, there's not > a lot of room for a big heat sink in there.. ;) I've been trying to work out how to do this through dynamic sysfs attributes, but I have not found a way to create arbitrary attributes like this. It's not hard to define them at kernel compile time, but selecting the right number of sensors to compile in seems arbitrary. My Inspiron 9200 has 4 sensors, and who knows how many next year's model will have. It just doesn't seem like the Linux Kernel way of doing things to arbitrarily limit it like this. I've looked into several ways of creating sysfs attributes, but haven't found anything that works right/well. One of the most interesting was in this past LKML thread - http://lkml.org/lkml/2004/8/20/287 If I could replace the sysfs_attr_show() with my own, I believe that might work (the attribute is passed into the function, so the name should be available). It's odd that it's so easy to compile sysfs attributes into the kernel, but nobody seems to know how to generate them dynamically. Thoughts? Suggestions? Thanks, Frank - -- Frank Sorenson - KD7TZK Systems Manager, Computer Science Department Brigham Young University [EMAIL PROTECTED] -BEGIN PGP SIGNATURE- Version: GnuPG v1.2.6 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org iD8DBQFCN2LeaI0dwg4A47wRAhWEAKC+CcoLmoyvS6RXy7n7gtTnKjPXsACgtCbE zofgMMEmc5mAzrQKdKwpIMQ= =xNOU -END PGP SIGNATURE- - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] I8K driver facelift
On Tue, 15 Mar 2005 11:59:22 +0100, Giuseppe Bilotta said: > > According to your patch, the C840 has 2 temp sensors. I'll have to figure > > out what the second one is (prob either the GPU or the disk drive?) > > If it runs over 40 C easily it's probably the GPU :) Well, (a) the next rev of the patch will hopefully provide more access to the second thermal probe than just detecting its existence (it still doesn't do the sysfs or whatever magic to make the actual value accessible), and (b) the probe I *know* about is on the CPU, and runs over 40C easily as well (it's sitting at 49C right now). Remember we're talking about a laptop here, there's not a lot of room for a big heat sink in there.. ;) pgpiRP3tti0Q6.pgp Description: PGP signature
Re: [PATCH 0/5] I8K driver facelift
> According to your patch, the C840 has 2 temp sensors. I'll have to figure > out what the second one is (prob either the GPU or the disk drive?) If it runs over 40 C easily it's probably the GPU :) -- Giuseppe "Oblomov" Bilotta Can't you see It all makes perfect sense Expressed in dollar and cents Pounds shillings and pence (Roger Waters) - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] I8K driver facelift
On Sat, 12 Mar 2005 20:41:14 MST, Frank Sorenson said: > These patches look pretty good. A few comments (with a patch--tested on > my Inspiron 9200): I tested your patch on top of Dmitry's on a Dell Latitude C840, seems to work. > - - Some of the Dell motherboards provide more than 1 temperature sensor. > ~ How about a generic i8k_get_temp function, and i8k_get_cpu_temp just > calls that with sensor 0. > > - - Also, I've added detection of the number of temperature sensors and > fans at init time. This way, we aren't hardcoded to 1 sensor and 2 > fans. I couldn't figure out how to set up the sysfs entries > dynamically, but that probably should happen too. According to your patch, the C840 has 2 temp sensors. I'll have to figure out what the second one is (prob either the GPU or the disk drive?) pgp3OjiePzAAX.pgp Description: PGP signature
Re: [PATCH 0/5] I8K driver facelift
On Sat, 12 Mar 2005 20:41:14 MST, Frank Sorenson said: These patches look pretty good. A few comments (with a patch--tested on my Inspiron 9200): I tested your patch on top of Dmitry's on a Dell Latitude C840, seems to work. - - Some of the Dell motherboards provide more than 1 temperature sensor. ~ How about a generic i8k_get_temp function, and i8k_get_cpu_temp just calls that with sensor 0. - - Also, I've added detection of the number of temperature sensors and fans at init time. This way, we aren't hardcoded to 1 sensor and 2 fans. I couldn't figure out how to set up the sysfs entries dynamically, but that probably should happen too. According to your patch, the C840 has 2 temp sensors. I'll have to figure out what the second one is (prob either the GPU or the disk drive?) pgp3OjiePzAAX.pgp Description: PGP signature
Re: [PATCH 0/5] I8K driver facelift
According to your patch, the C840 has 2 temp sensors. I'll have to figure out what the second one is (prob either the GPU or the disk drive?) If it runs over 40 C easily it's probably the GPU :) -- Giuseppe Oblomov Bilotta Can't you see It all makes perfect sense Expressed in dollar and cents Pounds shillings and pence (Roger Waters) - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] I8K driver facelift
On Tue, 15 Mar 2005 11:59:22 +0100, Giuseppe Bilotta said: According to your patch, the C840 has 2 temp sensors. I'll have to figure out what the second one is (prob either the GPU or the disk drive?) If it runs over 40 C easily it's probably the GPU :) Well, (a) the next rev of the patch will hopefully provide more access to the second thermal probe than just detecting its existence (it still doesn't do the sysfs or whatever magic to make the actual value accessible), and (b) the probe I *know* about is on the CPU, and runs over 40C easily as well (it's sitting at 49C right now). Remember we're talking about a laptop here, there's not a lot of room for a big heat sink in there.. ;) pgpiRP3tti0Q6.pgp Description: PGP signature
Re: [PATCH 0/5] I8K driver facelift
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 [EMAIL PROTECTED] wrote: Well, (a) the next rev of the patch will hopefully provide more access to the second thermal probe than just detecting its existence (it still doesn't do the sysfs or whatever magic to make the actual value accessible), and (b) the probe I *know* about is on the CPU, and runs over 40C easily as well (it's sitting at 49C right now). Remember we're talking about a laptop here, there's not a lot of room for a big heat sink in there.. ;) I've been trying to work out how to do this through dynamic sysfs attributes, but I have not found a way to create arbitrary attributes like this. It's not hard to define them at kernel compile time, but selecting the right number of sensors to compile in seems arbitrary. My Inspiron 9200 has 4 sensors, and who knows how many next year's model will have. It just doesn't seem like the Linux Kernel way of doing things to arbitrarily limit it like this. I've looked into several ways of creating sysfs attributes, but haven't found anything that works right/well. One of the most interesting was in this past LKML thread - http://lkml.org/lkml/2004/8/20/287 If I could replace the sysfs_attr_show() with my own, I believe that might work (the attribute is passed into the function, so the name should be available). It's odd that it's so easy to compile sysfs attributes into the kernel, but nobody seems to know how to generate them dynamically. Thoughts? Suggestions? Thanks, Frank - -- Frank Sorenson - KD7TZK Systems Manager, Computer Science Department Brigham Young University [EMAIL PROTECTED] -BEGIN PGP SIGNATURE- Version: GnuPG v1.2.6 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org iD8DBQFCN2LeaI0dwg4A47wRAhWEAKC+CcoLmoyvS6RXy7n7gtTnKjPXsACgtCbE zofgMMEmc5mAzrQKdKwpIMQ= =xNOU -END PGP SIGNATURE- - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] I8K driver facelift
On Saturday 12 March 2005 22:41, Frank Sorenson wrote: > Dmitry Torokhov wrote: > | Hi, > | > | here are some changes that freshen I8K driver (Dell Inspiron/Latitude > | platform driver). The patches have been tested on Inspiron 8100. > > | Please consider for inclusion. > | > | Thanks! > > These patches look pretty good. A few comments (with a patch--tested on > my Inspiron 9200): > > - The "return i8k_smm() < 0 ? : regs.eax;" construction is nice and > tidy, but it isn't passing on the return value of the called function, > and is returning TRUE or 1 on failure. This makes it difficult to check > the return value for valid data. Old behavior returned negative, so > I'll return -1. Hi, Actually I am not sure what I was thinkinhg when I wrtote it, the correct version should be "return i8k_smm() ? : regs.eax;" since i8k_smm return 0 on success. I will think about dynamically adding attributes... -- Dmitry - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] I8K driver facelift
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
Dmitry Torokhov wrote:
| Hi,
|
| here are some changes that freshen I8K driver (Dell Inspiron/Latitude
| platform driver). The patches have been tested on Inspiron 8100.
| Please consider for inclusion.
|
| Thanks!
These patches look pretty good. A few comments (with a patch--tested on
my Inspiron 9200):
- - The "return i8k_smm() < 0 ? : regs.eax;" construction is nice and
tidy, but it isn't passing on the return value of the called function,
and is returning TRUE or 1 on failure. This makes it difficult to check
the return value for valid data. Old behavior returned negative, so
I'll return -1.
- - The I8K_VERSION string should probably be changed to something more
unique. The version maintained outside the kernel tree by Massimo Dal
Zotto (http://www.debian.org/~dz/i8k/) is up to 1.25, so 1.14 isn't very
distinguishing. Maybe just use the date? Not sure here...
- - Also, some newer Dell laptops require a different function to get the
Dell signature, so the i8k_get_dell_signature test should check both
(borrowing some code from the out-of-kernel version).
- - Some newer Dell laptops report DMI_SYS_VENDOR as "Dell Inc." rather
than "Dell Computer"
- - Some of the Dell motherboards provide more than 1 temperature sensor.
~ How about a generic i8k_get_temp function, and i8k_get_cpu_temp just
calls that with sensor 0.
- - Also, I've added detection of the number of temperature sensors and
fans at init time. This way, we aren't hardcoded to 1 sensor and 2
fans. I couldn't figure out how to set up the sysfs entries
dynamically, but that probably should happen too.
- - Some boards don't need the I8K_FAN_MULT to get the right fan RPM (I
don't think my fans are rotating at over 90,000 RPM)! I guess we'll
need to address this sometime, but I have not done so.
Patch follows:
Signed-off-by: Frank Sorenson <[EMAIL PROTECTED]>
- --- 2.6.11-a/drivers/char/i8k.c 2005-03-12 18:47:55.0 -0700
+++ 2.6.11-b/drivers/char/i8k.c 2005-03-12 20:29:01.0 -0700
@@ -28,7 +28,7 @@
~ #include
- -#define I8K_VERSION "1.14 21/02/2005"
+#define I8K_VERSION"2005-03-12"
~ #define I8K_SMM_FN_STATUS 0x0025
~ #define I8K_SMM_POWER_STATUS 0x0069
@@ -36,7 +36,8 @@
~ #define I8K_SMM_GET_FAN 0x00a3
~ #define I8K_SMM_GET_SPEED 0x02a3
~ #define I8K_SMM_GET_TEMP 0x10a3
- -#define I8K_SMM_GET_DELL_SIG 0xffa3
+#define I8K_SMM_GET_DELL_SIG1 0xfea3
+#define I8K_SMM_GET_DELL_SIG2 0xffa3
~ #define I8K_SMM_BIOS_VERSION 0x00a6
~ #define I8K_FAN_MULT 30
@@ -55,6 +56,8 @@
~ #define I8K_TEMPERATURE_BUG 1
~ static char bios_version[4];
+static int num_temps = 0;
+static int num_fans = 0;
~ MODULE_AUTHOR("Massimo Dal Zotto ([EMAIL PROTECTED])");
~ MODULE_DESCRIPTION("Driver for accessing SMM BIOS on Dell laptops");
@@ -201,10 +204,10 @@
~ */
~ static int i8k_get_fan_status(int fan)
~ {
- - struct smm_regs regs = { .eax = I8K_SMM_GET_FAN, };
+ struct smm_regs regs = { .eax = I8K_SMM_GET_FAN, .ebx = (fan & 0xff)};
~ regs.ebx = fan & 0xff;
- - return i8k_smm() < 0 ? : regs.eax & 0xff;
+ return i8k_smm() < 0 ? -1 : regs.eax & 0xff;
~ }
~ /*
@@ -215,7 +218,7 @@
~ struct smm_regs regs = { .eax = I8K_SMM_GET_SPEED, };
~ regs.ebx = fan & 0xff;
- - return i8k_smm() < 0 ? : (regs.eax & 0x) * I8K_FAN_MULT;
+ return i8k_smm() < 0 ? -1 : (regs.eax & 0x) * I8K_FAN_MULT;
~ }
~ /*
@@ -228,15 +231,15 @@
~ speed = (speed < 0) ? 0 : ((speed > I8K_FAN_MAX) ? I8K_FAN_MAX : speed);
~ regs.ebx = (fan & 0xff) | (speed << 8);
- - return i8k_smm() < 0 ? : i8k_get_fan_status(fan);
+ return i8k_smm() < 0 ? -1 : i8k_get_fan_status(fan);
~ }
~ /*
~ * Read the cpu temperature.
~ */
- -static int i8k_get_cpu_temp(void)
+static int i8k_get_temp(int sensor)
~ {
- - struct smm_regs regs = { .eax = I8K_SMM_GET_TEMP, };
+ struct smm_regs regs = { .eax = I8K_SMM_GET_TEMP, .ebx = sensor };
~ int rc;
~ int temp;
@@ -268,9 +271,14 @@
~ return temp;
~ }
- -static int i8k_get_dell_signature(void)
+static int i8k_get_cpu_temp(void)
+{
+ return i8k_get_temp(0);
+}
+
+static int i8k_get_dell_sig_aux(int fn)
~ {
- - struct smm_regs regs = { .eax = I8K_SMM_GET_DELL_SIG, };
+ struct smm_regs regs = { .eax = fn, };
~ int rc;
~ if ((rc = i8k_smm()) < 0)
@@ -279,6 +287,17 @@
~ return regs.eax == 1145651527 && regs.edx == 1145392204 ? 0 : -1;
~ }
+static int i8k_get_dell_signature(void)
+{
+ int rc;
+
+ if (((rc=i8k_get_dell_sig_aux(I8K_SMM_GET_DELL_SIG1)) < 0) &&
+ ((rc=i8k_get_dell_sig_aux(I8K_SMM_GET_DELL_SIG2)) < 0)) {
+ return rc;
+ }
+ return 0;
+}
+
~ static int i8k_ioctl(struct inode *ip, struct file *fp, unsigned int cmd,
~unsigned long arg)
~ {
@@ -506,6 +525,13 @@
~ },
~ },
~ {
+ .ident = "Dell Inspiron",
+
Re: [PATCH 0/5] I8K driver facelift
On Saturday 12 March 2005 22:41, Frank Sorenson wrote: Dmitry Torokhov wrote: | Hi, | | here are some changes that freshen I8K driver (Dell Inspiron/Latitude | platform driver). The patches have been tested on Inspiron 8100. snip | Please consider for inclusion. | | Thanks! These patches look pretty good. A few comments (with a patch--tested on my Inspiron 9200): - The return i8k_smm(regs) 0 ? : regs.eax; construction is nice and tidy, but it isn't passing on the return value of the called function, and is returning TRUE or 1 on failure. This makes it difficult to check the return value for valid data. Old behavior returned negative, so I'll return -1. Hi, Actually I am not sure what I was thinkinhg when I wrtote it, the correct version should be return i8k_smm(regs) ? : regs.eax; since i8k_smm return 0 on success. I will think about dynamically adding attributes... -- Dmitry - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] I8K driver facelift
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
Dmitry Torokhov wrote:
| Hi,
|
| here are some changes that freshen I8K driver (Dell Inspiron/Latitude
| platform driver). The patches have been tested on Inspiron 8100.
snip
| Please consider for inclusion.
|
| Thanks!
These patches look pretty good. A few comments (with a patch--tested on
my Inspiron 9200):
- - The return i8k_smm(regs) 0 ? : regs.eax; construction is nice and
tidy, but it isn't passing on the return value of the called function,
and is returning TRUE or 1 on failure. This makes it difficult to check
the return value for valid data. Old behavior returned negative, so
I'll return -1.
- - The I8K_VERSION string should probably be changed to something more
unique. The version maintained outside the kernel tree by Massimo Dal
Zotto (http://www.debian.org/~dz/i8k/) is up to 1.25, so 1.14 isn't very
distinguishing. Maybe just use the date? Not sure here...
- - Also, some newer Dell laptops require a different function to get the
Dell signature, so the i8k_get_dell_signature test should check both
(borrowing some code from the out-of-kernel version).
- - Some newer Dell laptops report DMI_SYS_VENDOR as Dell Inc. rather
than Dell Computer
- - Some of the Dell motherboards provide more than 1 temperature sensor.
~ How about a generic i8k_get_temp function, and i8k_get_cpu_temp just
calls that with sensor 0.
- - Also, I've added detection of the number of temperature sensors and
fans at init time. This way, we aren't hardcoded to 1 sensor and 2
fans. I couldn't figure out how to set up the sysfs entries
dynamically, but that probably should happen too.
- - Some boards don't need the I8K_FAN_MULT to get the right fan RPM (I
don't think my fans are rotating at over 90,000 RPM)! I guess we'll
need to address this sometime, but I have not done so.
Patch follows:
Signed-off-by: Frank Sorenson [EMAIL PROTECTED]
- --- 2.6.11-a/drivers/char/i8k.c 2005-03-12 18:47:55.0 -0700
+++ 2.6.11-b/drivers/char/i8k.c 2005-03-12 20:29:01.0 -0700
@@ -28,7 +28,7 @@
~ #include linux/i8k.h
- -#define I8K_VERSION 1.14 21/02/2005
+#define I8K_VERSION2005-03-12
~ #define I8K_SMM_FN_STATUS 0x0025
~ #define I8K_SMM_POWER_STATUS 0x0069
@@ -36,7 +36,8 @@
~ #define I8K_SMM_GET_FAN 0x00a3
~ #define I8K_SMM_GET_SPEED 0x02a3
~ #define I8K_SMM_GET_TEMP 0x10a3
- -#define I8K_SMM_GET_DELL_SIG 0xffa3
+#define I8K_SMM_GET_DELL_SIG1 0xfea3
+#define I8K_SMM_GET_DELL_SIG2 0xffa3
~ #define I8K_SMM_BIOS_VERSION 0x00a6
~ #define I8K_FAN_MULT 30
@@ -55,6 +56,8 @@
~ #define I8K_TEMPERATURE_BUG 1
~ static char bios_version[4];
+static int num_temps = 0;
+static int num_fans = 0;
~ MODULE_AUTHOR(Massimo Dal Zotto ([EMAIL PROTECTED]));
~ MODULE_DESCRIPTION(Driver for accessing SMM BIOS on Dell laptops);
@@ -201,10 +204,10 @@
~ */
~ static int i8k_get_fan_status(int fan)
~ {
- - struct smm_regs regs = { .eax = I8K_SMM_GET_FAN, };
+ struct smm_regs regs = { .eax = I8K_SMM_GET_FAN, .ebx = (fan 0xff)};
~ regs.ebx = fan 0xff;
- - return i8k_smm(regs) 0 ? : regs.eax 0xff;
+ return i8k_smm(regs) 0 ? -1 : regs.eax 0xff;
~ }
~ /*
@@ -215,7 +218,7 @@
~ struct smm_regs regs = { .eax = I8K_SMM_GET_SPEED, };
~ regs.ebx = fan 0xff;
- - return i8k_smm(regs) 0 ? : (regs.eax 0x) * I8K_FAN_MULT;
+ return i8k_smm(regs) 0 ? -1 : (regs.eax 0x) * I8K_FAN_MULT;
~ }
~ /*
@@ -228,15 +231,15 @@
~ speed = (speed 0) ? 0 : ((speed I8K_FAN_MAX) ? I8K_FAN_MAX : speed);
~ regs.ebx = (fan 0xff) | (speed 8);
- - return i8k_smm(regs) 0 ? : i8k_get_fan_status(fan);
+ return i8k_smm(regs) 0 ? -1 : i8k_get_fan_status(fan);
~ }
~ /*
~ * Read the cpu temperature.
~ */
- -static int i8k_get_cpu_temp(void)
+static int i8k_get_temp(int sensor)
~ {
- - struct smm_regs regs = { .eax = I8K_SMM_GET_TEMP, };
+ struct smm_regs regs = { .eax = I8K_SMM_GET_TEMP, .ebx = sensor };
~ int rc;
~ int temp;
@@ -268,9 +271,14 @@
~ return temp;
~ }
- -static int i8k_get_dell_signature(void)
+static int i8k_get_cpu_temp(void)
+{
+ return i8k_get_temp(0);
+}
+
+static int i8k_get_dell_sig_aux(int fn)
~ {
- - struct smm_regs regs = { .eax = I8K_SMM_GET_DELL_SIG, };
+ struct smm_regs regs = { .eax = fn, };
~ int rc;
~ if ((rc = i8k_smm(regs)) 0)
@@ -279,6 +287,17 @@
~ return regs.eax == 1145651527 regs.edx == 1145392204 ? 0 : -1;
~ }
+static int i8k_get_dell_signature(void)
+{
+ int rc;
+
+ if (((rc=i8k_get_dell_sig_aux(I8K_SMM_GET_DELL_SIG1)) 0)
+ ((rc=i8k_get_dell_sig_aux(I8K_SMM_GET_DELL_SIG2)) 0)) {
+ return rc;
+ }
+ return 0;
+}
+
~ static int i8k_ioctl(struct inode *ip, struct file *fp, unsigned int cmd,
~unsigned long arg)
~ {
@@ -506,6 +525,13 @@
~ },
~ },
~ {
+ .ident = Dell Inspiron,
+
[PATCH 0/5] I8K driver facelift
Hi,
here are some changes that freshen I8K driver (Dell Inspiron/Latitude
platform driver). The patches have been tested on Inspiron 8100.
i8k-lindent.patch
- pass the driver through Lindent to comply with CondingStyle requirements
(4 spaces vs. TAB indentation)
i8k-use-dmi.patch
- use standard DMI handling functions instead of homemade ones. The driver
now requires DMI data to match list of supported models - this way driver
can be safely enabled without fear of it poking into SMM code on wrong
box. DMI checks can be ignored with i8k.ignore_dmi option.
i8k-seqfile.patch
- switch proc handlig code to seq_file instead of having custom read
function splitting output to fit into user's buffer.
i8k-cleanup.patch
- use module_{init|exit} instead of old-style module intialization code,
some formatting changes.
i8k-sysfs.patch
- make i8k a platform device and export temperatiure and both fan states
as sysfs attributes. Wringing into fan1_state and fan2_state attributes
allows switching fans on and off without need for special utility.
Please consider for inclusion.
Thanks!
--
Dmitry
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 0/5] I8K driver facelift
Hi,
here are some changes that freshen I8K driver (Dell Inspiron/Latitude
platform driver). The patches have been tested on Inspiron 8100.
i8k-lindent.patch
- pass the driver through Lindent to comply with CondingStyle requirements
(4 spaces vs. TAB indentation)
i8k-use-dmi.patch
- use standard DMI handling functions instead of homemade ones. The driver
now requires DMI data to match list of supported models - this way driver
can be safely enabled without fear of it poking into SMM code on wrong
box. DMI checks can be ignored with i8k.ignore_dmi option.
i8k-seqfile.patch
- switch proc handlig code to seq_file instead of having custom read
function splitting output to fit into user's buffer.
i8k-cleanup.patch
- use module_{init|exit} instead of old-style module intialization code,
some formatting changes.
i8k-sysfs.patch
- make i8k a platform device and export temperatiure and both fan states
as sysfs attributes. Wringing into fan1_state and fan2_state attributes
allows switching fans on and off without need for special utility.
Please consider for inclusion.
Thanks!
--
Dmitry
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
