[i2c] [PATCH 1/2] Add polling transfer for i2c-pxa

[Jean Delvare]

Hi Mike,

On Thu, 06 Dec 2007 11:10:26 +0200, Mike Rapoport wrote:
> This patchset adds ability to make i2c transfers in interrupts-off contexts and
> implements polling transfers for i2c-pxa.
> The i2c-core patch includes fixes for the issues you raised. The only problem
> left is the mutex nesting, since, as you pointed there's no mutex_trylock_nested...
> 
> > I'm curious how many drivers will need this; I'm not sure I see the
> > value. This could be implemented at the driver-level as well, and I
> > think I'd prefer it that way, unless you have reasons to believe that
> > many drivers will want this.
> 
> I think that most PMIC drivers will need this. There are cases when you need to
> access the PMIC in interrupts-off context, for example during late suspend and
> early resume.

My remark was aimed at the "use_pio" (now "use_nosleep_xfer") flag
only, not the whole mechanism. I am well aware that the mechanism is
needed. Forcing the driver to use the nosleep method when you do not
have to doesn't seem to be very desirable in general, as it will take
more resources, which is why I am wondering who needs this. As this
doesn't need to be implemented in i2c-core, I'd rather see it
implemented in the few bus drivers that want it, unless someone
explains to me why it is so desirable.

It seems that you implemented this on request by Russell, but I don't
get the idea. I don't understand why you would want to use a method
that performs worse just "so that the bus is driven in the same way all
the time." Russell, what's the benefit of doing this?

> Signed-off-by: Mike Rapoport <mike at compulab.co.il>
> 
>  drivers/i2c/i2c-core.c |   50 +++++++++++++++++++++++++++++++++++------------
>  include/linux/i2c.h    |    5 ++++
>  2 files changed, 42 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index b5e13e4..059c6bd 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -33,6 +33,8 @@
>  #include <linux/platform_device.h>
>  #include <linux/mutex.h>
>  #include <linux/completion.h>
> +#include <linux/hardirq.h>
> +#include <linux/irqflags.h>
>  #include <asm/uaccess.h>
> 
>  #include "i2c-core.h"
> @@ -868,26 +870,48 @@ module_exit(i2c_exit);
>  int i2c_transfer(struct i2c_adapter * adap, struct i2c_msg *msgs, int num)
>  {
>  	int ret;
> +	int (*xfer)(struct i2c_adapter *adap, struct i2c_msg *msgs, int num);
> 
> -	if (adap->algo->master_xfer) {
> -#ifdef DEBUG
> -		for (ret = 0; ret < num; ret++) {
> -			dev_dbg(&adap->dev, "master_xfer[%d] %c, addr=0x%02x, "
> -				"len=%d%s\n", ret, (msgs[ret].flags & I2C_M_RD)
> -				? 'R' : 'W', msgs[ret].addr, msgs[ret].len,
> -				(msgs[ret].flags & I2C_M_RECV_LEN) ? "+" : "");
> +	if (in_atomic() || irqs_disabled()) {
> +		if (adap->algo->master_nosleep_xfer == NULL) {
> +			dev_dbg(&adap->dev, "algo has no nosleep xfer");
> +			return -EINVAL;
>  		}
> -#endif
> -
> -		mutex_lock_nested(&adap->bus_lock, adap->level);
> -		ret = adap->algo->master_xfer(adap,msgs,num);
> -		mutex_unlock(&adap->bus_lock);
> +		ret = mutex_trylock(&adap->bus_lock);
> +		if (ret) {
> +			/* We get bus_lock, no ongoing I2C activity on
> +			   the Linux side */
> +			ret = adap->algo->master_nosleep_xfer(adap, msgs, num);
> +			mutex_unlock(&adap->bus_lock);
> +			return ret;
> +		} else {
> +			/* I2C activity is ongoing. */
> +			return -EAGAIN;
> +		}
> +	}
> 
> -		return ret;
> +	if (adap->use_nosleep_xfer && adap->algo->master_nosleep_xfer) {
> +		xfer = adap->algo->master_nosleep_xfer;
> +	} else if (adap->algo->master_xfer) {
> +		xfer = adap->algo->master_xfer;
>  	} else {
>  		dev_dbg(&adap->dev, "I2C level transfers not supported\n");
>  		return -ENOSYS;
>  	}
> +
> +#ifdef DEBUG
> +	for (ret = 0; ret < num; ret++)
> +		dev_dbg(&adap->dev, "master_xfer[%d] %c, addr=0x%02x, "
> +			"len=%d%s\n", ret, (msgs[ret].flags & I2C_M_RD)
> +			? 'R' : 'W', msgs[ret].addr, msgs[ret].len,
> +			(msgs[ret].flags & I2C_M_RECV_LEN) ? "+" : "");
> +#endif
> +
> +	mutex_lock_nested(&adap->bus_lock, adap->level);
> +	ret = xfer(adap, msgs, num);
> +	mutex_unlock(&adap->bus_lock);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL(i2c_transfer);
> 
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index a100c9f..bf4dca3 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -290,6 +290,10 @@ struct i2c_algorithm {
>  	                   unsigned short flags, char read_write,
>  	                   u8 command, int size, union i2c_smbus_data * data);
> 
> +	/* PIO xfer can be used in no-interrupts context */
> +	int (*master_nosleep_xfer)(struct i2c_adapter *adap,
> +				   struct i2c_msg *msgs, int num);
> +
>  	/* To determine what the adapter supports */
>  	u32 (*functionality) (struct i2c_adapter *);
>  };
> @@ -304,6 +308,7 @@ struct i2c_adapter {
>  	unsigned int class;
>  	const struct i2c_algorithm *algo; /* the algorithm to access the bus */
>  	void *algo_data;
> +	int use_nosleep_xfer;
> 
>  	/* --- administration stuff. */
>  	int (*client_register)(struct i2c_client *);
> 

That's my only objection to this patch, the rest looks fine to me.

-- 
Jean Delvare