[RFC Patch v0 1/3] i2c-smbus: Add poll interface for smbus alert

[RFC Patch v0 1/3] i2c-smbus: Add poll interface for smbus alert

[Srinivas Pandruvada]

The current i2c smbus alert module depends on smbus alert mechanism
supported by underlying bus drivers. By specifications, these alerts
can be polled if there is no hardware support.
Currently multiple drivers who needs smbus alerts are creating
a new i2c dummy device with address 0x0C (ARA register), by luck
they don't co-exist. Otherwise i2c device creation will fail.
Added a poll interface, so that all these driver can call a common
interface to poll. Even if they polli, all drivers bound to an
adapater will be notified by their alert callback if ARA register
read is successful.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/i2c/i2c-smbus.c   | 23 +++++++++++++++++++++++
 include/linux/i2c-smbus.h |  3 +++
 2 files changed, 26 insertions(+)

diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
index fc99f0d..e274f20 100644
--- a/drivers/i2c/i2c-smbus.c
+++ b/drivers/i2c/i2c-smbus.c
@@ -72,6 +72,29 @@ static int smbus_do_alert(struct device *dev, void *addrp)
 	return -EBUSY;
 }
 
+int i2c_smbus_ara_poll(const struct i2c_client *client)
+{
+	union i2c_smbus_data data;
+	int status;
+	struct alert_data alert_data;
+
+	status = i2c_smbus_xfer(client->adapter, 0x0C, 0,
+				I2C_SMBUS_READ, 0,
+				I2C_SMBUS_BYTE, &data);
+	if (status < 0)
+		return status;
+
+	alert_data.flag = data.byte & 1;
+	alert_data.addr = data.byte >> 1;
+
+	/* Notify driver for the device which issued the alert */
+	device_for_each_child(&client->adapter->dev, &alert_data,
+				smbus_do_alert);
+
+	return data.byte;
+}
+EXPORT_SYMBOL_GPL(i2c_smbus_ara_poll);
+
 /*
  * The alert IRQ handler needs to hand work off to a task which can issue
  * SMBus calls, because those sleeping calls can't be made in IRQ context.
diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
index 8f1b086..f70755d 100644
--- a/include/linux/i2c-smbus.h
+++ b/include/linux/i2c-smbus.h
@@ -48,4 +48,7 @@ struct i2c_client *i2c_setup_smbus_alert(struct i2c_adapter *adapter,
 					 struct i2c_smbus_alert_setup *setup);
 int i2c_handle_smbus_alert(struct i2c_client *ara);
 
+/* Interface to poll smbus alert */
+int i2c_smbus_ara_poll(const struct i2c_client *client);
+
 #endif /* _LINUX_I2C_SMBUS_H */
-- 
1.8.3.2

[RFC Patch v0 2/3] i2c-smbus: Allow building with I2C_HELPER_AUTO support

[Srinivas Pandruvada]

Currentlty the config is conditional on !I2C_HELPER_AUTO.
I don't know their dependency, but without this we can't build.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/i2c/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
index 7b7ea32..212ed73 100644
--- a/drivers/i2c/Kconfig
+++ b/drivers/i2c/Kconfig
@@ -74,7 +74,7 @@ config I2C_HELPER_AUTO
 	  In doubt, say Y.
 
 config I2C_SMBUS
-	tristate "SMBus-specific protocols" if !I2C_HELPER_AUTO
+	tristate "SMBus-specific protocols"
 	help
 	  Say Y here if you want support for SMBus extensions to the I2C
 	  specification. At the moment, the only supported extension is
-- 
1.8.3.2

Re: [RFC Patch v0 2/3] i2c-smbus: Allow building with I2C_HELPER_AUTO support

[Jean Delvare]

On Wed, 26 Mar 2014 17:42:11 -0700, Srinivas Pandruvada wrote:
> Currentlty the config is conditional on !I2C_HELPER_AUTO.
> I don't know their dependency, but without this we can't build.

Nack. Just because you don't understand something and did not bother
looking into it, is no good reason to break a perfectly working and
valid mechanism.

Just read the Kconfig help text for I2C_HELPER_AUTO and you should be
able to figure it out. Also grep the kernel tree for "select I2C_SMBUS".

As a summary: any driver which depends on i2c-smbus, should select it
in Kconfig.

> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
>  drivers/i2c/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
> index 7b7ea32..212ed73 100644
> --- a/drivers/i2c/Kconfig
> +++ b/drivers/i2c/Kconfig
> @@ -74,7 +74,7 @@ config I2C_HELPER_AUTO
>  	  In doubt, say Y.
>  
>  config I2C_SMBUS
> -	tristate "SMBus-specific protocols" if !I2C_HELPER_AUTO
> +	tristate "SMBus-specific protocols"
>  	help
>  	  Say Y here if you want support for SMBus extensions to the I2C
>  	  specification. At the moment, the only supported extension is


