i8k: move driver from char to hwmon

i8k: move driver from char to hwmon

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1446 bytes --]

Hello,

currently i8k driver for DELLDIAG SMM operations (charger, 
battery, hotkeys, temperature and fan control) supports two 
interfaces:

1) old /proc/i8k char device
2) new hwmon interface

Driver now lives in char subsystem, but nowadays I think lot of 
people are interested only in hwmon interface which provides 
temperature & fan control. Power management and hotkeys support 
(which is missing in 2) but provided by 1)) is already handled by 
ACPI or input subsystem for new laptops.

Also name "i8k" is really non intuitive what it means and 
supports... (I was told it is abbreviation for Inspirion 8000 
model which was probably first dell model supported by that 
DELLDIAG interface). Now driver is used on Vostro, Latitude and 
also Precision and XPS models. I think at least "dell" should be 
part of driver name and config option.

So I have idea for some changes:

1) Move driver from char subsystem to hwmon

2) Rename driver from i8k to something like delldiag or dellsmm

3) Add new CONFIG_ option to enable/disable /proc/i8k device and 
compile only hwmon interface support

What do you think about it?

I do not want to delete /proc/i8k interface (or break support for 
it) as there are still userspace applications which use this old 
interface and for old models it is probably only way how to 
detect power supply and multimedia hotkeys...

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: i8k: move driver from char to hwmon

[Guenter Roeck]

On 02/22/2015 03:50 AM, Pali Rohár wrote:
> Hello,
>
> currently i8k driver for DELLDIAG SMM operations (charger,
> battery, hotkeys, temperature and fan control) supports two
> interfaces:
>
> 1) old /proc/i8k char device
> 2) new hwmon interface
>
> Driver now lives in char subsystem, but nowadays I think lot of
> people are interested only in hwmon interface which provides
> temperature & fan control. Power management and hotkeys support
> (which is missing in 2) but provided by 1)) is already handled by
> ACPI or input subsystem for new laptops.
>
> Also name "i8k" is really non intuitive what it means and
> supports... (I was told it is abbreviation for Inspirion 8000
> model which was probably first dell model supported by that
> DELLDIAG interface). Now driver is used on Vostro, Latitude and
> also Precision and XPS models. I think at least "dell" should be
> part of driver name and config option.
>
> So I have idea for some changes:
>
> 1) Move driver from char subsystem to hwmon
>
I am ok with this one.

> 2) Rename driver from i8k to something like delldiag or dellsmm
>
Might be problematic, as it would break existing configurations
unless you keep the CONFIG_I8K but rename the driver. Driver names
are never perfect, so I would just leave it as is.

> 3) Add new CONFIG_ option to enable/disable /proc/i8k device and
> compile only hwmon interface support

Makes sense. I would suggest to make hwmon suppport the default
and mandatory if the driver is enabled and add a new CONFIG_I8K_PROCIF
or similar which should be enabled by default.

Are you interested in adding yourself as maintainer for this driver ?

Thanks,
Guenter

Re: i8k: move driver from char to hwmon

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2141 bytes --]

On Sunday 22 February 2015 18:33:16 Guenter Roeck wrote:
> On 02/22/2015 03:50 AM, Pali Rohár wrote:
> > Hello,
> > 
> > currently i8k driver for DELLDIAG SMM operations (charger,
> > battery, hotkeys, temperature and fan control) supports two
> > interfaces:
> > 
> > 1) old /proc/i8k char device
> > 2) new hwmon interface
> > 
> > Driver now lives in char subsystem, but nowadays I think lot
> > of people are interested only in hwmon interface which
> > provides temperature & fan control. Power management and
> > hotkeys support (which is missing in 2) but provided by 1))
> > is already handled by ACPI or input subsystem for new
> > laptops.
> > 
> > Also name "i8k" is really non intuitive what it means and
> > supports... (I was told it is abbreviation for Inspirion
> > 8000 model which was probably first dell model supported by
> > that DELLDIAG interface). Now driver is used on Vostro,
> > Latitude and also Precision and XPS models. I think at
> > least "dell" should be part of driver name and config
> > option.
> > 
> > So I have idea for some changes:
> > 
> > 1) Move driver from char subsystem to hwmon
> 
> I am ok with this one.
> 
> > 2) Rename driver from i8k to something like delldiag or
> > dellsmm
> 
> Might be problematic, as it would break existing
> configurations unless you keep the CONFIG_I8K but rename the
> driver. Driver names are never perfect, so I would just leave
> it as is.
> 
> > 3) Add new CONFIG_ option to enable/disable /proc/i8k device
> > and compile only hwmon interface support
> 
> Makes sense. I would suggest to make hwmon suppport the
> default and mandatory if the driver is enabled and add a new
> CONFIG_I8K_PROCIF or similar which should be enabled by
> default.
> 

