[PATCH 0/3] i2c: updates to SMBus alert setup

[PATCH 0/3] i2c: updates to SMBus alert setup

[Wolfram Sang]

The main motivation for this series is to convert
i2c_setup_smbus_alert() to a function which returns an ERRPTR instead of
NULL. Because there are only a few driver using this function they are
all converted in one go (patch 1). The function is also renamed to make
sure out-of-tree users will note they have to update (or better
upstream). Patch 2 renames the of-equivalent, too, so both functions
keep in sync. Patch 3 cleans up some outdated documentation which was
discovered while working on the earlier patches.

Patches are on top of v5.6-rc1 and can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/new_client_device

Only build tested. Testing and reviews from people actually using SMBus
alert would be much appreciated!

Thanks,

   Wolfram


Wolfram Sang (3):
  i2c: convert SMBus alert setup function to return an ERRPTR
  i2c: rename of_i2c_setup_smbus_alert() to keep in sync
  i2c: smbus: remove outdated references to irq level triggers

 Documentation/i2c/smbus-protocol.rst     |  2 +-
 drivers/i2c/busses/i2c-parport.c         | 11 ++++++---
 drivers/i2c/busses/i2c-thunderx-pcidrv.c | 11 ++++++---
 drivers/i2c/busses/i2c-xlp9xx.c          | 10 +++++---
 drivers/i2c/i2c-core-base.c              |  2 +-
 drivers/i2c/i2c-core-smbus.c             | 31 ++++++++++--------------
 drivers/i2c/i2c-smbus.c                  |  2 +-
 include/linux/i2c-smbus.h                | 11 +++------
 8 files changed, 41 insertions(+), 39 deletions(-)

-- 
2.20.1

[PATCH 1/3] i2c: convert SMBus alert setup function to return an ERRPTR

[Wolfram Sang]

Only few drivers use this call, so drivers and I2C core are converted at
once with this patch. By simply using i2c_new_client_device() instead of
i2c_new_device(), we easily can return an ERRPTR for this function as
well. To make out of tree users aware that something changed, the
function is renamed to i2c_install_smbus_alert().

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 Documentation/i2c/smbus-protocol.rst     |  2 +-
 drivers/i2c/busses/i2c-parport.c         | 11 +++++++----
 drivers/i2c/busses/i2c-thunderx-pcidrv.c | 11 ++++++++---
 drivers/i2c/busses/i2c-xlp9xx.c          | 10 +++++++---
 drivers/i2c/i2c-core-smbus.c             | 22 +++++++++++-----------
 drivers/i2c/i2c-smbus.c                  |  2 +-
 include/linux/i2c-smbus.h                |  2 +-
 7 files changed, 36 insertions(+), 24 deletions(-)

diff --git a/Documentation/i2c/smbus-protocol.rst b/Documentation/i2c/smbus-protocol.rst
index c122ed239f7f..4783670a0420 100644
--- a/Documentation/i2c/smbus-protocol.rst
+++ b/Documentation/i2c/smbus-protocol.rst
@@ -274,7 +274,7 @@ to know which slave triggered the interrupt.
 This is implemented the following way in the Linux kernel:
 
 * I2C bus drivers which support SMBus alert should call
-  i2c_setup_smbus_alert() to setup SMBus alert support.
+  i2c_install_smbus_alert() to install SMBus alert support.
 * I2C drivers for devices which can trigger SMBus alerts should implement
   the optional alert() callback.
 
diff --git a/drivers/i2c/busses/i2c-parport.c b/drivers/i2c/busses/i2c-parport.c
index 81eb441b2387..1aee13e2d3da 100644
--- a/drivers/i2c/busses/i2c-parport.c
+++ b/drivers/i2c/busses/i2c-parport.c
@@ -333,13 +333,16 @@ static void i2c_parport_attach(struct parport *port)
 
 	/* Setup SMBus alert if supported */
 	if (adapter_parm[type].smbus_alert) {
-		adapter->ara = i2c_setup_smbus_alert(&adapter->adapter,
-						     &adapter->alert_data);
-		if (adapter->ara)
+		struct i2c_client *ara;
+
+		ara = i2c_install_smbus_alert(&adapter->adapter, &adapter->alert_data);
+		if (!IS_ERR(ara)) {
+			adapter->ara = ara;
 			parport_enable_irq(port);
-		else
+		} else {
 			dev_warn(&adapter->pdev->dev,
 				 "Failed to register ARA client\n");
+		}
 	}
 
 	/* Add the new adapter to the list */