-- 
Jean Delvare
SUSE L3 Support

Re: [RFC Patch v0 2/3] i2c-smbus: Allow building with I2C_HELPER_AUTO support

[Srinivas Pandruvada]

On 03/26/2014 11:57 PM, Jean Delvare wrote:
> On Wed, 26 Mar 2014 17:42:11 -0700, Srinivas Pandruvada wrote:
>> Currentlty the config is conditional on !I2C_HELPER_AUTO.
>> I don't know their dependency, but without this we can't build.
> Nack. Just because you don't understand something and did not bother
> looking into it, is no good reason to break a perfectly working and
> valid mechanism.
That is why it RFC.
>
> Just read the Kconfig help text for I2C_HELPER_AUTO and you should be
> able to figure it out. Also grep the kernel tree for "select I2C_SMBUS".
>
> As a summary: any driver which depends on i2c-smbus, should select it
> in Kconfig.
>
>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>> ---
>>   drivers/i2c/Kconfig | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
>> index 7b7ea32..212ed73 100644
>> --- a/drivers/i2c/Kconfig
>> +++ b/drivers/i2c/Kconfig
>> @@ -74,7 +74,7 @@ config I2C_HELPER_AUTO
>>   	  In doubt, say Y.
>>   
>>   config I2C_SMBUS
>> -	tristate "SMBus-specific protocols" if !I2C_HELPER_AUTO
>> +	tristate "SMBus-specific protocols"
>>   	help
>>   	  Say Y here if you want support for SMBus extensions to the I2C
>>   	  specification. At the moment, the only supported extension is
>

[RFC Patch v0 3/3] iio: Capella cm3218x: smbus alert processing

[Srinivas Pandruvada]

Added driver alert callback and modified existing polls to the
core calls.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/iio/light/cm3218x.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/light/cm3218x.c b/drivers/iio/light/cm3218x.c
index e422f68..774b199 100644
--- a/drivers/iio/light/cm3218x.c
+++ b/drivers/iio/light/cm3218x.c
@@ -23,6 +23,7 @@
 #include <linux/iio/events.h>
 #include <linux/init.h>
 #include <linux/acpi.h>
+#include <linux/i2c-smbus.h>
 
 /* Registers Address */
 #define CM3218x_REG_ADDR_CMD		0x00
@@ -130,6 +131,12 @@ static int cm3218x_get_lux(struct cm3218x_chip *chip);
 static int cm3218x_threshold_update(struct cm3218x_chip *chip, int percent);
 static int cm3218x_read_als_it(struct cm3218x_chip *chip, int *val2);
 
+/* alert callback for i2c driver */
+static void cm3218x_alert(struct i2c_client *client, unsigned int flag)
+{
+	dev_err(&client->dev, "SMBUS alert flag %d\n", flag);
+}
+
 /**
  * cm3218x_read_ara() - Read ARA register
  * @cm3218x:	pointer of struct cm3218x.
@@ -142,19 +149,7 @@ static int cm3218x_read_als_it(struct cm3218x_chip *chip, int *val2);
  */
 static int cm3218x_read_ara(struct cm3218x_chip *chip)
 {
-	struct i2c_client *client = chip->client;
-	int status;
-	unsigned short addr;
-
-	addr = client->addr;
-	client->addr = 0x0C;
-	status = i2c_smbus_read_byte(client);
-	client->addr = addr;
-
-	if (status < 0)
-		return -ENODEV;
-
-	return status;
+	return i2c_smbus_ara_poll(chip->client);
 }
 
 /**
@@ -760,6 +755,7 @@ static struct i2c_driver cm3218x_driver = {
 	.id_table	= cm3218x_id,
 	.probe		= cm3218x_probe,
 	.remove		= cm3218x_remove,
+	.alert		= cm3218x_alert,
 };
 
 module_i2c_driver(cm3218x_driver);
-- 
1.8.3.2

Re: [RFC Patch v0 1/3] i2c-smbus: Add poll interface for smbus alert

[Jean Delvare]

On Wed, 26 Mar 2014 17:42:10 -0700, Srinivas Pandruvada wrote:
> The current i2c smbus alert module depends on smbus alert mechanism
> supported by underlying bus drivers. By specifications, these alerts
> can be polled if there is no hardware support.
> Currently multiple drivers who needs smbus alerts are creating
> a new i2c dummy device with address 0x0C (ARA register), by luck
> they don't co-exist. Otherwise i2c device creation will fail.
> Added a poll interface, so that all these driver can call a common
> interface to poll. Even if they polli, all drivers bound to an
> adapater will be notified by their alert callback if ARA register
> read is successful.
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
>  drivers/i2c/i2c-smbus.c   | 23 +++++++++++++++++++++++
>  include/linux/i2c-smbus.h |  3 +++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
> index fc99f0d..e274f20 100644
> --- a/drivers/i2c/i2c-smbus.c
> +++ b/drivers/i2c/i2c-smbus.c
> @@ -72,6 +72,29 @@ static int smbus_do_alert(struct device *dev, void *addrp)
>  	return -EBUSY;
>  }
>  
> +int i2c_smbus_ara_poll(const struct i2c_client *client)
> +{
> +	union i2c_smbus_data data;
> +	int status;
> +	struct alert_data alert_data;
> +
> +	status = i2c_smbus_xfer(client->adapter, 0x0C, 0,
> +				I2C_SMBUS_READ, 0,
> +				I2C_SMBUS_BYTE, &data);
> +	if (status < 0)
> +		return status;
> +
> +	alert_data.flag = data.byte & 1;
> +	alert_data.addr = data.byte >> 1;
> +
> +	/* Notify driver for the device which issued the alert */
> +	device_for_each_child(&client->adapter->dev, &alert_data,
> +				smbus_do_alert);
> +
> +	return data.byte;
> +}

