[RFC,v2,2/3] i2c: Kconfig: rename variable I2C_SMBUS to I2C_SMBUS_ALERT

Message ID 1511840309-37964-3-git-send-email-preid@electromag.com.au
State New
Headers show
Series
  • i2c: core: move smbus_alert into i2c-core
Related show

Commit Message

Phil Reid Nov. 28, 2017, 3:38 a.m.
i2c-smbus now only contains code related to the smbus_alert driver.
Other smbus protocols have been moved from this into the i2c-core-smbus.
Change the Kconfig variable name to reflect this.

Signed-off-by: Phil Reid <preid@electromag.com.au>
---
 arch/blackfin/configs/BF527-TLL6527M_defconfig |  2 +-
 drivers/i2c/Kconfig                            | 11 +++++------
 drivers/i2c/Makefile                           |  2 +-
 drivers/i2c/busses/Kconfig                     |  8 ++++----
 drivers/i2c/i2c-core-smbus.c                   |  2 +-
 drivers/i2c/i2c-core.h                         |  2 +-
 drivers/power/supply/Kconfig                   |  2 +-
 7 files changed, 14 insertions(+), 15 deletions(-)

Comments

Jean Delvare Nov. 28, 2017, 10:08 p.m. | #1
Hi Phil,

On Tue, 28 Nov 2017 11:38:28 +0800, Phil Reid wrote:
> i2c-smbus now only contains code related to the smbus_alert driver.
> Other smbus protocols have been moved from this into the i2c-core-smbus.
> Change the Kconfig variable name to reflect this.

Not really. i2c-core-smbus.c contains essentially SMBus transaction
helpers, which have never been in i2c-smbus.c. They aren't really SMBus
protocols, more like a subset of I2C transactions suitable for general
purpose. The only really SMBus protocol specific portion is relative to
SMBus Alert, and that's only a small part of it. The other supported
SMBus-specific protocol, Host Notify, is in i2c-core-base.c.

My original intent was to have all the SMBus protocols specific code in
one file, controlled by one Kconfig option, which could be built as a
separate module. I wasn't certain it would be viable, it was a bet
which I lost, as it turns out there are too many dependencies.

If the code can no longer build as a separate module, there is no
benefit in having it in a separate file. Let's just merge it into
i2c-code-smbus.c, where the rest of the SMBus alert code already is.
That would make things more simple.

I also don't think renaming this configuration option and moving code
to a separate file (as your patch 3/3 does) makes sense. Having a
separate option for each SMBus-specific protocol seems overkill to me,
having one for all of them was and still is more reasonable. I wonder
why the SMBus Notify code does NOT depend on CONFIG_I2C_SMBUS, and why
it is in i2c-core-base.c instead of i2c-core-smbus.c. Wolfram?

And more generally, renaming a Kconfig option has a non-trivial cost
for distribution kernel maintainers, so it shall not be done lightly.
So you need a clear and strong rationale.

> Signed-off-by: Phil Reid <preid@electromag.com.au>
> ---
>  arch/blackfin/configs/BF527-TLL6527M_defconfig |  2 +-
>  drivers/i2c/Kconfig                            | 11 +++++------
>  drivers/i2c/Makefile                           |  2 +-
>  drivers/i2c/busses/Kconfig                     |  8 ++++----
>  drivers/i2c/i2c-core-smbus.c                   |  2 +-
>  drivers/i2c/i2c-core.h                         |  2 +-
>  drivers/power/supply/Kconfig                   |  2 +-
>  7 files changed, 14 insertions(+), 15 deletions(-)
> (...)
> diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
> index efc3354..fcd4bea 100644
> --- a/drivers/i2c/Kconfig
> +++ b/drivers/i2c/Kconfig
> @@ -55,7 +55,7 @@ config I2C_CHARDEV
>  	  programs use the I2C bus.  Information on how to do this is
>  	  contained in the file <file:Documentation/i2c/dev-interface>.
>  
> -	  This support is also available as a module.  If so, the module 
> +	  This support is also available as a module.  If so, the module
>  	  will be called i2c-dev.
>  
>  config I2C_MUX