What about CONFIG_DELLDIAG (or DELLSMM or CONFIG_SENSORS_*) for 
mandatory core & hwmon code and (keep existing) CONFIG_I8K for 
/proc/i8k?

> Are you interested in adding yourself as maintainer for this
> driver ?
> 

Ok I could. But I do not know how much time will have in future.

> Thanks,
> Guenter

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: i8k: move driver from char to hwmon

[Guenter Roeck]

On 02/22/2015 09:44 AM, Pali Rohár wrote:
> On Sunday 22 February 2015 18:33:16 Guenter Roeck wrote:
>> On 02/22/2015 03:50 AM, Pali Rohár wrote:
>>> Hello,
>>>
>>> currently i8k driver for DELLDIAG SMM operations (charger,
>>> battery, hotkeys, temperature and fan control) supports two
>>> interfaces:
>>>
>>> 1) old /proc/i8k char device
>>> 2) new hwmon interface
>>>
>>> Driver now lives in char subsystem, but nowadays I think lot
>>> of people are interested only in hwmon interface which
>>> provides temperature & fan control. Power management and
>>> hotkeys support (which is missing in 2) but provided by 1))
>>> is already handled by ACPI or input subsystem for new
>>> laptops.
>>>
>>> Also name "i8k" is really non intuitive what it means and
>>> supports... (I was told it is abbreviation for Inspirion
>>> 8000 model which was probably first dell model supported by
>>> that DELLDIAG interface). Now driver is used on Vostro,
>>> Latitude and also Precision and XPS models. I think at
>>> least "dell" should be part of driver name and config
>>> option.
>>>
>>> So I have idea for some changes:
>>>
>>> 1) Move driver from char subsystem to hwmon
>>
>> I am ok with this one.
>>
>>> 2) Rename driver from i8k to something like delldiag or
>>> dellsmm
>>
>> Might be problematic, as it would break existing
>> configurations unless you keep the CONFIG_I8K but rename the
>> driver. Driver names are never perfect, so I would just leave
>> it as is.
>>
>>> 3) Add new CONFIG_ option to enable/disable /proc/i8k device
>>> and compile only hwmon interface support
>>
>> Makes sense. I would suggest to make hwmon suppport the
>> default and mandatory if the driver is enabled and add a new
>> CONFIG_I8K_PROCIF or similar which should be enabled by
>> default.
>>
>
> What about CONFIG_DELLDIAG (or DELLSMM or CONFIG_SENSORS_*) for
> mandatory core & hwmon code and (keep existing) CONFIG_I8K for
> /proc/i8k?
>
CONFIG_SENSORS_DELL would probably be best, but that would still
break backward compatibility for existing configurations unless
you enable CONFIG_SENSORS_DELL by default which would not be
a good idea.

Guenter

Re: i8k: move driver from char to hwmon

[Guenter Roeck]

On 02/22/2015 09:44 AM, Pali Rohár wrote:
> On Sunday 22 February 2015 18:33:16 Guenter Roeck wrote:
>>
>>> 2) Rename driver from i8k to something like delldiag or
>>> dellsmm
>>
>> Might be problematic, as it would break existing
>> configurations unless you keep the CONFIG_I8K but rename the
>> driver. Driver names are never perfect, so I would just leave
>> it as is.
>>
>>> 3) Add new CONFIG_ option to enable/disable /proc/i8k device
>>> and compile only hwmon interface support
>>
>> Makes sense. I would suggest to make hwmon suppport the
>> default and mandatory if the driver is enabled and add a new
>> CONFIG_I8K_PROCIF or similar which should be enabled by
>> default.
>>
>
> What about CONFIG_DELLDIAG (or DELLSMM or CONFIG_SENSORS_*) for
> mandatory core & hwmon code and (keep existing) CONFIG_I8K for
> /proc/i8k?
>

One option might be to leave CONFIG_I8K more or less alone
(it is in arch/x86/Kconfig anyway, which is odd by itself),
but add something like
	select SENSORS_DELL
	select I8K_PROC