This is essentially duplicating code from smbus_alert(), but in a
hackish way, as the ARA is never properly reserved. Your bus driver
should really register the ARA with i2c_setup_smbus_alert().

I see that the code may not properly deal with the polled case
everywhere but it should be pretty trivial to deal with. For example,
check for alert->irq > 0 before re-enabling the irq in smbus_alert(). I
don't immediately see any other change needed.

If SMBus alert polling is done from the i2c device driver, we'll have
to find a standard way for i2c device drivers to retrieve the ara
client associated with an i2c_adapter. However I still need to be
convinced that this makes any sense at all. Ultimately the alert will
call the i2c device drivers's alert() callback. If the i2c device
driver needs to do that, there's no need to go through ARA, it might as
well just call the callback by itself.

So can you please explain what problem exactly you are trying to solve?

> +EXPORT_SYMBOL_GPL(i2c_smbus_ara_poll);
> +
>  /*
>   * The alert IRQ handler needs to hand work off to a task which can issue
>   * SMBus calls, because those sleeping calls can't be made in IRQ context.
> diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
> index 8f1b086..f70755d 100644
> --- a/include/linux/i2c-smbus.h
> +++ b/include/linux/i2c-smbus.h
> @@ -48,4 +48,7 @@ struct i2c_client *i2c_setup_smbus_alert(struct i2c_adapter *adapter,
>  					 struct i2c_smbus_alert_setup *setup);
>  int i2c_handle_smbus_alert(struct i2c_client *ara);
>  
> +/* Interface to poll smbus alert */
> +int i2c_smbus_ara_poll(const struct i2c_client *client);
> +
>  #endif /* _LINUX_I2C_SMBUS_H */


-- 
Jean Delvare
SUSE L3 Support

Re: [RFC Patch v0 1/3] i2c-smbus: Add poll interface for smbus alert

[Srinivas Pandruvada]

On 03/27/2014 12:44 AM, Jean Delvare wrote:
> On Wed, 26 Mar 2014 17:42:10 -0700, Srinivas Pandruvada wrote:
>> The current i2c smbus alert module depends on smbus alert mechanism
>> supported by underlying bus drivers. By specifications, these alerts
>> can be polled if there is no hardware support.
>> Currently multiple drivers who needs smbus alerts are creating
>> a new i2c dummy device with address 0x0C (ARA register), by luck
>> they don't co-exist. Otherwise i2c device creation will fail.
>> Added a poll interface, so that all these driver can call a common
>> interface to poll. Even if they polli, all drivers bound to an
>> adapater will be notified by their alert callback if ARA register
>> read is successful.
>>
>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>> ---
>>   drivers/i2c/i2c-smbus.c   | 23 +++++++++++++++++++++++
>>   include/linux/i2c-smbus.h |  3 +++
>>   2 files changed, 26 insertions(+)
>>
>> diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
>> index fc99f0d..e274f20 100644
>> --- a/drivers/i2c/i2c-smbus.c
>> +++ b/drivers/i2c/i2c-smbus.c
>> @@ -72,6 +72,29 @@ static int smbus_do_alert(struct device *dev, void *addrp)
>>   	return -EBUSY;
>>   }
>>   
>> +int i2c_smbus_ara_poll(const struct i2c_client *client)
>> +{
>> +	union i2c_smbus_data data;
>> +	int status;
>> +	struct alert_data alert_data;
>> +
>> +	status = i2c_smbus_xfer(client->adapter, 0x0C, 0,
>> +				I2C_SMBUS_READ, 0,
>> +				I2C_SMBUS_BYTE, &data);
>> +	if (status < 0)
>> +		return status;
>> +
>> +	alert_data.flag = data.byte & 1;
>> +	alert_data.addr = data.byte >> 1;
>> +
>> +	/* Notify driver for the device which issued the alert */
>> +	device_for_each_child(&client->adapter->dev, &alert_data,
>> +				smbus_do_alert);
>> +
>> +	return data.byte;
>> +}
> This is essentially duplicating code from smbus_alert(), but in a
> hackish way, as the ARA is never properly reserved. Your bus driver
> should really register the ARA with i2c_setup_smbus_alert().
>
> I see that the code may not properly deal with the polled case
> everywhere but it should be pretty trivial to deal with. For example,
> check for alert->irq > 0 before re-enabling the irq in smbus_alert(). I
> don't immediately see any other change needed.
>
> If SMBus alert polling is done from the i2c device driver, we'll have
> to find a standard way for i2c device drivers to retrieve the ara
> client associated with an i2c_adapter. However I still need to be
> convinced that this makes any sense at all. Ultimately the alert will
> call the i2c device drivers's alert() callback. If the i2c device
> driver needs to do that, there's no need to go through ARA, it might as
> well just call the callback by itself.
>
> So can you please explain what problem exactly you are trying to solve?
The current i2c clients and proposed patches are creating dummy devices.
You can't create two devices with same address. So they can't coexist.
Also 0x0c is a valid i2c address, so we can't create a new i2c device.
We have products there are devices on i2C on 0x0c. So don't you think
after calling
i2c_setup_smbus_alert()
the other device can really be setup by calling  i2c_new_device(). This 
function
does a busy_check for same addresses.

