Re: [PATCH 21/66] rtl2830: implement own I2C locking

[Antti Palosaari <crope-X3B1VOXEql0@public.gmane.org>]

On 02/02/2015 10:07 PM, Mauro Carvalho Chehab wrote:
> Em Tue, 23 Dec 2014 22:49:14 +0200
> Antti Palosaari <crope-X3B1VOXEql0@public.gmane.org> escreveu:
>
>> Own I2C locking is needed due to two special reasons:
>> 1) Chips uses multiple register pages/banks on single I2C slave.
>> Page is changed via I2C register access.
>> 2) Chip offers muxed/gated I2C adapter for tuner. Gate/mux is
>> controlled by I2C register access.
>>
>> Due to these reasons, I2C locking did not fit very well.
>
> I don't like the idea of calling __i2c_transfer() without calling first
> i2c_lock_adapter(). This can be dangerous, as the I2C core itself uses
> the lock for its own usage.
>
> Ok, this may eventually work ok for now, but a further change at the I2C
> core could easily break it. So, we need to double check about such
> patch with the I2C maintainer.
>
> Jean,
>
> Are you ok with such patch? If so, please ack.

Basic problem here is that I2C-mux itself is controlled by same I2C 
device which implements I2C adapter for tuner.

Here is what connections looks like:
  ___________         ____________         ____________
|  USB IF   |       |   demod    |       |    tuner   |
|-----------|       |------------|       |------------|
|           |--I2C--|-----/ -----|--I2C--|            |
|I2C master |       |  I2C mux   |       | I2C slave  |
|___________|       |____________|       |____________|


So when tuner is called via I2C, it needs recursively call same I2C 
adapter which is already locked. More elegant solution would be indeed nice.

regards
Antti