to it. Then add new config options SENSORS_DELL and
I8K_PROC to drivers/hwmon/Kconfig.

I would still leave the driver name alone, though; the problem
is that "modprobe i8k" is mentioned in pretty much all references
to the driver.

We could try to make it two separate drivers, one for the hwmon
interface and one for the proc interface (which would depend on
the hwmon driver), but I am not sure if that is worth it or even
feasible. It would save us a bunch of ifdefs in the code, though,
so it might be worth looking into. In that case you would have a
dell hwmon driver and a dell i8k driver (which would only exist if
I8K_PROC is configured).

Guenter

Re: i8k: move driver from char to hwmon

[Jean Delvare]

On Sun, 22 Feb 2015 10:11:16 -0800, Guenter Roeck wrote:
> I would still leave the driver name alone, though; the problem
> is that "modprobe i8k" is mentioned in pretty much all references
> to the driver.

This might be solved with a module alias? You can pass any arbitrary
string to MODULE_ALIAS(). This would still break insmod but pretty much
everyone is calling modprobe to load kernel modules anyway.

-- 
Jean Delvare
SUSE L3 Support

Re: i8k: move driver from char to hwmon

[Guenter Roeck]

On 02/22/2015 02:07 PM, Jean Delvare wrote:
> On Sun, 22 Feb 2015 10:11:16 -0800, Guenter Roeck wrote:
>> I would still leave the driver name alone, though; the problem
>> is that "modprobe i8k" is mentioned in pretty much all references
>> to the driver.
>
> This might be solved with a module alias? You can pass any arbitrary
> string to MODULE_ALIAS(). This would still break insmod but pretty much
> everyone is calling modprobe to load kernel modules anyway.
>

You are right, that might work.

Thanks,
Guenter

Re: i8k: move driver from char to hwmon

[Jean Delvare]

On Sun, 22 Feb 2015 10:11:16 -0800, Guenter Roeck wrote:
> On 02/22/2015 09:44 AM, Pali Rohár wrote:
> > What about CONFIG_DELLDIAG (or DELLSMM or CONFIG_SENSORS_*) for
> > mandatory core & hwmon code and (keep existing) CONFIG_I8K for
> > /proc/i8k?
> 
> One option might be to leave CONFIG_I8K more or less alone
> (it is in arch/x86/Kconfig anyway, which is odd by itself),
> but add something like
> 	select SENSORS_DELL
> 	select I8K_PROC
> to it. Then add new config options SENSORS_DELL and
> I8K_PROC to drivers/hwmon/Kconfig.

On a related note...

Changing Kconfig symbol names is allowed, it has happened a lot in the
past, although we should make it the least painful possible for the
users.

I thought about it earlier as I considered several times the
possibility to rename all CONFIG_SENSORS_* symbols to CONFIG_HWMON_*
for consistency. I never actually took the time to do it, but here's
how I would do it, taking the lm75 driver as an example.

As a first step, I would introduce a hidden symbol that would be
selected automatically by the existing symbol:

 drivers/hwmon/Kconfig  |    4 ++++
 drivers/hwmon/Makefile |    2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

--- linux-4.1-rc0.orig/drivers/hwmon/Kconfig	2015-04-22 09:54:21.965974944 +0200
+++ linux-4.1-rc0/drivers/hwmon/Kconfig	2015-04-22 10:10:16.974551936 +0200
@@ -909,10 +909,14 @@ config SENSORS_LM73
 	  This driver can also be built as a module.  If so, the module
 	  will be called lm73.
 
+config HWMON_LM75
+	tristate
+
 config SENSORS_LM75
 	tristate "National Semiconductor LM75 and compatibles"
 	depends on I2C
 	depends on THERMAL || !THERMAL_OF
+	select HWMON_LM75
 	help
 	  If you say yes here you get support for one common type of
 	  temperature sensor chip, with models including:
--- linux-4.1-rc0.orig/drivers/hwmon/Makefile	2015-04-22 09:54:21.965974944 +0200
+++ linux-4.1-rc0/drivers/hwmon/Makefile	2015-04-22 10:09:03.712048849 +0200
@@ -85,7 +85,7 @@ obj-$(CONFIG_SENSORS_LINEAGE)	+= lineage
 obj-$(CONFIG_SENSORS_LM63)	+= lm63.o
 obj-$(CONFIG_SENSORS_LM70)	+= lm70.o
 obj-$(CONFIG_SENSORS_LM73)	+= lm73.o