The idea is if some i2c client triggers ARA read,  it doesn't destroy the
real receiver of the the smb alert and call respective alert() callbacks,
>> +EXPORT_SYMBOL_GPL(i2c_smbus_ara_poll);
>> +
>>   /*
>>    * The alert IRQ handler needs to hand work off to a task which can issue
>>    * SMBus calls, because those sleeping calls can't be made in IRQ context.
>> diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
>> index 8f1b086..f70755d 100644
>> --- a/include/linux/i2c-smbus.h
>> +++ b/include/linux/i2c-smbus.h
>> @@ -48,4 +48,7 @@ struct i2c_client *i2c_setup_smbus_alert(struct i2c_adapter *adapter,
>>   					 struct i2c_smbus_alert_setup *setup);
>>   int i2c_handle_smbus_alert(struct i2c_client *ara);
>>   
>> +/* Interface to poll smbus alert */
>> +int i2c_smbus_ara_poll(const struct i2c_client *client);
>> +
>>   #endif /* _LINUX_I2C_SMBUS_H */
>

Re: [RFC Patch v0 1/3] i2c-smbus: Add poll interface for smbus alert

[Jonathan Cameron]

On March 27, 2014 7:44:56 AM GMT+00:00, Jean Delvare <jdelvare-l3A5Bk7waGM@public.gmane.org> wrote:
>On Wed, 26 Mar 2014 17:42:10 -0700, Srinivas Pandruvada wrote:
>> The current i2c smbus alert module depends on smbus alert mechanism
>> supported by underlying bus drivers. By specifications, these alerts
>> can be polled if there is no hardware support.
>> Currently multiple drivers who needs smbus alerts are creating
>> a new i2c dummy device with address 0x0C (ARA register), by luck
>> they don't co-exist. Otherwise i2c device creation will fail.
>> Added a poll interface, so that all these driver can call a common
>> interface to poll. Even if they polli, all drivers bound to an
>> adapater will be notified by their alert callback if ARA register
>> read is successful.
>> 
>> Signed-off-by: Srinivas Pandruvada
><srinivas.pandruvada-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>> ---
>>  drivers/i2c/i2c-smbus.c   | 23 +++++++++++++++++++++++
>>  include/linux/i2c-smbus.h |  3 +++
>>  2 files changed, 26 insertions(+)
>> 
>> diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
>> index fc99f0d..e274f20 100644
>> --- a/drivers/i2c/i2c-smbus.c
>> +++ b/drivers/i2c/i2c-smbus.c
>> @@ -72,6 +72,29 @@ static int smbus_do_alert(struct device *dev, void
>*addrp)
>>  	return -EBUSY;
>>  }
>>  
>> +int i2c_smbus_ara_poll(const struct i2c_client *client)
>> +{
>> +	union i2c_smbus_data data;
>> +	int status;
>> +	struct alert_data alert_data;
>> +
>> +	status = i2c_smbus_xfer(client->adapter, 0x0C, 0,
>> +				I2C_SMBUS_READ, 0,
>> +				I2C_SMBUS_BYTE, &data);
>> +	if (status < 0)
>> +		return status;
>> +
>> +	alert_data.flag = data.byte & 1;
>> +	alert_data.addr = data.byte >> 1;
>> +
>> +	/* Notify driver for the device which issued the alert */
>> +	device_for_each_child(&client->adapter->dev, &alert_data,
>> +				smbus_do_alert);
>> +
>> +	return data.byte;
>> +}
>
>This is essentially duplicating code from smbus_alert(), but in a
>hackish way, as the ARA is never properly reserved. Your bus driver
>should really register the ARA with i2c_setup_smbus_alert().
>
>I see that the code may not properly deal with the polled case
>everywhere but it should be pretty trivial to deal with. For example,
>check for alert->irq > 0 before re-enabling the irq in smbus_alert(). I
>don't immediately see any other change needed.
>
>If SMBus alert polling is done from the i2c device driver, we'll have
>to find a standard way for i2c device drivers to retrieve the ara
>client associated with an i2c_adapter. However I still need to be
>convinced that this makes any sense at all. Ultimately the alert will
>call the i2c device drivers's alert() callback. If the i2c device
>driver needs to do that, there's no need to go through ARA, it might as
>well just call the callback by itself.
>
>So can you please explain what problem exactly you are trying to solve?

