On 4 March 2011 23:11, Andrew de Quincey <adq_dvb@xxxxxxxxxxxxx> wrote:
> On 4 March 2011 22:59, Antti Palosaari <crope@xxxxxx> wrote:
>> On 03/05/2011 12:44 AM, Andrew de Quincey wrote:
>>>>>
>>>>> Adding a "bus lock" to af9015_i2c_xfer() will not work as demod/tuner
>>>>> accesses will take multiple i2c transactions.
>>>>>
>>>>> Therefore, the following patch overrides the dvb_frontend_ops
>>>>> functions to add a per-device lock around them: only one frontend can
>>>>> now use the i2c bus at a time. Testing with the scripts above shows
>>>>> this has eliminated the errors.
>>>>
>>>> This have annoyed me too, but since it does not broken functionality much
>>>> I
>>>> haven't put much effort for fixing it. I like that fix since it is in
>>>> AF9015
>>>> driver where it logically belongs to. But it looks still rather complex.
>>>> I
>>>> see you have also considered "bus lock" to af9015_i2c_xfer() which could
>>>> be
>>>> much smaller in code size (that's I have tried to implement long time
>>>> back).
>>>>
>>>> I would like to ask if it possible to check I2C gate open / close inside
>>>> af9015_i2c_xfer() and lock according that? Something like:
>>>
>>> Hmm, I did think about that, but I felt overriding the functions was
>>> just cleaner: I felt it was more obvious what it was doing. Doing
>>> exactly this sort of tweaking was one of the main reasons we added
>>> that function overriding feature.
>>>
>>> I don't like the idea of returning "error locked by FE" since that'll
>>> mean the tuning will randomly fail sometimes in a way visible to
>>> userspace (unless we change the core dvb_frontend code), which was one
>>> of the things I was trying to avoid. Unless, of course, I've
>>> misunderstood your proposal.
>>
>> Not returning error, but waiting in lock like that:
>> if (mutex_lock_interruptible(&d->i2c_mutex) < 0)
>> return -EAGAIN;
>
> Ah k, sorry
>
>>> However, looking at the code again, I realise it is possible to
>>> simplify it. Since its only the demod gates that cause a problem, we
>>> only /actually/ need to lock the get_frontend() and set_frontend()
>>> calls.
>>
>> I don't understand why .get_frontend() causes problem, since it does not
>> access tuner at all. It only reads demod registers. The main problem is
>> (like schema in af9015.c shows) that there is two tuners on same I2C bus
>> using same address. And demod gate is only way to open access for desired
>> tuner only.
>
> AFAIR /some/ tuner code accesses the tuner hardware to read the exact
> tuned frequency back on a get_frontend(); was just being extra
> paranoid :)
>
>> You should block traffic based of tuner not demod. And I think those
>> callbacks which are needed for override are tuner driver callbacks. Consider
>> situation device goes it v4l-core calls same time both tuner .sleep() ==
>> problem.
>
> Hmm, yeah, you're right, let me have another look tomorrow.
>
Hi, must admit I misunderstood your diagram originally, I thought it
was the demods AND the tuners that had the same i2c addresses.
As you say though. its just the tuners, so adding the locking into the
gate ctrl as you suggested makes perfect sense. Attached is v3
implementing this; it seems to be working fine here.
diff --git a/drivers/media/dvb/dvb-usb/af9015.c b/drivers/media/dvb/dvb-usb/af9015.c
index 31c0a0e..d9f32fe 100644
--- a/drivers/media/dvb/dvb-usb/af9015.c
+++ b/drivers/media/dvb/dvb-usb/af9015.c
@@ -1083,6 +1083,24 @@ static int af9015_i2c_init(struct dvb_usb_device *d)
return ret;
}
+static int af9015_lock_i2c_gate_ctrl(struct dvb_frontend *fe, int enable)
+{
+ int result;
+ struct dvb_usb_adapter *adap = fe->dvb->priv;
+ struct af9015_state *state = adap->dev->priv;
+
+ if (enable)
+ if (mutex_lock_interruptible(&adap->dev->usb_mutex))
+ return -EAGAIN;
+
+ result = state->i2c_gate_ctrl[adap->id](fe, enable);
+
+ if (!enable)
+ mutex_unlock(&adap->dev->usb_mutex);
+
+ return result;
+}
+
static int af9015_af9013_frontend_attach(struct dvb_usb_adapter *adap)
{
int ret;
@@ -1116,6 +1134,11 @@ static int af9015_af9013_frontend_attach(struct dvb_usb_adapter *adap)
/* attach demodulator */
adap->fe = dvb_attach(af9013_attach, &af9015_af9013_config[adap->id],
i2c_adap);
+
+ if (adap->fe && adap->fe->ops.i2c_gate_ctrl) {
+ state->i2c_gate_ctrl[adap->id] = adap->fe->ops.i2c_gate_ctrl;
+ adap->fe->ops.i2c_gate_ctrl = af9015_lock_i2c_gate_ctrl;
+ }
return adap->fe == NULL ? -ENODEV : 0;
}
diff --git a/drivers/media/dvb/dvb-usb/af9015.h b/drivers/media/dvb/dvb-usb/af9015.h
index f20cfa6..094b1e3 100644
--- a/drivers/media/dvb/dvb-usb/af9015.h
+++ b/drivers/media/dvb/dvb-usb/af9015.h
@@ -102,6 +102,7 @@ struct af9015_state {
struct i2c_adapter i2c_adap; /* I2C adapter for 2nd FE */
u8 rc_repeat;
u32 rc_keycode;
+ int (*i2c_gate_ctrl[2])(struct dvb_frontend *fe, int enable);
};
struct af9015_config {
