[i2c] [PATCH] Add polling transfer for i2c-pxa

[Jean Delvare]

Hi Mike,

On Tue, 04 Dec 2007 11:24:06 +0200, Mike Rapoport wrote:
> I've added per-adapter 'use_pio' flag, and a method allowing platform code
> to enable this flag.
> 
> Signed-off-by: Mike Rapoport <mike at compulab.co.il>
> 
>  arch/arm/mach-pxa/pxa27x.c     |    6 ++
>  drivers/i2c/busses/i2c-pxa.c   |  123 ++++++++++++++++++++++++++++++++++++----
>  drivers/i2c/i2c-core.c         |   47 ++++++++++-----
>  include/asm-arm/arch-pxa/i2c.h |    6 ++
>  include/linux/i2c.h            |    5 ++
>  5 files changed, 161 insertions(+), 26 deletions(-)

For proper submission, this needs to be split into two parts: the
i2c-core changes in one patch and the i2c-pxa implementation in another.

I can only comment on the i2c-core part:

> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index b5e13e4..5c21035 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -864,30 +864,47 @@ module_exit(i2c_exit);
>   * the functional interface to the i2c busses.
>   * ----------------------------------------------------
>   */
> -

Please revert this whitespace change.

>  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()) {

You need to include <linux/hardirq.h> for in_atomic() to be defined,
and <linux/irqflags.h> for irqs_disabled().

> +		if (adap->algo->pio_xfer == NULL) {

if (!adap->algo->pio_xfer) {

> +			pr_debug("%s: algo has no poll xfer", __FUNCTION__);

Please use dev_dbg() instead.

> +			return -EINVAL;
>  		}
> -#endif
> -
> -		mutex_lock_nested(&adap->bus_lock, adap->level);
> -		ret = adap->algo->master_xfer(adap,msgs,num);
> -		mutex_unlock(&adap->bus_lock);
> -
> -		return ret;
> +		ret = mutex_trylock(&adap->bus_lock);

You don't pass the nesting level here. I guess that lockdep will
complain? OTOH there doesn't appear to be a mutex_trylock_nested()
defined anywhere...

> +		if (ret) { /* We get bus_lock, no ongoing I2C activity */

Just to make things clear: no ongoing I2C activity _on the Linux side_.
The bus might still be busy if there are other masters; then you'll
find out later.

> +			ret = adap->algo->pio_xfer(adap, msgs, num);
> +			mutex_unlock(&adap->bus_lock);
> +			return ret;
> +		} else {
> +			/* I2C activity is ongoing. */
> +			return -EBUSY;

-EAGAIN seems more suited.

> +		}
> +	} else if (adap->use_pio && adap->algo->pio_xfer != NULL) {

The "else" isn't needed, as all code paths above have already returned.
Drop "!= NULL".

> +		xfer = adap->algo->pio_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..2976c15 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 (*pio_xfer)(struct i2c_adapter *adap, struct i2c_msg *msgs,
> +			int num);
> +

I don't like the name "pio_xfer", for two reasons.

First reason is that we currently have two possible transfer methods,
smbus_xfer and master_xfer. "pio_xfer" doesn't make it clear what
transfer method is being "replaced". The name should include "master"
to make it clear that it is a replacement for master_xfer, not
smbus_xfer. We might want a similar mechanism to "replace" smbus_xfer
in the future.

Second reason is that many drivers _already_ do polled I/O in
master_xfer and/or smbus_xfer. What makes pio_xfer different is more
than just "can't use interrupts", if I understand correctly it also
means that we can't sleep, right? And maybe other limitations. So I'd
like a more generic name that doesn't focus on polled I/O.

>  	/* 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_pio;

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.

> 
>  	/* --- administration stuff. */
>  	int (*client_register)(struct i2c_client *);
> 


-- 
Jean Delvare