As I understand it the issue is that some parts will not clear their internal interrupt status unless an at a read occurs and presumably the read has to get the correct address?  

To my mind we should have polling inside the core and it should ensure all devices that want to have replied.  Have I miss understood how Ara is supposed to work but should we not know which device to call the callback on?
>
>> +EXPORT_SYMBOL_GPL(i2c_smbus_ara_poll);
>> +
>>  /*
>>   * The alert IRQ handler needs to hand work off to a task which can
>issue
>>   * SMBus calls, because those sleeping calls can't be made in IRQ
>context.
>> diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
>> index 8f1b086..f70755d 100644
>> --- a/include/linux/i2c-smbus.h
>> +++ b/include/linux/i2c-smbus.h
>> @@ -48,4 +48,7 @@ struct i2c_client *i2c_setup_smbus_alert(struct
>i2c_adapter *adapter,
>>  					 struct i2c_smbus_alert_setup *setup);
>>  int i2c_handle_smbus_alert(struct i2c_client *ara);
>>  
>> +/* Interface to poll smbus alert */
>> +int i2c_smbus_ara_poll(const struct i2c_client *client);
>> +
>>  #endif /* _LINUX_I2C_SMBUS_H */

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

Re: [RFC Patch v0 1/3] i2c-smbus: Add poll interface for smbus alert

[Jonathan Cameron]

On 27/03/14 21:50, Srinivas Pandruvada wrote:
> On 03/27/2014 10:34 AM, Jonathan Cameron wrote:
>>
>> On March 27, 2014 7:44:56 AM GMT+00:00, Jean Delvare<jdelvare-l3A5Bk7waGM@public.gmane.org>  wrote:
>>> On Wed, 26 Mar 2014 17:42:10 -0700, Srinivas Pandruvada wrote:
>>>> The current i2c smbus alert module depends on smbus alert mechanism
>>>> supported by underlying bus drivers. By specifications, these alerts
>>>> can be polled if there is no hardware support.
>>>> Currently multiple drivers who needs smbus alerts are creating
>>>> a new i2c dummy device with address 0x0C (ARA register), by luck
>>>> they don't co-exist. Otherwise i2c device creation will fail.
>>>> Added a poll interface, so that all these driver can call a common
>>>> interface to poll. Even if they polli, all drivers bound to an
>>>> adapater will be notified by their alert callback if ARA register
>>>> read is successful.
>>>>
>>>> Signed-off-by: Srinivas Pandruvada
>>> <srinivas.pandruvada-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>>>> ---
>>>>   drivers/i2c/i2c-smbus.c   | 23 +++++++++++++++++++++++
>>>>   include/linux/i2c-smbus.h |  3 +++
>>>>   2 files changed, 26 insertions(+)
>>>>
>>>> diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
>>>> index fc99f0d..e274f20 100644
>>>> --- a/drivers/i2c/i2c-smbus.c
>>>> +++ b/drivers/i2c/i2c-smbus.c
>>>> @@ -72,6 +72,29 @@ static int smbus_do_alert(struct device *dev, void
>>> *addrp)
>>>>   	return -EBUSY;
>>>>   }
>>>>
>>>> +int i2c_smbus_ara_poll(const struct i2c_client *client)
>>>> +{
>>>> +	union i2c_smbus_data data;
>>>> +	int status;
>>>> +	struct alert_data alert_data;
>>>> +
>>>> +	status = i2c_smbus_xfer(client->adapter, 0x0C, 0,
>>>> +				I2C_SMBUS_READ, 0,
>>>> +				I2C_SMBUS_BYTE, &data);
>>>> +	if (status < 0)
>>>> +		return status;
>>>> +
>>>> +	alert_data.flag = data.byte & 1;
>>>> +	alert_data.addr = data.byte >> 1;
>>>> +
>>>> +	/* Notify driver for the device which issued the alert */
>>>> +	device_for_each_child(&client->adapter->dev, &alert_data,
>>>> +				smbus_do_alert);
>>>> +
>>>> +	return data.byte;
>>>> +}
>>> This is essentially duplicating code from smbus_alert(), but in a
>>> hackish way, as the ARA is never properly reserved. Your bus driver
>>> should really register the ARA with i2c_setup_smbus_alert().
>>>
>>> I see that the code may not properly deal with the polled case
>>> everywhere but it should be pretty trivial to deal with. For example,
>>> check for alert->irq > 0 before re-enabling the irq in smbus_alert(). I
>>> don't immediately see any other change needed.
>>>
>>> If SMBus alert polling is done from the i2c device driver, we'll have
>>> to find a standard way for i2c device drivers to retrieve the ara
>>> client associated with an i2c_adapter. However I still need to be
>>> convinced that this makes any sense at all. Ultimately the alert will
>>> call the i2c device drivers's alert() callback. If the i2c device
>>> driver needs to do that, there's no need to go through ARA, it might as
>>> well just call the callback by itself.
>>>
>>> So can you please explain what problem exactly you are trying to solve?
>> As I understand it the issue is that some parts will not clear
>> their internal interrupt status unless an at a read occurs and
>> presumably the read has to get the correct address?
>>
>> To my mind we should have polling inside the core and it should
>> ensure all devices that want to have replied. Have I miss
>> understood how Ara is supposed to work but should we not know which
>> device to call the callback on?
Ah, I was barking up the wrong tree with this one and had missed the
check that was conducted to ensure that only the right device is
notified.  Sorry about that.