Please don't mix white-space clean-ups with actual changes. It might
be tolerated by some maintainers if within a chunk you are already
touching. But if such a change creates a new patch chunk then it's a
no-go.

Your patch 1/3 suffers from similar issues.

> @@ -84,12 +84,11 @@ config I2C_HELPER_AUTO
>  
>  	  In doubt, say Y.
>  
> -config I2C_SMBUS
> -	tristate "SMBus-specific protocols" if !I2C_HELPER_AUTO
> +config I2C_SMBUS_ALERT
> +	tristate "SMBus-alert protocol" if !I2C_HELPER_AUTO

"SMBus Alert" is the correct spelling.

>  	help
> -	  Say Y here if you want support for SMBus extensions to the I2C
> -	  specification. At the moment, two extensions are supported:
> -	  the SMBus Alert protocol and the SMBus Host Notify protocol.
> +	  Say Y here if you want support for SMBus alert extensions to the I2C
> +	  specification.

"extension" without "s".

>  
>  	  This support is also available as a module.  If so, the module
>  	  will be called i2c-smbus.
Phil Reid Nov. 29, 2017, 1:58 a.m. | #2
G'day Jean,

Thanks for looking at this.

On 29/11/2017 06:08, Jean Delvare wrote:
> Hi Phil,
> 
> On Tue, 28 Nov 2017 11:38:28 +0800, Phil Reid wrote:
>> i2c-smbus now only contains code related to the smbus_alert driver.
>> Other smbus protocols have been moved from this into the i2c-core-smbus.
>> Change the Kconfig variable name to reflect this.
> 
> Not really. i2c-core-smbus.c contains essentially SMBus transaction
> helpers, which have never been in i2c-smbus.c. They aren't really SMBus
> protocols, more like a subset of I2C transactions suitable for general
> purpose. The only really SMBus protocol specific portion is relative to
> SMBus Alert, and that's only a small part of it. The other supported
> SMBus-specific protocol, Host Notify, is in i2c-core-base.c.
> 
> My original intent was to have all the SMBus protocols specific code in
> one file, controlled by one Kconfig option, which could be built as a
> separate module. I wasn't certain it would be viable, it was a bet
> which I lost, as it turns out there are too many dependencies.
> 
> If the code can no longer build as a separate module, there is no
> benefit in having it in a separate file. Let's just merge it into
> i2c-code-smbus.c, where the rest of the SMBus alert code already is.
> That would make things more simple.
I had to put of_i2c_setup_smbus_alert in there to deal with dependencies.
This was seen as a little messy at the time. It was accepted provided
that I clean things up somehow.

The question is how. This series is just one proposal for that to stir some
discussion. Your approach is certainly the simpler one.

> 
> I also don't think renaming this configuration option and moving code
> to a separate file (as your patch 3/3 does) makes sense. Having a
> separate option for each SMBus-specific protocol seems overkill to me,
> having one for all of them was and still is more reasonable. I wonder
> why the SMBus Notify code does NOT depend on CONFIG_I2C_SMBUS, and why
> it is in i2c-core-base.c instead of i2c-core-smbus.c. Wolfram?
> 
> And more generally, renaming a Kconfig option has a non-trivial cost
> for distribution kernel maintainers, so it shall not be done lightly.
> So you need a clear and strong rationale.

My only rational is that it didn't encompass all the SMBUS specific protocols.
It currently seems a little ambiguous to me. Especially with regard the current
Kconfig help description is not accurate with regard the Host Notify protocol.

