[PATCH] i2c-mux-pca954x: Disable mux after 200ms timeout

[PATCH] i2c-mux-pca954x: Disable mux after 200ms timeout

[Mike Looijmans]

Leaving the mux enabled causes needless I2C traffic on the downstream
bus. De-selecting after every request causes excess I2C traffic and
switching.

This patch implements a hybrid solution: After 200ms of inactivity,
the mux is disabled.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
---
 drivers/i2c/muxes/i2c-mux-pca954x.c |   45 ++++++++++++++++++++++++++++++++---
 1 file changed, 42 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index a531d80..1241c97 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -41,6 +41,7 @@
 #include <linux/device.h>
 #include <linux/i2c.h>
 #include <linux/i2c-mux.h>
+#include <linux/workqueue.h>
 
 #include <linux/i2c/pca954x.h>
 
@@ -60,7 +61,8 @@ enum pca_type {
 struct pca954x {
 	enum pca_type type;
 	struct i2c_adapter *virt_adaps[PCA954X_MAX_NCHANS];
-
+	struct i2c_client *client;
+	struct delayed_work deselect_work;
 	u8 last_chan;		/* last register value */
 };
 
@@ -168,11 +170,43 @@ static int pca954x_select_chan(struct i2c_adapter *adap,
 	return ret;
 }
 
-static int pca954x_deselect_mux(struct i2c_adapter *adap,
+static void pca954x_deselect_work(struct work_struct *work)
+{
+	struct pca954x *data = container_of(
+				work, struct pca954x, deselect_work.work);
+	/* Use the adapter lock as a mutex because any method that changes
+	 * the mux state will also hold this mutex. If the bus is in use,
+	 * the lock will assure that at least that transaction will
+	 * complete. */
+	i2c_lock_adapter(data->client->adapter);
+	if (data->last_chan != 0) {
+		int res;
+		/* Disable mux */
+		dev_dbg(&client->dev, "deselecting mux\n");
+		data->last_chan = 0;
+		res = pca954x_reg_write(data->client->adapter,
+				data->client, data->last_chan);
+		if (res < 0)
+			dev_err(&client->dev,
+				"%s: pca954x_reg_write failed: %d\n",
+				__func__, res);
+	}
+	i2c_unlock_adapter(data->client->adapter);
+}
+
+static int pca954x_deselect_mux_delayed(struct i2c_adapter *adap,
 				void *client, u32 chan)
 {
 	struct pca954x *data = i2c_get_clientdata(client);
+	/* Setup timer to disable at a later interval */
+	schedule_delayed_work(&data->deselect_work, msecs_to_jiffies(200));
+	return 0;
+}
 
+static int pca954x_deselect_mux_immediate(struct i2c_adapter *adap,
+				void *client, u32 chan)
+{
+	struct pca954x *data = i2c_get_clientdata(client);
 	/* Deselect active channel */
 	data->last_chan = 0;
 	return pca954x_reg_write(adap, client, data->last_chan);
@@ -212,6 +246,8 @@ static int pca954x_probe(struct i2c_client *client,
 
 	data->type = id->driver_data;
 	data->last_chan = 0;		   /* force the first selection */
+	data->client = client;
+	INIT_DELAYED_WORK(&data->deselect_work, pca954x_deselect_work);
 
 	/* Now create an adapter for each channel */
 	for (num = 0; num < chips[data->type].nchans; num++) {
@@ -231,7 +267,8 @@ static int pca954x_probe(struct i2c_client *client,
 			i2c_add_mux_adapter(adap, &client->dev, client,
 				force, num, class, pca954x_select_chan,
 				(pdata && pdata->modes[num].deselect_on_exit)
-					? pca954x_deselect_mux : NULL);
+					? pca954x_deselect_mux_immediate
+					: pca954x_deselect_mux_delayed);
 
 		if (data->virt_adaps[num] == NULL) {
 			ret = -ENODEV;
@@ -264,6 +301,8 @@ static int pca954x_remove(struct i2c_client *client)
 	const struct chip_desc *chip = &chips[data->type];
 	int i;
 
+	cancel_delayed_work_sync(&data->deselect_work);
+
 	for (i = 0; i < chip->nchans; ++i)
 		if (data->virt_adaps[i]) {
 			i2c_del_mux_adapter(data->virt_adaps[i]);
-- 
1.7.9.5

Re: [PATCH] i2c-mux-pca954x: Disable mux after 200ms timeout

[Michael Lawnick]

Hi,

Am 25.11.2013 14:43, schrieb Mike Looijmans:
> Leaving the mux enabled causes needless I2C traffic on the downstream
> bus. De-selecting after every request causes excess I2C traffic and
> switching.
>
> This patch implements a hybrid solution: After 200ms of inactivity,
> the mux is disabled.
...

Have you checked against behavior on cascaded muxes?
At least your desired timing will not look as expected.
200 ms: disable mux1
201 ms: enable mux1 - disable mux2
202 ms: enable mux2 - disable mux3
401 ms: disable mux1
402 ms: enable mux1 - disable mux2
602 ms: disable mux1
And on full second temperature sensor behind mux3 is read again ...

KR
Michael

Re: [PATCH] i2c-mux-pca954x: Disable mux after 200ms timeout

[Wolfram Sang]

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

On Mon, Nov 25, 2013 at 02:43:57PM +0100, Mike Looijmans wrote:

> Leaving the mux enabled causes needless I2C traffic on the downstream
> bus.

This is a bus. Why is this bad?


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH] i2c-mux-pca954x: Disable mux after 200ms timeout

[Mike Looijmans]

Leaving the mux enabled causes needless I2C traffic on the downstream
bus. De-selecting after every request causes excess I2C traffic and
switching.

This patch implements a hybrid solution: After 200ms of inactivity,
the mux is disabled.

Signed-off-by: Mike Looijmans <mike.looijmans-Oq418RWZeHk@public.gmane.org>
---
 drivers/i2c/muxes/i2c-mux-pca954x.c |   45 ++++++++++++++++++++++++++++++++---
 1 file changed, 42 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index a531d80..1241c97 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -41,6 +41,7 @@
 #include <linux/device.h>
 #include <linux/i2c.h>
 #include <linux/i2c-mux.h>
+#include <linux/workqueue.h>
 
 #include <linux/i2c/pca954x.h>
 
@@ -60,7 +61,8 @@ enum pca_type {
 struct pca954x {
 	enum pca_type type;
 	struct i2c_adapter *virt_adaps[PCA954X_MAX_NCHANS];
-
+	struct i2c_client *client;
+	struct delayed_work deselect_work;
 	u8 last_chan;		/* last register value */
 };
 
@@ -168,11 +170,43 @@ static int pca954x_select_chan(struct i2c_adapter *adap,
 	return ret;
 }
 
-static int pca954x_deselect_mux(struct i2c_adapter *adap,
+static void pca954x_deselect_work(struct work_struct *work)
+{
+	struct pca954x *data = container_of(
+				work, struct pca954x, deselect_work.work);
+	/* Use the adapter lock as a mutex because any method that changes
+	 * the mux state will also hold this mutex. If the bus is in use,
+	 * the lock will assure that at least that transaction will
+	 * complete. */
+	i2c_lock_adapter(data->client->adapter);
+	if (data->last_chan != 0) {
+		int res;
+		/* Disable mux */
+		dev_dbg(&client->dev, "deselecting mux\n");
+		data->last_chan = 0;
+		res = pca954x_reg_write(data->client->adapter,
+				data->client, data->last_chan);
+		if (res < 0)
+			dev_err(&client->dev,
+				"%s: pca954x_reg_write failed: %d\n",
+				__func__, res);
+	}
+	i2c_unlock_adapter(data->client->adapter);
+}
+
+static int pca954x_deselect_mux_delayed(struct i2c_adapter *adap,
 				void *client, u32 chan)
 {
 	struct pca954x *data = i2c_get_clientdata(client);
+	/* Setup timer to disable at a later interval */
+	schedule_delayed_work(&data->deselect_work, msecs_to_jiffies(200));
+	return 0;
+}
 
+static int pca954x_deselect_mux_immediate(struct i2c_adapter *adap,
+				void *client, u32 chan)
+{
+	struct pca954x *data = i2c_get_clientdata(client);
 	/* Deselect active channel */
 	data->last_chan = 0;
 	return pca954x_reg_write(adap, client, data->last_chan);
@@ -212,6 +246,8 @@ static int pca954x_probe(struct i2c_client *client,
 
 	data->type = id->driver_data;
 	data->last_chan = 0;		   /* force the first selection */
+	data->client = client;
+	INIT_DELAYED_WORK(&data->deselect_work, pca954x_deselect_work);
 
 	/* Now create an adapter for each channel */
 	for (num = 0; num < chips[data->type].nchans; num++) {
@@ -231,7 +267,8 @@ static int pca954x_probe(struct i2c_client *client,
 			i2c_add_mux_adapter(adap, &client->dev, client,
 				force, num, class, pca954x_select_chan,
 				(pdata && pdata->modes[num].deselect_on_exit)
-					? pca954x_deselect_mux : NULL);
+					? pca954x_deselect_mux_immediate
+					: pca954x_deselect_mux_delayed);
 
 		if (data->virt_adaps[num] == NULL) {
 			ret = -ENODEV;
@@ -264,6 +301,8 @@ static int pca954x_remove(struct i2c_client *client)
 	const struct chip_desc *chip = &chips[data->type];
 	int i;
 
+	cancel_delayed_work_sync(&data->deselect_work);
+
 	for (i = 0; i < chip->nchans; ++i)
 		if (data->virt_adaps[i]) {
 			i2c_del_mux_adapter(data->virt_adaps[i]);
-- 
1.7.9.5

[PATCH] i2c-mux-pca954x: Disable mux after 200ms timeout

[Mike Looijmans]

Leaving the mux enabled causes needless I2C traffic on the downstream
bus. De-selecting after every request causes excess I2C traffic and
switching.

This patch implements a hybrid solution: After 200ms of inactivity,
the mux is disabled.

Signed-off-by: Mike Looijmans <mike.looijmans-Oq418RWZeHk@public.gmane.org>
---
 drivers/i2c/muxes/i2c-mux-pca954x.c |   45 ++++++++++++++++++++++++++++++++---
 1 file changed, 42 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index a531d80..1241c97 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -41,6 +41,7 @@
 #include <linux/device.h>
 #include <linux/i2c.h>
 #include <linux/i2c-mux.h>
+#include <linux/workqueue.h>
 
 #include <linux/i2c/pca954x.h>
 
@@ -60,7 +61,8 @@ enum pca_type {
 struct pca954x {
 	enum pca_type type;
 	struct i2c_adapter *virt_adaps[PCA954X_MAX_NCHANS];
-
+	struct i2c_client *client;
+	struct delayed_work deselect_work;
 	u8 last_chan;		/* last register value */
 };
 
@@ -168,11 +170,43 @@ static int pca954x_select_chan(struct i2c_adapter *adap,
 	return ret;
 }
 
-static int pca954x_deselect_mux(struct i2c_adapter *adap,
+static void pca954x_deselect_work(struct work_struct *work)
+{
+	struct pca954x *data = container_of(
+				work, struct pca954x, deselect_work.work);
+	/* Use the adapter lock as a mutex because any method that changes
+	 * the mux state will also hold this mutex. If the bus is in use,
+	 * the lock will assure that at least that transaction will
+	 * complete. */
+	i2c_lock_adapter(data->client->adapter);
+	if (data->last_chan != 0) {
+		int res;
+		/* Disable mux */
+		dev_dbg(&client->dev, "deselecting mux\n");
+		data->last_chan = 0;
+		res = pca954x_reg_write(data->client->adapter,
+				data->client, data->last_chan);
+		if (res < 0)
+			dev_err(&client->dev,
+				"%s: pca954x_reg_write failed: %d\n",
+				__func__, res);
+	}
+	i2c_unlock_adapter(data->client->adapter);
+}
+
+static int pca954x_deselect_mux_delayed(struct i2c_adapter *adap,
 				void *client, u32 chan)
 {
 	struct pca954x *data = i2c_get_clientdata(client);
+	/* Setup timer to disable at a later interval */
+	schedule_delayed_work(&data->deselect_work, msecs_to_jiffies(200));
+	return 0;
+}
 
+static int pca954x_deselect_mux_immediate(struct i2c_adapter *adap,
+				void *client, u32 chan)
+{
+	struct pca954x *data = i2c_get_clientdata(client);
 	/* Deselect active channel */
 	data->last_chan = 0;
 	return pca954x_reg_write(adap, client, data->last_chan);
@@ -212,6 +246,8 @@ static int pca954x_probe(struct i2c_client *client,
 
 	data->type = id->driver_data;
 	data->last_chan = 0;		   /* force the first selection */
+	data->client = client;
+	INIT_DELAYED_WORK(&data->deselect_work, pca954x_deselect_work);
 
 	/* Now create an adapter for each channel */
 	for (num = 0; num < chips[data->type].nchans; num++) {
@@ -231,7 +267,8 @@ static int pca954x_probe(struct i2c_client *client,
 			i2c_add_mux_adapter(adap, &client->dev, client,
 				force, num, class, pca954x_select_chan,
 				(pdata && pdata->modes[num].deselect_on_exit)
-					? pca954x_deselect_mux : NULL);
+					? pca954x_deselect_mux_immediate
+					: pca954x_deselect_mux_delayed);
 
 		if (data->virt_adaps[num] == NULL) {
 			ret = -ENODEV;
@@ -264,6 +301,8 @@ static int pca954x_remove(struct i2c_client *client)
 	const struct chip_desc *chip = &chips[data->type];
 	int i;
 
+	cancel_delayed_work_sync(&data->deselect_work);
+
 	for (i = 0; i < chip->nchans; ++i)
 		if (data->virt_adaps[i]) {
 			i2c_del_mux_adapter(data->virt_adaps[i]);
-- 
1.7.9.5

Re: [PATCH] i2c-mux-pca954x: Disable mux after 200ms timeout

[Wolfram Sang]

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

On Tue, Nov 26, 2013 at 07:32:00AM +0100, Mike Looijmans wrote:
> Leaving the mux enabled causes needless I2C traffic on the downstream
> bus. De-selecting after every request causes excess I2C traffic and
> switching.
> 
> This patch implements a hybrid solution: After 200ms of inactivity,
> the mux is disabled.
> 
> Signed-off-by: Mike Looijmans <mike.looijmans-Oq418RWZeHk@public.gmane.org>

You still haven't answered my initial question. Also, please don't
resend without saying what changed since the last version.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] i2c-mux-pca954x: Disable mux after 200ms timeout

[Mike Looijmans]

On 11/26/2013 10:06 AM, Wolfram Sang wrote:
> On Tue, Nov 26, 2013 at 07:32:00AM +0100, Mike Looijmans wrote:
>> Leaving the mux enabled causes needless I2C traffic on the downstream
>> bus. De-selecting after every request causes excess I2C traffic and
>> switching.
>>
>> This patch implements a hybrid solution: After 200ms of inactivity,
>> the mux is disabled.
>>
>> Signed-off-by: Mike Looijmans <mike.looijmans-Oq418RWZeHk@public.gmane.org>
>
> You still haven't answered my initial question. Also, please don't
> resend without saying what changed since the last version.

Sorry, I never received the patch mail nor any replies into my own mailbox, so 
I assumed the message was never actually sent (probably our mail server 
considered it spam). I found it online though, so I'll try to answer your 
questions now.

Michael Lawnick:
 > Have you checked against behavior on cascaded muxes?

No, I didn't even realize it was at all possible to cascade muxes. And you're 
right, this patch will break those.

Would it be acceptable if I made the timeout optional, by making the 
"deselect_on_exit" boolean into a three-state value (off, on, timeout)? Or, 
alternatively, implement "deselect_on_exit" using the timeout scheme (probably 
with a very short timeout)?


Wolfram Sang:
 > This is a bus. Why is this bad?

My customer has two reasons for wanting this change:

The extra I2C traffic consumes extra power. If the bus is terminated using 2k 
resistors, approximately 1mA of current (assuming ~2V signals) is flowing when 
the bus is pulled low. On low power designs, this extra power consumption is 
noticable. There is no way to turn the mux "off" from either user or kernel 
space. The signals going through the mux to a place where no I2C device is 
actually listening are only wasting power.

The I2C signals are used to control sensitive codecs. When looking at the 
sampled data, the I2C traffic is visible in the captured analog signal. To 
prevent this from happening, the mux can be disabled whenever not 
communicating with the codec. This could be accomplished with the 
"deselect_on_exit" boolean, but because audio driver sends the codec 
parameters in dozens of small transactions, this will result in a lot more 
needless I2C traffic, constantly switching the mux for each codec setting.


Kind regards,

Mike.

Re: [PATCH] i2c-mux-pca954x: Disable mux after 200ms timeout

[Wolfram Sang]

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


CCing linux-pm, maybe they know more...

> The extra I2C traffic consumes extra power. If the bus is terminated
> using 2k resistors, approximately 1mA of current (assuming ~2V
> signals) is flowing when the bus is pulled low. On low power
> designs, this extra power consumption is noticable. There is no way
> to turn the mux "off" from either user or kernel space. The signals
> going through the mux to a place where no I2C device is actually
> listening are only wasting power.

I only have an overview of current linux pm mechanisms. I wonder if
those can't be used somehow. Like if devices on the multiplexed bus are
idle (well, only regarding transfers), then we can switch off the muxer.

> The I2C signals are used to control sensitive codecs. When looking
> at the sampled data, the I2C traffic is visible in the captured
> analog signal. To prevent this from happening, the mux can be

I wonder: Is this really a feature of sensitive codecs or an issue of
the board design?

> disabled whenever not communicating with the codec. This could be
> accomplished with the "deselect_on_exit" boolean, but because audio
> driver sends the codec parameters in dozens of small transactions,
> this will result in a lot more needless I2C traffic, constantly
> switching the mux for each codec setting.

Has this been looked at? ASoC supports grouping of tranfers IIRC. Maybe
your I2C driver is only missing I2C_M_NOSTART?.

> Would it be acceptable if I made the timeout optional, by making the
> "deselect_on_exit" boolean into a three-state value (off, on,
> timeout)? Or, alternatively, implement "deselect_on_exit" using the
> timeout scheme (probably with a very short timeout)?

I have a number of concerns designwise. First, if we do something like
shutting-down-a-bus-if-there-are-no-transfers-expected, it definately
should be in the core, not the driver. As said before, I have the
assumption it should even be connected to the runtime pm core via some
callback. And if we have that for I2C, we surely want that for other
buses as well, at least SPI. Also, the timeout thing sounds too
heuristic to me. Later, people might want to change the timeout value
depending on workloads or so, and then a governor, etc... No, thanks.

BTW is it feasible to shut down the whole I2C bus at controller level
after transfers? No needless transfers and maybe even more power
savings.

Anyway, thanks for letting me know about your requirements (they should
have been in the original commit message, though ;))


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] i2c-mux-pca954x: Disable mux after 200ms timeout

[Ulf Hansson]

On 26 November 2013 13:28, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
>
> CCing linux-pm, maybe they know more...
>
>> The extra I2C traffic consumes extra power. If the bus is terminated
>> using 2k resistors, approximately 1mA of current (assuming ~2V
>> signals) is flowing when the bus is pulled low. On low power
>> designs, this extra power consumption is noticable. There is no way
>> to turn the mux "off" from either user or kernel space. The signals
>> going through the mux to a place where no I2C device is actually
>> listening are only wasting power.
>
> I only have an overview of current linux pm mechanisms. I wonder if
> those can't be used somehow. Like if devices on the multiplexed bus are
> idle (well, only regarding transfers), then we can switch off the muxer.
>
>> The I2C signals are used to control sensitive codecs. When looking
>> at the sampled data, the I2C traffic is visible in the captured
>> analog signal. To prevent this from happening, the mux can be
>
> I wonder: Is this really a feature of sensitive codecs or an issue of
> the board design?
>
>> disabled whenever not communicating with the codec. This could be
>> accomplished with the "deselect_on_exit" boolean, but because audio
>> driver sends the codec parameters in dozens of small transactions,
>> this will result in a lot more needless I2C traffic, constantly
>> switching the mux for each codec setting.
>
> Has this been looked at? ASoC supports grouping of tranfers IIRC. Maybe
> your I2C driver is only missing I2C_M_NOSTART?.
>
>> Would it be acceptable if I made the timeout optional, by making the
>> "deselect_on_exit" boolean into a three-state value (off, on,
>> timeout)? Or, alternatively, implement "deselect_on_exit" using the
>> timeout scheme (probably with a very short timeout)?
>
> I have a number of concerns designwise. First, if we do something like
> shutting-down-a-bus-if-there-are-no-transfers-expected, it definately
> should be in the core, not the driver. As said before, I have the
> assumption it should even be connected to the runtime pm core via some
> callback. And if we have that for I2C, we surely want that for other
> buses as well, at least SPI. Also, the timeout thing sounds too
> heuristic to me. Later, people might want to change the timeout value
> depending on workloads or so, and then a governor, etc... No, thanks.

It very much sounds like runtime PM should help you here. Then you get
the reference counting and the so called runtime_autosuspend feature
for free. :-)

>
> BTW is it feasible to shut down the whole I2C bus at controller level
> after transfers? No needless transfers and maybe even more power
> savings.

For sure this should be possible. Some controller drivers have started
to adapt some runtime PM code for this is already, nomadik and omap
for example. I think typically clocks, pins and if there are a power
domain regulator for the controller, those should be handled through
runtime PM to save power at "request inactivity".

Kind regards
Ulf Hansson

>
> Anyway, thanks for letting me know about your requirements (they should
> have been in the original commit message, though ;))
>

Re: [PATCH] i2c-mux-pca954x: Disable mux after 200ms timeout

[Mike Looijmans]

On 11/26/2013 01:28 PM, Wolfram Sang wrote:
>
> CCing linux-pm, maybe they know more...
>
>> The extra I2C traffic consumes extra power. If the bus is terminated
>> using 2k resistors, approximately 1mA of current (assuming ~2V
>> signals) is flowing when the bus is pulled low. On low power
>> designs, this extra power consumption is noticable. There is no way
>> to turn the mux "off" from either user or kernel space. The signals
>> going through the mux to a place where no I2C device is actually
>> listening are only wasting power.
>
> I only have an overview of current linux pm mechanisms. I wonder if
> those can't be used somehow. Like if devices on the multiplexed bus are
> idle (well, only regarding transfers), then we can switch off the muxer.

I had looked a bit in that direction, but I think there's currently no way for 
a driver to say "I won't be needing the bus for a while". Something like that 
would be critical for such a pm system to work.

>> The I2C signals are used to control sensitive codecs. When looking
>> at the sampled data, the I2C traffic is visible in the captured
>> analog signal. To prevent this from happening, the mux can be
>
> I wonder: Is this really a feature of sensitive codecs or an issue of
> the board design?

A bit of both I guess. I guess it's the reason that "deselect_on_exit" existed 
in the first place. A lot of guessing that is.

Unlike the I2S bus that transfers the data at multimegahertzes, the I2C bus 
operates in the kHz range which is where audio codecs tend to operate too. 
That might explain why we've seen this issue on more than one design.

>> disabled whenever not communicating with the codec. This could be
>> accomplished with the "deselect_on_exit" boolean, but because audio
>> driver sends the codec parameters in dozens of small transactions,
>> this will result in a lot more needless I2C traffic, constantly
>> switching the mux for each codec setting.
>
> Has this been looked at? ASoC supports grouping of tranfers IIRC. Maybe
> your I2C driver is only missing I2C_M_NOSTART?.

I ported this from a 2.6.37 kernel, so I wouldn't be surprised if that option 
doesn't exist. There has been a lot of changes in the use of regmaps in ASoC 
in the past years.

>> Would it be acceptable if I made the timeout optional, by making the
>> "deselect_on_exit" boolean into a three-state value (off, on,
>> timeout)? Or, alternatively, implement "deselect_on_exit" using the
>> timeout scheme (probably with a very short timeout)?
>
> I have a number of concerns designwise. First, if we do something like
> shutting-down-a-bus-if-there-are-no-transfers-expected, it definately
> should be in the core, not the driver. As said before, I have the
> assumption it should even be connected to the runtime pm core via some
> callback. And if we have that for I2C, we surely want that for other
> buses as well, at least SPI. Also, the timeout thing sounds too
> heuristic to me. Later, people might want to change the timeout value
> depending on workloads or so, and then a governor, etc... No, thanks.

In any case, it doesn't sound like something I can sell - it's understandable 
for my employer that I spent an extra hour or so to clean up and submit the 
code to upstream, but this appears to go into a different class of rework.

So where do you want to go with this? Should I rework the patch to make the 
timeout optional, or should I simply forget about integrating at all?

> BTW is it feasible to shut down the whole I2C bus at controller level
> after transfers? No needless transfers and maybe even more power
> savings.

In fact, on the customer's board, the pca mux is powered by a supply so the 
whole mux can be powered-down too, for which I also needed to patch the pca 
driver to reset its state when the powersupply reported that it was going 
down. I think that particular patch isn't of much use to the community though, 
or is it?

Most hardware can control power and clocks to the I2C controller, which would 
indeed account for some power savings. But again, that would require drivers 
to provide estimations as to when they will need the bus. And it would require 
much more information on the bus controller too, though I suspect that to be 
the easier part.

 > Anyway, thanks for letting me know about your requirements (they should
 > have been in the original commit message, though ;))

I'm new to Linux kernel development, so please forgive me...

Kind regards,
Mike.

Re: [PATCH] i2c-mux-pca954x: Disable mux after 200ms timeout

[Wolfram Sang]

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


> I had looked a bit in that direction, but I think there's currently
> no way for a driver to say "I won't be needing the bus for a while".
> Something like that would be critical for such a pm system to work.

Yes. I wasn't sure if something already existed.

> In any case, it doesn't sound like something I can sell - it's
> understandable for my employer that I spent an extra hour or so to
> clean up and submit the code to upstream, but this appears to go
> into a different class of rework.
>
> So where do you want to go with this? Should I rework the patch to
> make the timeout optional, or should I simply forget about
> integrating at all?

I understand your constraints, yet from a maintenance perspective I
shouldn't have such code in a driver. So, out-of-tree that is for now.

> In fact, on the customer's board, the pca mux is powered by a supply
> so the whole mux can be powered-down too, for which I also needed to
> patch the pca driver to reset its state when the powersupply
> reported that it was going down. I think that particular patch isn't
> of much use to the community though, or is it?

If it uses standard pm callbacks, I'd think this makes sense.

> Most hardware can control power and clocks to the I2C controller,
> which would indeed account for some power savings. But again, that
> would require drivers to provide estimations as to when they will
> need the bus. And it would require much more information on the bus
> controller too, though I suspect that to be the easier part.

There are drivers gating off the clocks simply after every transfer. I
don't know your HW details and workloads, but I wondered if you can
unconditionally switch off the core, do some pinmuxing...

> > Anyway, thanks for letting me know about your requirements (they should
> > have been in the original commit message, though ;))
> 
> I'm new to Linux kernel development, so please forgive me...

That was just a pointer, no complaint. You did a fine job in supplying
information around your patch, so thanks.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]