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

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

[Mauro Carvalho Chehab]

Em Tue, 23 Dec 2014 22:49:14 +0200
Antti Palosaari <crope@iki.fi> 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.

Regards,
Mauro
> 
> Signed-off-by: Antti Palosaari <crope@iki.fi>
> ---
>  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;

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

[Antti Palosaari]

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/

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1139 bytes --]


> >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.

Jean handed over I2C to me in late 2012 :)

> 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.

So, AFAIU this is the same problem that I2C based mux devices have (like
drivers/i2c/muxes/i2c-mux-pca954x.c)? They also use the unlocked
transfers...


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Lars-Peter Clausen]

On 02/02/2015 09:33 PM, Wolfram Sang wrote:
>
>>> 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.
>
> Jean handed over I2C to me in late 2012 :)
>
>> 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.
>
> So, AFAIU this is the same problem that I2C based mux devices have (like
> drivers/i2c/muxes/i2c-mux-pca954x.c)? They also use the unlocked
> transfers...

But those are all called with the lock for the adapter being held. I'm not 
convinced this is the same for this patch. This patch seems to add a device 
mutex which protects against concurrent access to the bus with the device 
itself, but not against concurrent access with other devices on the same bus.

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

[Mauro Carvalho Chehab]

[-- Attachment #1: Type: text/plain, Size: 1565 bytes --]

Em Mon, 02 Feb 2015 21:33:24 +0100
Wolfram Sang <wsa@the-dreams.de> escreveu:

> 
> > >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.
> 
> Jean handed over I2C to me in late 2012 :)

Sorry for the mess... I mis-read MAINTAINERS.

> > 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.
> 
> So, AFAIU this is the same problem that I2C based mux devices have (like
> drivers/i2c/muxes/i2c-mux-pca954x.c)? They also use the unlocked
> transfers...

If I understood your comment correct, you're ok with this approach,
right? I'll then merge the remaining of this 66-patch series.

If latter a better way to lock the I2C mux appears, we can reverse
this change.

Regards,
Mauro
> 

[-- Attachment #2: Assinatura digital OpenPGP --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Antti Palosaari]

On 02/03/2015 07:53 PM, Mauro Carvalho Chehab wrote:
> Em Mon, 02 Feb 2015 21:33:24 +0100
> Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> escreveu:
>
>>
>>>> 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.
>>
>> Jean handed over I2C to me in late 2012 :)
>
> Sorry for the mess... I mis-read MAINTAINERS.
>
>>> 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.
>>
>> So, AFAIU this is the same problem that I2C based mux devices have (like
>> drivers/i2c/muxes/i2c-mux-pca954x.c)? They also use the unlocked
>> transfers...
>
> If I understood your comment correct, you're ok with this approach,
> right? I'll then merge the remaining of this 66-patch series.
>
> If latter a better way to lock the I2C mux appears, we can reverse
> this change.

More I am worried about next patch in a serie, which converts all that 
to regmap API... Same recursive mux register access comes to problem 
there, which I work-arounded by defining own I2C IO... And in that case 
I used i2c_lock_adapter/i2c_unlock_adapter so adapter is locked properly.

[PATCH 22/66] rtl2830: convert to regmap API
http://www.spinics.net/lists/linux-media/msg84969.html

regards
Antti

-- 
http://palosaari.fi/

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 611 bytes --]

On Tue, Feb 03, 2015 at 08:34:01PM +0200, Antti Palosaari wrote:
> On 02/03/2015 07:53 PM, Mauro Carvalho Chehab wrote:

> >If latter a better way to lock the I2C mux appears, we can reverse
> >this change.

> More I am worried about next patch in a serie, which converts all that to
> regmap API... Same recursive mux register access comes to problem there,
> which I work-arounded by defining own I2C IO... And in that case I used
> i2c_lock_adapter/i2c_unlock_adapter so adapter is locked properly.

Opne coding the register I/O is a terrible solution, we should allow
people to keep this code factored out.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Jean Delvare]

