diff mbox

[v6,2/4] i2c-smbus : Add client discovered ARA support

Message ID 20171225155723.6338-2-m.capdeville@no-log.org (mailing list archive)
State New, archived
Headers show

Commit Message

Marc CAPDEVILLE Dec. 25, 2017, 3:57 p.m. UTC 
This is from rfc by Alan Cox : https://patchwork.ozlabs.org/patch/381773

The idea is as follows (extract from above rfc) :
- If an adapter knows about its ARA and smbus alerts then the adapter
  creates its own interrupt handler as before

- If a client knows it needs smbus alerts it calls
  i2c_require_smbus_alert(). This ensures that there is an ARA handler
  registered and tells the client whether the adapter is handling it
  anyway or not.

- When the client learns that an ARA event has occurred it calls
  i2c_smbus_alert_event() which uses the existing ARA mechanism to kick
  things off.

Suggested-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Marc CAPDEVILLE <m.capdeville@no-log.org>
---
 drivers/i2c/i2c-smbus.c   | 83 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c-smbus.h | 15 +++++++++
 2 files changed, 98 insertions(+)

Comments

kernel test robot Dec. 26, 2017, 1:43 a.m. UTC | #1 
Hi Marc,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on wsa/i2c/for-next]
[also build test WARNING on v4.15-rc5 next-20171222]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Marc-CAPDEVILLE/i2c-core-acpi-Add-i2c_acpi_set_connection/20171226-083729
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: ia64-defconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=ia64 

All warnings (new ones prefixed by >>):

   In file included from include/uapi/linux/posix_types.h:5:0,
                    from include/uapi/linux/types.h:14,
                    from include/linux/compiler.h:164,
                    from include/linux/ioport.h:13,
                    from include/linux/acpi.h:25,
                    from drivers//i2c/i2c-core-base.c:24:
   include/linux/i2c-smbus.h: In function 'i2c_require_smbus_alert':
>> include/linux/stddef.h:8:14: warning: return makes integer from pointer without a cast [-Wint-conversion]
    #define NULL ((void *)0)
                 ^
>> include/linux/i2c-smbus.h:67:9: note: in expansion of macro 'NULL'
     return NULL;
            ^~~~
   include/linux/i2c-smbus.h: In function 'i2c_smbus_alert_event':
>> include/linux/stddef.h:8:14: warning: return makes integer from pointer without a cast [-Wint-conversion]
    #define NULL ((void *)0)
                 ^
   include/linux/i2c-smbus.h:72:9: note: in expansion of macro 'NULL'
     return NULL;
            ^~~~

vim +/NULL +67 include/linux/i2c-smbus.h

    60	
    61	#if IS_ENABLED(CONFIG_I2C_SMBUS)
    62	int i2c_require_smbus_alert(struct i2c_client *client);
    63	int i2c_smbus_alert_event(struct i2c_client *client);
    64	#else
    65	static inline int i2c_require_smbus_alert(struct i2c_client *client)
    66	{
  > 67		return NULL;
    68	}
    69	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Jonathan Cameron Dec. 29, 2017, 1:04 p.m. UTC | #2 
On Mon, 25 Dec 2017 16:57:21 +0100
Marc CAPDEVILLE <m.capdeville@no-log.org> wrote:

> This is from rfc by Alan Cox : https://patchwork.ozlabs.org/patch/381773
> 
> The idea is as follows (extract from above rfc) :
> - If an adapter knows about its ARA and smbus alerts then the adapter
>   creates its own interrupt handler as before
> 
> - If a client knows it needs smbus alerts it calls
>   i2c_require_smbus_alert(). This ensures that there is an ARA handler
>   registered and tells the client whether the adapter is handling it
>   anyway or not.
> 
> - When the client learns that an ARA event has occurred it calls
>   i2c_smbus_alert_event() which uses the existing ARA mechanism to kick
>   things off.

A few minor things inline.  I'm not sure the balance between what is
done in driver and what is in the core is quite right yet.

Jonathan