-obj-$(CONFIG_SENSORS_LM75)	+= lm75.o
+obj-$(CONFIG_HWMON_LM75)	+= lm75.o
 obj-$(CONFIG_SENSORS_LM77)	+= lm77.o
 obj-$(CONFIG_SENSORS_LM78)	+= lm78.o
 obj-$(CONFIG_SENSORS_LM80)	+= lm80.o

Running "make oldconfig" after that silently selects the new symbol as
needed:

--- .config.old	2015-04-20 10:16:42.645359313 +0200
+++ .config	2015-04-22 10:09:09.358164599 +0200
@@ -2805,6 +2805,7 @@
 CONFIG_SENSORS_LM63=m
 # CONFIG_SENSORS_LM70 is not set
 CONFIG_SENSORS_LM73=m
+CONFIG_HWMON_LM75=m
 CONFIG_SENSORS_LM75=m
 CONFIG_SENSORS_LM77=m
 CONFIG_SENSORS_LM78=m

Then I would let several kernel versions pass. After a year maybe, or 5
kernel versions, we could get rid of the original symbol:

 drivers/hwmon/Kconfig |    4 ----
 1 file changed, 4 deletions(-)

--- linux-4.1-rc0.orig/drivers/hwmon/Kconfig	2015-04-22 10:08:39.017542771 +0200
+++ linux-4.1-rc0/drivers/hwmon/Kconfig	2015-04-22 10:09:57.376149603 +0200
@@ -910,13 +910,9 @@ config SENSORS_LM73
 	  will be called lm73.
 
 config HWMON_LM75
-	tristate
-
-config SENSORS_LM75
 	tristate "National Semiconductor LM75 and compatibles"
 	depends on I2C
 	depends on THERMAL || !THERMAL_OF
-	select HWMON_LM75
 	help
 	  If you say yes here you get support for one common type of
 	  temperature sensor chip, with models including:

Again running "make oldconfig" will do the right thing silently:

--- .config.old	2015-04-22 10:09:09.358164599 +0200
+++ .config	2015-04-22 10:10:02.942263851 +0200
@@ -2806,7 +2806,6 @@
 # CONFIG_SENSORS_LM70 is not set
 CONFIG_SENSORS_LM73=m
 CONFIG_HWMON_LM75=m
-CONFIG_SENSORS_LM75=m
 CONFIG_SENSORS_LM77=m
 CONFIG_SENSORS_LM78=m
 CONFIG_SENSORS_LM80=m

This allows for a transparent migration to the new names from a user's
perspective, as long as they do not jump directly from the original
state to the final state. Maybe something like that can be done for
CONFIG_I8K if you want to rename it.

-- 
Jean Delvare
SUSE L3 Support

Re: i8k: move driver from char to hwmon

[Pali Rohár]

