Re: [PATCH 1/3] i2c: virtual i2c adapter support.

[Rodolfo Giometti <giometti-AVVDYK/kqiJWk0Htik3J/w@public.gmane.org>]

[-- Attachment #1.1: Type: text/plain, Size: 4974 bytes --]

On Wed, Jun 18, 2008 at 08:00:08PM +0100, Ben Dooks wrote:

> > +static int i2c_virt_master_xfer(struct i2c_adapter *adap,
> > +				struct i2c_msg msgs[], int num)
> > +{
> > +	struct i2c_virt_priv *priv = adap->algo_data;
> > +	struct i2c_adapter *parent = priv->parent_adap;
> > +	int ret;
> > +
> > +	/* Grab the lock for the parent adapter. We already hold the lock for
> > +	 * the virtual adapter. Then select the right mux port and perform
> > +	 * the transfer.
> > +	 */
> > +
> > +	mutex_lock(&parent->bus_lock);
> > +
> > +	ret = priv->select(parent, priv->client, priv->id);
> > +	if (ret >= 0)
> > +		ret = parent->algo->master_xfer(parent, msgs, num);
> > +	priv->deselect(parent, priv->client, priv->id);
> > +
> > +	mutex_unlock(&parent->bus_lock);
> > +
> > +	return ret;
> > +}
> 
> Out of interest, is it going to be better to hide the parent
> away completely from the system? I suppose if clients have
> already bound to it then we're probably going to just have to
> live with it being available.

I'm sorry but I don't understand what you mean. =:-o

The parent is strictly connected with the children virtual adapter, how
can I hide it?

> > +struct i2c_adapter *i2c_add_virt_adapter(struct i2c_adapter *parent,
> > +				struct i2c_client *client,
> > +				u32 force_nr, u32 mux_val,
> > +				int (*select_cb) (struct i2c_adapter *,
> > +						struct i2c_client *, u32),
> > +				int (*deselect_cb) (struct i2c_adapter *,
> > +						struct i2c_client *, u32))
> > +{
> > +	struct i2c_adapter *adap;
> > +	struct i2c_virt_priv *priv;
> > +	struct i2c_algorithm *algo;
> > +	int ret;
> > +
> > +	adap = kzalloc(sizeof(struct i2c_adapter)
> > +			+ sizeof(struct i2c_virt_priv)
> > +			+ sizeof(struct i2c_algorithm), GFP_KERNEL);
> > +	if (!adap)
> > +		return NULL;
> > +
> > +	priv = (struct i2c_virt_priv *)(adap + 1);
> > +	algo = (struct i2c_algorithm *)(priv + 1);
> 
> you shouldn't need force-typecast here.
> 
> you could always make your i2c_virt_priv data contain the i2c_adapter
> and i2c_algorithm structure so you can easily go between them.

Ok.

> > +	/* Set up private adapter data */
> > +	priv->parent_adap = parent;
> > +	priv->client = client;
> > +	priv->id = mux_val;
> > +	priv->select = select_cb;
> > +	priv->deselect = deselect_cb;
> > +
> > +	/* Need to do algo dynamically because we don't know ahead
> > +	 * of time what sort of physical adapter we'll be dealing with.
> > +	 */
> > +	algo->master_xfer = (parent->algo->master_xfer
> > +					? i2c_virt_master_xfer : NULL);
> > +	algo->smbus_xfer = (parent->algo->smbus_xfer
> > +					? i2c_virt_smbus_xfer : NULL);
> > +	algo->functionality = i2c_virt_functionality;
> 
> you're using kzalloc, so this boils down to the neater:
> 
> 	if (parent->algo->master_xfer)
> 		algo->master_xfer = i2c_virt_master_xfer;
> etc.

Ok.

> > +	/* Now fill out new adapter structure */
> > +	snprintf(adap->name, sizeof(adap->name),
> > +			"i2c-%d-virt (mux %02x:%02x)",
> > +			i2c_adapter_id(parent), client->addr, mux_val);
> > +	adap->id = I2C_HW_VIRT | i2c_adapter_id(parent);
> > +	adap->algo = algo;
> > +	adap->algo_data = priv;
> > +	adap->timeout = VIRT_TIMEOUT;
> > +	adap->retries = VIRT_RETRIES;
> > +	adap->dev.parent = &parent->dev;
> 
> How about creating a struct with the callbacks in, and the data
> for timeout, retries and any other data that would be needed?

There are few assignments... :)

> > +	if (force_nr) {
> > +		adap->nr = force_nr;
> > +		ret = i2c_add_numbered_adapter(adap);
> > +	} else
> > +		ret = i2c_add_adapter(adap);
> > +	if (ret < 0) {
> > +		kfree(adap);
> > +		return NULL;
> > +	}
> > +
> > +	dev_info(&parent->dev, "i2c-virt-%d: Virtual I2C bus - "
> > +		"physical bus i2c-%d, multiplexer 0x%02x port %d\n",
> > +		i2c_adapter_id(adap), i2c_adapter_id(parent),
> > +		client->addr, mux_val);
> > +
> > +	return adap;
> > +}
> > +EXPORT_SYMBOL_GPL(i2c_add_virt_adapter);
> > +
> > +int i2c_del_virt_adapter(struct i2c_adapter *adap)
> > +{
> > +	int ret;
> > +
> > +	ret = i2c_del_adapter(adap);
> > +	if (ret < 0)
> > +		return ret;
> > +	kfree(adap);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(i2c_del_virt_adapter);
> > +
> > +MODULE_AUTHOR("Rodolfo Giometti <giometti-k2GhghHVRtY@public.gmane.org, " \
> > +		"Kumar Gala <galak-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>");
> > +MODULE_DESCRIPTION("Virtual I2C driver for multiplexed I2C busses");
> > +MODULE_LICENSE("GPL");
> 
> "GPL v2" is the definition you probably want here.

Ok.

Ciao,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail: giometti-AVVDYK/kqiJWk0Htik3J/w@public.gmane.org
Linux Device Driver                          giometti-k2GhghHVRtY@public.gmane.org
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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