diff --git a/drivers/i2c/busses/i2c-thunderx-pcidrv.c b/drivers/i2c/busses/i2c-thunderx-pcidrv.c
index 19f8eec38717..8626a97739e1 100644
--- a/drivers/i2c/busses/i2c-thunderx-pcidrv.c
+++ b/drivers/i2c/busses/i2c-thunderx-pcidrv.c
@@ -118,6 +118,8 @@ static void thunder_i2c_clock_disable(struct device *dev, struct clk *clk)
 static int thunder_i2c_smbus_setup_of(struct octeon_i2c *i2c,
 				      struct device_node *node)
 {
+	struct i2c_client *ara;
+
 	if (!node)
 		return -EINVAL;
 
@@ -125,9 +127,12 @@ static int thunder_i2c_smbus_setup_of(struct octeon_i2c *i2c,
 	if (!i2c->alert_data.irq)
 		return -EINVAL;
 
-	i2c->ara = i2c_setup_smbus_alert(&i2c->adap, &i2c->alert_data);
-	if (!i2c->ara)
-		return -ENODEV;
+	ara = i2c_install_smbus_alert(&i2c->adap, &i2c->alert_data);
+	if (IS_ERR(ara))
+		return PTR_ERR(ara);
+
+	i2c->ara = ara;
+
 	return 0;
 }
 
diff --git a/drivers/i2c/busses/i2c-xlp9xx.c b/drivers/i2c/busses/i2c-xlp9xx.c
index 8a873975cf12..3e57723e1194 100644
--- a/drivers/i2c/busses/i2c-xlp9xx.c
+++ b/drivers/i2c/busses/i2c-xlp9xx.c
@@ -491,12 +491,16 @@ static int xlp9xx_i2c_get_frequency(struct platform_device *pdev,
 static int xlp9xx_i2c_smbus_setup(struct xlp9xx_i2c_dev *priv,
 				  struct platform_device *pdev)
 {
+	struct i2c_client *ara;
+
 	if (!priv->alert_data.irq)
 		return -EINVAL;
 
-	priv->ara = i2c_setup_smbus_alert(&priv->adapter, &priv->alert_data);
-	if (!priv->ara)
-		return -ENODEV;
+	ara = i2c_install_smbus_alert(&priv->adapter, &priv->alert_data);
+	if (IS_ERR(ara))
+		return PTR_ERR(ara);
+
+	priv->ara = ara;
 
 	return 0;
 }
diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index 3ac426a8ab5a..06f2e4d78d3c 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -666,12 +666,12 @@ s32 i2c_smbus_read_i2c_block_data_or_emulated(const struct i2c_client *client,
 EXPORT_SYMBOL(i2c_smbus_read_i2c_block_data_or_emulated);
 
 /**
- * i2c_setup_smbus_alert - Setup SMBus alert support
+ * i2c_install_smbus_alert - Install SMBus alert support
  * @adapter: the target adapter
  * @setup: setup data for the SMBus alert handler
  * Context: can sleep
  *
- * Setup handling of the SMBus alert protocol on a given I2C bus segment.
+ * Install handling of the SMBus alert protocol on a given I2C bus segment.
  *
  * Handling can be done either through our IRQ handler, or by the
  * adapter (from its handler, periodic polling, or whatever).
@@ -682,20 +682,20 @@ EXPORT_SYMBOL(i2c_smbus_read_i2c_block_data_or_emulated);
  * should have said it's level triggered.
  *
  * This returns the ara client, which should be saved for later use with
- * i2c_handle_smbus_alert() and ultimately i2c_unregister_device(); or NULL
- * to indicate an error.
+ * i2c_handle_smbus_alert() and ultimately i2c_unregister_device(); or an
+ * ERRPTR to indicate an error.
  */
-struct i2c_client *i2c_setup_smbus_alert(struct i2c_adapter *adapter,
-					 struct i2c_smbus_alert_setup *setup)
+struct i2c_client *i2c_install_smbus_alert(struct i2c_adapter *adapter,
+					   struct i2c_smbus_alert_setup *setup)
 {
 	struct i2c_board_info ara_board_info = {
 		I2C_BOARD_INFO("smbus_alert", 0x0c),
 		.platform_data = setup,
 	};
 
-	return i2c_new_device(adapter, &ara_board_info);
+	return i2c_new_client_device(adapter, &ara_board_info);
 }
-EXPORT_SYMBOL_GPL(i2c_setup_smbus_alert);
+EXPORT_SYMBOL_GPL(i2c_install_smbus_alert);
 
 #if IS_ENABLED(CONFIG_I2C_SMBUS) && IS_ENABLED(CONFIG_OF)
 int of_i2c_setup_smbus_alert(struct i2c_adapter *adapter)
@@ -710,9 +710,9 @@ int of_i2c_setup_smbus_alert(struct i2c_adapter *adapter)
 	else if (irq < 0)
 		return irq;
 
-	client = i2c_setup_smbus_alert(adapter, NULL);
-	if (!client)
-		return -ENODEV;
+	client = i2c_install_smbus_alert(adapter, NULL);
+	if (IS_ERR(client))
+		return PTR_ERR(client);
 
 	return 0;
 }
diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
index 7e2f5d0eacdb..174a89d3e415 100644
--- a/drivers/i2c/i2c-smbus.c
+++ b/drivers/i2c/i2c-smbus.c
@@ -184,7 +184,7 @@ static struct i2c_driver smbalert_driver = {
  * corresponding I2C device driver's alert function.
  *
  * It is assumed that ara is a valid i2c client previously returned by
- * i2c_setup_smbus_alert().
+ * i2c_install_smbus_alert().
  */
 int i2c_handle_smbus_alert(struct i2c_client *ara)
 {
diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
index 585ad6fc3847..409da1e478d6 100644
--- a/include/linux/i2c-smbus.h
+++ b/include/linux/i2c-smbus.h
@@ -31,7 +31,7 @@ struct i2c_smbus_alert_setup {
 	int			irq;
 };
 
-struct i2c_client *i2c_setup_smbus_alert(struct i2c_adapter *adapter,
+struct i2c_client *i2c_install_smbus_alert(struct i2c_adapter *adapter,
 					 struct i2c_smbus_alert_setup *setup);
 int i2c_handle_smbus_alert(struct i2c_client *ara);
 
-- 
2.20.1

Re: [PATCH 1/3] i2c: convert SMBus alert setup function to return an ERRPTR

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1010 bytes --]

Hi Wolfram,

I love your patch! Perhaps something to improve:

[auto build test WARNING on wsa/i2c/for-next]
[also build test WARNING on v5.6-rc1 next-20200212]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Wolfram-Sang/i2c-updates-to-SMBus-alert-setup/20200213-051237
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>


coccinelle warnings: (new ones prefixed by >>)

>> drivers/i2c/i2c-core-smbus.c:714:1-3: WARNING: PTR_ERR_OR_ZERO can be used

Please review and possibly fold the followup patch.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[PATCH] i2c: fix ptr_ret.cocci warnings

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1081 bytes --]

From: kbuild test robot <lkp@intel.com>

drivers/i2c/i2c-core-smbus.c:714:1-3: WARNING: PTR_ERR_OR_ZERO can be used


 Use PTR_ERR_OR_ZERO rather than if(IS_ERR(...)) + PTR_ERR

Generated by: scripts/coccinelle/api/ptr_ret.cocci

Fixes: b0497a4dfd24 ("i2c: convert SMBus alert setup function to return an ERRPTR")
CC: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: kbuild test robot <lkp@intel.com>
---

url:    https://github.com/0day-ci/linux/commits/Wolfram-Sang/i2c-updates-to-SMBus-alert-setup/20200213-051237
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next

 i2c-core-smbus.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -711,10 +711,7 @@ int of_i2c_setup_smbus_alert(struct i2c_
 		return irq;
 
 	client = i2c_install_smbus_alert(adapter, NULL);
-	if (IS_ERR(client))
-		return PTR_ERR(client);
-
-	return 0;
+	return PTR_ERR_OR_ZERO(client);
 }
 EXPORT_SYMBOL_GPL(of_i2c_setup_smbus_alert);
 #endif

Re: [PATCH 1/3] i2c: convert SMBus alert setup function to return an ERRPTR

[Jean Delvare]

Hi Wolfram,

On Mon, 10 Feb 2020 18:29:25 +0100, Wolfram Sang wrote:
> Only few drivers use this call, so drivers and I2C core are converted at
> once with this patch. By simply using i2c_new_client_device() instead of
> i2c_new_device(), we easily can return an ERRPTR for this function as
> well. To make out of tree users aware that something changed, the
> function is renamed to i2c_install_smbus_alert().

I wouldn't bother renaming the function. Chances that there actually
are out-of-tree users of this function are pretty small, and if that is
the case, they can adjust their code easily in a way that is still
compatible with old kernels.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 1/3] i2c: convert SMBus alert setup function to return an ERRPTR

[Jean Delvare]

Hi Wolfram,

On Mon, 10 Feb 2020 18:29:25 +0100, Wolfram Sang wrote:
> Only few drivers use this call, so drivers and I2C core are converted at
> once with this patch. By simply using i2c_new_client_device() instead of
> i2c_new_device(), we easily can return an ERRPTR for this function as
> well. To make out of tree users aware that something changed, the
> function is renamed to i2c_install_smbus_alert().

I wouldn't bother renaming the function. Chances that there actually
are out-of-tree users of this function are pretty small, and if that is
the case, they can adjust their code easily in a way that is still
compatible with old kernels.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 1/3] i2c: convert SMBus alert setup function to return an ERRPTR

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 935 bytes --]