> 
>> Signed-off-by: Phil Reid <preid@electromag.com.au>
>> ---
>>   arch/blackfin/configs/BF527-TLL6527M_defconfig |  2 +-
>>   drivers/i2c/Kconfig                            | 11 +++++------
>>   drivers/i2c/Makefile                           |  2 +-
>>   drivers/i2c/busses/Kconfig                     |  8 ++++----
>>   drivers/i2c/i2c-core-smbus.c                   |  2 +-
>>   drivers/i2c/i2c-core.h                         |  2 +-
>>   drivers/power/supply/Kconfig                   |  2 +-
>>   7 files changed, 14 insertions(+), 15 deletions(-)
>> (...)
>> diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
>> index efc3354..fcd4bea 100644
>> --- a/drivers/i2c/Kconfig
>> +++ b/drivers/i2c/Kconfig
>> @@ -55,7 +55,7 @@ config I2C_CHARDEV
>>   	  programs use the I2C bus.  Information on how to do this is
>>   	  contained in the file <file:Documentation/i2c/dev-interface>.
>>   
>> -	  This support is also available as a module.  If so, the module
>> +	  This support is also available as a module.  If so, the module
>>   	  will be called i2c-dev.
>>   
>>   config I2C_MUX
> 
> Please don't mix white-space clean-ups with actual changes. It might
> be tolerated by some maintainers if within a chunk you are already
> touching. But if such a change creates a new patch chunk then it's a
> no-go.
> 
> Your patch 1/3 suffers from similar issues.
> 
>> @@ -84,12 +84,11 @@ config I2C_HELPER_AUTO
>>   
>>   	  In doubt, say Y.
>>   
>> -config I2C_SMBUS
>> -	tristate "SMBus-specific protocols" if !I2C_HELPER_AUTO
>> +config I2C_SMBUS_ALERT
>> +	tristate "SMBus-alert protocol" if !I2C_HELPER_AUTO
> 
> "SMBus Alert" is the correct spelling.
> 
>>   	help
>> -	  Say Y here if you want support for SMBus extensions to the I2C
>> -	  specification. At the moment, two extensions are supported:
>> -	  the SMBus Alert protocol and the SMBus Host Notify protocol.
>> +	  Say Y here if you want support for SMBus alert extensions to the I2C
>> +	  specification.
> 
> "extension" without "s".
> 
>>   
>>   	  This support is also available as a module.  If so, the module
>>   	  will be called i2c-smbus.
>
Jean Delvare Nov. 29, 2017, 12:48 p.m. | #3
On Wed, 29 Nov 2017 09:58:44 +0800, Phil Reid wrote:
> G'day Jean,
> 
> Thanks for looking at this.
> 
> On 29/11/2017 06:08, Jean Delvare wrote:
> > Hi Phil,
> > 
> > On Tue, 28 Nov 2017 11:38:28 +0800, Phil Reid wrote:  
> >> i2c-smbus now only contains code related to the smbus_alert driver.
> >> Other smbus protocols have been moved from this into the i2c-core-smbus.
> >> Change the Kconfig variable name to reflect this.  
> > 
> > Not really. i2c-core-smbus.c contains essentially SMBus transaction
> > helpers, which have never been in i2c-smbus.c. They aren't really SMBus
> > protocols, more like a subset of I2C transactions suitable for general
> > purpose. The only really SMBus protocol specific portion is relative to
> > SMBus Alert, and that's only a small part of it. The other supported
> > SMBus-specific protocol, Host Notify, is in i2c-core-base.c.
> > 
> > My original intent was to have all the SMBus protocols specific code in
> > one file, controlled by one Kconfig option, which could be built as a
> > separate module. I wasn't certain it would be viable, it was a bet
> > which I lost, as it turns out there are too many dependencies.
> > 
> > If the code can no longer build as a separate module, there is no
> > benefit in having it in a separate file. Let's just merge it into
> > i2c-code-smbus.c, where the rest of the SMBus alert code already is.
> > That would make things more simple.  
> I had to put of_i2c_setup_smbus_alert in there to deal with dependencies.
> This was seen as a little messy at the time. It was accepted provided
> that I clean things up somehow.
> 
> The question is how. This series is just one proposal for that to stir some
> discussion. Your approach is certainly the simpler one.