> 
> Suggested-by: Alan Cox <alan@linux.intel.com>
> Signed-off-by: Marc CAPDEVILLE <m.capdeville@no-log.org>
> ---
>  drivers/i2c/i2c-smbus.c   | 83 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/i2c-smbus.h | 15 +++++++++
>  2 files changed, 98 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
> index 5a1dd7f13bac..e97fbafd10ef 100644
> --- a/drivers/i2c/i2c-smbus.c
> +++ b/drivers/i2c/i2c-smbus.c
> @@ -161,6 +161,9 @@ static int smbalert_probe(struct i2c_client *ara,
>  	}
>  
>  	i2c_set_clientdata(ara, alert);
> +
> +	ara->adapter->smbus_ara = ara;
> +
>  	dev_info(&adapter->dev, "supports SMBALERT#\n");
>  
>  	return 0;
> @@ -172,6 +175,9 @@ static int smbalert_remove(struct i2c_client *ara)
>  	struct i2c_smbus_alert *alert = i2c_get_clientdata(ara);
>  
>  	cancel_work_sync(&alert->alert);
> +
> +	ara->adapter->smbus_ara = NULL;
> +
>  	return 0;
>  }
>  
> @@ -210,6 +216,83 @@ int i2c_handle_smbus_alert(struct i2c_client *ara)
>  }
>  EXPORT_SYMBOL_GPL(i2c_handle_smbus_alert);
>  
> +/*
/** It looks like kernel doc to me.
> + * i2c_require_smbus_alert - Client discovered SMBus alert
> + * @c: client requiring ARA
> + *
> + * When a client needs an ARA it calls this method. If the bus adapter
> + * supports ARA and already knows how to do so then it will already have
> + * configured for ARA and this is a no-op. If not then we set up an ARA
> + * on the adapter.
> + *
> + * We *cannot* simply register a new IRQ handler for this because we might
> + * have multiple GPIO interrupts to devices all of which trigger an ARA.
I'm a little confused by this comment.  Do you mean:

1. Same gpio irq is provided as as shared irq to a number of devices?
(In this case we are just aiming not to register the same IRQ multiple times
as each driver may have already done it?)  If it is this case, I think we
should be pushing the irq register and checking logic to the smbus core which
can then verify if it is the same interrupt or not? (could be case 2 below
in which case we need to register another handler).

2. Different gpio irqs provided to a number of devices on same bus with
   each acting as ARA interrupt?  (this is hideous so I hope not - but
   I would be surprised if we never see a board doing this)

> + *
> + * Return:
> + *	0 if ara support already registered
> + *	1 if call register a new smbus_alert device
> + *	<0 on error
> + */
> +int i2c_require_smbus_alert(struct i2c_client *client)
> +{
> +	struct i2c_adapter *adapter = client->adapter;
> +	struct i2c_smbus_alert_setup setup;
> +	struct i2c_client *ara;
> +
> +	/* ARA is already known and handled by the adapter (ideal case)

/*
 * ARA

> +	 * or another client has specified ARA is needed
> +	 */
> +	if (adapter->smbus_ara)
> +		return 0;
> +
> +	/* Client driven, do not set up a new IRQ handler */
> +	setup.irq = 0;
> +
> +	ara = i2c_setup_smbus_alert(adapter, &setup);
> +	if (!ara)
> +		return -ENODEV;
> +
> +	return 1;
> +}
> +EXPORT_SYMBOL_GPL(i2c_require_smbus_alert);
> +
> +/*

Fix this up (needs a short description) and make it kernel-doc /**

> + * i2c_smbus_alert_event
> + * @client: the client who known of a probable ara event
> + * Context: can't sleep
> + *
> + * Helper function to be called from an I2C device driver's interrupt
> + * handler. It will schedule the alert work, in turn calling the
> + * corresponding I2C device driver's alert function.
> + *
> + * It is assumed that client is an i2c client who previously call
> + * i2c_require_smbus_alert().
> + */
> +int i2c_smbus_alert_event(struct i2c_client *client)
> +{
> +	struct i2c_adapter *adapter;
> +	struct i2c_client *ara;
> +	struct i2c_smbus_alert *alert;
> +
> +	if (!client)
> +		return -EINVAL;
> +
> +	adapter = client->adapter;
> +	if (!adapter)
> +		return -EINVAL;
> +
> +	ara = adapter->smbus_ara;
> +	if (!ara)
> +		return -EINVAL;
> +
> +	alert = i2c_get_clientdata(ara);
> +	if (!alert)
> +		return -EINVAL;
> +
> +	return schedule_work(&alert->alert);
> +}
> +EXPORT_SYMBOL_GPL(i2c_smbus_alert_event);
> +
>  module_i2c_driver(smbalert_driver);
>  
>  MODULE_AUTHOR("Jean Delvare <jdelvare@suse.de>");
> diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
> index fb0e040b1abb..49f362fa6ac5 100644
> --- a/include/linux/i2c-smbus.h
> +++ b/include/linux/i2c-smbus.h
> @@ -58,4 +58,19 @@ static inline int of_i2c_setup_smbus_alert(struct i2c_adapter *adap)
>  }
>  #endif
>  
> +#if IS_ENABLED(CONFIG_I2C_SMBUS)
> +int i2c_require_smbus_alert(struct i2c_client *client);
> +int i2c_smbus_alert_event(struct i2c_client *client);
> +#else
> +static inline int i2c_require_smbus_alert(struct i2c_client *client)
> +{
> +	return NULL;
> +}
> +
> +static inline int i2c_smbus_alert_event(struct i2c_client *client)
> +{
> +	return NULL;
> +}
> +#endif
> +
>  #endif /* _LINUX_I2C_SMBUS_H */

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
index 5a1dd7f13bac..e97fbafd10ef 100644
--- a/drivers/i2c/i2c-smbus.c
+++ b/drivers/i2c/i2c-smbus.c
@@ -161,6 +161,9 @@  static int smbalert_probe(struct i2c_client *ara,
 	}
 
 	i2c_set_clientdata(ara, alert);