On Sat, Feb 15, 2020 at 07:20:20AM +0100, Jean Delvare wrote:
> Hi Wolfram,
> 
> On Mon, 10 Feb 2020 18:29:25 +0100, Wolfram Sang wrote:
> > Only few drivers use this call, so drivers and I2C core are converted at
> > once with this patch. By simply using i2c_new_client_device() instead of
> > i2c_new_device(), we easily can return an ERRPTR for this function as
> > well. To make out of tree users aware that something changed, the
> > function is renamed to i2c_install_smbus_alert().
> 
> I wouldn't bother renaming the function. Chances that there actually
> are out-of-tree users of this function are pretty small, and if that is
> the case, they can adjust their code easily in a way that is still
> compatible with old kernels.

Agreed, it is easy. But they need to *know* that they need to adjust.
Build error makes it obvious. Otherwise, the code will compile and
silently not work anymore, I am afraid.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3] i2c: convert SMBus alert setup function to return an ERRPTR

[Jean Delvare]

On Sat, 15 Feb 2020 07:50:18 +0100, Wolfram Sang wrote:
> On Sat, Feb 15, 2020 at 07:20:20AM +0100, Jean Delvare wrote:
> > Hi Wolfram,
> > 
> > On Mon, 10 Feb 2020 18:29:25 +0100, Wolfram Sang wrote:  
> > > Only few drivers use this call, so drivers and I2C core are converted at
> > > once with this patch. By simply using i2c_new_client_device() instead of
> > > i2c_new_device(), we easily can return an ERRPTR for this function as
> > > well. To make out of tree users aware that something changed, the
> > > function is renamed to i2c_install_smbus_alert().  
> > 
> > I wouldn't bother renaming the function. Chances that there actually
> > are out-of-tree users of this function are pretty small, and if that is
> > the case, they can adjust their code easily in a way that is still
> > compatible with old kernels.  
> 
> Agreed, it is easy. But they need to *know* that they need to adjust.
> Build error makes it obvious. Otherwise, the code will compile and
> silently not work anymore, I am afraid.