> This is summary: May be Jean can suggest some better solution: - We
> have some sensor devices on i2C bus, which need to read ARA register
> to ack smbus alert signal. Without acking this, they will not allow
> further reads. They currently are calling i2c_new_dummy() for
> reserving ARA (0x0C) register. But the problem is if there are two
> devices on this bus both have to read ARA, they can't call
> i2c_new_dummy() as one of them will fail (__i2c_check_addr_busy()
> will return -EBUSY). - In above case, If they use hacked way to use
> i2_transfer function, but it will not address the real issue. Because
> one driver is reading ARA basically can acknowledge ALERT from wrong
> device.
>
> So their needs to be a core level poll so that devices can be
> notified, if there is an alert for their device only.
Agreed.  Right now at a quick glance, all devices are notified which
seems odd...
>
> From physical interface level many i2controllers (including x86s)
> don't have separate signal for SMBALERT#. So somewhere it needs
> polling. For some devices SMBALERT is wired through a GPIO for GPIO
> interrupt. But they still need to read ARA to ACK.
If it is wired through a gpio interrupt (and that interrupt is shared
by various smbalert equipped devices - or there is only one present
on the i2c bus) then it should be handled using the existing
infrastructure. (perhaps modified slightly)
>
> Also we can't blindly reserve 0x0c for every i2c-bus as 0x0C is a
> valid i2C address. There are already occupied (e.g. AK8963 slave
> address is 0x0C on the same platforms). So can't call
> i2c_setup_smbus_alert from the bus driver for all i2c buses.
If platforms are wiring up both devices using this i2c address and
smbus alert devices then we are in an irritating corner case.
This is effectively the same as having two different smbus devices
on the same address and relying on the fact they don't have
overlapping address spaces.  Nuts but I guess we want to find some
way of coping with it?