+
+	ara->adapter->smbus_ara = ara;
+
 	dev_info(&adapter->dev, "supports SMBALERT#\n");
 
 	return 0;
@@ -172,6 +175,9 @@  static int smbalert_remove(struct i2c_client *ara)
 	struct i2c_smbus_alert *alert = i2c_get_clientdata(ara);
 
 	cancel_work_sync(&alert->alert);
+
+	ara->adapter->smbus_ara = NULL;
+
 	return 0;
 }
 
@@ -210,6 +216,83 @@  int i2c_handle_smbus_alert(struct i2c_client *ara)
 }
 EXPORT_SYMBOL_GPL(i2c_handle_smbus_alert);
 
+/*
+ * i2c_require_smbus_alert - Client discovered SMBus alert
+ * @c: client requiring ARA
+ *
+ * When a client needs an ARA it calls this method. If the bus adapter
+ * supports ARA and already knows how to do so then it will already have
+ * configured for ARA and this is a no-op. If not then we set up an ARA
+ * on the adapter.
+ *
+ * We *cannot* simply register a new IRQ handler for this because we might
+ * have multiple GPIO interrupts to devices all of which trigger an ARA.
+ *
+ * Return:
+ *	0 if ara support already registered
+ *	1 if call register a new smbus_alert device
+ *	<0 on error
+ */
+int i2c_require_smbus_alert(struct i2c_client *client)
+{
+	struct i2c_adapter *adapter = client->adapter;
+	struct i2c_smbus_alert_setup setup;
+	struct i2c_client *ara;
+
+	/* ARA is already known and handled by the adapter (ideal case)
+	 * or another client has specified ARA is needed
+	 */
+	if (adapter->smbus_ara)
+		return 0;
+
+	/* Client driven, do not set up a new IRQ handler */
+	setup.irq = 0;
+
+	ara = i2c_setup_smbus_alert(adapter, &setup);
+	if (!ara)
+		return -ENODEV;
+
+	return 1;
+}
+EXPORT_SYMBOL_GPL(i2c_require_smbus_alert);
+
+/*
+ * i2c_smbus_alert_event
+ * @client: the client who known of a probable ara event
+ * Context: can't sleep
+ *
+ * Helper function to be called from an I2C device driver's interrupt
+ * handler. It will schedule the alert work, in turn calling the
+ * corresponding I2C device driver's alert function.
+ *
+ * It is assumed that client is an i2c client who previously call
+ * i2c_require_smbus_alert().
+ */
+int i2c_smbus_alert_event(struct i2c_client *client)
+{
+	struct i2c_adapter *adapter;
+	struct i2c_client *ara;
+	struct i2c_smbus_alert *alert;
+
+	if (!client)
+		return -EINVAL;
+
+	adapter = client->adapter;
+	if (!adapter)
+		return -EINVAL;
+
+	ara = adapter->smbus_ara;
+	if (!ara)
+		return -EINVAL;
+
+	alert = i2c_get_clientdata(ara);
+	if (!alert)
+		return -EINVAL;
+
+	return schedule_work(&alert->alert);
+}
+EXPORT_SYMBOL_GPL(i2c_smbus_alert_event);
+
 module_i2c_driver(smbalert_driver);
 
 MODULE_AUTHOR("Jean Delvare <jdelvare@suse.de>");
diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
index fb0e040b1abb..49f362fa6ac5 100644
--- a/include/linux/i2c-smbus.h
+++ b/include/linux/i2c-smbus.h
@@ -58,4 +58,19 @@  static inline int of_i2c_setup_smbus_alert(struct i2c_adapter *adap)
 }
 #endif
 
+#if IS_ENABLED(CONFIG_I2C_SMBUS)
+int i2c_require_smbus_alert(struct i2c_client *client);
+int i2c_smbus_alert_event(struct i2c_client *client);
+#else
+static inline int i2c_require_smbus_alert(struct i2c_client *client)
+{
+	return NULL;
+}
+
+static inline int i2c_smbus_alert_event(struct i2c_client *client)
+{
+	return NULL;
+}
+#endif
+
 #endif /* _LINUX_I2C_SMBUS_H */