>
> Regards,
> Mauro
>>
>> Signed-off-by: Antti Palosaari <crope-X3B1VOXEql0@public.gmane.org>
>> ---
>>   drivers/media/dvb-frontends/rtl2830.c      | 45 +++++++++++++++++++++++++-----
>>   drivers/media/dvb-frontends/rtl2830_priv.h |  1 +
>>   2 files changed, 39 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/media/dvb-frontends/rtl2830.c b/drivers/media/dvb-frontends/rtl2830.c
>> index 8abaca6..3a9e4e9 100644
>> --- a/drivers/media/dvb-frontends/rtl2830.c
>> +++ b/drivers/media/dvb-frontends/rtl2830.c
>> @@ -43,7 +43,7 @@ static int rtl2830_wr(struct i2c_client *client, u8 reg, const u8 *val, int len)
>>   	buf[0] = reg;
>>   	memcpy(&buf[1], val, len);
>>
>> -	ret = i2c_transfer(client->adapter, msg, 1);
>> +	ret = __i2c_transfer(client->adapter, msg, 1);
>>   	if (ret == 1) {
>>   		ret = 0;
>>   	} else {
>> @@ -73,7 +73,7 @@ static int rtl2830_rd(struct i2c_client *client, u8 reg, u8 *val, int len)
>>   		}
>>   	};
>>
>> -	ret = i2c_transfer(client->adapter, msg, 2);
>> +	ret = __i2c_transfer(client->adapter, msg, 2);
>>   	if (ret == 2) {
>>   		ret = 0;
>>   	} else {
>> @@ -93,16 +93,23 @@ static int rtl2830_wr_regs(struct i2c_client *client, u16 reg, const u8 *val, in
>>   	u8 reg2 = (reg >> 0) & 0xff;
>>   	u8 page = (reg >> 8) & 0xff;
>>
>> +	mutex_lock(&dev->i2c_mutex);
>> +
>>   	/* switch bank if needed */
>>   	if (page != dev->page) {
>>   		ret = rtl2830_wr(client, 0x00, &page, 1);
>>   		if (ret)
>> -			return ret;
>> +			goto err_mutex_unlock;
>>
>>   		dev->page = page;
>>   	}
>>
>> -	return rtl2830_wr(client, reg2, val, len);
>> +	ret = rtl2830_wr(client, reg2, val, len);
>> +
>> +err_mutex_unlock:
>> +	mutex_unlock(&dev->i2c_mutex);
>> +
>> +	return ret;
>>   }
>>
>>   /* read multiple registers */
>> @@ -113,16 +120,23 @@ static int rtl2830_rd_regs(struct i2c_client *client, u16 reg, u8 *val, int len)
>>   	u8 reg2 = (reg >> 0) & 0xff;
>>   	u8 page = (reg >> 8) & 0xff;
>>
>> +	mutex_lock(&dev->i2c_mutex);
>> +
>>   	/* switch bank if needed */
>>   	if (page != dev->page) {
>>   		ret = rtl2830_wr(client, 0x00, &page, 1);
>>   		if (ret)
>> -			return ret;
>> +			goto err_mutex_unlock;
>>
>>   		dev->page = page;
>>   	}
>>
>> -	return rtl2830_rd(client, reg2, val, len);
>> +	ret = rtl2830_rd(client, reg2, val, len);
>> +
>> +err_mutex_unlock:
>> +	mutex_unlock(&dev->i2c_mutex);
>> +
>> +	return ret;
>>   }
>>
>>   /* read single register */
>> @@ -815,6 +829,10 @@ static int rtl2830_select(struct i2c_adapter *adap, void *mux_priv, u32 chan_id)
>>   	};
>>   	int ret;
>>
>> +	dev_dbg(&client->dev, "\n");
>> +
>> +	mutex_lock(&dev->i2c_mutex);
>> +
>>   	/* select register page */
>>   	ret = __i2c_transfer(client->adapter, select_reg_page_msg, 1);
>>   	if (ret != 1) {
>> @@ -841,6 +859,18 @@ err:
>>   	return ret;
>>   }
>>
>> +static int rtl2830_deselect(struct i2c_adapter *adap, void *mux_priv, u32 chan)
>> +{
>> +	struct i2c_client *client = mux_priv;
>> +	struct rtl2830_dev *dev = i2c_get_clientdata(client);
>> +
>> +	dev_dbg(&client->dev, "\n");
>> +
>> +	mutex_unlock(&dev->i2c_mutex);
>> +
>> +	return 0;
>> +}
>> +
>>   static struct dvb_frontend *rtl2830_get_dvb_frontend(struct i2c_client *client)
>>   {
>>   	struct rtl2830_dev *dev = i2c_get_clientdata(client);
>> @@ -886,6 +916,7 @@ static int rtl2830_probe(struct i2c_client *client,
>>   	dev->client = client;
>>   	dev->pdata = client->dev.platform_data;
>>   	dev->sleeping = true;
>> +	mutex_init(&dev->i2c_mutex);
>>   	INIT_DELAYED_WORK(&dev->stat_work, rtl2830_stat_work);
>>
>>   	/* check if the demod is there */
>> @@ -895,7 +926,7 @@ static int rtl2830_probe(struct i2c_client *client,
>>
>>   	/* create muxed i2c adapter for tuner */
>>   	dev->adapter = i2c_add_mux_adapter(client->adapter, &client->dev,
>> -			client, 0, 0, 0, rtl2830_select, NULL);
>> +			client, 0, 0, 0, rtl2830_select, rtl2830_deselect);
>>   	if (dev->adapter == NULL) {
>>   		ret = -ENODEV;
>>   		goto err_kfree;
>> diff --git a/drivers/media/dvb-frontends/rtl2830_priv.h b/drivers/media/dvb-frontends/rtl2830_priv.h
>> index 2931889..517758a 100644
>> --- a/drivers/media/dvb-frontends/rtl2830_priv.h
>> +++ b/drivers/media/dvb-frontends/rtl2830_priv.h
>> @@ -30,6 +30,7 @@ struct rtl2830_dev {
>>   	struct i2c_adapter *adapter;
>>   	struct dvb_frontend fe;
>>   	bool sleeping;
>> +	struct mutex i2c_mutex;
>>   	u8 page; /* active register page */
>>   	unsigned long filters;
>>   	struct delayed_work stat_work;

-- 
http://palosaari.fi/