Perhaps we need  away of allow multiple i2c devices to share a bus
in this case?  Is this something that could be done via a nefarious
hack of using a mux to pretend that they are separately wired
(when they aren't).
>
> For polling, a bus driver can't decide periodic polling as this is
> power/perf drain.
Agreed, so hints from the drivers to the core would be needed.
> So my idea was to somehow kick a read on ARA on a
> bus from interested clients may be when the receive GPIO interrupt.
Then you can end up with considerable waste if you have lots of devices
using this functionality on the same bus.  Each driver polls at say
50Hz, because it wants 50Hz, but you have 20 devices hence you get
uneven sampling at say 1kHz and might even block the i2c bus up entirely.
Hence the polling needs to be handled by the core.  I'd suggest
a call to request enabling of polling and a frequency.  Then the
core just uses the highest frequency currently requested.

It's a bit more complex than the nice sensible interrupt driven
version, but if board designers are going to do silly things
I guess we have to figure out how to make them work.

> But the results are notified to interested drivers who needs smbus
> alert processing.
> Thanks,
> Srinivas
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>>>> +EXPORT_SYMBOL_GPL(i2c_smbus_ara_poll);
>>>> +
>>>>   /*
>>>>    * The alert IRQ handler needs to hand work off to a task which can
>>> issue
>>>>    * SMBus calls, because those sleeping calls can't be made in IRQ
>>> context.
>>>> diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
>>>> index 8f1b086..f70755d 100644
>>>> --- a/include/linux/i2c-smbus.h
>>>> +++ b/include/linux/i2c-smbus.h
>>>> @@ -48,4 +48,7 @@ struct i2c_client *i2c_setup_smbus_alert(struct
>>> i2c_adapter *adapter,
>>>>   					 struct i2c_smbus_alert_setup *setup);
>>>>   int i2c_handle_smbus_alert(struct i2c_client *ara);
>>>>
>>>> +/* Interface to poll smbus alert */
>>>> +int i2c_smbus_ara_poll(const struct i2c_client *client);
>>>> +
>>>>   #endif /* _LINUX_I2C_SMBUS_H */
>

Re: [RFC Patch v0 1/3] i2c-smbus: Add poll interface for smbus alert

[Srinivas Pandruvada]

On 03/29/2014 03:15 AM, Jonathan Cameron wrote:
> On 27/03/14 21:50, Srinivas Pandruvada wrote:
>> On 03/27/2014 10:34 AM, Jonathan Cameron wrote:
>>>
>>> On March 27, 2014 7:44:56 AM GMT+00:00, Jean 
>>> Delvare<jdelvare-l3A5Bk7waGM@public.gmane.org>  wrote:
>>>> On Wed, 26 Mar 2014 17:42:10 -0700, Srinivas Pandruvada wrote:
>>>>> The current i2c smbus alert module depends on smbus alert mechanism
>>>>> supported by underlying bus drivers. By specifications, these alerts
>>>>> can be polled if there is no hardware support.
>>>>> Currently multiple drivers who needs smbus alerts are creating
>>>>> a new i2c dummy device with address 0x0C (ARA register), by luck
>>>>> they don't co-exist. Otherwise i2c device creation will fail.
>>>>> Added a poll interface, so that all these driver can call a common
>>>>> interface to poll. Even if they polli, all drivers bound to an
>>>>> adapater will be notified by their alert callback if ARA register
>>>>> read is successful.
>>>>>
>>>>> Signed-off-by: Srinivas Pandruvada
>>>> <srinivas.pandruvada-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>>>>> ---
>>>>>   drivers/i2c/i2c-smbus.c   | 23 +++++++++++++++++++++++
>>>>>   include/linux/i2c-smbus.h |  3 +++
>>>>>   2 files changed, 26 insertions(+)
>>>>>
>>>>> diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
>>>>> index fc99f0d..e274f20 100644
>>>>> --- a/drivers/i2c/i2c-smbus.c
>>>>> +++ b/drivers/i2c/i2c-smbus.c
>>>>> @@ -72,6 +72,29 @@ static int smbus_do_alert(struct device *dev, void
>>>> *addrp)
>>>>>       return -EBUSY;
>>>>>   }
>>>>>
>>>>> +int i2c_smbus_ara_poll(const struct i2c_client *client)
>>>>> +{
>>>>> +    union i2c_smbus_data data;
>>>>> +    int status;
>>>>> +    struct alert_data alert_data;
>>>>> +
>>>>> +    status = i2c_smbus_xfer(client->adapter, 0x0C, 0,
>>>>> +                I2C_SMBUS_READ, 0,
>>>>> +                I2C_SMBUS_BYTE, &data);
>>>>> +    if (status < 0)
>>>>> +        return status;
>>>>> +
>>>>> +    alert_data.flag = data.byte & 1;
>>>>> +    alert_data.addr = data.byte >> 1;
>>>>> +
>>>>> +    /* Notify driver for the device which issued the alert */
>>>>> + device_for_each_child(&client->adapter->dev, &alert_data,
>>>>> +                smbus_do_alert);
>>>>> +
>>>>> +    return data.byte;
>>>>> +}
>>>> This is essentially duplicating code from smbus_alert(), but in a
>>>> hackish way, as the ARA is never properly reserved. Your bus driver
>>>> should really register the ARA with i2c_setup_smbus_alert().
>>>>
>>>> I see that the code may not properly deal with the polled case
>>>> everywhere but it should be pretty trivial to deal with. For example,
>>>> check for alert->irq > 0 before re-enabling the irq in 
>>>> smbus_alert(). I
>>>> don't immediately see any other change needed.
>>>>
>>>> If SMBus alert polling is done from the i2c device driver, we'll have
>>>> to find a standard way for i2c device drivers to retrieve the ara
>>>> client associated with an i2c_adapter. However I still need to be
>>>> convinced that this makes any sense at all. Ultimately the alert will
>>>> call the i2c device drivers's alert() callback. If the i2c device
>>>> driver needs to do that, there's no need to go through ARA, it 
>>>> might as
>>>> well just call the callback by itself.
>>>>
>>>> So can you please explain what problem exactly you are trying to 
>>>> solve?
>>> As I understand it the issue is that some parts will not clear
>>> their internal interrupt status unless an at a read occurs and
>>> presumably the read has to get the correct address?
>>>
>>> To my mind we should have polling inside the core and it should
>>> ensure all devices that want to have replied. Have I miss
>>> understood how Ara is supposed to work but should we not know which
>>> device to call the callback on?
> Ah, I was barking up the wrong tree with this one and had missed the
> check that was conducted to ensure that only the right device is
> notified.  Sorry about that.
>
>> This is summary: May be Jean can suggest some better solution: - We
>> have some sensor devices on i2C bus, which need to read ARA register
>> to ack smbus alert signal. Without acking this, they will not allow
>> further reads. They currently are calling i2c_new_dummy() for
>> reserving ARA (0x0C) register. But the problem is if there are two
>> devices on this bus both have to read ARA, they can't call
>> i2c_new_dummy() as one of them will fail (__i2c_check_addr_busy()
>> will return -EBUSY). - In above case, If they use hacked way to use
>> i2_transfer function, but it will not address the real issue. Because
>> one driver is reading ARA basically can acknowledge ALERT from wrong
>> device.
>>
>> So their needs to be a core level poll so that devices can be
>> notified, if there is an alert for their device only.
> Agreed.  Right now at a quick glance, all devices are notified which
> seems odd...
>>
>> From physical interface level many i2controllers (including x86s)
>> don't have separate signal for SMBALERT#. So somewhere it needs
>> polling. For some devices SMBALERT is wired through a GPIO for GPIO
>> interrupt. But they still need to read ARA to ACK.
> If it is wired through a gpio interrupt (and that interrupt is shared
> by various smbalert equipped devices - or there is only one present
> on the i2c bus) then it should be handled using the existing
> infrastructure. (perhaps modified slightly)
If we somehow know that the interrupt is for SMBALERT, we could
do with the current mechanisms. The ACPI configuration only provides
an IRQ number for each device, but don't provide whether it is for SMBALERT.
If we know we can handle it better. I will ping ACPI committee, if they 
can add better support.
>>
>> Also we can't blindly reserve 0x0c for every i2c-bus as 0x0C is a
>> valid i2C address. There are already occupied (e.g. AK8963 slave
>> address is 0x0C on the same platforms). So can't call
>> i2c_setup_smbus_alert from the bus driver for all i2c buses.
> If platforms are wiring up both devices using this i2c address and
> smbus alert devices then we are in an irritating corner case.
> This is effectively the same as having two different smbus devices
> on the same address and relying on the fact they don't have
> overlapping address spaces.  Nuts but I guess we want to find some
> way of coping with it?
>
> Perhaps we need  away of allow multiple i2c devices to share a bus
> in this case?  Is this something that could be done via a nefarious
> hack of using a mux to pretend that they are separately wired
> (when they aren't).
>>
>> For polling, a bus driver can't decide periodic polling as this is
>> power/perf drain.
> Agreed, so hints from the drivers to the core would be needed.
>> So my idea was to somehow kick a read on ARA on a
>> bus from interested clients may be when the receive GPIO interrupt.
> Then you can end up with considerable waste if you have lots of devices
> using this functionality on the same bus.  Each driver polls at say
> 50Hz, because it wants 50Hz, but you have 20 devices hence you get
> uneven sampling at say 1kHz and might even block the i2c bus up entirely.
> Hence the polling needs to be handled by the core.  I'd suggest
> a call to request enabling of polling and a frequency.  Then the
> core just uses the highest frequency currently requested.
>
Agreed.
> It's a bit more complex than the nice sensible interrupt driven
> version, but if board designers are going to do silly things
> I guess we have to figure out how to make them work.
>
That is the reality.

Thanks,
Srinivas
>> But the results are notified to interested drivers who needs smbus
>> alert processing.
>> Thanks,
>> Srinivas
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>>>> +EXPORT_SYMBOL_GPL(i2c_smbus_ara_poll);
>>>>> +
>>>>>   /*
>>>>>    * The alert IRQ handler needs to hand work off to a task which can
>>>> issue
>>>>>    * SMBus calls, because those sleeping calls can't be made in IRQ
>>>> context.
>>>>> diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
>>>>> index 8f1b086..f70755d 100644
>>>>> --- a/include/linux/i2c-smbus.h
>>>>> +++ b/include/linux/i2c-smbus.h
>>>>> @@ -48,4 +48,7 @@ struct i2c_client *i2c_setup_smbus_alert(struct
>>>> i2c_adapter *adapter,
>>>>>                        struct i2c_smbus_alert_setup *setup);
>>>>>   int i2c_handle_smbus_alert(struct i2c_client *ara);
>>>>>
>>>>> +/* Interface to poll smbus alert */
>>>>> +int i2c_smbus_ara_poll(const struct i2c_client *client);
>>>>> +
>>>>>   #endif /* _LINUX_I2C_SMBUS_H */
>>
>
>