The code will keep working as long as there is no error, which is a
rare situation. If/when it happens, they'll get a very visible error
message in their kernel log.

I understand that this makes them discover the problem later. But we
are talking about a case which most likely does not exist. On the other
hand, renaming makes compatibility harder to achieve for them in the
long term (#ifdefs needed forever), and it makes the work of the
distribution kernel maintainers harder.

I don't think we should make our lives harder to help people who keep
their kernel drivers out of tree. If they are not happy, they know what
they should do.

Just my 2 cents anyway, your call.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 1/3] i2c: convert SMBus alert setup function to return an ERRPTR

[Robert Richter]

On 10.02.20 18:29:25, Wolfram Sang wrote:
> Only few drivers use this call, so drivers and I2C core are converted at
> once with this patch. By simply using i2c_new_client_device() instead of
> i2c_new_device(), we easily can return an ERRPTR for this function as
> well. To make out of tree users aware that something changed, the
> function is renamed to i2c_install_smbus_alert().
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

> -struct i2c_client *i2c_setup_smbus_alert(struct i2c_adapter *adapter,
> +struct i2c_client *i2c_install_smbus_alert(struct i2c_adapter *adapter,
>  					 struct i2c_smbus_alert_setup *setup);

This function naming is a bit odd. It creates a struct i2c_client.
Then, there is also i2c_new_client_device() and i2c_new_device(). For
i2c_new_client_device() there are no users at all outside of
i2c-core-base.c (except for Falcon NIC), it is only a wrapper.

So how about reducing the interface to those both only to:?

 i2c_new_device()
 i2c_new_device_smbus()

To avoid a namespace collision it could also get new names using
*_create_*.

Thanks,

-Robert

Re: [PATCH 1/3] i2c: convert SMBus alert setup function to return an ERRPTR

[Robert Richter]

On 10.02.20 18:29:25, Wolfram Sang wrote:
> Only few drivers use this call, so drivers and I2C core are converted at
> once with this patch. By simply using i2c_new_client_device() instead of
> i2c_new_device(), we easily can return an ERRPTR for this function as
> well. To make out of tree users aware that something changed, the
> function is renamed to i2c_install_smbus_alert().
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

> -struct i2c_client *i2c_setup_smbus_alert(struct i2c_adapter *adapter,
> +struct i2c_client *i2c_install_smbus_alert(struct i2c_adapter *adapter,
>  					 struct i2c_smbus_alert_setup *setup);

This function naming is a bit odd. It creates a struct i2c_client.
Then, there is also i2c_new_client_device() and i2c_new_device(). For
i2c_new_client_device() there are no users at all outside of
i2c-core-base.c (except for Falcon NIC), it is only a wrapper.

So how about reducing the interface to those both only to:?

 i2c_new_device()
 i2c_new_device_smbus()

To avoid a namespace collision it could also get new names using
*_create_*.

Thanks,

-Robert

Re: [PATCH 1/3] i2c: convert SMBus alert setup function to return an ERRPTR

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1326 bytes --]


> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> > -struct i2c_client *i2c_setup_smbus_alert(struct i2c_adapter *adapter,
> > +struct i2c_client *i2c_install_smbus_alert(struct i2c_adapter *adapter,
> >  					 struct i2c_smbus_alert_setup *setup);
> 
> This function naming is a bit odd. It creates a struct i2c_client.
> Then, there is also i2c_new_client_device() and i2c_new_device(). For
> i2c_new_client_device() there are no users at all outside of
> i2c-core-base.c (except for Falcon NIC), it is only a wrapper.

i2c_new_device (and friends) returned NULL on error. I am currently
converting all i2c_new_* functions to return an ERRPTR. So,
i2c_new_client_device is the new function, i2c_new_device is deprecated.
If you check v5.6-rc1, you will find many more users. Similarily,
i2c_new_dummy is deprecated (and removed already), i2c_new_dummy_device
is the new thing.

> So how about reducing the interface to those both only to:?
> 
>  i2c_new_device()
>  i2c_new_device_smbus()

Given the above, it would be:

	i2c_new_client_device()
	i2c_new_smbus_device()

Yet, I think this is too vague. Maybe

	i2c_new_smbus_alert_device()

? Note that I never used SMBus Alert, so I am happy for feedback from
people actually using it.

Thanks for the comment!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3] i2c: convert SMBus alert setup function to return an ERRPTR

[Luca Ceresoli]

Hi,

On 17/02/20 09:17, Wolfram Sang wrote:
> 
>>> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>>
>>> -struct i2c_client *i2c_setup_smbus_alert(struct i2c_adapter *adapter,
>>> +struct i2c_client *i2c_install_smbus_alert(struct i2c_adapter *adapter,
>>>  					 struct i2c_smbus_alert_setup *setup);
>>
>> This function naming is a bit odd. It creates a struct i2c_client.
>> Then, there is also i2c_new_client_device() and i2c_new_device(). For
>> i2c_new_client_device() there are no users at all outside of
>> i2c-core-base.c (except for Falcon NIC), it is only a wrapper.
> 
> i2c_new_device (and friends) returned NULL on error. I am currently
> converting all i2c_new_* functions to return an ERRPTR. So,
> i2c_new_client_device is the new function, i2c_new_device is deprecated.
> If you check v5.6-rc1, you will find many more users. Similarily,
> i2c_new_dummy is deprecated (and removed already), i2c_new_dummy_device
> is the new thing.
> 
>> So how about reducing the interface to those both only to:?
>>
>>  i2c_new_device()
>>  i2c_new_device_smbus()
> 
> Given the above, it would be:
> 
> 	i2c_new_client_device()
> 	i2c_new_smbus_device()
> 
> Yet, I think this is too vague. Maybe
> 
> 	i2c_new_smbus_alert_device()

I always found the function naming a bit messy in the linux i2c
implementation. Among the names proposed in this thread,
i2c_new_smbus_alert_device() is the only one that makes sense to me for
the new function: it is not vague, and it is coherent with other names
of recently introduced functions (i2c_new_*_device()). Of course the
"alert device" is not a real device, but it looks like it is as it has
its own "slave" address. So I vote for this name...

> ? Note that I never used SMBus Alert, so I am happy for feedback from
> people actually using it.

...but that said, I'm afraid I'm not using smbus alert.

My 2c,
-- 
Luca

Re: [PATCH 1/3] i2c: convert SMBus alert setup function to return an ERRPTR

[Robert Richter]

On 17.02.20 10:14:52, Luca Ceresoli wrote:
> Hi,
> 
> On 17/02/20 09:17, Wolfram Sang wrote:
> > 
> >>> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> >>
> >>> -struct i2c_client *i2c_setup_smbus_alert(struct i2c_adapter *adapter,
> >>> +struct i2c_client *i2c_install_smbus_alert(struct i2c_adapter *adapter,
> >>>  					 struct i2c_smbus_alert_setup *setup);
> >>
> >> This function naming is a bit odd. It creates a struct i2c_client.
> >> Then, there is also i2c_new_client_device() and i2c_new_device(). For
> >> i2c_new_client_device() there are no users at all outside of
> >> i2c-core-base.c (except for Falcon NIC), it is only a wrapper.
> > 
> > i2c_new_device (and friends) returned NULL on error. I am currently
> > converting all i2c_new_* functions to return an ERRPTR. So,
> > i2c_new_client_device is the new function, i2c_new_device is deprecated.
> > If you check v5.6-rc1, you will find many more users. Similarily,
> > i2c_new_dummy is deprecated (and removed already), i2c_new_dummy_device
> > is the new thing.
> > 
> >> So how about reducing the interface to those both only to:?
> >>
> >>  i2c_new_device()
> >>  i2c_new_device_smbus()
> > 
> > Given the above, it would be:
> > 
> > 	i2c_new_client_device()
> > 	i2c_new_smbus_device()
> > 
> > Yet, I think this is too vague. Maybe
> > 
> > 	i2c_new_smbus_alert_device()
> 
> I always found the function naming a bit messy in the linux i2c
> implementation. Among the names proposed in this thread,
> i2c_new_smbus_alert_device() is the only one that makes sense to me for
> the new function: it is not vague, and it is coherent with other names
> of recently introduced functions (i2c_new_*_device()). Of course the
> "alert device" is not a real device, but it looks like it is as it has
> its own "slave" address. So I vote for this name...

Yes, better fix the naming now that the i/f is new. As all these
function create a i2c_client struct, the whole set of functions could
be also named i2c_client_create*() with specialized functions such as
i2c_client_create_smbus() or so. It will be clear it is a subset.

Anyway, it's just a function name, but while reading the code it was
not obvious to me that i2c_install_smbus_alert() is actually a subset
of i2c_new_client_device(). That said, I like the i2c_client_create*()
variants.

-Robert

Re: [PATCH 1/3] i2c: convert SMBus alert setup function to return an ERRPTR

[Robert Richter]

On 17.02.20 10:14:52, Luca Ceresoli wrote:
> Hi,
> 
> On 17/02/20 09:17, Wolfram Sang wrote:
> > 
> >>> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> >>
> >>> -struct i2c_client *i2c_setup_smbus_alert(struct i2c_adapter *adapter,
> >>> +struct i2c_client *i2c_install_smbus_alert(struct i2c_adapter *adapter,
> >>>  					 struct i2c_smbus_alert_setup *setup);
> >>
> >> This function naming is a bit odd. It creates a struct i2c_client.
> >> Then, there is also i2c_new_client_device() and i2c_new_device(). For
> >> i2c_new_client_device() there are no users at all outside of
> >> i2c-core-base.c (except for Falcon NIC), it is only a wrapper.
> > 
> > i2c_new_device (and friends) returned NULL on error. I am currently
> > converting all i2c_new_* functions to return an ERRPTR. So,
> > i2c_new_client_device is the new function, i2c_new_device is deprecated.
> > If you check v5.6-rc1, you will find many more users. Similarily,
> > i2c_new_dummy is deprecated (and removed already), i2c_new_dummy_device
> > is the new thing.
> > 
> >> So how about reducing the interface to those both only to:?
> >>
> >>  i2c_new_device()
> >>  i2c_new_device_smbus()
> > 
> > Given the above, it would be:
> > 
> > 	i2c_new_client_device()
> > 	i2c_new_smbus_device()
> > 
> > Yet, I think this is too vague. Maybe
> > 
> > 	i2c_new_smbus_alert_device()
> 
> I always found the function naming a bit messy in the linux i2c
> implementation. Among the names proposed in this thread,
> i2c_new_smbus_alert_device() is the only one that makes sense to me for
> the new function: it is not vague, and it is coherent with other names
> of recently introduced functions (i2c_new_*_device()). Of course the
> "alert device" is not a real device, but it looks like it is as it has
> its own "slave" address. So I vote for this name...

Yes, better fix the naming now that the i/f is new. As all these
function create a i2c_client struct, the whole set of functions could
be also named i2c_client_create*() with specialized functions such as
i2c_client_create_smbus() or so. It will be clear it is a subset.

Anyway, it's just a function name, but while reading the code it was
not obvious to me that i2c_install_smbus_alert() is actually a subset
of i2c_new_client_device(). That said, I like the i2c_client_create*()
variants.

-Robert

Re: [PATCH 1/3] i2c: convert SMBus alert setup function to return an ERRPTR

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 556 bytes --]


> Anyway, it's just a function name, but while reading the code it was
> not obvious to me that i2c_install_smbus_alert() is actually a subset
> of i2c_new_client_device(). That said, I like the i2c_client_create*()
> variants.

I agree that i2c_client_create* is a nice naming, but it came in a bit
too late. Renaming the API is a tiresome job, and it shouldn't be done
(again) just for the sake of renaming IMO.

That all being said, I think i2c_new_smbus_alert_device is a better
naming than what is in this patchset and it is my favourite until now.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/3] i2c: rename of_i2c_setup_smbus_alert() to keep in sync

[Wolfram Sang]

The parent function i2c_setup_smbus_alert() has been renamed, so rename
this one, too, for consistency.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c  | 2 +-
 drivers/i2c/i2c-core-smbus.c | 4 ++--
 include/linux/i2c-smbus.h    | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index cefad0881942..0ac3d6d8b2e2 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1329,7 +1329,7 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 		goto out_list;
 	}
 
-	res = of_i2c_setup_smbus_alert(adap);
+	res = of_i2c_install_smbus_alert(adap);
 	if (res)
 		goto out_reg;
 
diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index 06f2e4d78d3c..5ab30d627b4d 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -698,7 +698,7 @@ struct i2c_client *i2c_install_smbus_alert(struct i2c_adapter *adapter,
 EXPORT_SYMBOL_GPL(i2c_install_smbus_alert);
 
 #if IS_ENABLED(CONFIG_I2C_SMBUS) && IS_ENABLED(CONFIG_OF)
-int of_i2c_setup_smbus_alert(struct i2c_adapter *adapter)
+int of_i2c_install_smbus_alert(struct i2c_adapter *adapter)
 {
 	struct i2c_client *client;
 	int irq;
@@ -716,5 +716,5 @@ int of_i2c_setup_smbus_alert(struct i2c_adapter *adapter)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(of_i2c_setup_smbus_alert);
+EXPORT_SYMBOL_GPL(of_i2c_install_smbus_alert);
 #endif
diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
index 409da1e478d6..aa24a11d1a83 100644
--- a/include/linux/i2c-smbus.h
+++ b/include/linux/i2c-smbus.h
@@ -36,9 +36,9 @@ struct i2c_client *i2c_install_smbus_alert(struct i2c_adapter *adapter,
 int i2c_handle_smbus_alert(struct i2c_client *ara);
 
 #if IS_ENABLED(CONFIG_I2C_SMBUS) && IS_ENABLED(CONFIG_OF)
-int of_i2c_setup_smbus_alert(struct i2c_adapter *adap);
+int of_i2c_install_smbus_alert(struct i2c_adapter *adap);
 #else
-static inline int of_i2c_setup_smbus_alert(struct i2c_adapter *adap)
+static inline int of_i2c_install_smbus_alert(struct i2c_adapter *adap)
 {
 	return 0;
 }
-- 
2.20.1

[PATCH 3/3] i2c: smbus: remove outdated references to irq level triggers

[Wolfram Sang]

IRQ levels are now handled within the IRQ core. Remove the forgotten
references from the documentation.

Fixes: 9b9f2b8bc2ac ("i2c: i2c-smbus: Use threaded irq for smbalert")
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-smbus.c | 5 -----
 include/linux/i2c-smbus.h    | 5 -----
 2 files changed, 10 deletions(-)

diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index 5ab30d627b4d..d11a8ce2aa61 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -676,11 +676,6 @@ EXPORT_SYMBOL(i2c_smbus_read_i2c_block_data_or_emulated);
  * Handling can be done either through our IRQ handler, or by the
  * adapter (from its handler, periodic polling, or whatever).
  *
- * NOTE that if we manage the IRQ, we *MUST* know if it's level or
- * edge triggered in order to hand it to the workqueue correctly.
- * If triggering the alert seems to wedge the system, you probably
- * should have said it's level triggered.
- *
  * This returns the ara client, which should be saved for later use with
  * i2c_handle_smbus_alert() and ultimately i2c_unregister_device(); or an
  * ERRPTR to indicate an error.
diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
index aa24a11d1a83..b89b91eb1c72 100644
--- a/include/linux/i2c-smbus.h
+++ b/include/linux/i2c-smbus.h
@@ -15,17 +15,12 @@
 
 /**
  * i2c_smbus_alert_setup - platform data for the smbus_alert i2c client
- * @alert_edge_triggered: whether the alert interrupt is edge (1) or level (0)
- *		triggered
  * @irq: IRQ number, if the smbus_alert driver should take care of interrupt
  *		handling
  *
  * If irq is not specified, the smbus_alert driver doesn't take care of
  * interrupt handling. In that case it is up to the I2C bus driver to either
  * handle the interrupts or to poll for alerts.
- *
- * If irq is specified then it it crucial that alert_edge_triggered is
- * properly set.
  */
 struct i2c_smbus_alert_setup {
 	int			irq;
-- 
2.20.1