On Wednesday 22 April 2015 10:23:04 Jean Delvare wrote:
> On Sun, 22 Feb 2015 10:11:16 -0800, Guenter Roeck wrote:
> > On 02/22/2015 09:44 AM, Pali Rohár wrote:
> > > What about CONFIG_DELLDIAG (or DELLSMM or CONFIG_SENSORS_*) for
> > > mandatory core & hwmon code and (keep existing) CONFIG_I8K for
> > > /proc/i8k?
> > 
> > One option might be to leave CONFIG_I8K more or less alone
> > (it is in arch/x86/Kconfig anyway, which is odd by itself),
> > but add something like
> > 	select SENSORS_DELL
> > 	select I8K_PROC
> > to it. Then add new config options SENSORS_DELL and
> > I8K_PROC to drivers/hwmon/Kconfig.
> 
> On a related note...
> 
> Changing Kconfig symbol names is allowed, it has happened a lot in the
> past, although we should make it the least painful possible for the
> users.
> 
> I thought about it earlier as I considered several times the
> possibility to rename all CONFIG_SENSORS_* symbols to CONFIG_HWMON_*
> for consistency. I never actually took the time to do it, but here's
> how I would do it, taking the lm75 driver as an example.
> 
> As a first step, I would introduce a hidden symbol that would be
> selected automatically by the existing symbol:
> 
>  drivers/hwmon/Kconfig  |    4 ++++
>  drivers/hwmon/Makefile |    2 +-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> --- linux-4.1-rc0.orig/drivers/hwmon/Kconfig	2015-04-22 09:54:21.965974944 +0200
> +++ linux-4.1-rc0/drivers/hwmon/Kconfig	2015-04-22 10:10:16.974551936 +0200
> @@ -909,10 +909,14 @@ config SENSORS_LM73
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called lm73.
>  
> +config HWMON_LM75
> +	tristate
> +
>  config SENSORS_LM75
>  	tristate "National Semiconductor LM75 and compatibles"
>  	depends on I2C
>  	depends on THERMAL || !THERMAL_OF
> +	select HWMON_LM75
>  	help
>  	  If you say yes here you get support for one common type of
>  	  temperature sensor chip, with models including:
> --- linux-4.1-rc0.orig/drivers/hwmon/Makefile	2015-04-22 09:54:21.965974944 +0200
> +++ linux-4.1-rc0/drivers/hwmon/Makefile	2015-04-22 10:09:03.712048849 +0200
> @@ -85,7 +85,7 @@ obj-$(CONFIG_SENSORS_LINEAGE)	+= lineage
>  obj-$(CONFIG_SENSORS_LM63)	+= lm63.o
>  obj-$(CONFIG_SENSORS_LM70)	+= lm70.o
>  obj-$(CONFIG_SENSORS_LM73)	+= lm73.o
> -obj-$(CONFIG_SENSORS_LM75)	+= lm75.o
> +obj-$(CONFIG_HWMON_LM75)	+= lm75.o
>  obj-$(CONFIG_SENSORS_LM77)	+= lm77.o
>  obj-$(CONFIG_SENSORS_LM78)	+= lm78.o
>  obj-$(CONFIG_SENSORS_LM80)	+= lm80.o
> 
> Running "make oldconfig" after that silently selects the new symbol as
> needed:
> 
> --- .config.old	2015-04-20 10:16:42.645359313 +0200
> +++ .config	2015-04-22 10:09:09.358164599 +0200
> @@ -2805,6 +2805,7 @@
>  CONFIG_SENSORS_LM63=m
>  # CONFIG_SENSORS_LM70 is not set
>  CONFIG_SENSORS_LM73=m
> +CONFIG_HWMON_LM75=m
>  CONFIG_SENSORS_LM75=m
>  CONFIG_SENSORS_LM77=m
>  CONFIG_SENSORS_LM78=m
> 
> Then I would let several kernel versions pass. After a year maybe, or 5
> kernel versions, we could get rid of the original symbol:
> 
>  drivers/hwmon/Kconfig |    4 ----
>  1 file changed, 4 deletions(-)
> 
> --- linux-4.1-rc0.orig/drivers/hwmon/Kconfig	2015-04-22 10:08:39.017542771 +0200
> +++ linux-4.1-rc0/drivers/hwmon/Kconfig	2015-04-22 10:09:57.376149603 +0200
> @@ -910,13 +910,9 @@ config SENSORS_LM73
>  	  will be called lm73.
>  
>  config HWMON_LM75
> -	tristate
> -
> -config SENSORS_LM75
>  	tristate "National Semiconductor LM75 and compatibles"
>  	depends on I2C
>  	depends on THERMAL || !THERMAL_OF
> -	select HWMON_LM75
>  	help
>  	  If you say yes here you get support for one common type of
>  	  temperature sensor chip, with models including:
> 
> Again running "make oldconfig" will do the right thing silently:
> 
> --- .config.old	2015-04-22 10:09:09.358164599 +0200
> +++ .config	2015-04-22 10:10:02.942263851 +0200
> @@ -2806,7 +2806,6 @@
>  # CONFIG_SENSORS_LM70 is not set
>  CONFIG_SENSORS_LM73=m
>  CONFIG_HWMON_LM75=m
> -CONFIG_SENSORS_LM75=m
>  CONFIG_SENSORS_LM77=m
>  CONFIG_SENSORS_LM78=m
>  CONFIG_SENSORS_LM80=m
> 
> This allows for a transparent migration to the new names from a user's
> perspective, as long as they do not jump directly from the original
> state to the final state. Maybe something like that can be done for
> CONFIG_I8K if you want to rename it.
> 

Hi, I already sent rename patch series, see [1]. And Greg will merge it
into 4.1-rc2, see [2].

[1] - https://lkml.org/lkml/2015/3/29/35
[2] - https://lkml.org/lkml/2015/4/21/429

-- 
Pali Rohár
pali.rohar@gmail.com