OK, can you please give it a try and wee how it compares with your own
approach?

> > I also don't think renaming this configuration option and moving code
> > to a separate file (as your patch 3/3 does) makes sense. Having a
> > separate option for each SMBus-specific protocol seems overkill to me,
> > having one for all of them was and still is more reasonable. I wonder
> > why the SMBus Notify code does NOT depend on CONFIG_I2C_SMBUS, and why
> > it is in i2c-core-base.c instead of i2c-core-smbus.c. Wolfram?
> > 
> > And more generally, renaming a Kconfig option has a non-trivial cost
> > for distribution kernel maintainers, so it shall not be done lightly.
> > So you need a clear and strong rationale.  
> 
> My only rational is that it didn't encompass all the SMBUS specific protocols.
> It currently seems a little ambiguous to me. Especially with regard the current
> Kconfig help description is not accurate with regard the Host Notify protocol.

I agree, the I2C_SMBUS Kconfig help text no longer reflects the reality
since over a year now. But I think the text is how things should be and
the code is what should be fixed (that is, all the SMBus Host Notify
code should be left out when I2C_SMBUS isn't set, and it should live in
i2c-core-smbus.c.) But I did not look at the details, maybe this isn't
possible for some reason. Maybe you can look into it after you are done
with moving the SMBus Alert code into the i2c core? If you don't, I
will.

Thanks,
Phil Reid Nov. 30, 2017, 10:31 a.m. | #4
On 29/11/2017 20:48, Jean Delvare wrote:
> On Wed, 29 Nov 2017 09:58:44 +0800, Phil Reid wrote:
>> G'day Jean,
>>
>> Thanks for looking at this.
>>
>> On 29/11/2017 06:08, Jean Delvare wrote:
>>> Hi Phil,
>>>
>>> On Tue, 28 Nov 2017 11:38:28 +0800, Phil Reid wrote:
>>>> i2c-smbus now only contains code related to the smbus_alert driver.
>>>> Other smbus protocols have been moved from this into the i2c-core-smbus.
>>>> Change the Kconfig variable name to reflect this.
>>>
>>> Not really. i2c-core-smbus.c contains essentially SMBus transaction
>>> helpers, which have never been in i2c-smbus.c. They aren't really SMBus
>>> protocols, more like a subset of I2C transactions suitable for general
>>> purpose. The only really SMBus protocol specific portion is relative to
>>> SMBus Alert, and that's only a small part of it. The other supported
>>> SMBus-specific protocol, Host Notify, is in i2c-core-base.c.
>>>
>>> My original intent was to have all the SMBus protocols specific code in
>>> one file, controlled by one Kconfig option, which could be built as a
>>> separate module. I wasn't certain it would be viable, it was a bet
>>> which I lost, as it turns out there are too many dependencies.
>>>
>>> If the code can no longer build as a separate module, there is no
>>> benefit in having it in a separate file. Let's just merge it into
>>> i2c-code-smbus.c, where the rest of the SMBus alert code already is.
>>> That would make things more simple.
>> I had to put of_i2c_setup_smbus_alert in there to deal with dependencies.
>> This was seen as a little messy at the time. It was accepted provided
>> that I clean things up somehow.
>>
>> The question is how. This series is just one proposal for that to stir some
>> discussion. Your approach is certainly the simpler one.
> 
> OK, can you please give it a try and wee how it compares with your own
> approach?

Should be able to get to it next week.