Hi Mauro, Antti,

On Mon, 2 Feb 2015 18:07:26 -0200, Mauro Carvalho Chehab wrote:
> Em Tue, 23 Dec 2014 22:49:14 +0200
> Antti Palosaari <crope@iki.fi> 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.

This is no good reason to implement your own i2c bus locking. Lots of
i2c slave device work that way, and the way to handle it is through a
dedicated lock at the i2c slave device level. This is in addition to
the standard i2c bus locking and not a replacement.

> > 2) Chip offers muxed/gated I2C adapter for tuner. Gate/mux is
> > controlled by I2C register access.

This, OTOH, is a valid reason for calling __i2c_transfer, and as a
matter of fact a number of dvb frontend drivers already do so.

> > 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.

I think the idea is that the i2c bus lock is already held at the time
the muxing code is called. This happens each time the I2C muxing chip
is an I2C chip itself.

> 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.

If it breaks than it'll break a dozen drivers which are already doing
that, not just this one. But it's OK, I don't see this happening soon.

> Jean,
> 
> Are you ok with such patch? If so, please ack.

First of all: I am no longer the maintainer of the I2C subsystem. That
being said...

The changes look OK to me. I think it's how they are presented which
make them look suspect. As I understand it, the extra locking at device
level is unrelated with calling unlocked i2c transfer functions. The
former change is to address the multi-page/bank register mapping, while
the latter is to solve the deadlock due to the i2c bus topology and
i2c-based muxing. If I am correct then it would be clearer to make that
two separate patches with better descriptions.

And if I'm wrong then the patch needs a better description too ;-)

-- 
Jean Delvare
SUSE L3 Support

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

[Antti Palosaari]

On 02/02/2015 10:56 PM, Jean Delvare wrote:
> Hi Mauro, Antti,
>
> On Mon, 2 Feb 2015 18:07:26 -0200, 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.
>
> This is no good reason to implement your own i2c bus locking. Lots of
> i2c slave device work that way, and the way to handle it is through a
> dedicated lock at the i2c slave device level. This is in addition to
> the standard i2c bus locking and not a replacement.

Patch description is bit misleading as it does not implement own I2C bus 
lock but own 'I2C lock' is there to warrant none will interrupt I/O 
operation as it needs multiple I2C calls.

*** take own I2C lock here
1) I2C mux read to read current register page
2) I2C mux write to switch register page (if needed)
3) I2C mux write to change mux (open gate/repeater for I2C bus tuner is)
4) perform tuner I2C access
*** release own I2C lock here

Mux is closed automatically after tuner I2C in that case, but very often 
there is I2C commands needed for that too.


>>> 2) Chip offers muxed/gated I2C adapter for tuner. Gate/mux is
>>> controlled by I2C register access.
>
> This, OTOH, is a valid reason for calling __i2c_transfer, and as a
> matter of fact a number of dvb frontend drivers already do so.
>
>>> 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.
>
> I think the idea is that the i2c bus lock is already held at the time
> the muxing code is called. This happens each time the I2C muxing chip
> is an I2C chip itself.
>
>> 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.
>
> If it breaks than it'll break a dozen drivers which are already doing
> that, not just this one. But it's OK, I don't see this happening soon.
>
>> Jean,
>>
>> Are you ok with such patch? If so, please ack.
>
> First of all: I am no longer the maintainer of the I2C subsystem. That
> being said...
>
> The changes look OK to me. I think it's how they are presented which
> make them look suspect. As I understand it, the extra locking at device
> level is unrelated with calling unlocked i2c transfer functions. The
> former change is to address the multi-page/bank register mapping, while
> the latter is to solve the deadlock due to the i2c bus topology and
> i2c-based muxing. If I am correct then it would be clearer to make that
> two separate patches with better descriptions.
>
> And if I'm wrong then the patch needs a better description too ;-)
>

regards
Antti

-- 
http://palosaari.fi/