[PATCH 2/6]: i2c-pcf: Add adapter hooks around xfer begin and end.

[PATCH 2/6]: i2c-pcf: Add adapter hooks around xfer begin and end.

[David Miller]

i2c-pcf: Add adapter hooks around xfer begin and end.

Some I2C bus implementations need to synchronize with external
entities, such as system firmware, which might also be programming the
same I2C bus.

In order to facilitate this add ->xfer_begin() and ->xfer_end() hooks
which are invoked around pcf_xfer().

Signed-off-by: David S. Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>

diff --git a/drivers/i2c/algos/i2c-algo-pcf.c b/drivers/i2c/algos/i2c-algo-pcf.c
index a8a5b6d..b57bc9a 100644
--- a/drivers/i2c/algos/i2c-algo-pcf.c
+++ b/drivers/i2c/algos/i2c-algo-pcf.c
@@ -331,13 +331,15 @@ static int pcf_xfer(struct i2c_adapter *i2c_adap,
 	int i;
 	int ret=0, timeout, status;
     
+	adap->xfer_begin(adap->data);
 
 	/* Check for bus busy */
 	timeout = wait_for_bb(adap);
 	if (timeout) {
 		DEB2(printk(KERN_ERR "i2c-algo-pcf.o: "
 		            "Timeout waiting for BB in pcf_xfer\n");)
-		return -EIO;
+		i = -EIO;
+		goto out;
 	}
 	
 	for (i = 0;ret >= 0 && i < num; i++) {
@@ -359,12 +361,14 @@ static int pcf_xfer(struct i2c_adapter *i2c_adap,
 		if (timeout) {
 			if (timeout == -EINTR) {
 				/* arbitration lost */
-				return (-EINTR);
+				i = -EINTR;
+				goto out;
 			}
 			i2c_stop(adap);
 			DEB2(printk(KERN_ERR "i2c-algo-pcf.o: Timeout waiting "
 				    "for PIN(1) in pcf_xfer\n");)
-			return (-EREMOTEIO);
+			i = -EREMOTEIO;
+			goto out;
 		}
     
 #ifndef STUB_I2C
@@ -372,7 +376,8 @@ static int pcf_xfer(struct i2c_adapter *i2c_adap,
 		if (status & I2C_PCF_LRB) {
 			i2c_stop(adap);
 			DEB2(printk(KERN_ERR "i2c-algo-pcf.o: No LRB(1) in pcf_xfer\n");)
-			return (-EREMOTEIO);
+			i = -EREMOTEIO;
+			goto out;
 		}
 #endif
     
@@ -404,6 +409,8 @@ static int pcf_xfer(struct i2c_adapter *i2c_adap,
 		}
 	}
 
+out:
+	adap->xfer_end(adap->data);
 	return (i);
 }
 
diff --git a/drivers/i2c/busses/i2c-elektor.c b/drivers/i2c/busses/i2c-elektor.c
index 05f7d09..37bb528 100644
--- a/drivers/i2c/busses/i2c-elektor.c
+++ b/drivers/i2c/busses/i2c-elektor.c
@@ -186,6 +186,14 @@ static int pcf_isa_init(void)
 	return 0;
 }
 
+static void pcf_isa_xfer_begin(void *data)
+{
+}
+
+static void pcf_isa_xfer_end(void *data)
+{
+}
+
 /* ------------------------------------------------------------------------
  * Encapsulate the above functions in the correct operations structure.
  * This is only done when more than one hardware adapter is supported.
@@ -196,6 +204,8 @@ static struct i2c_algo_pcf_data pcf_isa_data = {
 	.getown	    = pcf_isa_getown,
 	.getclock   = pcf_isa_getclock,
 	.waitforpin = pcf_isa_waitforpin,
+	.xfer_begin = pcf_isa_xfer_begin,
+	.xfer_end   = pcf_isa_xfer_end,
 };
 
 static struct i2c_adapter pcf_isa_ops = {
diff --git a/include/linux/i2c-algo-pcf.h b/include/linux/i2c-algo-pcf.h
index 5de8a31..78c62ed 100644
--- a/include/linux/i2c-algo-pcf.h
+++ b/include/linux/i2c-algo-pcf.h
@@ -33,6 +33,9 @@ struct i2c_algo_pcf_data {
 	int  (*getclock) (void *data);
 	void (*waitforpin) (void *data);
 
+	void (*xfer_begin)(void *data);
+	void (*xfer_end)(void *data);
+
 	/* Multi-master lost arbitration back-off delay (msecs)
 	 * This should be set by the bus adapter or knowledgable client
 	 * if bus is multi-mastered, else zero
-- 
1.5.6.5.GIT


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH 2/6]: i2c-pcf: Add adapter hooks around xfer begin and end.

[Jean Delvare]

Hi David,

On Thu, 21 Aug 2008 02:43:22 -0700 (PDT), David Miller wrote:
> 
> i2c-pcf: Add adapter hooks around xfer begin and end.
> 
> Some I2C bus implementations need to synchronize with external
> entities, such as system firmware, which might also be programming the
> same I2C bus.
> 
> In order to facilitate this add ->xfer_begin() and ->xfer_end() hooks
> which are invoked around pcf_xfer().

I get the idea, but I'm not sure about the implementation. I had a
request several months ago for something similar for the i2c-algo-bit
driver. I don't think we want to duplicate this mechanism in every
i2c-algo driver, so it would probably make sense to implement it
at the i2c-core level?

Michael, do you still need this for i2c-algo-bit? We did not discuss
this again recently, so I admit I don't know what is the status of this
request.

Probably only bus drivers which rely on an abstracted algorithm driver
will ever need this. Or maybe not... maybe it could be used later to
synchronize SMBus access between ACPI and native drivers?

Of course we can start with a local implementation in i2c-algo-pcf and
we move it to i2c-core later if needed.

More comments below:

> 
> Signed-off-by: David S. Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
> 
> diff --git a/drivers/i2c/algos/i2c-algo-pcf.c b/drivers/i2c/algos/i2c-algo-pcf.c
> index a8a5b6d..b57bc9a 100644
> --- a/drivers/i2c/algos/i2c-algo-pcf.c
> +++ b/drivers/i2c/algos/i2c-algo-pcf.c
> @@ -331,13 +331,15 @@ static int pcf_xfer(struct i2c_adapter *i2c_adap,
>  	int i;
>  	int ret=0, timeout, status;
>      
> +	adap->xfer_begin(adap->data);
>  
>  	/* Check for bus busy */
>  	timeout = wait_for_bb(adap);
>  	if (timeout) {
>  		DEB2(printk(KERN_ERR "i2c-algo-pcf.o: "
>  		            "Timeout waiting for BB in pcf_xfer\n");)
> -		return -EIO;
> +		i = -EIO;
> +		goto out;
>  	}
>  	
>  	for (i = 0;ret >= 0 && i < num; i++) {
> @@ -359,12 +361,14 @@ static int pcf_xfer(struct i2c_adapter *i2c_adap,
>  		if (timeout) {
>  			if (timeout == -EINTR) {
>  				/* arbitration lost */
> -				return (-EINTR);
> +				i = -EINTR;
> +				goto out;
>  			}
>  			i2c_stop(adap);
>  			DEB2(printk(KERN_ERR "i2c-algo-pcf.o: Timeout waiting "
>  				    "for PIN(1) in pcf_xfer\n");)
> -			return (-EREMOTEIO);
> +			i = -EREMOTEIO;
> +			goto out;
>  		}
>      
>  #ifndef STUB_I2C
> @@ -372,7 +376,8 @@ static int pcf_xfer(struct i2c_adapter *i2c_adap,
>  		if (status & I2C_PCF_LRB) {
>  			i2c_stop(adap);
>  			DEB2(printk(KERN_ERR "i2c-algo-pcf.o: No LRB(1) in pcf_xfer\n");)
> -			return (-EREMOTEIO);
> +			i = -EREMOTEIO;
> +			goto out;
>  		}
>  #endif
>      
> @@ -404,6 +409,8 @@ static int pcf_xfer(struct i2c_adapter *i2c_adap,
>  		}
>  	}
>  
> +out:
> +	adap->xfer_end(adap->data);
>  	return (i);
>  }
>  
> diff --git a/drivers/i2c/busses/i2c-elektor.c b/drivers/i2c/busses/i2c-elektor.c
> index 05f7d09..37bb528 100644
> --- a/drivers/i2c/busses/i2c-elektor.c
> +++ b/drivers/i2c/busses/i2c-elektor.c
> @@ -186,6 +186,14 @@ static int pcf_isa_init(void)
>  	return 0;
>  }
>  
> +static void pcf_isa_xfer_begin(void *data)
> +{
> +}
> +
> +static void pcf_isa_xfer_end(void *data)
> +{
> +}
> +
>  /* ------------------------------------------------------------------------
>   * Encapsulate the above functions in the correct operations structure.
>   * This is only done when more than one hardware adapter is supported.
> @@ -196,6 +204,8 @@ static struct i2c_algo_pcf_data pcf_isa_data = {
>  	.getown	    = pcf_isa_getown,
>  	.getclock   = pcf_isa_getclock,
>  	.waitforpin = pcf_isa_waitforpin,
> +	.xfer_begin = pcf_isa_xfer_begin,
> +	.xfer_end   = pcf_isa_xfer_end,
>  };

That's not exactly elegant. I'd rather make .xfer_begin and .xfer_end
optional and check for NULL before calling them from pcf_xfer(). That's
less intrusive also, you don't even need to touch the individual bus
drivers using the i2c-algo-pcf module if they don't need these hooks.

>  static struct i2c_adapter pcf_isa_ops = {

> diff --git a/include/linux/i2c-algo-pcf.h b/include/linux/i2c-algo-pcf.h
> index 5de8a31..78c62ed 100644
> --- a/include/linux/i2c-algo-pcf.h
> +++ b/include/linux/i2c-algo-pcf.h
> @@ -33,6 +33,9 @@ struct i2c_algo_pcf_data {
>  	int  (*getclock) (void *data);
>  	void (*waitforpin) (void *data);
>  
> +	void (*xfer_begin)(void *data);
> +	void (*xfer_end)(void *data);
> +
>  	/* Multi-master lost arbitration back-off delay (msecs)
>  	 * This should be set by the bus adapter or knowledgable client
>  	 * if bus is multi-mastered, else zero


-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH 2/6]: i2c-pcf: Add adapter hooks around xfer begin and end.

[David Miller]

From: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Date: Wed, 15 Oct 2008 14:19:14 +0200

> Of course we can start with a local implementation in i2c-algo-pcf and
> we move it to i2c-core later if needed.

Please let's be practical and handle things this way.

> That's not exactly elegant. I'd rather make .xfer_begin and .xfer_end
> optional and check for NULL before calling them from pcf_xfer(). That's
> less intrusive also, you don't even need to touch the individual bus
> drivers using the i2c-algo-pcf module if they don't need these hooks.

I think it's more elegant to remove as many conditionals from
generic code as possible.

Where were you 2 months ago when I submitted these patches?

I've forgotten most of the state of these changes since then
and now I have to reload all of my context in.

And because you're doing this feedback _NOW_ it's a very real
chance that we'll miss 2.6.28 with these changes.  And I can't
work on any further sparc I2C stuff I had in mind until this
stuff is settled :-(

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH 2/6]: i2c-pcf: Add adapter hooks around xfer begin and end.

[Michael Krufky]

Jean Delvare wrote:
> Hi David,
>
> On Thu, 21 Aug 2008 02:43:22 -0700 (PDT), David Miller wrote:
>   
>> i2c-pcf: Add adapter hooks around xfer begin and end.
>>
>> Some I2C bus implementations need to synchronize with external
>> entities, such as system firmware, which might also be programming the
>> same I2C bus.
>>
>> In order to facilitate this add ->xfer_begin() and ->xfer_end() hooks
>> which are invoked around pcf_xfer().
>>     
>
> I get the idea, but I'm not sure about the implementation. I had a
> request several months ago for something similar for the i2c-algo-bit
> driver. I don't think we want to duplicate this mechanism in every
> i2c-algo driver, so it would probably make sense to implement it
> at the i2c-core level?
>
> Michael, do you still need this for i2c-algo-bit? We did not discuss
> this again recently, so I admit I don't know what is the status of this
> request.

Jean,

Yes, the problem still exists, and I still need a way to lock the i2c 
adapter during certain volitile operations at driver start-up.

Right now I have an artificial delay in place as a workaround, but 
that's only because there currently is no other choice.

Basically, I have a single piece of silicon that is capable of bringing 
down the entire i2c bus if somebody attempts to communicate with any 
other IC during initialization of this silicon.

Regards,

Mike

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH 2/6]: i2c-pcf: Add adapter hooks around xfer begin and end.

[Jean Delvare]

Hi David,

On Wed, 15 Oct 2008 14:32:12 -0700 (PDT), David Miller wrote:
> From: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> Date: Wed, 15 Oct 2008 14:19:14 +0200
> 
> > Of course we can start with a local implementation in i2c-algo-pcf and
> > we move it to i2c-core later if needed.
> 
> Please let's be practical and handle things this way.
> 
> > That's not exactly elegant. I'd rather make .xfer_begin and .xfer_end
> > optional and check for NULL before calling them from pcf_xfer(). That's
> > less intrusive also, you don't even need to touch the individual bus
> > drivers using the i2c-algo-pcf module if they don't need these hooks.
> 
> I think it's more elegant to remove as many conditionals from
> generic code as possible.

I see many good reasons to prefer having conditionals in a central
place over empty callback functions in virtually every i2c bus driver.

First reason: I don't expect many drivers to make use of the pre/post
transaction hooks. Given that a test is cheaper than a function call,
overall performance with conditionals should be better. I suspect this
will also make the builds faster and the binary code smaller (although
I'm not going to benchmark it.)

Second reason: with empty callback functions, it's not immediately
obvious which i2c bus drivers use these hooks and which do not. This
case is specific enough that I think there is value in being able to
trace it.

Third reason: changing things in a central place is easier. You may
not think it's important because there is a rather small number of
PCF8454-based bus drivers are the moment, but if we do the same for
i2c-algo-bit, it has no less than 27 users and I don't feel like
changing them all. If the prototype of the hooks ever changes, having
to go through several dozen drivers to update otherwise useless
callback functions sounds plain wrong. Same if we implement the hooks in
i2c-algo-pcf and i2c-algo-bit today and later decide to move them to
i2c-core: having 2 i2c algo drivers to update is fairly easy, having 30
i2c bus drivers to update is much more work.

I'm not sure what are your reasons for preferring empty callback
functions? I can't remember seeing this approach used in any part of
the kernel I touched. And for what it's worth, i2c-core is full of
tests for callback functions, and I can't remember anyone objecting.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH 2/6]: i2c-pcf: Add adapter hooks around xfer begin and end.

[Jean Delvare]

Hi Michael,

On Wed, 15 Oct 2008 17:37:41 -0400, Michael Krufky wrote:
> Jean Delvare wrote:
> > Hi David,
> >
> > On Thu, 21 Aug 2008 02:43:22 -0700 (PDT), David Miller wrote:
> >   
> >> i2c-pcf: Add adapter hooks around xfer begin and end.
> >>
> >> Some I2C bus implementations need to synchronize with external
> >> entities, such as system firmware, which might also be programming the
> >> same I2C bus.
> >>
> >> In order to facilitate this add ->xfer_begin() and ->xfer_end() hooks
> >> which are invoked around pcf_xfer().
> >>     
> >
> > I get the idea, but I'm not sure about the implementation. I had a
> > request several months ago for something similar for the i2c-algo-bit
> > driver. I don't think we want to duplicate this mechanism in every
> > i2c-algo driver, so it would probably make sense to implement it
> > at the i2c-core level?
> >
> > Michael, do you still need this for i2c-algo-bit? We did not discuss
> > this again recently, so I admit I don't know what is the status of this
> > request.
> 
> Jean,
> 
> Yes, the problem still exists, and I still need a way to lock the i2c 
> adapter during certain volitile operations at driver start-up.
> 
> Right now I have an artificial delay in place as a workaround, but 
> that's only because there currently is no other choice.

I'm really sorry. As you didn't come back to me about this issue, I
wrongly assumed that you had solved it in another way and you were no
longer interested in adding these hooks to i2c-algo-bit. If you still
need it then let's get this sorted out.

> Basically, I have a single piece of silicon that is capable of bringing 
> down the entire i2c bus if somebody attempts to communicate with any 
> other IC during initialization of this silicon.

Can you clarify your requirements? I'm not sure I understand exactly
what you need. Do you need exclusive access to the I2C bus for a number
of transactions? Or do you need exclusive access to the I2C bus for an
arbitrary period of time? Or do you need to block access to the I2C bus
entirely? How do you know when to start the blackout period, and when
to end it?

We have to design a new mechanism so I would like to make sure that
whatever I come up with, fits at least your needs and those of David.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH 2/6]: i2c-pcf: Add adapter hooks around xfer begin and end.

[Michael Krufky]

Jean Delvare wrote:
>>> Michael, do you still need this for i2c-algo-bit? We did not discuss
>>> this again recently, so I admit I don't know what is the status of this
>>> request.
>>>       
>> Jean,
>>
>> Yes, the problem still exists, and I still need a way to lock the i2c 
>> adapter during certain volitile operations at driver start-up.
>>
>> Right now I have an artificial delay in place as a workaround, but 
>> that's only because there currently is no other choice.
>>     
>
> I'm really sorry. As you didn't come back to me about this issue, I
> wrongly assumed that you had solved it in another way and you were no
> longer interested in adding these hooks to i2c-algo-bit. If you still
> need it then let's get this sorted out.
>   

It's not my top priority right now.  I have been able to work around the 
issue by sacrificing efficiency, although a real fix would be better for 
the long run.


>> Basically, I have a single piece of silicon that is capable of bringing 
>> down the entire i2c bus if somebody attempts to communicate with any 
>> other IC during initialization of this silicon.
>>     
>
> Can you clarify your requirements? I'm not sure I understand exactly
> what you need. Do you need exclusive access to the I2C bus for a number
> of transactions? Or do you need exclusive access to the I2C bus for an
> arbitrary period of time? Or do you need to block access to the I2C bus
> entirely? How do you know when to start the blackout period, and when
> to end it?
>
> We have to design a new mechanism so I would like to make sure that
> whatever I come up with, fits at least your needs and those of David.
I need to block access to the i2c bus for the duration of a long series 
of i2c_transfer calls from anybody other than the client module 
currently being initialized.

Basically, I have an i2c client that must not be interrupted during 
initialization of the part.  I need to be able to implement the 
following behavior (in pseudocode)

int crappychip_init() {
    lock host adapter i2c bus -- if bus is already locked, then block 
until it is unlocked.

    do initialization (many calls to i2c_transfer)

    unlock host adapter i2c bus.
}


...so I basically need the ability to host a mutex on the i2c adapter, 
have a way for the i2c client to lock and unlock the mutex, and all 
OTHER i2c clients on the bus must not be able to do i2c transactions 
while the mutex is locked.

...but there is a gotcha -- the other i2c client drivers will not know 
anything about the host adapter, nor will they know that they need to 
use a locking mechanism before handling i2c transactions -- the locking 
needs to be built-in to the host adapter's handling of i2c_transfer.

So, what I *really* need is for the i2c host adapter itself to lock or 
unlock the i2c bus, based on a signal from a given i2c client module 
that a series of i2c transactions are volatile and no other i2c traffic 
is allowed until this long block of transactions are complete.  The 
"large blocks of transactions" can be a large series of multiple calls 
to i2c_transfer, and can be directed to more than just one i2c address.

I think you made a suggestion a few months back, but I don't remember 
exactly what you had in mind.  One idea that I can think of is that the 
i2c client module should request some uniquely generated id, and pass 
this id into the call to i2c_transfer so that it knows that this 
particular transaction is allowed to occur regardless of the state of 
the lock on the i2c bus.

Making matters more complicated, the "do initialization" pseudocode part 
above involves i2c transactions to two different IC's, but we must not 
allow any communication to these IC's to occur other than the 
transactions occurring in the "do initialization" sequence.  ie, no 
probing allowed during this time, not even probing of IC's being 
initialized.  The part being initialized is behind an i2c gate of 
another part, and the i2c gate _must_ be closed after each i2c write and 
opened before the next i2c write, due to a bug in the silicon.


I hope that describes the situation well enough -- Please let me know if 
you need any clarification.

Regards,

Mike

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH 2/6]: i2c-pcf: Add adapter hooks around xfer begin and end.

[Jean Delvare]

Hi Michael,

On Thu, 16 Oct 2008 14:32:32 -0400, Michael Krufky wrote:
> Jean Delvare wrote:
> > Can you clarify your requirements? I'm not sure I understand exactly
> > what you need. Do you need exclusive access to the I2C bus for a number
> > of transactions? Or do you need exclusive access to the I2C bus for an
> > arbitrary period of time? Or do you need to block access to the I2C bus
> > entirely? How do you know when to start the blackout period, and when
> > to end it?
> >
> > We have to design a new mechanism so I would like to make sure that
> > whatever I come up with, fits at least your needs and those of David.
> I need to block access to the i2c bus for the duration of a long series 
> of i2c_transfer calls from anybody other than the client module 
> currently being initialized.
> 
> Basically, I have an i2c client that must not be interrupted during 
> initialization of the part.  I need to be able to implement the 
> following behavior (in pseudocode)
> 
> int crappychip_init() {
>     lock host adapter i2c bus -- if bus is already locked, then block 
> until it is unlocked.
> 
>     do initialization (many calls to i2c_transfer)
> 
>     unlock host adapter i2c bus.
> }
> 
> 
> ...so I basically need the ability to host a mutex on the i2c adapter, 
> have a way for the i2c client to lock and unlock the mutex, and all 
> OTHER i2c clients on the bus must not be able to do i2c transactions 
> while the mutex is locked.
> 
> ...but there is a gotcha -- the other i2c client drivers will not know 
> anything about the host adapter, nor will they know that they need to 
> use a locking mechanism before handling i2c transactions -- the locking 
> needs to be built-in to the host adapter's handling of i2c_transfer.
> 
> So, what I *really* need is for the i2c host adapter itself to lock or 
> unlock the i2c bus, based on a signal from a given i2c client module 
> that a series of i2c transactions are volatile and no other i2c traffic 
> is allowed until this long block of transactions are complete.  The 
> "large blocks of transactions" can be a large series of multiple calls 
> to i2c_transfer, and can be directed to more than just one i2c address.
> 
> I think you made a suggestion a few months back, but I don't remember 
> exactly what you had in mind.  One idea that I can think of is that the 
> i2c client module should request some uniquely generated id, and pass 
> this id into the call to i2c_transfer so that it knows that this 
> particular transaction is allowed to occur regardless of the state of 
> the lock on the i2c bus.
> 
> Making matters more complicated, the "do initialization" pseudocode part 
> above involves i2c transactions to two different IC's, but we must not 
> allow any communication to these IC's to occur other than the 
> transactions occurring in the "do initialization" sequence.  ie, no 
> probing allowed during this time, not even probing of IC's being 
> initialized.  The part being initialized is behind an i2c gate of 
> another part, and the i2c gate _must_ be closed after each i2c write and 
> opened before the next i2c write, due to a bug in the silicon.
> 
> 
> I hope that describes the situation well enough -- Please let me know if 
> you need any clarification.

Thanks for the detailed explanation. It's not clear to me that pre/post
transaction hooks are the way to solve your problem, so let's not try
to come up with a generic solution now, that would only slow down
everything. Instead, I'll commit David's i2c-algo-pcf-specific hooks
(or actually a modified version thereof, see the conditional vs. empty
callbacks discussion elsewhere in this thread) now, and we can
generalize it later if and only if it seems appropriate.

As far as your bus exclusivity problem is concerned, there might
already a solution to it. I am a bit hesitant to tell you because it
would make you use structure members which so far were meant for
internal use of i2c-core. But OTOH, if you only need it for one
specific driver, it may be better to do that than add a new
full-fledged API.

The adapter-hosted mutex you wish to add already exists. It's called
i2c_adapter.bus_lock. It's currently used by i2c_transfer() and
i2c_smbus_xfer(). This is how we serialize access to each I2C bus. So,
you could take this mutex in your code, do the device initialization,
and then release the mutex.

Of course, while you hold the mutex, you can't use i2c_transfer (nor
i2c_master_send nor i2c i2c_master_recv, as these functions call
i2c_transfer) so you would instead have to call
adapter->algo->master_xfer() directly. Not exactly elegant, but it
should work.

The alternative is to implement a new API granting exclusive access to
an I2C bus. Something like:

void i2c_adapter_exclusive_get(struct i2c_adapter *adapter);
void i2c_adapter_exclusive_put(struct i2c_adapter *adapter);
int i2c_transfer_exclusive(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num);

These would essentially be wrappers around adapter->bus_lock and
adapter->algo->master_xfer. This should work, but I'm not sure if the
additional overhead is worth it. I guess it depends on how many drivers
need to do that kind of things in the future.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH 2/6]: i2c-pcf: Add adapter hooks around xfer begin and end.

[Jean Delvare]

Hi David,

On Thu, 16 Oct 2008 14:57:41 +0200, Jean Delvare wrote:
> On Wed, 15 Oct 2008 14:32:12 -0700 (PDT), David Miller wrote:
> > I think it's more elegant to remove as many conditionals from
> > generic code as possible.
> 
> I see many good reasons to prefer having conditionals in a central
> place over empty callback functions in virtually every i2c bus driver.
> 
> First reason: I don't expect many drivers to make use of the pre/post
> transaction hooks. Given that a test is cheaper than a function call,
> overall performance with conditionals should be better. I suspect this
> will also make the builds faster and the binary code smaller (although
> I'm not going to benchmark it.)
> 
> Second reason: with empty callback functions, it's not immediately
> obvious which i2c bus drivers use these hooks and which do not. This
> case is specific enough that I think there is value in being able to
> trace it.
> 
> Third reason: changing things in a central place is easier. You may
> not think it's important because there is a rather small number of
> PCF8454-based bus drivers are the moment, but if we do the same for
> i2c-algo-bit, it has no less than 27 users and I don't feel like
> changing them all. If the prototype of the hooks ever changes, having
> to go through several dozen drivers to update otherwise useless
> callback functions sounds plain wrong. Same if we implement the hooks in
> i2c-algo-pcf and i2c-algo-bit today and later decide to move them to
> i2c-core: having 2 i2c algo drivers to update is fairly easy, having 30
> i2c bus drivers to update is much more work.
> 
> I'm not sure what are your reasons for preferring empty callback
> functions? I can't remember seeing this approach used in any part of
> the kernel I touched. And for what it's worth, i2c-core is full of
> tests for callback functions, and I can't remember anyone objecting.

I've applied the modified patch below:

From: David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Subject: i2c-algo-pcf: Add adapter hooks around xfer begin and end

Some I2C bus implementations need to synchronize with external
entities, such as system firmware, which might also be programming the
same I2C bus.

In order to facilitate this add ->xfer_begin() and ->xfer_end() hooks
which are invoked around pcf_xfer().

[JD: Make these hooks optional.]

Signed-off-by: David S. Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
---
 drivers/i2c/algos/i2c-algo-pcf.c |   17 +++++++++++++----
 include/linux/i2c-algo-pcf.h     |    3 +++
 2 files changed, 16 insertions(+), 4 deletions(-)

--- linux-2.6.28-rc0.orig/drivers/i2c/algos/i2c-algo-pcf.c	2008-10-17 13:21:29.000000000 +0200
+++ linux-2.6.28-rc0/drivers/i2c/algos/i2c-algo-pcf.c	2008-10-17 13:22:59.000000000 +0200
@@ -331,13 +331,16 @@ static int pcf_xfer(struct i2c_adapter *
 	int i;
 	int ret=0, timeout, status;
     
+	if (adap->xfer_begin)
+		adap->xfer_begin(adap->data);
 
 	/* Check for bus busy */
 	timeout = wait_for_bb(adap);
 	if (timeout) {
 		DEB2(printk(KERN_ERR "i2c-algo-pcf.o: "
 		            "Timeout waiting for BB in pcf_xfer\n");)
-		return -EIO;
+		i = -EIO;
+		goto out;
 	}
 	
 	for (i = 0;ret >= 0 && i < num; i++) {
@@ -359,12 +362,14 @@ static int pcf_xfer(struct i2c_adapter *
 		if (timeout) {
 			if (timeout == -EINTR) {
 				/* arbitration lost */
-				return (-EINTR);
+				i = -EINTR;
+				goto out;
 			}
 			i2c_stop(adap);
 			DEB2(printk(KERN_ERR "i2c-algo-pcf.o: Timeout waiting "
 				    "for PIN(1) in pcf_xfer\n");)
-			return (-EREMOTEIO);
+			i = -EREMOTEIO;
+			goto out;
 		}
     
 #ifndef STUB_I2C
@@ -372,7 +377,8 @@ static int pcf_xfer(struct i2c_adapter *
 		if (status & I2C_PCF_LRB) {
 			i2c_stop(adap);
 			DEB2(printk(KERN_ERR "i2c-algo-pcf.o: No LRB(1) in pcf_xfer\n");)
-			return (-EREMOTEIO);
+			i = -EREMOTEIO;
+			goto out;
 		}
 #endif
     
@@ -404,6 +410,9 @@ static int pcf_xfer(struct i2c_adapter *
 		}
 	}
 
+out:
+	if (adap->xfer_end)
+		adap->xfer_end(adap->data);
 	return (i);
 }
 
--- linux-2.6.28-rc0.orig/include/linux/i2c-algo-pcf.h	2008-10-17 13:21:29.000000000 +0200
+++ linux-2.6.28-rc0/include/linux/i2c-algo-pcf.h	2008-10-17 13:22:05.000000000 +0200
@@ -33,6 +33,9 @@ struct i2c_algo_pcf_data {
 	int  (*getclock) (void *data);
 	void (*waitforpin) (void *data);
 
+	void (*xfer_begin) (void *data);
+	void (*xfer_end) (void *data);
+
 	/* Multi-master lost arbitration back-off delay (msecs)
 	 * This should be set by the bus adapter or knowledgable client
 	 * if bus is multi-mastered, else zero

This will go to Linus as part of my second i2c pull request (which is
still blocked by a missing pci_ids patch at the moment.)

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c