> 
>>> I also don't think renaming this configuration option and moving code
>>> to a separate file (as your patch 3/3 does) makes sense. Having a
>>> separate option for each SMBus-specific protocol seems overkill to me,
>>> having one for all of them was and still is more reasonable. I wonder
>>> why the SMBus Notify code does NOT depend on CONFIG_I2C_SMBUS, and why
>>> it is in i2c-core-base.c instead of i2c-core-smbus.c. Wolfram?
>>>
>>> And more generally, renaming a Kconfig option has a non-trivial cost
>>> for distribution kernel maintainers, so it shall not be done lightly.
>>> So you need a clear and strong rationale.
>>
>> My only rational is that it didn't encompass all the SMBUS specific protocols.
>> It currently seems a little ambiguous to me. Especially with regard the current
>> Kconfig help description is not accurate with regard the Host Notify protocol.
> 
> I agree, the I2C_SMBUS Kconfig help text no longer reflects the reality
> since over a year now. But I think the text is how things should be and
> the code is what should be fixed (that is, all the SMBus Host Notify
> code should be left out when I2C_SMBUS isn't set, and it should live in
> i2c-core-smbus.c.) But I did not look at the details, maybe this isn't
> possible for some reason. Maybe you can look into it after you are done
> with moving the SMBus Alert code into the i2c core? If you don't, I
> will.
> 
> Thanks,
>

Patch

diff --git a/arch/blackfin/configs/BF527-TLL6527M_defconfig b/arch/blackfin/configs/BF527-TLL6527M_defconfig
index cdeb518..30f3b5c 100644
--- a/arch/blackfin/configs/BF527-TLL6527M_defconfig
+++ b/arch/blackfin/configs/BF527-TLL6527M_defconfig
@@ -109,7 +109,7 @@  CONFIG_SERIAL_BFIN_UART1=y
 # CONFIG_HW_RANDOM is not set
 CONFIG_I2C_CHARDEV=y
 # CONFIG_I2C_HELPER_AUTO is not set
-CONFIG_I2C_SMBUS=y
+CONFIG_I2C_SMBUS_ALERT=y
 CONFIG_I2C_BLACKFIN_TWI=y
 CONFIG_I2C_BLACKFIN_TWI_CLK_KHZ=100
 CONFIG_GPIOLIB=y
diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
index efc3354..fcd4bea 100644
--- a/drivers/i2c/Kconfig
+++ b/drivers/i2c/Kconfig
@@ -55,7 +55,7 @@  config I2C_CHARDEV
 	  programs use the I2C bus.  Information on how to do this is
 	  contained in the file <file:Documentation/i2c/dev-interface>.
 
-	  This support is also available as a module.  If so, the module 
+	  This support is also available as a module.  If so, the module
 	  will be called i2c-dev.
 
 config I2C_MUX
@@ -84,12 +84,11 @@  config I2C_HELPER_AUTO
 
 	  In doubt, say Y.
 
-config I2C_SMBUS
-	tristate "SMBus-specific protocols" if !I2C_HELPER_AUTO
+config I2C_SMBUS_ALERT
+	tristate "SMBus-alert protocol" if !I2C_HELPER_AUTO
 	help
-	  Say Y here if you want support for SMBus extensions to the I2C
-	  specification. At the moment, two extensions are supported:
-	  the SMBus Alert protocol and the SMBus Host Notify protocol.
+	  Say Y here if you want support for SMBus alert extensions to the I2C
+	  specification.
 
 	  This support is also available as a module.  If so, the module
 	  will be called i2c-smbus.
diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
index 7bb65a4..b0116a1 100644
--- a/drivers/i2c/Makefile
+++ b/drivers/i2c/Makefile
@@ -9,7 +9,7 @@  i2c-core-$(CONFIG_ACPI)		+= i2c-core-acpi.o
 i2c-core-$(CONFIG_I2C_SLAVE) 	+= i2c-core-slave.o
 i2c-core-$(CONFIG_OF) 		+= i2c-core-of.o
 
