On 5 March 2011 01:43, adq <adq@xxxxxxxxxxxxx> wrote: > 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. > Unfortunately even with this fix, I'm still seeing the problem I was trying to fix to begin with. Although I no longer get any i2c errors (or *any* reported errors), after a bit, one of the frontends just.. stops working. All attempts to tune it fail. I can even unload and reload the driver module, and its stuck in the same state, indicating its a problem with the hardware. :( -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