-obj-$(CONFIG_I2C_SMBUS)		+= i2c-smbus.o
+obj-$(CONFIG_I2C_SMBUS_ALERT)	+= i2c-smbus.o
 obj-$(CONFIG_I2C_CHARDEV)	+= i2c-dev.o
 obj-$(CONFIG_I2C_MUX)		+= i2c-mux.o
 obj-y				+= algos/ busses/ muxes/
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 009345d..310d5ec 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -91,7 +91,7 @@  config I2C_I801
 	tristate "Intel 82801 (ICH/PCH)"
 	depends on PCI
 	select CHECK_SIGNATURE if X86 && DMI
-	select I2C_SMBUS
+	select I2C_SMBUS_ALERT
 	help
 	  If you say yes to this option, support will be included for the Intel
 	  801 family of mainboard I2C interfaces.  Specifically, the following
@@ -1056,7 +1056,7 @@  config I2C_OCTEON
 config I2C_THUNDERX
 	tristate "Cavium ThunderX I2C bus support"
 	depends on 64BIT && PCI && (ARM64 || COMPILE_TEST)
-	select I2C_SMBUS
+	select I2C_SMBUS_ALERT
 	help
 	  Say yes if you want to support the I2C serial bus on Cavium
 	  ThunderX SOC.
@@ -1132,7 +1132,7 @@  config I2C_PARPORT
 	tristate "Parallel port adapter"
 	depends on PARPORT
 	select I2C_ALGOBIT
-	select I2C_SMBUS
+	select I2C_SMBUS_ALERT
 	help
 	  This supports parallel port I2C adapters such as the ones made by
 	  Philips or Velleman, Analog Devices evaluation boards, and more.
@@ -1156,7 +1156,7 @@  config I2C_PARPORT
 config I2C_PARPORT_LIGHT
 	tristate "Parallel port adapter (light)"
 	select I2C_ALGOBIT
-	select I2C_SMBUS
+	select I2C_SMBUS_ALERT
 	help
 	  This supports parallel port I2C adapters such as the ones made by
 	  Philips or Velleman, Analog Devices evaluation boards, and more.
diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index 4bb9927..f4c3b18 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -626,7 +626,7 @@  struct i2c_client *i2c_setup_smbus_alert(struct i2c_adapter *adapter,
 }
 EXPORT_SYMBOL_GPL(i2c_setup_smbus_alert);
 
-#if IS_ENABLED(CONFIG_I2C_SMBUS) && IS_ENABLED(CONFIG_OF)
+#if IS_ENABLED(CONFIG_I2C_SMBUS_ALERT) && IS_ENABLED(CONFIG_OF)
 int of_i2c_setup_smbus_alert(struct i2c_adapter *adapter)
 {
 	struct i2c_client *client;
diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
index 8ef0402..2ee5396 100644
--- a/drivers/i2c/i2c-core.h
+++ b/drivers/i2c/i2c-core.h
@@ -61,7 +61,7 @@  static inline void of_i2c_register_devices(struct i2c_adapter *adap) { }
 #endif
 extern struct notifier_block i2c_of_notifier;
 
-#if IS_ENABLED(CONFIG_I2C_SMBUS) && IS_ENABLED(CONFIG_OF)
+#if IS_ENABLED(CONFIG_I2C_SMBUS_ALERT) && IS_ENABLED(CONFIG_OF)
 int of_i2c_setup_smbus_alert(struct i2c_adapter *adap);
 #else
 static inline int of_i2c_setup_smbus_alert(struct i2c_adapter *adap)
diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index fbca0ba..fac8252 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -187,7 +187,7 @@  config CHARGER_SBS
 config MANAGER_SBS
 	tristate "Smart Battery System Manager"
 	depends on I2C && I2C_MUX && GPIOLIB
-	select I2C_SMBUS
+	select I2C_SMBUS_ALERT
 	help
 	  Say Y here to include support for Smart Battery System Manager
 	  ICs. The driver reports online and charging status via sysfs.